diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b21df91be9..8a57b0c155e 100644 --- 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 index 961d9ea69cf..2ef567b70c1 100644 --- 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 index aa458dbc69c..ae7a2dd126b 100644 --- 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 index 366e5787078..81374e6ffb1 100644 --- 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 {