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,14 +4,16 @@
 
 Add an entry to the unreleased provider section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a provider release.
 
-* (refactor) [#1294](https://github.com/cosmos/interchain-security/pull/1294) Remove the equivocation proposal handler.
 * (feature!) [#1280](https://github.com/cosmos/interchain-security/pull/1280) provider proposal for changing reward denoms
 * (feature!) [#1244](https://github.com/cosmos/interchain-security/pull/1244) Update the default consumer unbonding period to 2 weeks.
 * (deps) [#1259](https://github.com/cosmos/interchain-security/pull/1259) Bump [cosmos-sdk](https://github.com/cosmos/cosmos-sdk) to [v0.47.5](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.5).
 * (deps!) [#1258](https://github.com/cosmos/interchain-security/pull/1258) Bump [ibc-go](https://github.com/cosmos/ibc-go) to [v7.3.0](https://github.com/cosmos/ibc-go/releases/tag/v7.3.0).
 * (deps) [#1258](https://github.com/cosmos/interchain-security/pull/1258) Bump [cosmos-sdk](https://github.com/cosmos/cosmos-sdk) to [v0.47.4](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.4).
 * (deps!) [#1196](https://github.com/cosmos/interchain-security/pull/1196) Bump [ibc-go](https://github.com/cosmos/ibc-go) to [v7.2.0](https://github.com/cosmos/ibc-go/releases/tag/v7.2.0).
 * `[x/ccv/provider]` (fix) [#1076](https://github.com/cosmos/interchain-security/pull/1076) Add `InitTimeoutTimestamps` and `ExportedVscSendTimestamps` to exported genesis.
+* (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
 
 ### Cryptographic verification of equivocation
 * New feature enabling the provider chain to verify equivocation evidence on its own instead of trusting consumer chains, see [EPIC](https://github.com/cosmos/interchain-security/issues/732).

tests/integration/misbehaviour.go

@@ -9,6 +9,7 @@ import (
 
 	tmtypes "github.com/cometbft/cometbft/types"
 
+	testutil "github.com/cosmos/interchain-security/v3/testutil/crypto"
 	"github.com/cosmos/interchain-security/v3/x/ccv/provider/types"
 )
 
@@ -137,6 +138,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 {
@@ -226,8 +340,8 @@ 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
 				if len(tc.expByzantineValidators) > 0 {
 					equivocatingVals := tc.getMisbehaviour().Header2.ValidatorSet
 					s.Equal(len(equivocatingVals.Validators), len(byzantineValidators))
@@ -267,7 +381,7 @@ 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()]
 
@@ -347,7 +461,16 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
 			},
 			false,
 		},
-		// TODO: Add valset signature test case here
+		// TODO: should pass after 1401 is merged
+		// {
+		// 	"one header of the misbehaviour has insufficient voting power - shouldn't pass",
+		// 	&ibctmtypes.Misbehaviour{
+		// 		ClientId: s.path.EndpointA.ClientID,
+		// 		Header1:  clientHeader,
+		// 		Header2:  clientHeader2,
+		// 	},
+		// 	false,
+		// },
 		{
 			"valid misbehaviour - should pass",
 			&ibctmtypes.Misbehaviour{

testutil/crypto/evidence.go

@@ -3,7 +3,10 @@ package crypto
 import (
 	"time"
 
+	ibctmtypes "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint"
+
 	"github.com/cometbft/cometbft/crypto/tmhash"
+	"github.com/cometbft/cometbft/libs/bytes"
 	tmtypes "github.com/cometbft/cometbft/types"
 )
 
@@ -66,7 +69,6 @@ func MakeAndSignVoteWithForgedValAddress(
 	valAddressSigner tmtypes.PrivValidator,
 	chainID string,
 ) *tmtypes.Vote {
-
 	// create the vote using a different key than the signing key
 	vote, err := tmtypes.MakeVote(
 		blockHeight,
@@ -90,3 +92,44 @@ func MakeAndSignVoteWithForgedValAddress(
 	vote.Signature = v.Signature
 	return vote
 }
+
+// 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 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 valset.Validators {
+		if v.Address.String() == valAddress.String() {
+			v.PubKey = tmtypes.NewMockPV().PrivKey.PubKey()
+		}
+	}
+
+	vs, err := valset.ToProto()
+	if err != nil {
+		panic(err)
+	}
+	header.ValidatorSet = vs
+}

testutil/keeper/expectations.go

@@ -3,7 +3,6 @@ package keeper
 import (
 	time "time"
 
-	math "cosmossdk.io/math"
 	clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
 	conntypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
@@ -12,6 +11,8 @@ import (
 	"github.com/golang/mock/gomock"
 	extra "github.com/oxyno-zeta/gomock-extra-matcher"
 
+	math "cosmossdk.io/math"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
@@ -217,5 +218,4 @@ func GetMocksForSlashValidator(
 			SlashWithInfractionReason(ctx, consAddr, expectedInfractionHeight, expectedSlashPower, slashFraction, stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN).
 			Times(1),
 	}
-
 }

x/ccv/provider/keeper/misbehaviour.go

@@ -104,20 +104,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 {
+	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 {
+	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)
 		}
@@ -182,3 +190,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
+}

x/ccv/provider/keeper/punish_validator_test.go

@@ -412,7 +412,7 @@ func TestSlashValidator(t *testing.T) {
 		mocks.MockStakingKeeper.EXPECT().
 			SlashUnbondingDelegation(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
 			DoAndReturn(
-				func(_ sdk.Context, undelegation stakingtypes.UnbondingDelegation, _ int64, _ sdk.Dec) sdk.Int {
+				func(_ sdk.Context, undelegation stakingtypes.UnbondingDelegation, _ int64, _ sdk.Dec) math.Int {
 					sum := sdk.NewInt(0)
 					for _, r := range undelegation.Entries {
 						if r.IsMature(ctx.BlockTime()) {
@@ -425,7 +425,7 @@ func TestSlashValidator(t *testing.T) {
 		mocks.MockStakingKeeper.EXPECT().
 			SlashRedelegation(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
 			DoAndReturn(
-				func(_ sdk.Context, _ stakingtypes.Validator, redelegation stakingtypes.Redelegation, _ int64, _ sdk.Dec) sdk.Int {
+				func(_ sdk.Context, _ stakingtypes.Validator, redelegation stakingtypes.Redelegation, _ int64, _ sdk.Dec) math.Int {
 					sum := sdk.NewInt(0)
 					for _, r := range redelegation.Entries {
 						if r.IsMature(ctx.BlockTime()) {

x/ccv/provider/types/proposal.go

@@ -9,10 +9,11 @@ import (
 	clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
 
 	errorsmod "cosmossdk.io/errors"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	evidencetypes "github.com/cosmos/cosmos-sdk/x/evidence/types"
 	govv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"
 
-	evidencetypes "github.com/cosmos/cosmos-sdk/x/evidence/types"
 	ccvtypes "github.com/cosmos/interchain-security/v3/x/ccv/types"
 )