Fixes #503 prevents two key assignment key overlap security issues (#556

)

* Deletes out of date duplicate code

* Adds check that validator with key does not already exist

* Partially adjust assign unit test

* Finishes adjusting unit

* Updates stress test to never find a validator

* Improves comment

* Fixes handler_test

* Adds validatorI iterator to expected keeper

* Implements AfterValidatorCreated hook

* Names

* Simplifies validator query

* Adds hooks test

* Remove TODO

* Fix random sim test

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

x/ccv/provider/handler_test.go

@@ -16,7 +16,6 @@ import (
 	"github.com/cosmos/interchain-security/x/ccv/provider"
 	keeper "github.com/cosmos/interchain-security/x/ccv/provider/keeper"
 	"github.com/cosmos/interchain-security/x/ccv/provider/types"
-	"github.com/cosmos/interchain-security/x/ccv/utils"
 )
 
 func TestInvalidMsg(t *testing.T) {
@@ -50,6 +49,9 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) {
 						ctx, testValProvider.SDKValAddress(),
 						// Return a valid validator, found!
 					).Return(testValProvider.SDKStakingValidator(), true).Times(1),
+					mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx,
+						testValConsumer.SDKConsAddress(),
+					).Return(stakingtypes.Validator{}, false),
 				)
 			},
 			expError: false,
@@ -76,13 +78,16 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) {
 				k keeper.Keeper, mocks testkeeper.MockedKeepers) {
 
 				// Use the consumer key already
-				pAddr := utils.TMCryptoPublicKeyToConsAddr(testValProvider.TMProtoCryptoPublicKey())
-				k.SetValidatorConsumerPubKey(ctx, "chainid", pAddr, testValConsumer.TMProtoCryptoPublicKey())
+				k.SetValidatorByConsumerAddr(ctx, "chainid", testValConsumer.SDKConsAddress(), testValProvider.SDKConsAddress())
 
 				gomock.InOrder(
 					mocks.MockStakingKeeper.EXPECT().GetValidator(
 						ctx, testValProvider.SDKValAddress(),
-					).Return(stakingtypes.Validator{}, false).Times(1),
+						// Return a valid validator, found!
+					).Return(testValProvider.SDKStakingValidator(), true).Times(1),
+					mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx,
+						testValConsumer.SDKConsAddress(),
+					).Return(stakingtypes.Validator{}, false),
 				)
 			},
 			expError: true,

x/ccv/provider/keeper/hooks.go

@@ -64,8 +64,43 @@ func (h Hooks) AfterUnbondingInitiated(ctx sdk.Context, ID uint64) error {
 	return nil
 }
 
