Merge pull request from GHSA-86h5-xcpx-cfqc

* fix slashing logic * add test * changelog + release notes * word --------- Co-authored-by: Julien Robert <[email protected]>
yihuang · Feb 28, 2024 · 7d76e2f · 7d76e2f
1 parent 0f7197f
commit 7d76e2f
Show file tree

Hide file tree

Showing 5 changed files with 221 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -37,6 +37,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+* (x/staking) Fix a possible bypass of delagator slashing: [GHSA-86h5-xcpx-cfqc](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-86h5-xcpx-cfqc)
+
 ## [v0.47.9](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.9) - 2024-02-19
 
 ### Bug Fixes

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
@@ -1,19 +1,20 @@
-# Cosmos SDK v0.47.9 Release Notes
+# Cosmos SDK v0.47.10 Release Notes
 
 💬 [**Release Discussion**](https://github.com/orgs/cosmos/discussions/6)
 
 ## 🚀 Highlights
 
-This patch release includes a fix in baseapp in `DefaultProposalHandler` and fixes [GHSA-4j93-fm92-rp4m](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-4j93-fm92-rp4m).
+This early monthly patch release fixes [GHSA-86h5-xcpx-cfqc](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-86h5-xcpx-cfqc).
 
 We recommended to upgrade to this patch release as soon as possible.
-When upgrading from <= v0.47.8, please ensure that 2/3 of the validator power upgrade to v0.47.9.
+When upgrading from <= v0.47.9, please ensure that 2/3 of the validator power upgrade to v0.47.10.
 
-Curious? Check out the [changelog](https://github.com/cosmos/cosmos-sdk/blob/v0.47.9/CHANGELOG.md) for an exhaustive list of changes or [compare changes](https://github.com/cosmos/cosmos-sdk/compare/v0.47.8...v0.47.9) from last release.
+Curious? Check out the [changelog](https://github.com/cosmos/cosmos-sdk/blob/v0.47.10/CHANGELOG.md) for an exhaustive list of changes or [compare changes](https://github.com/cosmos/cosmos-sdk/compare/v0.47.9...v0.47.10) from last release.
 
 Refer to the [upgrading guide](https://github.com/cosmos/cosmos-sdk/blob/release/v0.50.x/UPGRADING.md) when migrating from `v0.47.x` to `v0.50.x`.
 
 ## Maintenance Policy
 
-v0.50 has been released which means the v0.47.x line is now supported for bug fixes only, as per our release policy.
-Start integrating with [Cosmos SDK Eden (v0.50)](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.4) and enjoy and the new features and performance improvements.
+v0.50 has been released which means the v0.47.x line is now supported for bug fixes only, as per our release policy. Earlier versions are not maintained.  
+
+Start integrating with [Cosmos SDK Eden (v0.50)](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.5) and enjoy and the new features and performance improvements.
diff --git a/testutil/sims/app_helpers.go b/testutil/sims/app_helpers.go
@@ -106,6 +106,21 @@ func SetupAtGenesis(appConfig depinject.Config, extraOutputs ...interface{}) (*r
 	return SetupWithConfiguration(appConfig, cfg, extraOutputs...)
 }
 
+// NextBlock starts a new block.
+func NextBlock(app *runtime.App, ctx sdk.Context, jumpTime time.Duration) sdk.Context {
+	app.EndBlock(abci.RequestEndBlock{Height: ctx.BlockHeight()})
+
+	app.Commit()
+
+	newBlockTime := ctx.BlockTime().Add(jumpTime)
+	nextHeight := ctx.BlockHeight() + 1
+	newHeader := tmproto.Header{Height: nextHeight, Time: newBlockTime}
+
+	app.BeginBlock(abci.RequestBeginBlock{Header: newHeader})
+
+	return app.NewUncachedContext(false, newHeader)
+}
+
 // SetupWithConfiguration initializes a new runtime.App. A Nop logger is set in runtime.App.
 // appConfig defines the application configuration (f.e. app_config.go).
 // extraOutputs defines the extra outputs to be assigned by the dependency injector (depinject).

diff --git a/x/slashing/keeper/slash_redelegation_test.go b/x/slashing/keeper/slash_redelegation_test.go
@@ -0,0 +1,159 @@
+package keeper_test
+
+import (
+	"testing"
+	"time"
+
+	"cosmossdk.io/depinject"
+	"cosmossdk.io/log"
+	"cosmossdk.io/math"
+	tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
+	bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
+	banktestutil "github.com/cosmos/cosmos-sdk/x/bank/testutil"
+	slashingkeeper "github.com/cosmos/cosmos-sdk/x/slashing/keeper"
+	"github.com/cosmos/cosmos-sdk/x/slashing/testutil"
+	stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper"
+	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+
+	distributionkeeper "github.com/cosmos/cosmos-sdk/x/distribution/keeper"
+
+	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/stretchr/testify/require"
+
+	simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
+)
+
+func TestSlashRedelegation(t *testing.T) {
+	// setting up
+	var stakingKeeper *stakingkeeper.Keeper
+	var bankKeeper bankkeeper.Keeper
+	var slashKeeper slashingkeeper.Keeper
+	var distrKeeper distributionkeeper.Keeper
+
+	app, err := simtestutil.Setup(depinject.Configs(
+		depinject.Supply(log.NewNopLogger()),
+		testutil.AppConfig,
+	), &stakingKeeper, &bankKeeper, &slashKeeper, &distrKeeper)
+	require.NoError(t, err)
+
+	// get sdk context, staking msg server and bond denom
+	ctx := app.BaseApp.NewContext(false, tmproto.Header{Height: app.LastBlockHeight() + 1})
+	stakingMsgServer := stakingkeeper.NewMsgServerImpl(stakingKeeper)
+	bondDenom := stakingKeeper.BondDenom(ctx)
+	require.NoError(t, err)
+
+	// evilVal will be slashed, goodVal won't be slashed
+	evilValPubKey := secp256k1.GenPrivKey().PubKey()
+	goodValPubKey := secp256k1.GenPrivKey().PubKey()
+
+	// both test acc 1 and 2 delegated to evil val, both acc should be slashed when evil val is slashed
+	// test acc 1 use the "undelegation after redelegation" trick (redelegate to good val and then undelegate) to avoid slashing
+	// test acc 2 only undelegate from evil val
+	testAcc1 := sdk.AccAddress([]byte("addr1_______________"))
+	testAcc2 := sdk.AccAddress([]byte("addr2_______________"))
+
+	// fund acc 1 and acc 2
+	testCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, stakingKeeper.TokensFromConsensusPower(ctx, 10)))
+	banktestutil.FundAccount(bankKeeper, ctx, testAcc1, testCoins)
+	banktestutil.FundAccount(bankKeeper, ctx, testAcc2, testCoins)
+
+	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())
+
+	// creating evil val
+	evilValAddr := sdk.ValAddress(evilValPubKey.Address())
+	banktestutil.FundAccount(bankKeeper, ctx, sdk.AccAddress(evilValAddr), testCoins)
+	createValMsg1, _ := stakingtypes.NewMsgCreateValidator(
+		evilValAddr, evilValPubKey, testCoins[0], 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())
+	banktestutil.FundAccount(bankKeeper, ctx, sdk.AccAddress(goodValAddr), testCoins)
+	createValMsg2, _ := stakingtypes.NewMsgCreateValidator(
+		goodValAddr, goodValPubKey, testCoins[0], 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, commit height 2, move to height 3
+	// acc 1 and acc 2 delegate to evil val
+	ctx = simtestutil.NextBlock(app, ctx, time.Duration(1))
+	require.NoError(t, err)
+
+	// Acc 2 delegate
+	delMsg := stakingtypes.NewMsgDelegate(testAcc2, evilValAddr, testCoins[0])
+	_, err = stakingMsgServer.Delegate(ctx, delMsg)
+	require.NoError(t, err)
+
+	// Acc 1 delegate
+	delMsg = stakingtypes.NewMsgDelegate(testAcc1, evilValAddr, testCoins[0])
+	_, err = stakingMsgServer.Delegate(ctx, delMsg)
+	require.NoError(t, err)
+
+	// next block, commit height 3, move to height 4
+	// with the new delegations, evil val increases in voting power and commit byzantine behaviour at height 4 consensus
+	// at the same time, acc 1 and acc 2 withdraw delegation from evil val
+	ctx = simtestutil.NextBlock(app, ctx, time.Duration(1))
+	require.NoError(t, err)
+
+	evilVal, found := stakingKeeper.GetValidator(ctx, evilValAddr)
+	require.True(t, found)
+
+	evilPower := stakingKeeper.TokensToConsensusPower(ctx, evilVal.Tokens)
+
+	// Acc 1 redelegate from evil val to good val
+	redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1, evilValAddr, goodValAddr, testCoins[0])
+	_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
+	require.NoError(t, err)
+
+	// Acc 1 undelegate from good val
+	undelMsg := stakingtypes.NewMsgUndelegate(testAcc1, goodValAddr, testCoins[0])
+	_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
+	require.NoError(t, err)
+
+	// Acc 2 undelegate from evil val
+	undelMsg = stakingtypes.NewMsgUndelegate(testAcc2, evilValAddr, testCoins[0])
+	_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
+	require.NoError(t, err)
+
+	// next block, commit height 4, move to height 5
+	// Slash evil val for byzantine behaviour at height 4 consensus,
+	// at which acc 1 and acc 2 still contributed to evil val voting power
+	// even tho they undelegate at block 4, the valset update is applied after commited block 4 when height 4 consensus already passes
+	ctx = simtestutil.NextBlock(app, ctx, time.Duration(1))
+	require.NoError(t, err)
+
+	// slash evil val with slash factor = 0.9, leaving only 10% of stake after slashing
+	evilVal, _ = stakingKeeper.GetValidator(ctx, evilValAddr)
+	evilValConsAddr, err := evilVal.GetConsAddr()
+	require.NoError(t, err)
+
+	slashKeeper.Slash(ctx, evilValConsAddr, math.LegacyMustNewDecFromStr("0.9"), evilPower, 4)
+
+	// assert invariant to make sure we conduct slashing correctly
+	_, 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)
+
+	// one eternity later
+	ctx = simtestutil.NextBlock(app, ctx, time.Duration(1000000000000000000))
+	ctx = simtestutil.NextBlock(app, ctx, time.Duration(1))
+
+	// confirm that account 1 and account 2 has been slashed, and the slash amount is correct
+	balance1AfterSlashing := bankKeeper.GetBalance(ctx, testAcc1, bondDenom)
+	balance2AfterSlashing := bankKeeper.GetBalance(ctx, testAcc2, bondDenom)
+
+	require.Equal(t, balance1AfterSlashing.Amount.Mul(math.NewIntFromUint64(10)).String(), balance1Before.Amount.String())
+	require.Equal(t, balance2AfterSlashing.Amount.Mul(math.NewIntFromUint64(10)).String(), balance2Before.Amount.String())
+}
diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go
@@ -260,9 +260,46 @@ func (k Keeper) SlashRedelegation(ctx sdk.Context, srcValidator types.Validator,
 		slashAmount := slashAmountDec.TruncateInt()
 		totalSlashAmount = totalSlashAmount.Add(slashAmount)
 
+		validatorDstAddress, err := sdk.ValAddressFromBech32(redelegation.ValidatorDstAddress)
+		if err != nil {
+			panic(err)
+		}
+		// Handle undelegation after redelegation
+		// Prioritize slashing unbondingDelegation than delegation
+		unbondingDelegation, found := k.GetUnbondingDelegation(ctx, sdk.MustAccAddressFromBech32(redelegation.DelegatorAddress), validatorDstAddress)
+		if found {
+			for i, entry := range unbondingDelegation.Entries {
+				// slash with the amount of `slashAmount` if possible, else slash all unbonding token
+				unbondingSlashAmount := math.MinInt(slashAmount, entry.Balance)
+
+				switch {
+				// There's no token to slash
+				case unbondingSlashAmount.IsZero():
+					continue
+				// If unbonding started before this height, stake didn't contribute to infraction
+				case entry.CreationHeight < infractionHeight:
+					continue
+				// Unbonding delegation no longer eligible for slashing, skip it
+				case entry.IsMature(now) && !entry.OnHold():
+					continue
+				// Slash the unbonding delegation
+				default:
+					// update remaining slashAmount
+					slashAmount = slashAmount.Sub(unbondingSlashAmount)
+
+					notBondedBurnedAmount = notBondedBurnedAmount.Add(unbondingSlashAmount)
+					entry.Balance = entry.Balance.Sub(unbondingSlashAmount)
+					unbondingDelegation.Entries[i] = entry
+					k.SetUnbondingDelegation(ctx, unbondingDelegation)
+				}
+			}
+		}
+
+		// Slash the moved delegation
+
 		// Unbond from target validator
 		sharesToUnbond := slashFactor.Mul(entry.SharesDst)
-		if sharesToUnbond.IsZero() {
+		if sharesToUnbond.IsZero() || slashAmount.IsZero() {
 			continue
 		}