diff --git a/tests/e2e/steps_consumer_misbehaviour.go b/tests/e2e/steps_consumer_misbehaviour.go index d41e0bd2c0..7d7fda94b1 100644 --- a/tests/e2e/steps_consumer_misbehaviour.go +++ b/tests/e2e/steps_consumer_misbehaviour.go @@ -233,12 +233,11 @@ func stepsCauseConsumerMisbehaviour(consumerName string) []Step { validatorID("alice"): 0, validatorID("bob"): 20, }, - // The consumer light client should be frozen - // TODO: to be changed when merging PR #1148 + // The consumer light client should not be frozen ClientsFrozenHeights: &map[string]clienttypes.Height{ "07-tendermint-0": { RevisionNumber: 0, - RevisionHeight: 1, + RevisionHeight: 0, }, }, }, diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go index 26cfb6d3be..c3b590d5c1 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -12,7 +12,6 @@ import ( // TestHandleConsumerMisbehaviour tests that handling a valid misbehaviour, // with conflicting headers forming an equivocation, results in the jailing and tombstoning of the validators -// TODO: figure out how to signed headers with a subset a the validator set. func (s *CCVTestSuite) TestHandleConsumerMisbehaviour() { s.SetupCCVChannel(s.path) // required to have the consumer client revision height greater than 0 @@ -219,3 +218,177 @@ func (s *CCVTestSuite) TestGetByzantineValidators() { }) } } + +func (s *CCVTestSuite) TestCheckMisbehaviour() { + s.SetupCCVChannel(s.path) + // required to have the consumer client revision height greater than 0 + s.SendEmptyVSCPacket() + + // create signing info for all validators + for _, v := range s.providerChain.Vals.Validators { + s.setDefaultValSigningInfo(*v) + } + + // create a new header timestamp + headerTs := s.providerCtx().BlockTime().Add(time.Minute) + + // get trusted validators and height + clientHeight := s.consumerChain.LastHeader.TrustedHeight + clientTMValset := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators) + clientSigners := s.consumerChain.Signers + + // create an alternative validator set using more than 1/3 of the trusted validator set + altValset := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators[0:2]) + altSigners := make(map[string]tmtypes.PrivValidator, 1) + altSigners[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()] + altSigners[clientTMValset.Validators[1].Address.String()] = clientSigners[clientTMValset.Validators[1].Address.String()] + testCases := []struct { + name string + misbehaviour *ibctmtypes.Misbehaviour + expPass bool + }{ + { + "client state not found - shouldn't pass", + &ibctmtypes.Misbehaviour{ + ClientId: "clientID", + Header1: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ), + Header2: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + altValset, + altValset, + clientTMValset, + altSigners, + ), + }, + false, + }, + { + "invalid misbehaviour with empty header1 - shouldn't pass", + &ibctmtypes.Misbehaviour{ + Header1: &ibctmtypes.Header{}, + Header2: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + altValset, + altValset, + clientTMValset, + altSigners, + ), + }, + false, + }, + { + "invalid misbehaviour with different header height - shouldn't pass", + &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, + Header1: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ), + Header2: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+2), + clientHeight, + headerTs, + altValset, + altValset, + clientTMValset, + altSigners, + ), + }, + false, + }, + { + "valid misbehaviour - should pass", + &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, + Header1: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ), + // create header using a different validator set + Header2: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + altValset, + altValset, + clientTMValset, + altSigners, + ), + }, + true, + }, + { + "valid misbehaviour with already frozen client - should pass", + &ibctmtypes.Misbehaviour{ + ClientId: s.path.EndpointA.ClientID, + Header1: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + clientTMValset, + clientTMValset, + clientTMValset, + clientSigners, + ), + // the resulting Header2 will have a different BlockID + // than Header1 since doesn't share the same valset and signers + Header2: s.consumerChain.CreateTMClientHeader( + s.consumerChain.ChainID, + int64(clientHeight.RevisionHeight+1), + clientHeight, + headerTs, + altValset, + altValset, + clientTMValset, + altSigners, + ), + }, + true, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + err := s.providerApp.GetProviderKeeper().CheckMisbehaviour(s.providerCtx(), *tc.misbehaviour) + cs, ok := s.providerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.providerCtx(), s.path.EndpointA.ClientID) + s.Require().True(ok) + // verify that the client wasn't frozen + s.Require().Zero(cs.(*ibctmtypes.ClientState).FrozenHeight) + if tc.expPass { + s.NoError(err) + } else { + s.Error(err) + } + }) + } +} diff --git a/testutil/integration/debug_test.go b/testutil/integration/debug_test.go index 6715d6368e..2d9421300b 100644 --- a/testutil/integration/debug_test.go +++ b/testutil/integration/debug_test.go @@ -268,3 +268,7 @@ func TestHandleConsumerMisbehaviour(t *testing.T) { func TestGetByzantineValidators(t *testing.T) { runCCVTestByName(t, "TestGetByzantineValidators") } + +func TestCheckMisbehaviour(t *testing.T) { + runCCVTestByName(t, "TestCheckMisbehaviour") +} diff --git a/x/ccv/provider/client/cli/tx.go b/x/ccv/provider/client/cli/tx.go index 491e1e618f..22dbd6b864 100644 --- a/x/ccv/provider/client/cli/tx.go +++ b/x/ccv/provider/client/cli/tx.go @@ -10,9 +10,9 @@ import ( "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/client/tx" "github.com/cosmos/cosmos-sdk/version" + ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types" sdk "github.com/cosmos/cosmos-sdk/types" - ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types" "github.com/cosmos/interchain-security/v2/x/ccv/provider/types" ) @@ -125,12 +125,12 @@ Examples: WithTxConfig(clientCtx.TxConfig).WithAccountRetriever(clientCtx.AccountRetriever) submitter := clientCtx.GetFromAddress() - var misbehavior ibctmtypes.Misbehaviour - if err := clientCtx.Codec.UnmarshalInterfaceJSON([]byte(args[1]), &misbehavior); err != nil { + var misbehaviour ibctmtypes.Misbehaviour + if err := clientCtx.Codec.UnmarshalInterfaceJSON([]byte(args[1]), &misbehaviour); err != nil { return err } - msg, err := types.NewMsgSubmitConsumerMisbehaviour(submitter, &misbehavior) + msg, err := types.NewMsgSubmitConsumerMisbehaviour(submitter, &misbehaviour) if err != nil { return err } diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go index 844a5523ea..c44c1d46dc 100644 --- a/x/ccv/provider/keeper/misbehaviour.go +++ b/x/ccv/provider/keeper/misbehaviour.go @@ -6,7 +6,9 @@ import ( "github.com/cosmos/interchain-security/v2/x/ccv/provider/types" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" evidencetypes "github.com/cosmos/cosmos-sdk/x/evidence/types" + ibcclienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types" ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types" tmtypes "github.com/tendermint/tendermint/types" ) @@ -17,7 +19,7 @@ func (k Keeper) HandleConsumerMisbehaviour(ctx sdk.Context, misbehaviour ibctmty logger := k.Logger(ctx) // Check that the misbehaviour is valid and that the client isn't expired - if err := k.clientKeeper.CheckMisbehaviourAndUpdateState(ctx, &misbehaviour); err != nil { + if err := k.CheckMisbehaviour(ctx, misbehaviour); err != nil { logger.Info("Misbehaviour rejected", err.Error()) return err @@ -129,3 +131,35 @@ func headerToLightBlock(h ibctmtypes.Header) (*tmtypes.LightBlock, error) { ValidatorSet: vs, }, nil } + +// CheckMisbehaviour checks that headers in the given misbehaviour forms +// a valid light client attack and that the corresponding light client isn't expired +func (k Keeper) CheckMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbehaviour) error { + if err := misbehaviour.ValidateBasic(); err != nil { + return err + } + + clientState, found := k.clientKeeper.GetClientState(ctx, misbehaviour.GetClientID()) + if !found { + return sdkerrors.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID()) + } + + clientStore := k.clientKeeper.ClientStore(ctx, misbehaviour.GetClientID()) + + // Check that the headers are at the same height to ensure that + // the misbehaviour is for a light client attack and not a time violation, + // see ibc-go/modules/light-clients/07-tendermint/types/misbehaviour_handle.go#L54 + if !misbehaviour.Header1.GetHeight().EQ(misbehaviour.Header2.GetHeight()) { + return sdkerrors.Wrap(ibcclienttypes.ErrInvalidMisbehaviour, "headers are not at same height") + } + + // CheckMisbehaviourAndUpdateState verifies the misbehaviour against the consensus states + // but does NOT update the light client status. + // Note CheckMisbehaviourAndUpdateState returns an error if the light client is expired + _, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, &misbehaviour) + if err != nil { + return err + } + + return nil +} diff --git a/x/ccv/provider/types/codec.go b/x/ccv/provider/types/codec.go index 0ef3c2d296..2f28b398f5 100644 --- a/x/ccv/provider/types/codec.go +++ b/x/ccv/provider/types/codec.go @@ -6,6 +6,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/msgservice" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + "github.com/cosmos/ibc-go/v4/modules/core/exported" ) // RegisterLegacyAminoCodec registers the necessary x/ibc transfer interfaces and concrete types @@ -40,6 +41,10 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) { (*sdk.Msg)(nil), &MsgSubmitConsumerMisbehaviour{}, ) + registry.RegisterInterface( + "ibc.core.client.v1.Misbehaviour", + (*exported.Misbehaviour)(nil), + ) msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc) } diff --git a/x/ccv/provider/types/msg.go b/x/ccv/provider/types/msg.go index 9496122342..5c217f53b8 100644 --- a/x/ccv/provider/types/msg.go +++ b/x/ccv/provider/types/msg.go @@ -6,7 +6,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types" ) @@ -147,8 +146,8 @@ func (msg MsgRegisterConsumerRewardDenom) ValidateBasic() error { return nil } -func NewMsgSubmitConsumerMisbehaviour(submitter sdk.AccAddress, m *ibctmtypes.Misbehaviour) (*MsgSubmitConsumerMisbehaviour, error) { - return &MsgSubmitConsumerMisbehaviour{Submitter: submitter.String(), Misbehaviour: m}, nil +func NewMsgSubmitConsumerMisbehaviour(submitter sdk.AccAddress, misbehaviour *ibctmtypes.Misbehaviour) (*MsgSubmitConsumerMisbehaviour, error) { + return &MsgSubmitConsumerMisbehaviour{Submitter: submitter.String(), Misbehaviour: misbehaviour}, nil } // Route implements the sdk.Msg interface. @@ -164,6 +163,7 @@ func (msg MsgSubmitConsumerMisbehaviour) ValidateBasic() error { if msg.Submitter == "" { return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, msg.Submitter) } + if err := msg.Misbehaviour.ValidateBasic(); err != nil { return err }