fix: moved non-verification misbehaviour checks to checkForMisbehavio…

…ur (#3046) (#3084) * move misbehaviour check * add test coverage (cherry picked from commit eb23e9e) Co-authored-by: Charly <charly@interchain.io>
cosmos · Feb 2, 2023 · fae6c91 · fae6c91
1 parent f08fa66
commit fae6c91
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 91 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -93,6 +93,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [\#2434](https://github.com/cosmos/ibc-go/pull/2478) Removed all `TypeMsg` constants
 * (modules/core/exported) [#1689] (https://github.com/cosmos/ibc-go/pull/2539) Removing `GetVersions` from `ConnectionI` interface.
 * (core/02-connection) [#2419](https://github.com/cosmos/ibc-go/pull/2419) Add optional proof data to proto definitions of `MsgConnectionOpenTry` and `MsgConnectionOpenAck` for host state machines that are unable to introspect their own consensus state.
+* (modules/light-clients/07-tendermint) [#2007](https://github.com/cosmos/ibc-go/pull/3046) Moved non-verification misbehaviour checks to `CheckForMisbehaviour`
 
 ### Features
 

diff --git a/modules/light-clients/07-tendermint/misbehaviour_handle.go b/modules/light-clients/07-tendermint/misbehaviour_handle.go
@@ -15,6 +15,7 @@ import (
 )
 
 // CheckForMisbehaviour detects duplicate height misbehaviour and BFT time violation misbehaviour
+// in a submitted Header message and verifies the correctness of a submitted Misbehaviour ClientMessage
 func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, msg exported.ClientMessage) bool {
 	switch msg := msg.(type) {
 	case *Header:
@@ -51,9 +52,29 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode
 			return true
 		}
 	case *Misbehaviour:
-		// The correctness of Misbehaviour ClientMessage types is ensured by calling VerifyClientMessage prior to this function
-		// Thus, here we can return true, as ClientMessage is of type Misbehaviour
-		return true
+		// if heights are equal check that this is valid misbehaviour of a fork
+		// otherwise if heights are unequal check that this is valid misbehavior of BFT time violation
+		if msg.Header1.GetHeight().EQ(msg.Header2.GetHeight()) {
+			blockID1, err := tmtypes.BlockIDFromProto(&msg.Header1.SignedHeader.Commit.BlockID)
+			if err != nil {
+				return false
+			}
+
+			blockID2, err := tmtypes.BlockIDFromProto(&msg.Header2.SignedHeader.Commit.BlockID)
+			if err != nil {
+				return false
+			}
+
+			// Ensure that Commit Hashes are different
+			if !bytes.Equal(blockID1.Hash, blockID2.Hash) {
+				return true
+			}
+
+		} else if !msg.Header1.SignedHeader.Header.Time.After(msg.Header2.SignedHeader.Header.Time) {
+			// Header1 is at greater height than Header2, therefore Header1 time must be less than or equal to
+			// Header2 time in order to be valid misbehaviour (violation of monotonic time).
+			return true
+		}
 	}
 
 	return false
@@ -68,30 +89,6 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode
 // to misbehaviour.Header2
 // Misbehaviour sets frozen height to {0, 1} since it is only used as a boolean value (zero or non-zero).
 func (cs *ClientState) verifyMisbehaviour(ctx sdk.Context, clientStore sdk.KVStore, cdc codec.BinaryCodec, misbehaviour *Misbehaviour) error {
-	// if heights are equal check that this is valid misbehaviour of a fork
-	// otherwise if heights are unequal check that this is valid misbehavior of BFT time violation
-	if misbehaviour.Header1.GetHeight().EQ(misbehaviour.Header2.GetHeight()) {
-		blockID1, err := tmtypes.BlockIDFromProto(&misbehaviour.Header1.SignedHeader.Commit.BlockID)
-		if err != nil {
-			return sdkerrors.Wrap(err, "invalid block ID from header 1 in misbehaviour")
-		}
-
-		blockID2, err := tmtypes.BlockIDFromProto(&misbehaviour.Header2.SignedHeader.Commit.BlockID)
-		if err != nil {
-			return sdkerrors.Wrap(err, "invalid block ID from header 2 in misbehaviour")
-		}
-
-		// Ensure that Commit Hashes are different
-		if bytes.Equal(blockID1.Hash, blockID2.Hash) {
-			return sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers block hashes are equal")
-		}
-
-	} else if misbehaviour.Header1.SignedHeader.Header.Time.After(misbehaviour.Header2.SignedHeader.Header.Time) {
-		// Header1 is at greater height than Header2, therefore Header1 time must be less than or equal to
-		// Header2 time in order to be valid misbehaviour (violation of monotonic time).
-		return sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "headers are not at same height and are monotonically increasing")
-	}
-
 	// Regardless of the type of misbehaviour, ensure that both headers are valid and would have been accepted by light-client
 
 	// Retrieve trusted consensus states for each Header in misbehaviour

diff --git a/modules/light-clients/07-tendermint/misbehaviour_handle_test.go b/modules/light-clients/07-tendermint/misbehaviour_handle_test.go
@@ -204,38 +204,6 @@ func (suite *TendermintTestSuite) TestVerifyMisbehaviour() {
 				}
 			}, true,
 		},
-		{
-			"invalid fork misbehaviour: identical headers", func() {
-				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
-
-				trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1)
-				suite.Require().True(found)
-
-				err := path.EndpointA.UpdateClient()
-				suite.Require().NoError(err)
-
-				height := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
-
-				misbehaviourHeader := suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, int64(height.RevisionHeight), trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers)
-				misbehaviour = &ibctm.Misbehaviour{
-					Header1: misbehaviourHeader,
-					Header2: misbehaviourHeader,
-				}
-			}, false,
-		},
-		{
-			"invalid time misbehaviour: monotonically increasing time", func() {
-				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
-
-				trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1)
-				suite.Require().True(found)
-
-				misbehaviour = &ibctm.Misbehaviour{
-					Header1: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height+3, trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers),
-					Header2: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height, trustedHeight, suite.chainB.CurrentHeader.Time, suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers),
-				}
-			}, false,
-		},
 		{
 			"invalid misbehaviour: misbehaviour from different chain", func() {
 				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
@@ -523,38 +491,6 @@ func (suite *TendermintTestSuite) TestVerifyMisbehaviourNonRevisionChainID() {
 				}
 			}, true,
 		},
