fix!: drop nil votes in misbehaviour handling (#1404)

* update CHANGELOG for final release * save * update test to extract byzantine validators * improve testing * nits * nits * Update tests/integration/misbehaviour.go Co-authored-by: insumity <[email protected]> * Update testutil/crypto/evidence.go Co-authored-by: insumity <[email protected]> * update util func * doc * check misb client ID * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: insumity <[email protected]> * nits --------- Co-authored-by: insumity <[email protected]>
cosmos · Nov 10, 2023 · fb9f729 · fb9f729
1 parent de0c0fa
commit fb9f729
Show file tree

Hide file tree

Showing 3 changed files with 148 additions and 73 deletions.
diff --git a/tests/integration/misbehaviour.go b/tests/integration/misbehaviour.go
@@ -5,6 +5,7 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types"
+	testutil "github.com/cosmos/interchain-security/v2/testutil/crypto"
 	"github.com/cosmos/interchain-security/v2/x/ccv/provider/types"
 	tmtypes "github.com/tendermint/tendermint/types"
 )
@@ -211,6 +212,43 @@ func (s *CCVTestSuite) TestGetByzantineValidators() {
 			[]*tmtypes.Validator{},
 			true,
 		},
+		{
+			"validators who did vote nil should not be returned",
+			func() *ibctmtypes.Misbehaviour {
+				clientHeader := s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(clientHeight.RevisionHeight+1),
+					clientHeight,
+					altTime.Add(time.Minute),
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				)
+
+				// create conflicting header with 2/4 validators voting nil
+				clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(clientHeight.RevisionHeight+1),
+					clientHeight,
+					altTime.Add(time.Hour),
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				)
+				testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:2])
+
+				return &ibctmtypes.Misbehaviour{
+					ClientId: s.path.EndpointA.ClientID,
+					Header1:  clientHeader,
+					Header2:  clientHeaderWithNilVotes,
+				}
+			},
+			// Expect validators who did NOT vote nil
+			clientTMValset.Validators[2:],
+			true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -223,21 +261,17 @@ func (s *CCVTestSuite) TestGetByzantineValidators() {
 				s.NoError(err)
 				s.Equal(len(tc.expByzantineValidators), len(byzantineValidators))
 
-				// For both lunatic and equivocation attacks all the validators
-				// who signed the bad header (Header2) should be in returned in the evidence
+				// For both lunatic and equivocation attacks, all the validators
+				// who signed both headers and didn't vote nil should be returned
 				if len(tc.expByzantineValidators) > 0 {
-					equivocatingVals := tc.getMisbehaviour().Header2.ValidatorSet
-					s.Equal(len(equivocatingVals.Validators), len(byzantineValidators))
-
-					vs, err := tmtypes.ValidatorSetFromProto(equivocatingVals)
+					expValset := tmtypes.NewValidatorSet(tc.expByzantineValidators)
 					s.NoError(err)
 
 					for _, v := range tc.expByzantineValidators {
-						idx, _ := vs.GetByAddress(v.Address)
+						idx, _ := expValset.GetByAddress(v.Address)
 						s.True(idx >= 0)
 					}
 				}
-
 			} else {
 				s.Error(err)
 			}
@@ -268,27 +302,53 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 	altSigners := make(map[string]tmtypes.PrivValidator, 1)
 	altSigners[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()]
 	altSigners[clientTMValset.Validators[1].Address.String()] = clientSigners[clientTMValset.Validators[1].Address.String()]