-// Define unimplemented methods to satisfy the StakingHooks contract
+func ValidatorConsensusKeyInUse(k *Keeper, ctx sdk.Context, valAddr sdk.ValAddress) bool {
+	// Find the validator being added in the staking module
+	// This is necessary because only the operator address is received
+	// as argument at it is not possible to directly query the validator using
+	// the operator address. Nor is it possible to convert an operator addr
+	// to a consensus addr.
+	val, found := k.stakingKeeper.GetValidator(ctx, valAddr)
+	if !found {
+		panic("did not find newly created validator in staking module")
+	}
+
+	// Get the consensus address of the validator being added
+	cons, err := val.GetConsAddr()
+	if err != nil {
+		panic("could not get validator cons addr ")
+	}
+
+	inUse := false
+
+	// Search over all consumer keys which have been assigned in order to
+	// check if the validator being added is, or was, a consumer chain validator
+	k.IterateAllValidatorsByConsumerAddr(ctx, func(_ string, consumerAddr sdk.ConsAddress, _ sdk.ConsAddress) (stop bool) {
+		if consumerAddr.Equals(cons) {
+			inUse = true
+			return true
+		}
+		return false
+	})
+
+	return inUse
+}
+
 func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) {
+	if ValidatorConsensusKeyInUse(h.k, ctx, valAddr) {
+		// Abort TX, do NOT allow validator to be created
+		panic("cannot create a validator with a consensus key that is already in use or was recently in use as an assigned consumer chain key")
+	}
 }
 
 func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, valConsAddr sdk.ConsAddress, valAddr sdk.ValAddress) {

x/ccv/provider/keeper/hooks_test.go

@@ -0,0 +1,68 @@
+package keeper_test
+
+import (
+	"testing"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	cryptotestutil "github.com/cosmos/interchain-security/testutil/crypto"
+	testkeeper "github.com/cosmos/interchain-security/testutil/keeper"
+	providerkeeper "github.com/cosmos/interchain-security/x/ccv/provider/keeper"
+	"github.com/golang/mock/gomock"
+)
+
+func TestValidatorConsensusKeyInUse(t *testing.T) {
+
+	newValidator := cryptotestutil.NewCryptoIdentityFromIntSeed(0)
+	anotherValidator0 := cryptotestutil.NewCryptoIdentityFromIntSeed(1)
+	anotherValidator1 := cryptotestutil.NewCryptoIdentityFromIntSeed(2)
+
+	tests := []struct {
+		name   string
+		setup  func(sdk.Context, providerkeeper.Keeper)
+		expect bool
+	}{
+		{
+			name: "not in use by another validator",
+			setup: func(ctx sdk.Context, k providerkeeper.Keeper) {
+				// Another validator does not exist
+			},
+			expect: false,
+		},
+		{
+			name: "in use by another validator",
+			setup: func(ctx sdk.Context, k providerkeeper.Keeper) {
+				// We are trying to add a new validator, but its address has already been used
+				// by another validator
+				k.SetValidatorByConsumerAddr(ctx, "chainid", newValidator.SDKConsAddress(), anotherValidator0.SDKConsAddress())
+			},
+			expect: true,
+		},
+		{
+			name: "in use by one of several other validators",
+			setup: func(ctx sdk.Context, k providerkeeper.Keeper) {
+				// We are trying to add a new validator, but its address has already been used
+				// by another validator, of which there are several, across potentially several chains
+				k.SetValidatorByConsumerAddr(ctx, "chainid0", newValidator.SDKConsAddress(), anotherValidator0.SDKConsAddress())
+				k.SetValidatorByConsumerAddr(ctx, "chainid1", anotherValidator1.SDKConsAddress(), anotherValidator1.SDKConsAddress())
+			},
+			expect: true,
+		},
+	}
+	for _, tt := range tests {
+		k, ctx, _, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+
+		gomock.InOrder(
+			mocks.MockStakingKeeper.EXPECT().GetValidator(ctx,
+				newValidator.SDKStakingOperator(),
+			).Return(newValidator.SDKStakingValidator(), true),
+		)
+
+		tt.setup(ctx, k)
+
+		t.Run(tt.name, func(t *testing.T) {
+			if actual := providerkeeper.ValidatorConsensusKeyInUse(&k, ctx, newValidator.SDKStakingOperator()); actual != tt.expect {
+				t.Errorf("validatorConsensusKeyInUse() = %v, want %v", actual, tt.expect)
+			}
+		})
+	}
+}

x/ccv/provider/keeper/key_assignment.go

@@ -12,11 +12,6 @@ import (
 	tmprotocrypto "github.com/tendermint/tendermint/proto/tendermint/crypto"
 )
 
-// ValidatorConsumerPubKey: (chainID, providerAddr consAddr) -> consumerKey tmprotocrypto.PublicKey
-// ValidatorByConsumerAddr: (chainID, consumerAddr consAddr) -> providerAddr consAddr
-// KeyAssignmentReplacements: (chainID, providerAddr consAddr) -> replacement abci.ValidatorUpdate
-// ConsumerAddrsToPrune: (chainID, vscID uint64) -> consumerAddrsToPrune [][]byte
-
 // GetValidatorConsumerPubKey returns a validator's public key assigned for a consumer chain
 func (k Keeper) GetValidatorConsumerPubKey(
 	ctx sdk.Context,
@@ -363,17 +358,32 @@ func (k Keeper) AssignConsumerKey(
 	validator stakingtypes.Validator,
 	consumerKey tmprotocrypto.PublicKey,
 ) error {
+
 	consumerAddr := utils.TMCryptoPublicKeyToConsAddr(consumerKey)
 
 	providerAddr, err := validator.GetConsAddr()
+
 	if err != nil {
 		return err
 	}
 
+	if existingVal, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, consumerAddr); found {
+		if existingVal.OperatorAddress != validator.OperatorAddress {
+			// consumer key is already the key belonging to another existing
+			// and different provider
+			// prevent a validator from assigning a key which is the *provider*
+			// key of another validator
+			return sdkerrors.Wrapf(
+				types.ErrConsumerKeyInUse, "a different validator already uses the consumer key",
+			)
+		}
+	}
+
 	if _, found := k.GetValidatorByConsumerAddr(ctx, chainID, consumerAddr); found {
-		// mapping already exists; return error
+		// consumer key is already in use
+		// prevent multiple validators from assigning the same key
 		return sdkerrors.Wrapf(
-			types.ErrConsumerKeyExists, "consumer key already exists",
+			types.ErrConsumerKeyInUse, "a validator has assigned the consumer key already",
 		)
 	}