fix(x/slashing): Avoid overslashing on redelegation + unbonding in sp…

…ecific situations (#20688)
cosmos · Jun 24, 2024 · 720c108 · 720c108
1 parent ab1433b
commit 720c108
Show file tree

Hide file tree

Showing 3 changed files with 215 additions and 19 deletions.
diff --git a/tests/integration/slashing/keeper/slash_redelegation_test.go b/tests/integration/slashing/keeper/slash_redelegation_test.go
@@ -2,7 +2,6 @@ package keeper_test
 
 import (
 	"context"
-	"fmt"
 	"testing"
 	"time"
 
@@ -66,30 +65,30 @@ func TestSlashRedelegation(t *testing.T) {
 	testAcc2 := sdk.AccAddress([]byte("addr2_______________"))
 
 	// fund acc 1 and acc 2
-	testCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, stakingKeeper.TokensFromConsensusPower(ctx, 10)))
-	fundAccount(t, ctx, bankKeeper, authKeeper, testAcc1, testCoins)
-	fundAccount(t, ctx, bankKeeper, authKeeper, testAcc2, testCoins)
+	testCoin := sdk.NewCoin(bondDenom, stakingKeeper.TokensFromConsensusPower(ctx, 10))
+	fundAccount(t, ctx, bankKeeper, authKeeper, testAcc1, testCoin)
+	fundAccount(t, ctx, bankKeeper, authKeeper, testAcc2, testCoin)
 
 	balance1Before := bankKeeper.GetBalance(ctx, testAcc1, bondDenom)
 	balance2Before := bankKeeper.GetBalance(ctx, testAcc2, bondDenom)
 
 	// assert acc 1 and acc 2 balance
-	require.Equal(t, balance1Before.Amount.String(), testCoins[0].Amount.String())
-	require.Equal(t, balance2Before.Amount.String(), testCoins[0].Amount.String())
+	require.Equal(t, balance1Before.Amount.String(), testCoin.Amount.String())
+	require.Equal(t, balance2Before.Amount.String(), testCoin.Amount.String())
 
 	// creating evil val
 	evilValAddr := sdk.ValAddress(evilValPubKey.Address())
-	fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(evilValAddr), testCoins)
+	fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(evilValAddr), testCoin)
 	createValMsg1, _ := stakingtypes.NewMsgCreateValidator(
-		evilValAddr.String(), evilValPubKey, testCoins[0], stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)), math.OneInt())
+		evilValAddr.String(), evilValPubKey, testCoin, stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)), math.OneInt())
 	_, err = stakingMsgServer.CreateValidator(ctx, createValMsg1)
 	require.NoError(t, err)
 
 	// creating good val
 	goodValAddr := sdk.ValAddress(goodValPubKey.Address())
-	fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(goodValAddr), testCoins)
+	fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(goodValAddr), testCoin)
 	createValMsg2, _ := stakingtypes.NewMsgCreateValidator(
-		goodValAddr.String(), goodValPubKey, testCoins[0], stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)), math.OneInt())
+		goodValAddr.String(), goodValPubKey, testCoin, stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)), math.OneInt())
 	_, err = stakingMsgServer.CreateValidator(ctx, createValMsg2)
 	require.NoError(t, err)
 
@@ -100,12 +99,12 @@ func TestSlashRedelegation(t *testing.T) {
 	require.NoError(t, err)
 
 	// Acc 2 delegate
-	delMsg := stakingtypes.NewMsgDelegate(testAcc2.String(), evilValAddr.String(), testCoins[0])
+	delMsg := stakingtypes.NewMsgDelegate(testAcc2.String(), evilValAddr.String(), testCoin)
 	_, err = stakingMsgServer.Delegate(ctx, delMsg)
 	require.NoError(t, err)
 
 	// Acc 1 delegate
-	delMsg = stakingtypes.NewMsgDelegate(testAcc1.String(), evilValAddr.String(), testCoins[0])
+	delMsg = stakingtypes.NewMsgDelegate(testAcc1.String(), evilValAddr.String(), testCoin)
 	_, err = stakingMsgServer.Delegate(ctx, delMsg)
 	require.NoError(t, err)
 
@@ -119,20 +118,19 @@ func TestSlashRedelegation(t *testing.T) {
 	require.NoError(t, err)
 
 	evilPower := stakingKeeper.TokensToConsensusPower(ctx, evilVal.Tokens)
-	fmt.Println(evilPower)
 
 	// Acc 1 redelegate from evil val to good val
-	redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1.String(), evilValAddr.String(), goodValAddr.String(), testCoins[0])
+	redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1.String(), evilValAddr.String(), goodValAddr.String(), testCoin)
 	_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
 	require.NoError(t, err)
 
 	// Acc 1 undelegate from good val
