From 17ebdda7f3c253d918090b860f9bf9d17889f061 Mon Sep 17 00:00:00 2001 From: insumity Date: Wed, 26 Jun 2024 14:38:31 +0200 Subject: [PATCH 1/5] init commit --- tests/integration/double_vote.go | 21 +++++-- tests/integration/misbehaviour.go | 11 +++- .../provider/keeper/consumer_equivocation.go | 61 +++++++++++++++---- 3 files changed, 75 insertions(+), 18 deletions(-) diff --git a/tests/integration/double_vote.go b/tests/integration/double_vote.go index c22ba3b063..5861982b23 100644 --- a/tests/integration/double_vote.go +++ b/tests/integration/double_vote.go @@ -86,9 +86,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(), @@ -97,6 +97,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 @@ -120,8 +129,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(), @@ -134,7 +143,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 index acdcd0071c..1200ce7293 100644 --- a/tests/integration/misbehaviour.go +++ b/tests/integration/misbehaviour.go @@ -509,7 +509,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 index 8fc9808304..97bb2a6aac 100644 --- a/x/ccv/provider/keeper/consumer_equivocation.go +++ b/x/ccv/provider/keeper/consumer_equivocation.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/binary" "fmt" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ibcclienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" ibctmtypes "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint" @@ -35,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 @@ -269,20 +291,44 @@ 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 sdkerrors.Wrap(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) @@ -290,13 +336,6 @@ func (k Keeper) CheckMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbe 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 { From c8c7b19372eb86f7710ef0cbd16732d9292ff5c9 Mon Sep 17 00:00:00 2001 From: insumity Date: Wed, 26 Jun 2024 15:16:44 +0200 Subject: [PATCH 2/5] added check, setting, and deleting of the equivocation min height --- .../state-breaking/provider/xxx-evidence-min-height-filter | 2 ++ .../v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter | 2 ++ x/ccv/provider/keeper/proposal.go | 4 ++++ 3 files changed, 8 insertions(+) create mode 100644 .changelog/unreleased/state-breaking/provider/xxx-evidence-min-height-filter create mode 100644 .changelog/v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter diff --git a/.changelog/unreleased/state-breaking/provider/xxx-evidence-min-height-filter b/.changelog/unreleased/state-breaking/provider/xxx-evidence-min-height-filter new file mode 100644 index 0000000000..3171d79105 --- /dev/null +++ b/.changelog/unreleased/state-breaking/provider/xxx-evidence-min-height-filter @@ -0,0 +1,2 @@ +- Add missing check for the minimum height of evidence in the consumer double-vote handler. +[#xxx](https://github.com/cosmos/interchain-security/pull/xxx) diff --git a/.changelog/v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter b/.changelog/v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter new file mode 100644 index 0000000000..3171d79105 --- /dev/null +++ b/.changelog/v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter @@ -0,0 +1,2 @@ +- Add missing check for the minimum height of evidence in the consumer double-vote handler. +[#xxx](https://github.com/cosmos/interchain-security/pull/xxx) diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index e112e6b761..505fffc077 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -58,6 +58,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 @@ -163,6 +166,7 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, closeChan boo k.DeleteInitTimeoutTimestamp(ctx, chainID) // Note: this call panics if the key assignment state is invalid k.DeleteKeyAssignments(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 { From ccb9ec322d92a8699f69fcd81998bbbb0b991964 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Wed, 3 Jul 2024 15:18:32 +0200 Subject: [PATCH 3/5] update changelog entry --- .../provider/2007-evidence-min-height-filter.md} | 2 +- .../state-breaking/provider/2007-evidence-min-height-filter.md | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) rename .changelog/unreleased/{state-breaking/provider/xxx-evidence-min-height-filter => bug-fixes/provider/2007-evidence-min-height-filter.md} (58%) create mode 100644 .changelog/unreleased/state-breaking/provider/2007-evidence-min-height-filter.md diff --git a/.changelog/unreleased/state-breaking/provider/xxx-evidence-min-height-filter b/.changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md similarity index 58% rename from .changelog/unreleased/state-breaking/provider/xxx-evidence-min-height-filter rename to .changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md index 3171d79105..def0681a0a 100644 --- a/.changelog/unreleased/state-breaking/provider/xxx-evidence-min-height-filter +++ b/.changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md @@ -1,2 +1,2 @@ - Add missing check for the minimum height of evidence in the consumer double-vote handler. -[#xxx](https://github.com/cosmos/interchain-security/pull/xxx) +[#2007](https://github.com/cosmos/interchain-security/pull/2007) diff --git a/.changelog/unreleased/state-breaking/provider/2007-evidence-min-height-filter.md b/.changelog/unreleased/state-breaking/provider/2007-evidence-min-height-filter.md new file mode 100644 index 0000000000..def0681a0a --- /dev/null +++ b/.changelog/unreleased/state-breaking/provider/2007-evidence-min-height-filter.md @@ -0,0 +1,2 @@ +- Add missing check for the minimum height of evidence in the consumer double-vote handler. +[#2007](https://github.com/cosmos/interchain-security/pull/2007) From 145c8f5ee8dd01121e1e1f2b50838a3fe6a5a4d8 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Wed, 3 Jul 2024 15:25:15 +0200 Subject: [PATCH 4/5] remove unwwanted changelog entry --- .../v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter | 2 -- 1 file changed, 2 deletions(-) delete mode 100644 .changelog/v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter diff --git a/.changelog/v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter b/.changelog/v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter deleted file mode 100644 index 3171d79105..0000000000 --- a/.changelog/v4.3.0/bug-fixes/provider/xxx-evidence-min-height-filter +++ /dev/null @@ -1,2 +0,0 @@ -- Add missing check for the minimum height of evidence in the consumer double-vote handler. -[#xxx](https://github.com/cosmos/interchain-security/pull/xxx) From f0431234fee17d6d4cf2e56af4cc6323a3ed7ee9 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Wed, 3 Jul 2024 16:49:37 +0200 Subject: [PATCH 5/5] update changelog entries --- ...-min-height-filter.md => 2008-evidence-min-height-filter.md} | 2 +- ...-min-height-filter.md => 2008-evidence-min-height-filter.md} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename .changelog/unreleased/bug-fixes/provider/{2007-evidence-min-height-filter.md => 2008-evidence-min-height-filter.md} (59%) rename .changelog/unreleased/state-breaking/provider/{2007-evidence-min-height-filter.md => 2008-evidence-min-height-filter.md} (59%) diff --git a/.changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md b/.changelog/unreleased/bug-fixes/provider/2008-evidence-min-height-filter.md similarity index 59% rename from .changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md rename to .changelog/unreleased/bug-fixes/provider/2008-evidence-min-height-filter.md index def0681a0a..902d844adb 100644 --- a/.changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md +++ b/.changelog/unreleased/bug-fixes/provider/2008-evidence-min-height-filter.md @@ -1,2 +1,2 @@ - Add missing check for the minimum height of evidence in the consumer double-vote handler. -[#2007](https://github.com/cosmos/interchain-security/pull/2007) +[#2008](https://github.com/cosmos/interchain-security/pull/2008) diff --git a/.changelog/unreleased/state-breaking/provider/2007-evidence-min-height-filter.md b/.changelog/unreleased/state-breaking/provider/2008-evidence-min-height-filter.md similarity index 59% rename from .changelog/unreleased/state-breaking/provider/2007-evidence-min-height-filter.md rename to .changelog/unreleased/state-breaking/provider/2008-evidence-min-height-filter.md index def0681a0a..902d844adb 100644 --- a/.changelog/unreleased/state-breaking/provider/2007-evidence-min-height-filter.md +++ b/.changelog/unreleased/state-breaking/provider/2008-evidence-min-height-filter.md @@ -1,2 +1,2 @@ - Add missing check for the minimum height of evidence in the consumer double-vote handler. -[#2007](https://github.com/cosmos/interchain-security/pull/2007) +[#2008](https://github.com/cosmos/interchain-security/pull/2008)