Key assignment adds iteration on MsgCreateValidator

[mpoke]

Problem

Due to the key assignment protocol, when creating a new validator in the staking module (via the MsgCreateValidator message), the code iterates over all consumer keys which have been assigned in order to check if the validator being added is, or was, a consumer chain validator. This can be used as an attack vector.

Closing criteria

Figure out a way to get rid of this check.

Problem details

The ICS code implements the AfterValidatorCreated staking hook, which is called when a new validator is created. This hook iterates over all assigned consumer addresses, i.e.,

interchain-security/x/ccv/provider/keeper/hooks.go

Line 79 in 3e339ea

k.IterateAllValidatorsByConsumerAddr(ctx, func(_ string, consumerAddr sdk.ConsAddress, _ sdk.ConsAddress) (stop bool) {

TODOs

• Labels have been added for issue
• Issue has been added to the ICS project

[jtremback]

The loop iterates over all validators, which is the source of the problem. It does this because it is looking for a consumer address matching the new validator's address in an index with keys like chainId | consumerAddress. It can't just iterate over all chainIds and try to get the consumerAddress for each one because there could consumerAddress stored for chainIds that have not been created yet.

To solve this we could create a second index with keys like consumerAddress | chainId. This can then be used to check if an address is in use with one operation.

[mpoke]

We should also look into whether is possible to have a single index with keys like consumerAddress | chainId. @danwt do you see any problems with that?

[danwt]

a second index with keys like consumerAddress | chainId

I think it works and is simple 👍

have a single index with keys like consumerAddress | chainId

I think this also works, and this could replace the current chainID | consumerAddress index because currently chain wise iteration is only done during DeleteKeyAssignments(ctx sdk.Context, chainID string) and this is a very rare operation.

[yaruwangway]

Hi, the main is updated.
right now, if chainID is nil, GetAllValidatorsByConsumerAddr will get all the consensus addresses on all consumer chains.
It is a bit inefficient, when later looping through these addresses to get the CreateValidator. but what are the other issues this bring ?

If we change the index, from chainId | consumerAddress to consumerAddress | chainId, then it has to loop through all the addresses to check if the chainID is the chainID desired.
Instead, present len(chainID) | chainID is more efficient when getting addresses that ChainID not nil.

interchain-security/x/ccv/provider/keeper/key_assignment.go

Line 77 in d137d39

chainID, providerAddrTmp, err := types.ParseChainIdAndConsAddrKey(types.ConsumerValidatorsBytePrefix, iterator.Key())

[mpoke]

GetAllValidatorsByConsumerAddr is called in three places:

• ExportGenesis calls GetAllValidatorsByConsumerAddr(ctx, nil)
• AfterValidatorCreated calls ValidatorConsensusKeyInUse(h.k, ctx, valAddr), which calls GetAllValidatorsByConsumerAddr(ctx, nil)
• DeleteKeyAssignments calls k.GetAllValidatorsByConsumerAddr(ctx, &chainID)

Changing the index from chainId | consumerAddress to consumerAddress | chainId has the following effects:

• The first call is not changed at all. Still need to iterate over all consumer addresses.
• The second call requires no iteration, i.e.,

// ValidatorConsensusKeyInUse is called when a new validator is created
// in the x/staking module of cosmos-sdk. In case it panics, the TX aborts
// and thus, the validator is not created. See AfterValidatorCreated hook.
func ValidatorConsensusKeyInUse(k *Keeper, ctx sdk.Context, valAddr sdk.ValAddress) bool {
	// Get the validator being added in the staking module.
	val, found := k.stakingKeeper.GetValidator(ctx, valAddr)
	if !found {
		// Abort TX, do NOT allow validator to be created
		panic("did not find newly created validator in staking module")
	}

	// Get the consensus address of the validator being added
	consensusAddr, err := val.GetConsAddr()
	if err != nil {
		// Abort TX, do NOT allow validator to be created
		panic("could not get validator cons addr ")
	}

        store := ctx.KVStore(k.storeKey)
        prefix := ccvtypes.AppendMany(types.ValidatorsByConsumerAddrBytePrefix, consensusAddr)
        iterator := sdk.KVStorePrefixIterator(store, prefix)
	defer iterator.Close()

	for ; iterator.Valid(); iterator.Next() {
             return true
        }
        return false
}

• The third call is less efficient as now we need to iterate over all consumer addresses and just remove the ones for a particular chain. This doesn't really matter though, since DeleteKeyAssignments is called only when a consumer chain is removed.

[yaruwangway]

I see, in the above second call, when iterate the db to find the validatorConsumerAddrs, the consumer chainID is not known,(see below link the chainID aurgument is nil) , so that the prefix with |chainID| does not help fast find the addr, all the address with different chain-ID are returned. So chainId | consumerAddress and consumerAddress | chainId are the same in terms of iteration db.

interchain-security/x/ccv/provider/keeper/hooks.go

Line 103 in d137d39

for _, validatorConsumerAddrs := range k.GetAllValidatorsByConsumerAddr(ctx, nil) {

[yaruwangway]

the validatorConsumerAddrs is stored in kvstore:
key: bytePrefix | len(chainID) | chainID | ConsAddress,
value: validatorConsumerAddrs, now we want to store

• the migration will add the new key bytePrefix | ConsAddress|len(chainID) | chainID , value: validatorConsumerAddrs, so that a prefix bytePrefix | ConsAddress search can be used to fast find the added validator.
• the migration will not delete the old key-value, the old key-value will be deleted in the next upgrade after the upgrade containing this migration.

[mpoke]

• the migration will not delete the old key-value, the old key-value will be deleted in the next upgrade after the upgrade containing this migration.

Why can't we delete the old key-value?

[yaruwangway]

we can do all in one migration, i just feel it is secure if we still keep the old data for a bit longer ?

[MSalopek]

Just chiming in; it could be worthwhile to explore what cosmos-sdk collections bring us with regards to this issue.
There are some useful, more abstract types for handling data in the collections package that will be released in v50.

If this turns out to be too much trouble, let's consider using a collection if applicable and refactor this it in a later ICS version?

IndexedMap seems like a great abstraction for this usecase.

we can do all in one migration, i just feel it is secure if we still keep the old data for a bit longer ?

I'm not sure keeping old data adds to security.

[yaruwangway]

Big thanks to @MSalopek for this migration free solution ! just check with trying each consumer ChainID ( for each chainID, if bytePrefix | len(chainID) | chainID | ConsAddress exists as key), there are not so many chains!

[MSalopek]

A note from today's dev session.

It seems that the iterations discussed above can be refactored such that the store key is calculated and simply accessed via store.Get that way we could check if the key is in use by making a <10 calls to the store instead of iterating over all consumer keys.

e.g.

// ValidatorConsensusKeyInUse is called when a new validator is created
// in the x/staking module of cosmos-sdk. In case it panics, the TX aborts
// and thus, the validator is not created. See AfterValidatorCreated hook.
func ValidatorConsensusKeyInUse(k *Keeper, ctx sdk.Context, valAddr sdk.ValAddress) bool {
	// Get the validator being added in the staking module.
	val, found := k.stakingKeeper.GetValidator(ctx, valAddr)
	if !found {
		// Abort TX, do NOT allow validator to be created
		panic("did not find newly created validator in staking module")
	}

	// Get the consensus address of the validator being added
	consensusAddr, err := val.GetConsAddr()
	if err != nil {
		// Abort TX, do NOT allow validator to be created
		panic("could not get validator cons addr ")
	}

	consuChains := k.GetAllConsumerChains(ctx)
	for _, consumerChain := range consuChains {
		if _, found := k.GetValidatorByConsumerAddr(
			ctx,
			consumerChain.ChainId,
			providertypes.NewConsumerConsAddress(consensusAddr)); found {
			return true
		}
	}

	return false
}

Additionally, a delete operation can be performed on the entire store subtree.

e.g.

// ChainIdWithLenKey returns the key with the following format:
// bytePrefix | len(chainID) | chainID
func ChainIdWithLenKey(prefix byte, chainID string) []byte {
	chainIdL := len(chainID)
	return ccvtypes.AppendMany(
		// Append the prefix
		[]byte{prefix},
		// Append the chainID length
		sdk.Uint64ToBigEndian(uint64(chainIdL)),
		// Append the chainID
		[]byte(chainID),
	)
}


// DeleteKeyAssignments deletes all the state needed for key assignments on a consumer chain
func (k Keeper) DeleteKeyAssignments(ctx sdk.Context, chainID string) {
	// delete ValidatorsByConsumerAddr
	for _, validatorConsumerAddr := range k.GetAllValidatorsByConsumerAddr(ctx, &chainID) {
		consumerAddr := types.NewConsumerConsAddress(validatorConsumerAddr.ConsumerAddr)
		k.DeleteValidatorByConsumerAddr(ctx, chainID, consumerAddr)
	}

        // replace with
        store.Delete(ctx, ChainWithLenKey(prefix, chainID)
        ...
}

Not sure about the delete, but by intuition I'd wager it can be done like that.

[mpoke]

@MSalopek @yaruwangway It's not easy to iterate over all consumer chains as GetAllConsumerChains iterates only over registered consumers, i.e.,

// GetAllConsumerChains gets all of the consumer chains, for which the provider module
// created IBC clients. Consumer chains with created clients are also referred to as registered.

and consumer keys can be assigned for every chain, even those that are not yet registered, i.e.,

func (msg MsgAssignConsumerKey) ValidateBasic() error {
	if strings.TrimSpace(msg.ChainId) == "" {
		return ErrBlankConsumerChainID
	}
	// It is possible to assign keys for consumer chains that are not yet approved.
	// This can only be done by a signing validator, but it is still sensible
	// to limit the chainID size to prevent abuse.
	// TODO: In future, a mechanism will be added to limit assigning keys to chains
	// which are approved or pending approval, only.
	if 128 < len(msg.ChainId) {
		return ErrBlankConsumerChainID
	}

[yaruwangway]

The loop iterates over all validators, which is the source of the problem. It does this because it is looking for a consumer address matching the new validator's address in an index with keys like chainId | consumerAddress. It can't just iterate over all chainIds and try to get the consumerAddress for each one because there could consumerAddress stored for chainIds that have not been created yet.

To solve this we could create a second index with keys like consumerAddress | chainId. This can then be used to check if an address is in use with one operation.

@jtremback , "It can't just iterate over all chainIds and try to get the consumerAddress for each one because there could consumerAddress stored for chainIds that have not been created yet." what does this mean ? cc @MSalopek

[MSalopek]

@yaruwangway It means that you can create a consumer key for a chain that is not yet registered (the gov proposal has not passed and the clientID is unknown).

[MSalopek]

@mpoke
Can we implement the TODO from the message above?

	// TODO: In future, a mechanism will be added to limit assigning keys to chains
	// which are approved or pending approval, only

[mpoke]

@mpoke Can we implement the TODO from the message above?

	// TODO: In future, a mechanism will be added to limit assigning keys to chains
	// which are approved or pending approval, only

Sure. Please open a separate issue though.

[yaruwangway]

just to clarify some edge cases :

• is it possible chainID == "" in the store key: bytePrefix | len(chainID) | chainID | ConsAddress?
• GetAllConsumerChains does not return chains with chainID == "" ?
• GetAllConsumerChains might return chain1, chain2, but in the store key bytePrefix | len(chainID) | chainID | ConsAddress, the chainID might be chainX which is not in GetAllConsumerChains, so cannot use below code to find the addr in use ?

consuChains := k.GetAllConsumerChains(ctx)
	for _, consumerChain := range consuChains {
		if _, found := k.GetValidatorByConsumerAddr(
			ctx,
			consumerChain.ChainId,
			providertypes.NewConsumerConsAddress(consensusAddr)); found {
			return true
		}
	}

thanks!

@mpoke @MSalopek