Feat: avoid race condition in ICS misbehaviour handling (#1148)

* remove unwanted changes * fix hermes config with assigned key * revert unwanted changes * revert local setup * remove log file * typo * update doc * update ICS misbehaviour test * update ICS misbehaviour test * revert mixed commits * update ICS misbehaviour test * update ICS misbehaviour test * Add test for MsgSubmitConsumerMisbehaviour parsing * fix linter * save progress * add CheckMisbehaviourAndUpdateState * update integration tests * typo * remove e2e tests from another PRs * cleaning' * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: Anca Zamfir <[email protected]> * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: Anca Zamfir <[email protected]> * update integration tests * save * save * nits * remove todo * lint * Update x/ccv/provider/keeper/misbehaviour.go --------- Co-authored-by: Anca Zamfir <[email protected]> Co-authored-by: Marius Poke <[email protected]>
cosmos · Aug 11, 2023 · 43dc212 · 43dc212
1 parent 8844518
commit 43dc212
Show file tree

Hide file tree

Showing 7 changed files with 227 additions and 11 deletions.
diff --git 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
@@ -218,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 	}