fix!: add check for the height of evidence (backport #2007) (#2027)

* fix!: add check for the height of evidence (#2007) * init commit * added check, setting, and deleting of the equivocation min height * update changelog entry * remove unwwanted changelog entry --------- Co-authored-by: insumity <[email protected]> (cherry picked from commit de9db96) * fix errors from merge * add fix entry to changelog * fix changelog entry * fix changelog 2 --------- Co-authored-by: Simon Noetzlin <[email protected]>
cosmos · Jul 15, 2024 · 16ea5c7 · 16ea5c7
1 parent 9ee0312
commit 16ea5c7
Show file tree

Hide file tree

Showing 5 changed files with 146 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,8 +2,6 @@
 
 ## v5.0.0
 
-❗ *The provider module should not be used with this version.*
-
 *May 9, 2024*
 
 ### DEPENDENCIES
@@ -40,6 +38,74 @@
 ([\#1698](https://github.com/cosmos/interchain-security/pull/1698))
 - Revert `PutUnbondingOnHold` behavior to ICS@v1
 ([\#1819](https://github.com/cosmos/interchain-security/pull/1819))
+
+## v4.3.1
+
+*July 4, 2024*
+
+### BUG FIXES
+
+- [Provider](x/ccv/provider)
+  - Add missing check for the minimum height of evidence in the consumer double-vote handler.
+    [#2007](https://github.com/cosmos/interchain-security/pull/2007)
+
+### STATE BREAKING
+
+- [Provider](x/ccv/provider)
+  - Add missing check for the minimum height of evidence in the consumer double-vote handler.
+    [#2007](https://github.com/cosmos/interchain-security/pull/2007)
+
+## v4.3.0
+
+*June 20, 2024*
+
+### BUG FIXES
+
+- General
+  - Write unbonding period advisory to stderr instead of stdout
+    ([\#1921](https://github.com/cosmos/interchain-security/pull/1921))
+- [Provider](x/ccv/provider)
+  - Apply audit suggestions that include a bug fix in the way we compute the
+    maximum capped power.
+    ([\#1925](https://github.com/cosmos/interchain-security/pull/1925))
+  - Replace `GetAllConsumerChains` with lightweight version
+    (`GetAllRegisteredConsumerChainIDs`) that doesn't call into the staking module
+    ([\#1946](https://github.com/cosmos/interchain-security/pull/1946))
+
+### DEPENDENCIES
+
+- Bump [ibc-go](https://github.com/cosmos/ibc-go) to
+  [v7.6.0](https://github.com/cosmos/ibc-go/releases/tag/v7.6.0).
+  ([\#1974](https://github.com/cosmos/interchain-security/pull/1974))
+
+### FEATURES
+
+- [Provider](x/ccv/provider)
+  - Allow consumer chains to change their PSS parameters.
+    ([\#1932](https://github.com/cosmos/interchain-security/pull/1932))
+
+### IMPROVEMENTS
+
+- [Provider](x/ccv/provider)
+  - Only start distributing rewards to validators after they have been validating
+    for a fixed number of blocks. Introduces the `NumberOfEpochsToStartReceivingRewards` param.
+    ([\#1929](https://github.com/cosmos/interchain-security/pull/1929))
+
+### STATE BREAKING
+
+- General
+  - Bump [ibc-go](https://github.com/cosmos/ibc-go) to
+    [v7.6.0](https://github.com/cosmos/ibc-go/releases/tag/v7.6.0).
+    ([\#1974](https://github.com/cosmos/interchain-security/pull/1974))
+- [Provider](x/ccv/provider)
+  - Apply audit suggestions that include a bug fix in the way we compute the
+    maximum capped power. ([\#1925](https://github.com/cosmos/interchain-security/pull/1925))
+  - Only start distributing rewards to validators after they have been validating
+    for a fixed number of blocks. Introduces the `NumberOfEpochsToStartReceivingRewards` param.
+    ([\#1929](https://github.com/cosmos/interchain-security/pull/1929))
+  - Allow consumer chains to change their PSS parameters.
+    ([\#1932](https://github.com/cosmos/interchain-security/pull/1932))
+
 ## v4.2.0
 
 May 17, 2024

diff --git a/tests/integration/double_vote.go b/tests/integration/double_vote.go
@@ -87,9 +87,9 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
 		s.consumerChain.ChainID,
 	)
 
-	// create a vote using the consumer validator key
-	// with block height that is smaller than the equivocation evidence min height
-	consuVoteOld := testutil.MakeAndSignVote(
+	// create two votes using the consumer validator key that both have
+	// the same block height that is smaller than the equivocation evidence min height
+	consuVoteOld1 := testutil.MakeAndSignVote(
 		blockID1,
 		int64(equivocationEvidenceMinHeight-1),
 		s.consumerCtx().BlockTime(),
@@ -98,6 +98,15 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
 		s.consumerChain.ChainID,
 	)
 
+	consuVoteOld2 := testutil.MakeAndSignVote(
+		blockID2,
+		int64(equivocationEvidenceMinHeight-1),
+		s.consumerCtx().BlockTime(),
+		consuValSet,
+		consuSigner,
+		s.consumerChain.ChainID,
+	)
+
 	testCases := []struct {
 		name    string
 		ev      *tmtypes.DuplicateVoteEvidence
@@ -121,8 +130,8 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
 		{
 			"evidence is older than equivocation evidence min height - shouldn't pass",
 			&tmtypes.DuplicateVoteEvidence{
-				VoteA:            consuVoteOld,
-				VoteB:            consuBadVote,
+				VoteA:            consuVoteOld1,
+				VoteB:            consuVoteOld2,
 				ValidatorPower:   consuVal.VotingPower,
 				TotalVotingPower: consuVal.VotingPower,
 				Timestamp:        s.consumerCtx().BlockTime(),
@@ -135,7 +144,7 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
 			"the votes in the evidence are for different height - shouldn't pass",
 			&tmtypes.DuplicateVoteEvidence{
 				VoteA:            consuVote,
-				VoteB:            consuVoteOld,
+				VoteB:            consuVoteOld1,
 				ValidatorPower:   consuVal.VotingPower,
 				TotalVotingPower: consuVal.VotingPower,
 				Timestamp:        s.consumerCtx().BlockTime(),

diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go
@@ -511,7 +511,16 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 					clientTMValset,
 					altSigners,
 				),
-				Header2: clientHeader,
+				Header2: s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(equivocationEvidenceMinHeight-1),
+					clientHeight,
+					headerTs,
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				),
 			},
 			false,
 		},

diff --git a/x/ccv/provider/keeper/consumer_equivocation.go b/x/ccv/provider/keeper/consumer_equivocation.go
@@ -36,6 +36,27 @@ func (k Keeper) HandleConsumerDoubleVoting(
 	chainID string,
 	pubkey cryptotypes.PubKey,
 ) error {
+	// check that the evidence is for an ICS consumer chain
+	if _, found := k.GetConsumerClientId(ctx, chainID); !found {
+		return errorsmod.Wrapf(
+			ccvtypes.ErrInvalidDoubleVotingEvidence,
+			"cannot find consumer chain %s",
+			chainID,
+		)
+	}
+
+	// check that the evidence is not too old
+	minHeight := k.GetEquivocationEvidenceMinHeight(ctx, chainID)
+	if uint64(evidence.VoteA.Height) < minHeight {
+		return errorsmod.Wrapf(
+			ccvtypes.ErrInvalidDoubleVotingEvidence,
+			"evidence for consumer chain %s is too old - evidence height (%d), min (%d)",
+			chainID,
+			evidence.VoteA.Height,
+			minHeight,
+		)
+	}
+
 	// verifies the double voting evidence using the consumer chain public key
 	if err := k.VerifyDoubleVotingEvidence(*evidence, chainID, pubkey); err != nil {
 		return err
@@ -270,34 +291,51 @@ func headerToLightBlock(h ibctmtypes.Header) (*tmtypes.LightBlock, error) {
 }
 
 // CheckMisbehaviour checks that headers in the given misbehaviour forms
-// a valid light client attack on a light client that tracks an ICS consumer chain
+// a valid light client attack from an ICS consumer chain and that the light client isn't expired
 func (k Keeper) CheckMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbehaviour) error {
+	consumerChainID := misbehaviour.Header1.Header.ChainID
+
 	// check that the misbehaviour is for an ICS consumer chain
-	clientId, found := k.GetConsumerClientId(ctx, misbehaviour.Header1.Header.ChainID)
+	clientId, found := k.GetConsumerClientId(ctx, consumerChainID)
 	if !found {
-		return fmt.Errorf("incorrect misbehaviour with conflicting headers from a non-existent consumer chain: %s", misbehaviour.Header1.Header.ChainID)
+		return fmt.Errorf("incorrect misbehaviour with conflicting headers from a non-existent consumer chain: %s", consumerChainID)
 	} else if misbehaviour.ClientId != clientId {
 		return fmt.Errorf("incorrect misbehaviour: expected client ID for consumer chain %s is %s got %s",
-			misbehaviour.Header1.Header.ChainID,
+			consumerChainID,
 			clientId,
 			misbehaviour.ClientId,
 		)
 	}
 
+	// 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
+	if !misbehaviour.Header1.GetHeight().EQ(misbehaviour.Header2.GetHeight()) {
+		return errorsmod.Wrapf(ibcclienttypes.ErrInvalidMisbehaviour, "headers are not at same height")
+	}
+
+	// Check that the evidence is not too old
+	minHeight := k.GetEquivocationEvidenceMinHeight(ctx, consumerChainID)
+	evidenceHeight := misbehaviour.Header1.GetHeight().GetRevisionHeight()
+	// Note that the revision number is not relevant for checking the age of evidence
+	// as it's already part of the chain ID and the minimum height is mapped to chain IDs
+	if evidenceHeight < minHeight {
+		return errorsmod.Wrapf(
+			ccvtypes.ErrInvalidDoubleVotingEvidence,
+			"evidence for consumer chain %s is too old - evidence height (%d), min (%d)",
+			consumerChainID,
+			evidenceHeight,
+			minHeight,
+		)
+	}
+
 	clientState, found := k.clientKeeper.GetClientState(ctx, clientId)
 	if !found {
 		return errorsmod.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot find client state for client with ID %s", clientId)
 	}
 
 	clientStore := k.clientKeeper.ClientStore(ctx, clientId)
 
-	// 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 CheckForMisbehaviour in ibc-go/blob/v7.3.0/modules/light-clients/07-tendermint/misbehaviour_handle.go#L73
-	if !misbehaviour.Header1.GetHeight().EQ(misbehaviour.Header2.GetHeight()) {
-		return errorsmod.Wrap(ibcclienttypes.ErrInvalidMisbehaviour, "headers are not at same height")
-	}
-
 	// CheckForMisbehaviour verifies that the headers have different blockID hashes
 	ok := clientState.CheckForMisbehaviour(ctx, k.cdc, clientStore, &misbehaviour)
 	if !ok {

diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go
@@ -96,6 +96,9 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, prop *types.ConsumerAdditi
 			fmt.Sprintf("cannot create client for existent consumer chain: %s", chainID))
 	}
 
+	// Set minimum height for equivocation evidence from this consumer chain
+	k.SetEquivocationEvidenceMinHeight(ctx, chainID, prop.InitialHeight.RevisionHeight)
+
 	// Consumers start out with the unbonding period from the consumer addition prop
 	consumerUnbondingPeriod := prop.UnbondingPeriod
 
@@ -178,6 +181,7 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, closeChan boo
 	// Note: this call panics if the key assignment state is invalid
 	k.DeleteKeyAssignments(ctx, chainID)
 	k.DeleteMinimumPowerInTopN(ctx, chainID)
+	k.DeleteEquivocationEvidenceMinHeight(ctx, chainID)
 
 	// close channel and delete the mappings between chain ID and channel ID
 	if channelID, found := k.GetChainToChannel(ctx, chainID); found {