+
+	clientHeader := s.consumerChain.CreateTMClientHeader(
+		s.consumerChain.ChainID,
+		int64(clientHeight.RevisionHeight+1),
+		clientHeight,
+		headerTs,
+		clientTMValset,
+		clientTMValset,
+		clientTMValset,
+		clientSigners,
+	)
+
+	// create a conflicting client header with insufficient voting power
+	// by changing 3/4 of its validators votes to nil
+	clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader(
+		s.consumerChain.ChainID,
+		int64(clientHeight.RevisionHeight+1),
+		clientHeight,
+		// use a different block time to change the header BlockID
+		headerTs.Add(time.Hour),
+		clientTMValset,
+		clientTMValset,
+		clientTMValset,
+		clientSigners,
+	)
+	testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:4])
+
 	testCases := []struct {
 		name         string
 		misbehaviour *ibctmtypes.Misbehaviour
 		expPass      bool
 	}{
 		{
-			"client state not found - shouldn't pass",
+			"identical headers - shouldn't pass",
 			&ibctmtypes.Misbehaviour{
-				ClientId: "clientID",
+				ClientId: s.path.EndpointA.ClientID,
+				Header1:  clientHeader,
+				Header2:  clientHeader,
+			},
+			false,
+		},
+		{
+			"misbehaviour isn't for a consumer chain - shouldn't pass",
+			&ibctmtypes.Misbehaviour{
+				ClientId: s.path.EndpointA.ClientID,
 				Header1: s.consumerChain.CreateTMClientHeader(
-					s.consumerChain.ChainID,
-					int64(clientHeight.RevisionHeight+1),
-					clientHeight,
-					headerTs,
-					clientTMValset,
-					clientTMValset,
-					clientTMValset,
-					clientSigners,
-				),
-				Header2: s.consumerChain.CreateTMClientHeader(
-					s.consumerChain.ChainID,
+					"aChainID",
 					int64(clientHeight.RevisionHeight+1),
 					clientHeight,
 					headerTs,
@@ -297,13 +357,15 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 					clientTMValset,
 					altSigners,
 				),
+				Header2: clientHeader,
 			},
 			false,
 		},
 		{
-			"invalid misbehaviour with empty header1 - shouldn't pass",
+			"client ID doesn't correspond to the client ID of consumer chain  - shouldn't pass",
 			&ibctmtypes.Misbehaviour{
-				Header1: &ibctmtypes.Header{},
+				ClientId: "clientID",
+				Header1:  clientHeader,
 				Header2: s.consumerChain.CreateTMClientHeader(
 					s.consumerChain.ChainID,
 					int64(clientHeight.RevisionHeight+1),
@@ -321,16 +383,7 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 			"invalid misbehaviour with different header height  - shouldn't pass",
 			&ibctmtypes.Misbehaviour{
 				ClientId: s.path.EndpointA.ClientID,
-				Header1: s.consumerChain.CreateTMClientHeader(
-					s.consumerChain.ChainID,
-					int64(clientHeight.RevisionHeight+1),
-					clientHeight,
-					headerTs,
-					clientTMValset,
-					clientTMValset,
-					clientTMValset,
-					clientSigners,
-				),
+				Header1:  clientHeader,
 				Header2: s.consumerChain.CreateTMClientHeader(
 					s.consumerChain.ChainID,
 					int64(clientHeight.RevisionHeight+2),
@@ -345,49 +398,20 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 			false,
 		},
 		{
-			"valid misbehaviour - should pass",
+			"one header of the misbehaviour has insufficient voting power - shouldn't pass",
 			&ibctmtypes.Misbehaviour{
 				ClientId: s.path.EndpointA.ClientID,
-				Header1: s.consumerChain.CreateTMClientHeader(
-					s.consumerChain.ChainID,
-					int64(clientHeight.RevisionHeight+1),
-					clientHeight,
-					headerTs,
-					clientTMValset,
-					clientTMValset,
-					clientTMValset,
-					clientSigners,
-				),
-				// create header using a different validator set
-				Header2: s.consumerChain.CreateTMClientHeader(
-					s.consumerChain.ChainID,
-					int64(clientHeight.RevisionHeight+1),
-					clientHeight,
-					headerTs,
-					altValset,
-					altValset,
-					clientTMValset,
-					altSigners,
-				),
+				Header1:  clientHeader,
+				Header2:  clientHeaderWithNilVotes,
 			},
-			true,
+			false,
 		},
 		{
-			"valid misbehaviour with already frozen client - should pass",
+			"valid misbehaviour - should pass",
 			&ibctmtypes.Misbehaviour{
 				ClientId: s.path.EndpointA.ClientID,
-				Header1: s.consumerChain.CreateTMClientHeader(
-					s.consumerChain.ChainID,
-					int64(clientHeight.RevisionHeight+1),
-					clientHeight,
-					headerTs,
-					clientTMValset,
-					clientTMValset,
-					clientTMValset,
-					clientSigners,
-				),
-				// the resulting Header2 will have a different BlockID
-				// than Header1 since doesn't share the same valset and signers
+				Header1:  clientHeader,
+				// create header using a different validator set
 				Header2: s.consumerChain.CreateTMClientHeader(
 					s.consumerChain.ChainID,
 					int64(clientHeight.RevisionHeight+1),

diff --git a/testutil/crypto/evidence.go b/testutil/crypto/evidence.go
@@ -1,8 +1,10 @@
 package crypto
 
 import (
+	"fmt"
 	"time"
 
+	ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types"
 	"github.com/tendermint/tendermint/crypto/tmhash"
 	tmtypes "github.com/tendermint/tendermint/types"
 )
@@ -89,3 +91,40 @@ func MakeAndSignVoteWithForgedValAddress(
 	vote.Signature = v.Signature
 	return vote
 }
+
+// UpdateHeaderCommitWithNilVotes updates the given light client header
+// by changing the commit BlockIDFlag of the given validators to nil
+//
+// Note that this method is solely used for testing purposes
+func UpdateHeaderCommitWithNilVotes(header *ibctmtypes.Header, validators []*tmtypes.Validator) {
+	if len(validators) > len(header.ValidatorSet.Validators) {
+		panic(fmt.Sprintf("cannot change more than %d validators votes: got %d",
+			len(header.ValidatorSet.Validators), len(header.ValidatorSet.Validators)))
+	}
+
+	commit, err := tmtypes.CommitFromProto(header.Commit)
+	if err != nil {
+		panic(err)
+	}
+
+	valset, err := tmtypes.ValidatorSetFromProto(header.ValidatorSet)
+	if err != nil {
+		panic(err)
+	}
+
+	for _, v := range validators {
+		// get validator index in valset
+		idx, _ := valset.GetByAddress(v.Address)
+		if idx != -1 {
+			// get validator commit sig
+			s := commit.Signatures[idx]
+			// change BlockIDFlag to nil
+			s.BlockIDFlag = tmtypes.BlockIDFlagNil
+			// update the signatures
+			commit.Signatures[idx] = s
+		}
+	}
+
+	// update the commit in client the header
+	header.SignedHeader.Commit = commit.ToProto()
+}
diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go
@@ -103,15 +103,15 @@ func (k Keeper) GetByzantineValidators(ctx sdk.Context, misbehaviour ibctmtypes.
 	// create a map with the validators' address that signed header1
 	header1Signers := map[string]struct{}{}
 	for _, sign := range lightBlock1.Commit.Signatures {
-		if sign.Absent() {
+		if !sign.ForBlock() {
 			continue
 		}
 		header1Signers[sign.ValidatorAddress.String()] = struct{}{}
 	}
 
 	// iterate over the header2 signers and check if they signed header1
 	for _, sign := range lightBlock2.Commit.Signatures {
-		if sign.Absent() {
+		if !sign.ForBlock() {
 			continue
 		}
 		if _, ok := header1Signers[sign.ValidatorAddress.String()]; ok {
@@ -142,9 +142,21 @@ func headerToLightBlock(h ibctmtypes.Header) (*tmtypes.LightBlock, error) {
 }
 
 // CheckMisbehaviour checks that headers in the given misbehaviour forms
-// a valid light client attack and that the corresponding light client isn't expired
+// 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 {
-	clientState, found := k.clientKeeper.GetClientState(ctx, misbehaviour.GetClientID())
+	// check that the misbehaviour is for an ICS consumer chain
+	clientId, found := k.GetConsumerClientId(ctx, misbehaviour.Header1.Header.ChainID)
+	if !found {
+		return fmt.Errorf("incorrect misbehaviour with conflicting headers from a non-existent consumer chain: %s", misbehaviour.Header1.Header.ChainID)
+	} 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,
+			clientId,
+			misbehaviour.ClientId,
+		)
+	}
+
+	clientState, found := k.clientKeeper.GetClientState(ctx, clientId)
 	if !found {
 		return sdkerrors.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID())
 	}