fix!: apply audit suggestions

.changelog/unreleased/bug-fixes/provider/1925-apply-audit-suggestions.md

@@ -0,0 +1,3 @@
+- Apply audit suggestions that include a bug fix in the way we compute the
+  maximum capped power. ([\#1925](https://github.com/cosmos/interchain-
+  security/pull/1925))

.changelog/unreleased/state-breaking/provider/1925-apply-audit-suggestions.md

@@ -0,0 +1,3 @@
+- Apply audit suggestions that include a bug fix in the way we compute the
+  maximum capped power. ([\#1925](https://github.com/cosmos/interchain-
+  security/pull/1925))

docs/docs/features/power-shaping.md

@@ -6,7 +6,9 @@ several "power shaping" mechanisms for consumer chains.
 These are:
 1) **Capping the size of the validator set**: The consumer chain can specify a maximum number of validators it
 wants to have in its validator set. This can be used to limit the number of validators in the set, which can
-be useful for chains that want to have a smaller validator set for faster blocks or lower overhead.
+be useful for chains that want to have a smaller validator set for faster blocks or lower overhead. If more validators
+than the maximum size have opted in on a consumer chain, only the validators with the highest power, up to the specified
+maximum, will validate the consumer chain.
 :::info
 This is only applicable to Opt In chains (chains with Top N = 0).
 :::

tests/e2e/steps_partial_set_security.go

@@ -1391,11 +1391,11 @@ func stepsValidatorsPowerCappedChain() []Step {
 				ChainID("consu"): ChainState{
 					ValPowers: &map[ValidatorID]uint{
 						// the powers of the validators on the consumer chain are different from the provider chain
-						// because the consumer chain is power capped. Note that the total power is 600 (= 194 + 203 + 203)
-						// and 203 / 600.0 = 0.338 < 34% that is the power cap.
-						ValidatorID("alice"): 194,
-						ValidatorID("bob"):   203,
-						ValidatorID("carol"): 203,
+						// because the consumer chain is power capped. Note that the total power is 600 (= 192 + 204 + 204)
+						// and 204 / 600.0 = 0.34 <= 34% that is the power cap.
+						ValidatorID("alice"): 192,
+						ValidatorID("bob"):   204,
+						ValidatorID("carol"): 204,
 					},
 				},
 			},
