fix!: verify the signatures of byzantine validators in misbehaviour h…

…andling (#1422)

* update byzantine validators extraction

* nits

* Update x/ccv/provider/keeper/misbehaviour.go

Co-authored-by: insumity <[email protected]>

* last changes

* udpdate changelog

---------

Co-authored-by: insumity <[email protected]>

CHANGELOG.md

@@ -4,9 +4,7 @@
 
 Add an entry to the unreleased section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a release.
 
-## v2.3.0-provider-lsm
-* (fix!) [#1404](https://github.com/cosmos/interchain-security/pull/1404) Add conditions to the misbehaviour handling ensuring that validators who vote nil
-are not getting punished.
+* (fix!) [#1422](https://github.com/cosmos/interchain-security/pull/1422) Fix the misbehaviour handling by verifying the signatures of byzantine validators.
 
 ## v2.2.0-provider-lsm
 

tests/integration/misbehaviour.go

@@ -135,6 +135,119 @@ func (s *CCVTestSuite) TestGetByzantineValidators() {
 			nil,
 			false,
 		},
+		{
+			"incorrect valset - shouldn't pass",
+			func() *ibctmtypes.Misbehaviour {
+				clientHeader := s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(clientHeight.RevisionHeight+1),
+					clientHeight,
+					altTime.Add(time.Minute),
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				)
+
+				clientHeaderWithCorruptedValset := s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(clientHeight.RevisionHeight+1),
+					clientHeight,
+					altTime.Add(time.Hour),
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				)
+
+				// change a validator public key in one the second header
+				testutil.CorruptValidatorPubkeyInHeader(clientHeaderWithCorruptedValset, clientTMValset.Validators[0].Address)
+
+				return &ibctmtypes.Misbehaviour{
+					ClientId: s.path.EndpointA.ClientID,
+					Header1:  clientHeader,
+					Header2:  clientHeaderWithCorruptedValset,
+				}
+			},
+			[]*tmtypes.Validator{},
+			false,
+		},
+		{
+			"incorrect valset 2 - shouldn't pass",
+			func() *ibctmtypes.Misbehaviour {
+				clientHeader := s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(clientHeight.RevisionHeight+1),
+					clientHeight,
+					altTime.Add(time.Minute),
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				)
+
+				clientHeaderWithCorruptedSigs := s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(clientHeight.RevisionHeight+1),
+					clientHeight,
+					altTime.Add(time.Hour),
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				)
+
+				// change the valset in the header
+				vs, _ := altValset.ToProto()
+				clientHeader.ValidatorSet.Validators = vs.Validators[:3]
+				clientHeaderWithCorruptedSigs.ValidatorSet.Validators = vs.Validators[:3]
+
+				return &ibctmtypes.Misbehaviour{
+					ClientId: s.path.EndpointA.ClientID,
+					Header1:  clientHeader,
+					Header2:  clientHeaderWithCorruptedSigs,
+				}
+			},
+			[]*tmtypes.Validator{},
+			false,
+		},
+		{
+			"incorrect signatures - shouldn't pass",
+			func() *ibctmtypes.Misbehaviour {
+				clientHeader := s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(clientHeight.RevisionHeight+1),
+					clientHeight,
+					altTime.Add(time.Minute),
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				)
+
+				clientHeaderWithCorruptedSigs := s.consumerChain.CreateTMClientHeader(
+					s.consumerChain.ChainID,
+					int64(clientHeight.RevisionHeight+1),
+					clientHeight,
+					altTime.Add(time.Hour),
+					clientTMValset,
+					clientTMValset,
+					clientTMValset,
+					clientSigners,
+				)
+
+				// change the signature of one of the validator in the header
+				testutil.CorruptCommitSigsInHeader(clientHeaderWithCorruptedSigs, clientTMValset.Validators[0].Address)
+
+				return &ibctmtypes.Misbehaviour{
+					ClientId: s.path.EndpointA.ClientID,
+					Header1:  clientHeader,
+					Header2:  clientHeaderWithCorruptedSigs,
+				}
+			},
+			[]*tmtypes.Validator{},
+			false,
+		},
 		{
 			"light client attack - lunatic attack",
 			func() *ibctmtypes.Misbehaviour {
@@ -212,43 +325,6 @@ 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 {
@@ -262,7 +338,7 @@ func (s *CCVTestSuite) TestGetByzantineValidators() {
 				s.Equal(len(tc.expByzantineValidators), len(byzantineValidators))
 
 				// For both lunatic and equivocation attacks, all the validators
-				// who signed both headers and didn't vote nil should be returned
+				// who signed both headers
 				if len(tc.expByzantineValidators) > 0 {
 					expValset := tmtypes.NewValidatorSet(tc.expByzantineValidators)
 					s.NoError(err)
@@ -299,10 +375,15 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 
 	// create an alternative validator set using more than 1/3 of the trusted validator set
 	altValset := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators[0:2])
-	altSigners := make(map[string]tmtypes.PrivValidator, 1)
+	altSigners := make(map[string]tmtypes.PrivValidator, 2)
 	altSigners[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()]
 	altSigners[clientTMValset.Validators[1].Address.String()] = clientSigners[clientTMValset.Validators[1].Address.String()]
 
+	// create an alternative validator set using less than 1/3 of the trusted validator set
+	altValset2 := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators[0:1])
+	altSigners2 := make(map[string]tmtypes.PrivValidator, 1)
+	altSigners2[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()]
+
 	clientHeader := s.consumerChain.CreateTMClientHeader(
 		s.consumerChain.ChainID,
 		int64(clientHeight.RevisionHeight+1),
@@ -315,19 +396,17 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 	)
 
 	// create a conflicting client header with insufficient voting power
-	// by changing 3/4 of its validators votes to nil
-	clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader(
+	clientHeader2 := 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),
+		altValset2,
+		altValset2,
 		clientTMValset,
-		clientTMValset,
-		clientTMValset,
-		clientSigners,
+		altSigners2,
 	)
-	testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:4])
 
 	testCases := []struct {
 		name         string
@@ -402,7 +481,7 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 			&ibctmtypes.Misbehaviour{
 				ClientId: s.path.EndpointA.ClientID,
 				Header1:  clientHeader,
-				Header2:  clientHeaderWithNilVotes,
+				Header2:  clientHeader2,
 			},
 			false,
 		},

