From de9db96c53b86a766d5682db7975e0450216a448 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Wed, 3 Jul 2024 16:33:07 +0200 Subject: [PATCH] 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 --- .../2007-evidence-min-height-filter.md | 2 + .../2007-evidence-min-height-filter.md | 2 + tests/integration/double_vote.go | 21 +++++-- tests/integration/misbehaviour.go | 11 +++- .../provider/keeper/consumer_equivocation.go | 61 +++++++++++++++---- x/ccv/provider/keeper/proposal.go | 4 ++ 6 files changed, 83 insertions(+), 18 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md create mode 100644 .changelog/unreleased/state-breaking/provider/2007-evidence-min-height-filter.md diff --git a/.changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md b/.changelog/unreleased/bug-fixes/provider/2007-evidence-min-height-filter.md new file mode 100644 index 0000000000..def0681a0a --- /dev/null +++ b/.changelog/unreleased/bug-fixes/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) 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) 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 b8e19e6d9c..e6fc74072f 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 { diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index b0cccc36fb..dc769b152e 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 @@ -219,6 +222,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 {