Feat: avoid race condition in ICS misbehaviour handling

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,
 						},
 					},
 				},

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)
+			}
+		})
+	}
+}

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")
+}

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
 			}

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
 }
+
+// CheckMisbehaviourAndUpdateState 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
+}

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)
 }

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
 	}