testutil/crypto/evidence.go

@@ -1,11 +1,11 @@
 package crypto
 
 import (
-	"fmt"
 	"time"
 
 	ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types"
 	"github.com/tendermint/tendermint/crypto/tmhash"
+	"github.com/tendermint/tendermint/libs/bytes"
 	tmtypes "github.com/tendermint/tendermint/types"
 )
 
@@ -92,39 +92,43 @@ func MakeAndSignVoteWithForgedValAddress(
 	return vote
 }
 
-// UpdateHeaderCommitWithNilVotes updates the given light client header
-// by changing the commit BlockIDFlag of the given validators to nil
-//
+// CorruptCommitSigsInHeader corrupts the header by changing the value
+// of the commit signature for given validator address.
 // 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)))
-	}
-
+func CorruptCommitSigsInHeader(header *ibctmtypes.Header, valAddress bytes.HexBytes) {
 	commit, err := tmtypes.CommitFromProto(header.Commit)
 	if err != nil {
 		panic(err)
 	}
 
+	for idx, sig := range commit.Signatures {
+		if sig.ValidatorAddress.String() == valAddress.String() {
+			sig.Signature = []byte("randomsig")
+			commit.Signatures[idx] = sig
+		}
+	}
+	// update the commit in client the header
+	header.SignedHeader.Commit = commit.ToProto()
+}
+
+// CorruptValidatorPubkeyInHeader corrupts the header by changing the validator pubkey
+// of the given validator address in the validator set.
+// Note that this method is solely used for testing purposes
+func CorruptValidatorPubkeyInHeader(header *ibctmtypes.Header, valAddress bytes.HexBytes) {
 	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
+	for _, v := range valset.Validators {
+		if v.Address.String() == valAddress.String() {
+			v.PubKey = tmtypes.NewMockPV().PrivKey.PubKey()
 		}
 	}
 
-	// update the commit in client the header
-	header.SignedHeader.Commit = commit.ToProto()
+	vs, err := valset.ToProto()
+	if err != nil {
+		panic(err)
+	}
+	header.ValidatorSet = vs
 }