-	undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoins[0])
+	undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoin)
 	_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
 	require.NoError(t, err)
 
 	// Acc 2 undelegate from evil val
-	undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoins[0])
+	undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoin)
 	_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
 	require.NoError(t, err)
 
@@ -175,7 +173,7 @@ func TestSlashRedelegation(t *testing.T) {
 	require.Equal(t, balance2AfterSlashing.Amount.Mul(math.NewIntFromUint64(10)).String(), balance2Before.Amount.String())
 }
 
-func fundAccount(t *testing.T, ctx context.Context, bankKeeper bankkeeper.Keeper, authKeeper authkeeper.AccountKeeper, addr sdk.AccAddress, amount sdk.Coins) {
+func fundAccount(t *testing.T, ctx context.Context, bankKeeper bankkeeper.Keeper, authKeeper authkeeper.AccountKeeper, addr sdk.AccAddress, amount ...sdk.Coin) {
 	t.Helper()
 
 	if authKeeper.GetAccount(ctx, addr) == nil {
@@ -185,3 +183,185 @@ func fundAccount(t *testing.T, ctx context.Context, bankKeeper bankkeeper.Keeper
 
 	require.NoError(t, banktestutil.FundAccount(ctx, bankKeeper, addr, amount))
 }
+
+func TestOverSlashing(t *testing.T) {
+	// slash penalty percentage
+	slashFraction := "0.45"
+
+	// percentage of (undelegation/(undelegation + redelegation))
+	undelegationPercentageStr := "0.30"
+
+	// setting up
+	var (
+		stakingKeeper *stakingkeeper.Keeper
+		bankKeeper    bankkeeper.Keeper
+		slashKeeper   slashingkeeper.Keeper
+		distrKeeper   distributionkeeper.Keeper
+		authKeeper    authkeeper.AccountKeeper
+	)
+
+	app, err := simtestutil.Setup(depinject.Configs(
+		depinject.Supply(log.NewNopLogger()),
+		slashing.AppConfig,
+	), &stakingKeeper, &bankKeeper, &slashKeeper, &distrKeeper, &authKeeper)
+	require.NoError(t, err)
+
+	// get sdk context, staking msg server and bond denom
+	ctx := app.BaseApp.NewContext(false)
+	stakingMsgServer := stakingkeeper.NewMsgServerImpl(stakingKeeper)
+	bondDenom, err := stakingKeeper.BondDenom(ctx)
+	require.NoError(t, err)
+
+	// evilVal will be slashed, goodVal won't be slashed
+	evilValPubKey := secp256k1.GenPrivKey().PubKey()
+	goodValPubKey := secp256k1.GenPrivKey().PubKey()
+
+	/*
+	   all test accs will delegate to evil val, which evil validator will eventually be slashed
+
+	   - test acc 1: redelegate -> undelegate full amount
+	   - test acc 2: simple undelegation. intended scenario.
+	   - test acc 3: redelegate -> undelegate some amount
+
+	*/
+
+	testAcc1 := sdk.AccAddress([]byte("addr1new____________"))
+	testAcc2 := sdk.AccAddress([]byte("addr2new____________"))
+	testAcc3 := sdk.AccAddress([]byte("addr3new____________"))
+
+	// fund all accounts
+	testCoin := sdk.NewCoin(bondDenom, math.NewInt(1_000_000))
+	fundAccount(t, ctx, bankKeeper, authKeeper, testAcc1, testCoin)
+	fundAccount(t, ctx, bankKeeper, authKeeper, testAcc2, testCoin)
+	fundAccount(t, ctx, bankKeeper, authKeeper, testAcc3, testCoin)
+
+	balance1Before := bankKeeper.GetBalance(ctx, testAcc1, bondDenom)
+	balance2Before := bankKeeper.GetBalance(ctx, testAcc2, bondDenom)
+	balance3Before := bankKeeper.GetBalance(ctx, testAcc3, bondDenom)
+
+	// assert acc 1, 2 and 3 balance
+	require.Equal(t, testCoin.Amount.String(), balance1Before.Amount.String())
+	require.Equal(t, testCoin.Amount.String(), balance2Before.Amount.String())
+	require.Equal(t, testCoin.Amount.String(), balance3Before.Amount.String())
+
+	// create evil val
+	evilValAddr := sdk.ValAddress(evilValPubKey.Address())
+	fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(evilValAddr), testCoin)
+	createValMsg1, _ := stakingtypes.NewMsgCreateValidator(
+		evilValAddr.String(), evilValPubKey, testCoin, stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)), math.OneInt())
+	_, err = stakingMsgServer.CreateValidator(ctx, createValMsg1)
+	require.NoError(t, err)
+
+	// create good val 1
+	goodValAddr := sdk.ValAddress(goodValPubKey.Address())
+	fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(goodValAddr), testCoin)
+	createValMsg2, _ := stakingtypes.NewMsgCreateValidator(
+		goodValAddr.String(), goodValPubKey, testCoin, stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)), math.OneInt())
+	_, err = stakingMsgServer.CreateValidator(ctx, createValMsg2)
+	require.NoError(t, err)
+
+	// next block
+	ctx = ctx.WithBlockHeight(app.LastBlockHeight() + 1).WithHeaderInfo(header.Info{Height: app.LastBlockHeight() + 1})
+	ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
+	require.NoError(t, err)
+
+	// delegate all accs to evil val
+	delMsg := stakingtypes.NewMsgDelegate(testAcc1.String(), evilValAddr.String(), testCoin)
+	_, err = stakingMsgServer.Delegate(ctx, delMsg)
+	require.NoError(t, err)
+
+	delMsg = stakingtypes.NewMsgDelegate(testAcc2.String(), evilValAddr.String(), testCoin)
+	_, err = stakingMsgServer.Delegate(ctx, delMsg)
+	require.NoError(t, err)
+
+	delMsg = stakingtypes.NewMsgDelegate(testAcc3.String(), evilValAddr.String(), testCoin)
+	_, err = stakingMsgServer.Delegate(ctx, delMsg)
+	require.NoError(t, err)
+
+	// next block
+	ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
+	require.NoError(t, err)
+
+	// evilValAddr done something bad
+	misbehaveHeight := ctx.BlockHeader().Height
+	evilVal, err := stakingKeeper.GetValidator(ctx, evilValAddr)
+	require.NoError(t, err)
+
+	evilValConsAddr, err := evilVal.GetConsAddr()
+	require.NoError(t, err)
+
+	evilPower := stakingKeeper.TokensToConsensusPower(ctx, evilVal.Tokens)
+
+	// next block
+	ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
+	require.NoError(t, err)
+
+	// acc 1: redelegate to goodval1 and undelegate FULL amount
+	redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1.String(), evilValAddr.String(), goodValAddr.String(), testCoin)
+	_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
+	require.NoError(t, err)
+	undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoin)
+	_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
+	require.NoError(t, err)
+
+	// acc 2: undelegate full amount
+	undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoin)
+	_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
+	require.NoError(t, err)
+
+	// acc 3: redelegate to goodval1 and undelegate some amount
+	redelMsg = stakingtypes.NewMsgBeginRedelegate(testAcc3.String(), evilValAddr.String(), goodValAddr.String(), testCoin)
+	_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
+	require.NoError(t, err)
+
+	undelegationPercentage := math.LegacyMustNewDecFromStr(undelegationPercentageStr)
+	undelegationAmountDec := math.LegacyNewDecFromInt(testCoin.Amount).Mul(undelegationPercentage)
+	amountToUndelegate := undelegationAmountDec.TruncateInt()
+
+	// next block
+	ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
+	require.NoError(t, err)
+
+	portionofTestCoins := sdk.NewCoin(bondDenom, amountToUndelegate)
+	undelMsg = stakingtypes.NewMsgUndelegate(testAcc3.String(), goodValAddr.String(), portionofTestCoins)
+	_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
+	require.NoError(t, err)
+
+	// next block
+	ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
+	require.NoError(t, err)
+
+	// slash the evil val
+	err = slashKeeper.Slash(ctx, evilValConsAddr, math.LegacyMustNewDecFromStr(slashFraction), evilPower, misbehaveHeight)
+	require.NoError(t, err)
+
+	// assert invariants
+	_, stop := stakingkeeper.AllInvariants(stakingKeeper)(ctx)
+	require.False(t, stop)
+	_, stop = bankkeeper.AllInvariants(bankKeeper)(ctx)
+	require.False(t, stop)
+	_, stop = distributionkeeper.AllInvariants(distrKeeper)(ctx)
+	require.False(t, stop)
+
+	// fastforward 2 blocks to complete redelegations and unbondings
+	for i := 0; i < 2; i++ {
+		ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1000000000000000000))
+		require.NoError(t, err)
+	}
+
+	// we check all accounts should be slashed with the equal amount, and they should end up with same balance including staked amount
+	stakedAcc1, err := stakingKeeper.GetDelegatorBonded(ctx, testAcc1)
+	require.NoError(t, err)
+	stakedAcc2, err := stakingKeeper.GetDelegatorBonded(ctx, testAcc2)
+	require.NoError(t, err)
+	stakedAcc3, err := stakingKeeper.GetDelegatorBonded(ctx, testAcc3)
+	require.NoError(t, err)
+
+	balance1AfterSlashing := bankKeeper.GetBalance(ctx, testAcc1, bondDenom).Add(sdk.NewCoin(bondDenom, stakedAcc1))
+	balance2AfterSlashing := bankKeeper.GetBalance(ctx, testAcc2, bondDenom).Add(sdk.NewCoin(bondDenom, stakedAcc2))
+	balance3AfterSlashing := bankKeeper.GetBalance(ctx, testAcc3, bondDenom).Add(sdk.NewCoin(bondDenom, stakedAcc3))
+
+	require.Equal(t, "550000stake", balance1AfterSlashing.String())
+	require.Equal(t, "550000stake", balance2AfterSlashing.String())
+	require.Equal(t, "550000stake", balance3AfterSlashing.String())
+}
diff --git a/x/staking/CHANGELOG.md b/x/staking/CHANGELOG.md
@@ -25,6 +25,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+### Bug Fixes
+
+* [#20688](https://github.com/cosmos/cosmos-sdk/pull/20688) Avoid overslashing unbonding delegations after a redelegation.
+
 ### Features
 
 * [#19537](https://github.com/cosmos/cosmos-sdk/pull/19537) Changing `MinCommissionRate` in `MsgUpdateParams` now updates the minimum commission rate for all validators.

diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go
@@ -365,8 +365,20 @@ func (k Keeper) SlashRedelegation(ctx context.Context, srcValidator types.Valida
 
 		// Slash the moved delegation
 		// Unbond from target validator
-		sharesToUnbond := slashFactor.Mul(entry.SharesDst)
-		if sharesToUnbond.IsZero() || slashAmount.IsZero() {
+		if slashAmount.IsZero() {
+			continue
+		}
+
+		dstVal, err := k.GetValidator(ctx, valDstAddr)
+		if err != nil {
+			return math.ZeroInt(), err
+		}
+		sharesToUnbond, err := dstVal.SharesFromTokensTruncated(slashAmount)
+		if err != nil {
+			return math.ZeroInt(), err
+		}
+
+		if sharesToUnbond.IsZero() {
 			continue
 		}