-		{
-			"invalid fork misbehaviour: identical headers", func() {
-				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
-
-				trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1)
-				suite.Require().True(found)
-
-				err := path.EndpointA.UpdateClient()
-				suite.Require().NoError(err)
-
-				height := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
-
-				misbehaviourHeader := suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, int64(height.RevisionHeight), trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers)
-				misbehaviour = &ibctm.Misbehaviour{
-					Header1: misbehaviourHeader,
-					Header2: misbehaviourHeader,
-				}
-			}, false,
-		},
-		{
-			"invalid time misbehaviour: monotonically increasing time", func() {
-				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
-
-				trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1)
-				suite.Require().True(found)
-
-				misbehaviour = &ibctm.Misbehaviour{
-					Header1: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height+3, trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers),
-					Header2: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height, trustedHeight, suite.chainB.CurrentHeader.Time, suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers),
-				}
-			}, false,
-		},
 		{
 			"invalid misbehaviour: misbehaviour from different chain", func() {
 				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)

diff --git a/modules/light-clients/07-tendermint/update_test.go b/modules/light-clients/07-tendermint/update_test.go
@@ -635,6 +635,38 @@ func (suite *TendermintTestSuite) TestCheckForMisbehaviour() {
 			},
 			false,
 		},
+		{
+			"invalid fork misbehaviour: identical headers", func() {
+				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
+
+				trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1)
+				suite.Require().True(found)
+
+				err := path.EndpointA.UpdateClient()
+				suite.Require().NoError(err)
+
+				height := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
+
+				misbehaviourHeader := suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, int64(height.RevisionHeight), trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers)
+				clientMessage = &ibctm.Misbehaviour{
+					Header1: misbehaviourHeader,
+					Header2: misbehaviourHeader,
+				}
+			}, false,
+		},
+		{
+			"invalid time misbehaviour: monotonically increasing time", func() {
+				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
+
+				trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1)
+				suite.Require().True(found)
+
+				clientMessage = &ibctm.Misbehaviour{
+					Header1: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height+3, trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers),
+					Header2: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height, trustedHeight, suite.chainB.CurrentHeader.Time, suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers),
+				}
+			}, false,
+		},
 		{
 			"consensus state already exists, app hash mismatch",
 			func() {
@@ -705,6 +737,19 @@ func (suite *TendermintTestSuite) TestCheckForMisbehaviour() {
 			},
 			true,
 		},
+		{
+			"valid time misbehaviour: not monotonically increasing time", func() {
+				trustedHeight := path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height)
+
+				trustedVals, found := suite.chainB.GetValsAtHeight(int64(trustedHeight.RevisionHeight) + 1)
+				suite.Require().True(found)
+
+				clientMessage = &ibctm.Misbehaviour{
+					Header2: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height+3, trustedHeight, suite.chainB.CurrentHeader.Time.Add(time.Minute), suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers),
+					Header1: suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, suite.chainB.CurrentHeader.Height, trustedHeight, suite.chainB.CurrentHeader.Time, suite.chainB.Vals, suite.chainB.NextVals, trustedVals, suite.chainB.Signers),
+				}
+			}, true,
+		},
 	}
 
 	for _, tc := range testCases {