@@ -1428,9 +1428,9 @@ func stepsValidatorsPowerCappedChain() []Step {
 			State: State{
 				ChainID("consu"): ChainState{
 					ValPowers: &map[ValidatorID]uint{
-						ValidatorID("alice"): 194,
-						ValidatorID("bob"):   203,
-						ValidatorID("carol"): 203,
+						ValidatorID("alice"): 192,
+						ValidatorID("bob"):   204,
+						ValidatorID("carol"): 204,
 					},
 				},
 				ChainID("provi"): ChainState{
@@ -1454,9 +1454,9 @@ func stepsValidatorsPowerCappedChain() []Step {
 					ValPowers: &map[ValidatorID]uint{
 						// "carol" has opted out, and we only have 2 validators left validating. Power capping only operates
 						// in a best-effort basis and with 2 validators we cannot guarantee that no validator has more
-						// than 34% of the voting power. In this case, both validators get the same voting power (50% = 101 / 202).
-						ValidatorID("alice"): 101,
-						ValidatorID("bob"):   101,
+						// than 34% of the voting power. In this case, both validators get the same voting power (50% = 102 / 204).
+						ValidatorID("alice"): 102,
+						ValidatorID("bob"):   102,
 						ValidatorID("carol"): 0,
 					},
 				},

x/ccv/consumer/keeper/soft_opt_out.go

@@ -55,7 +55,7 @@ func (k Keeper) UpdateSmallestNonOptOutPower(ctx sdk.Context) {
 	// get total power in set
 	totalPower := sdk.ZeroDec()
 	for _, val := range valset {
-		totalPower = totalPower.Add(sdk.NewDecFromInt(sdk.NewInt(val.Power)))
+		totalPower = totalPower.Add(sdk.NewDec(val.Power))
 	}
 
 	// get power of the smallest validator that cannot soft opt out

x/ccv/provider/keeper/distribution.go

@@ -140,8 +140,8 @@ func (k Keeper) AllocateTokensToConsumerValidators(
 	}
 
 	// get the total voting power of the consumer valset
-	totalPower := k.ComputeConsumerTotalVotingPower(ctx, chainID)
-	if totalPower == 0 {
+	totalPower := math.LegacyNewDec(k.ComputeConsumerTotalVotingPower(ctx, chainID))
+	if totalPower.IsZero() {
 		return allocated
 	}
 
@@ -150,7 +150,7 @@ func (k Keeper) AllocateTokensToConsumerValidators(
 		consAddr := sdk.ConsAddress(consumerVal.ProviderConsAddr)
 
 		// get the validator tokens fraction using its voting power
-		powerFraction := math.LegacyNewDec(consumerVal.Power).QuoTruncate(math.LegacyNewDec(totalPower))
+		powerFraction := math.LegacyNewDec(consumerVal.Power).QuoTruncate(totalPower)
 		tokensFraction := tokens.MulDecTruncate(powerFraction)
 
 		// get the validator type struct for the consensus address
@@ -190,14 +190,14 @@ func (k Keeper) TransferConsumerRewardsToDistributionModule(
 	}
 
 	// Truncate coin rewards
-	rewardsToSend, _ := allocation.Rewards.TruncateDecimal()
+	rewardsToSend, remRewards := allocation.Rewards.TruncateDecimal()
 
 	// NOTE the consumer rewards allocation isn't a module account, however its coins
 	// are held in the consumer reward pool module account. Thus the consumer
 	// rewards allocation must be reduced separately from the SendCoinsFromModuleToAccount call.
 
 	// Update consumer rewards allocation with the remaining decimal coins
-	allocation.Rewards = allocation.Rewards.Sub(sdk.NewDecCoinsFromCoins(rewardsToSend...))
+	allocation.Rewards = remRewards
 
 	// Send coins to distribution module account
 	err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ConsumerRewardsPool, distrtypes.ModuleName, rewardsToSend)
@@ -238,7 +238,7 @@ func (k Keeper) GetConsumerRewardsPool(ctx sdk.Context) sdk.Coins {
 // ComputeConsumerTotalVotingPower returns the validator set total voting power
 // for the given consumer chain
 func (k Keeper) ComputeConsumerTotalVotingPower(ctx sdk.Context, chainID string) (totalPower int64) {
-	// sum the opted-in validators set voting powers
+	// sum the consumer validators set voting powers
 	for _, v := range k.GetConsumerValSet(ctx, chainID) {
 		totalPower += v.Power
 	}

x/ccv/provider/keeper/keeper.go

@@ -759,13 +759,9 @@ func (k Keeper) GetValidatorSetUpdateId(ctx sdk.Context) (validatorSetUpdateId u
 	bz := store.Get(types.ValidatorSetUpdateIdKey())
 
 	if bz == nil {
-		validatorSetUpdateId = 0
-	} else {
-		// Unmarshal
-		validatorSetUpdateId = binary.BigEndian.Uint64(bz)
+		return 0
 	}
-
-	return validatorSetUpdateId
+	return binary.BigEndian.Uint64(bz)
 }
 
 // SetValsetUpdateBlockHeight sets the block height for a given valset update id
@@ -1528,11 +1524,7 @@ func (k Keeper) IsAllowlistEmpty(ctx sdk.Context, chainID string) bool {
 	iterator := sdk.KVStorePrefixIterator(store, types.ChainIdWithLenKey(types.AllowlistPrefix, chainID))
 	defer iterator.Close()
 
-	if iterator.Valid() {
-		return false
-	}
-
-	return true
+	return !iterator.Valid()
 }
 
 // SetDenylist denylists validator with `providerAddr` address on chain `chainID`
@@ -1595,9 +1587,5 @@ func (k Keeper) IsDenylistEmpty(ctx sdk.Context, chainID string) bool {
 	iterator := sdk.KVStorePrefixIterator(store, types.ChainIdWithLenKey(types.DenylistPrefix, chainID))
 	defer iterator.Close()
 
-	if iterator.Valid() {
-		return false
-	}
-
-	return true
+	return !iterator.Valid()
 }

x/ccv/provider/keeper/msg_server.go

@@ -157,12 +157,7 @@ func (k msgServer) OptIn(goCtx context.Context, msg *types.MsgOptIn) (*types.Msg
 	}
 	providerConsAddr := types.NewProviderConsAddress(consAddrTmp)
 
-	if msg.ConsumerKey != "" {
-		err = k.Keeper.HandleOptIn(ctx, msg.ChainId, providerConsAddr, &msg.ConsumerKey)
-	} else {
-		err = k.Keeper.HandleOptIn(ctx, msg.ChainId, providerConsAddr, nil)
-	}
-
+	err = k.Keeper.HandleOptIn(ctx, msg.ChainId, providerConsAddr, msg.ConsumerKey)
 	if err != nil {
 		return nil, err
 	}

x/ccv/provider/keeper/partial_set_security.go

@@ -14,7 +14,7 @@ import (
 
 // HandleOptIn prepares validator `providerAddr` to opt in to `chainID` with an optional `consumerKey` consumer public key.
 // Note that the validator only opts in at the end of an epoch.
-func (k Keeper) HandleOptIn(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress, consumerKey *string) error {
+func (k Keeper) HandleOptIn(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress, consumerKey string) error {
 	if !k.IsConsumerProposedOrRegistered(ctx, chainID) {
 		return errorsmod.Wrapf(
 			types.ErrUnknownConsumerChainId,
@@ -23,8 +23,8 @@ func (k Keeper) HandleOptIn(ctx sdk.Context, chainID string, providerAddr types.
 
 	k.SetOptedIn(ctx, chainID, providerAddr)
 
-	if consumerKey != nil {
-		consumerTMPublicKey, err := k.ParseConsumerKey(*consumerKey)
+	if consumerKey != "" {
+		consumerTMPublicKey, err := k.ParseConsumerKey(consumerKey)
 		if err != nil {
 			return err
 		}
@@ -97,11 +97,11 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValid
 }
 
 // ComputeMinPowerToOptIn returns the minimum power needed for a validator (from the bonded validators)
-// to belong to the `topN` validators. `chainID` is only used for logging purposes.
+// to belong to the `topN` validators for a Top N chain.
 func (k Keeper) ComputeMinPowerToOptIn(ctx sdk.Context, chainID string, bondedValidators []stakingtypes.Validator, topN uint32) (int64, error) {
-	if topN == 0 || topN > 100 {
-		return 0, fmt.Errorf("trying to compute minimum power with an incorrect topN value (%d)."+
-			"topN has to be between (0, 100]", topN)
+	if topN < 50 || topN > 100 {
+		return 0, fmt.Errorf("trying to compute minimum power for a Top N chain (%s) with an incorrect"+
+			" topN value (%d). topN has to be in [50, 100]", chainID, topN)
 	}
 
 	totalPower := sdk.ZeroDec()
@@ -110,18 +110,18 @@ func (k Keeper) ComputeMinPowerToOptIn(ctx sdk.Context, chainID string, bondedVa
 	for _, val := range bondedValidators {
 		power := k.stakingKeeper.GetLastValidatorPower(ctx, val.GetOperator())
 		powers = append(powers, power)
-		totalPower = totalPower.Add(sdk.NewDecFromInt(sdk.NewInt(power)))
+		totalPower = totalPower.Add(sdk.NewDec(power))
 	}
 
 	// sort by powers descending
 	sort.Slice(powers, func(i, j int) bool {
 		return powers[i] > powers[j]
 	})
 
-	topNThreshold := sdk.NewDecFromInt(sdk.NewInt(int64(topN))).QuoInt64(int64(100))
+	topNThreshold := sdk.NewDec(int64(topN)).QuoInt64(int64(100))
 	powerSum := sdk.ZeroDec()
 	for _, power := range powers {
-		powerSum = powerSum.Add(sdk.NewDecFromInt(sdk.NewInt(power)))
+		powerSum = powerSum.Add(sdk.NewDec(power))
 		if powerSum.Quo(totalPower).GTE(topNThreshold) {
 			return power, nil
 		}
@@ -140,18 +140,12 @@ func (k Keeper) CapValidatorSet(ctx sdk.Context, chainID string, validators []ty
 		return validators
 	}
 
-	if validatorSetCap, found := k.GetValidatorSetCap(ctx, chainID); found && validatorSetCap != 0 {
+	if validatorSetCap, found := k.GetValidatorSetCap(ctx, chainID); found && validatorSetCap != 0 && int(validatorSetCap) < len(validators) {
 		sort.Slice(validators, func(i, j int) bool {
 			return validators[i].Power > validators[j].Power
 		})
 
-		minNumberOfValidators := 0
-		if len(validators) < int(validatorSetCap) {
-			minNumberOfValidators = len(validators)
-		} else {
-			minNumberOfValidators = int(validatorSetCap)
-		}
-		return validators[:minNumberOfValidators]
+		return validators[:int(validatorSetCap)]
 	} else {
 		return validators
 	}
@@ -209,9 +203,8 @@ func NoMoreThanPercentOfTheSum(validators []types.ConsumerValidator, percent uin
 	// If `s > n * maxPower` there's no solution and the algorithm would set everything to `maxPower`.
 	// ----------------
 
-	// Computes `(sum(validators) * percent) / 100`. Because `sdk.Dec` does not provide a `Floor` function, but only
-	// a `Ceil` one, we use `Ceil` and subtract one.
-	maxPower := sdk.NewDec(sum(validators)).Mul(sdk.NewDec(int64(percent))).QuoInt64(100).Ceil().RoundInt64() - 1
+	// Computes `floor((sum(validators) * percent) / 100)`
+	maxPower := sdk.NewDec(sum(validators)).Mul(sdk.NewDec(int64(percent))).QuoInt64(100).TruncateInt64()
 
 	if maxPower == 0 {
 		// edge case: set `maxPower` to 1 to avoid setting the power of a validator to 0
@@ -271,8 +264,9 @@ func NoMoreThanPercentOfTheSum(validators []types.ConsumerValidator, percent uin
 	return updatedValidators
 }
 
-// FilterOptedInAndAllowAndDenylistedPredicate filters the opted-in validators that are allowlisted and not denylisted
-func (k Keeper) FilterOptedInAndAllowAndDenylistedPredicate(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress) bool {
+// CanValidateChain returns true if the validator `providerAddr` is opted-in to chain `chainID` and the allowlist and
+// denylist do not prevent the validator from validating the chain.
+func (k Keeper) CanValidateChain(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress) bool {
 	// only consider opted-in validators
 	return k.IsOptedIn(ctx, chainID, providerAddr) &&
 		// if an allowlist is declared, only consider allowlisted validators
@@ -287,7 +281,7 @@ func (k Keeper) FilterOptedInAndAllowAndDenylistedPredicate(ctx sdk.Context, cha
 func (k Keeper) ComputeNextValidators(ctx sdk.Context, chainID string, bondedValidators []stakingtypes.Validator) []types.ConsumerValidator {
 	nextValidators := k.FilterValidators(ctx, chainID, bondedValidators,
 		func(providerAddr types.ProviderConsAddress) bool {
-			return k.FilterOptedInAndAllowAndDenylistedPredicate(ctx, chainID, providerAddr)
+			return k.CanValidateChain(ctx, chainID, providerAddr)
 		})
 
 	nextValidators = k.CapValidatorSet(ctx, chainID, nextValidators)

x/ccv/provider/keeper/partial_set_security_test.go

@@ -30,11 +30,11 @@ func TestHandleOptIn(t *testing.T) {
 	providerAddr := types.NewProviderConsAddress([]byte("providerAddr"))
 
 	// trying to opt in to a non-proposed and non-registered chain returns an error
-	require.Error(t, providerKeeper.HandleOptIn(ctx, "unknownChainID", providerAddr, nil))
+	require.Error(t, providerKeeper.HandleOptIn(ctx, "unknownChainID", providerAddr, ""))
 
 	providerKeeper.SetProposedConsumerChain(ctx, "chainID", 1)
 	require.False(t, providerKeeper.IsOptedIn(ctx, "chainID", providerAddr))
-	providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, nil)
+	providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, "")
 	require.True(t, providerKeeper.IsOptedIn(ctx, "chainID", providerAddr))
 }
 
@@ -71,7 +71,7 @@ func TestHandleOptInWithConsumerKey(t *testing.T) {
 	consumerKey := "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}"
 	expectedConsumerPubKey, _ := providerKeeper.ParseConsumerKey(consumerKey)
 
-	err := providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, &consumerKey)
+	err := providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, consumerKey)
 	require.NoError(t, err)
 
 	// assert that the consumeKey was assigned to `providerAddr` validator on chain with id `chainID`
@@ -327,28 +327,23 @@ func TestComputeMinPowerToOptIn(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, int64(6), m)
 
-	m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 41)
+	m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 50)
 	require.NoError(t, err)
 	require.Equal(t, int64(6), m)
 
-	m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 40)
-	require.NoError(t, err)
-	require.Equal(t, int64(10), m)
-
-	m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 1)
-	require.NoError(t, err)
-	require.Equal(t, int64(10), m)
-
-	// exceptional case when we erroneously call with `topN == 0` or `topN > 100`
+	// exceptional cases
 	_, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 0)
 	require.Error(t, err)
 
+	m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 49)
+	require.Error(t, err)
+
 	_, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 101)
 	require.Error(t, err)
 }
 
-// TestFilterOptedInAndAllowAndDenylistedPredicate returns true if `validator` is opted in, in `chainID.
-func TestFilterOptedInAndAllowAndDenylistedPredicate(t *testing.T) {
+// TestCanValidateChain returns true if `validator` is opted in, in `chainID.
+func TestCanValidateChain(t *testing.T) {
 	providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
 	defer ctrl.Finish()
 
@@ -357,24 +352,24 @@ func TestFilterOptedInAndAllowAndDenylistedPredicate(t *testing.T) {
 	providerAddr := types.NewProviderConsAddress(consAddr)
 
 	// with no allowlist or denylist, the validator has to be opted in, in order to consider it
-	require.False(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr))
+	require.False(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr))
 	providerKeeper.SetOptedIn(ctx, "chainID", types.NewProviderConsAddress(consAddr))
-	require.True(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr))
+	require.True(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr))
 
 	// create an allow list but do not add the validator `providerAddr` to it
 	validatorA := createStakingValidator(ctx, mocks, 1, 1)
 	consAddrA, _ := validatorA.GetConsAddr()
 	providerKeeper.SetAllowlist(ctx, "chainID", types.NewProviderConsAddress(consAddrA))
-	require.False(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr))
+	require.False(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr))
 	providerKeeper.SetAllowlist(ctx, "chainID", types.NewProviderConsAddress(consAddr))
-	require.True(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr))
+	require.True(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr))
 
 	// create a denylist but do not add validator `providerAddr` to it
 	providerKeeper.SetDenylist(ctx, "chainID", types.NewProviderConsAddress(consAddrA))
-	require.True(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr))
+	require.True(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr))
 	// add validator `providerAddr` to the denylist
 	providerKeeper.SetDenylist(ctx, "chainID", types.NewProviderConsAddress(consAddr))
-	require.False(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr))
+	require.False(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr))
 }
 
 func TestCapValidatorSet(t *testing.T) {
@@ -536,7 +531,7 @@ func noMoreThanPercent(validators []types.ConsumerValidator, percent uint32) boo
 	}
 
 	for _, v := range validators {
-		if (float64(v.Power)/float64(sum))*100.0 > float64(percent) {
+		if float64(v.Power)*100.0 > float64(percent)*float64(sum) {
 			return false
 		}
 	}

x/ccv/provider/keeper/relay.go

@@ -223,9 +223,9 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) {
 	for _, chain := range k.GetAllConsumerChains(ctx) {
 		currentValidators := k.GetConsumerValSet(ctx, chain.ChainId)
 
-		if topN, found := k.GetTopN(ctx, chain.ChainId); found && topN > 0 {
+		if chain.Top_N > 0 {
 			// in a Top-N chain, we automatically opt in all validators that belong to the top N
-			minPower, err := k.ComputeMinPowerToOptIn(ctx, chain.ChainId, bondedValidators, topN)
+			minPower, err := k.ComputeMinPowerToOptIn(ctx, chain.ChainId, bondedValidators, chain.Top_N)
 			if err == nil {
 				k.OptInTopNValidators(ctx, chain.ChainId, bondedValidators, minPower)
 			}