x/ccv/provider/keeper/misbehaviour.go

@@ -101,20 +101,28 @@ func (k Keeper) GetByzantineValidators(ctx sdk.Context, misbehaviour ibctmtypes.
 	// and return the intersection of validators who signed both
 
 	// create a map with the validators' address that signed header1
-	header1Signers := map[string]struct{}{}
-	for _, sign := range lightBlock1.Commit.Signatures {
-		if !sign.ForBlock() {
+	header1Signers := map[string]int{}
+	for idx, sign := range lightBlock1.Commit.Signatures {
+		if sign.Absent() {
 			continue
 		}
-		header1Signers[sign.ValidatorAddress.String()] = struct{}{}
+		header1Signers[sign.ValidatorAddress.String()] = idx
 	}
 
 	// iterate over the header2 signers and check if they signed header1
-	for _, sign := range lightBlock2.Commit.Signatures {
-		if !sign.ForBlock() {
+	for sigIdxHeader2, sign := range lightBlock2.Commit.Signatures {
+		if sign.Absent() {
 			continue
 		}
-		if _, ok := header1Signers[sign.ValidatorAddress.String()]; ok {
+		if sigIdxHeader1, ok := header1Signers[sign.ValidatorAddress.String()]; ok {
+			if err := verifyLightBlockCommitSig(*lightBlock1, sigIdxHeader1); err != nil {
+				return nil, err
+			}
+
+			if err := verifyLightBlockCommitSig(*lightBlock2, sigIdxHeader2); err != nil {
+				return nil, err
+			}
+
 			_, val := lightBlock1.ValidatorSet.GetByAddress(sign.ValidatorAddress)
 			validators = append(validators, val)
 		}
@@ -192,3 +200,27 @@ func headersStateTransitionsAreConflicting(h1, h2 tmtypes.Header) bool {
 		!bytes.Equal(h1.AppHash, h2.AppHash) ||
 		!bytes.Equal(h1.LastResultsHash, h2.LastResultsHash)
 }
+
+func verifyLightBlockCommitSig(lightBlock tmtypes.LightBlock, sigIdx int) error {
+	// get signature
+	sig := lightBlock.Commit.Signatures[sigIdx]
+
+	// get validator
+	idx, val := lightBlock.ValidatorSet.GetByAddress(sig.ValidatorAddress)
+	if idx == -1 {
+		return fmt.Errorf("incorrect signature: validator address %s isn't part of the validator set", sig.ValidatorAddress.String())
+	}
+
+	// verify validator pubkey corresponds to signature validator address
+	if !bytes.Equal(val.PubKey.Address(), sig.ValidatorAddress) {
+		return fmt.Errorf("validator public key doesn't correspond to signature validator address: %s!= %s", val.PubKey.Address(), sig.ValidatorAddress)
+	}
+
+	// validate signature
+	voteSignBytes := lightBlock.Commit.VoteSignBytes(lightBlock.ChainID, int32(sigIdx))
+	if !val.PubKey.VerifySignature(voteSignBytes, sig.Signature) {
+		return fmt.Errorf("wrong signature (#%d): %X", sigIdx, sig.Signature)
+	}
+
+	return nil
+}