x/slashing/keeper: Keeper.Slash should return immediately if tokensToBurn.IsZero

[odeke-em]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

Noticed while auditing and testing that this code

cosmos-sdk/x/staking/keeper/slash.go

Lines 146 to 168 in 599b58d

	// cannot decrease balance below zero
	tokensToBurn := math.MinInt(remainingSlashAmount, validator.Tokens)
	tokensToBurn = math.MaxInt(tokensToBurn, math.ZeroInt()) // defensive.
	// we need to calculate the *effective* slash fraction for distribution
	if validator.Tokens.IsPositive() {
	effectiveFraction := math.LegacyNewDecFromInt(tokensToBurn).QuoRoundUp(math.LegacyNewDecFromInt(validator.Tokens))
	// possible if power has changed
	if effectiveFraction.GT(math.LegacyOneDec()) {
	effectiveFraction = math.LegacyOneDec()
	}
	// call the before-slashed hook
	if err := k.Hooks().BeforeValidatorSlashed(ctx, operatorAddress, effectiveFraction); err != nil {
	k.Logger(ctx).Error("failed to call before validator slashed hook", "error", err)
	}
	}
	// Deduct from validator's bonded tokens and update the validator.
	// Burn the slashed tokens from the pool account and decrease the total supply.
	validator, err = k.RemoveValidatorTokens(ctx, validator, tokensToBurn)
	if err != nil {
	return math.NewInt(0), err
	}

if the value of tokensToBurn is zero, the code down below takes an unnecessary path
but in there:

• firstly tokensToBurn will be zero hence all unnecessary multiplications in

  cosmos-sdk/x/staking/keeper/slash.go

  Lines 152 to 159 in 599b58d

  	effectiveFraction := math.LegacyNewDecFromInt(tokensToBurn).QuoRoundUp(math.LegacyNewDecFromInt(validator.Tokens))
  	// possible if power has changed
  	if effectiveFraction.GT(math.LegacyOneDec()) {
  	effectiveFraction = math.LegacyOneDec()
  	}
  	// call the before-slashed hook
  	if err := k.Hooks().BeforeValidatorSlashed(ctx, operatorAddress, effectiveFraction); err != nil {
  	k.Logger(ctx).Error("failed to call before validator slashed hook", "error", err)

• invoking .BeforeSlashedValidator will be futile as effectiveFraction=0
• invoking k.RemoveValidatorTokens() with a tokensToRemove=0 performs unnecessary work and ends up with .Sub(0), erroneous removals as well in

  cosmos-sdk/x/staking/keeper/validator.go

  Lines 165 to 167 in 4001e2b

  	if err := k.DeleteValidatorByPowerIndex(ctx, validator); err != nil {
  	return validator, err
  	}

  which could actually be a bug in which I can trigger removal of a validator that has not yet been delegated tokens (debatable though)

Suggestion

diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go
index 5a0886141f..e61cbb5181 100644
--- a/x/staking/keeper/slash.go
+++ b/x/staking/keeper/slash.go
@@ -147,6 +147,18 @@ func (k Keeper) Slash(ctx context.Context, consAddr sdk.Con
sAddress, infractionH
        tokensToBurn := math.MinInt(remainingSlashAmount, validator.Tokens)
        tokensToBurn = math.MaxInt(tokensToBurn, math.ZeroInt()) // defensive.
 
+       if tokensToBurn.IsZero() {
+               // Nothing to burn, we can end this route immediately.
+               logger.Info(
+                       "no validator slashing because slash amount is zero",
+                       "validator", validator.GetOperator(),
+                       "slash_factor", slashFactor.String(),
+                       "burned", tokensToBurn,
+                       "validatorTokens", validator.Tokens,
+               )
+               return math.NewInt(0), nil
+       }
+

/cc @elias-orijtech

Cosmos SDK Version

main

How to reproduce?

No response