Problem: GHSA-86h5-xcpx-cfqc is not merged

- cherry-pick the fix in a hardfork style. * fix slashing logic * add test * changelog + release notes * word --------- Co-authored-by: Julien Robert <[email protected]>
yihuang · Feb 28, 2024 · 9051f8b · 9051f8b
1 parent 7799bba
commit 9051f8b
Show file tree

Hide file tree

Showing 8 changed files with 56 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -35,6 +35,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 # Changelog
 
+* (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.46.16](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.46.16) - 2023-11-07
 
 EOL notice. This is the last release of the `v0.46.x` line. Per this version, the v0.46.x line reached its end-of-life.

diff --git a/simapp/app.go b/simapp/app.go
@@ -264,7 +264,7 @@ func NewSimApp(
 		appCodec, keys[banktypes.StoreKey], app.AccountKeeper, app.GetSubspace(banktypes.ModuleName), app.ModuleAccountAddrs(),
 	)
 	stakingKeeper := stakingkeeper.NewKeeper(
-		appCodec, keys[stakingtypes.StoreKey], app.AccountKeeper, app.BankKeeper, app.GetSubspace(stakingtypes.ModuleName),
+		appCodec, keys[stakingtypes.StoreKey], app.AccountKeeper, app.BankKeeper, app.GetSubspace(stakingtypes.ModuleName), 0,
 	)
 	app.MintKeeper = mintkeeper.NewKeeper(
 		appCodec, keys[minttypes.StoreKey], app.GetSubspace(minttypes.ModuleName), &stakingKeeper,

diff --git a/x/auth/migrations/v043/store_test.go b/x/auth/migrations/v043/store_test.go
@@ -658,6 +658,7 @@ func createValidator(t *testing.T, ctx sdk.Context, app *simapp.SimApp, powers i
 		app.AccountKeeper,
 		app.BankKeeper,
 		app.GetSubspace(stakingtypes.ModuleName),
+		0,
 	)
 
 	val1, err := stakingtypes.NewValidator(valAddrs[0], pks[0], stakingtypes.Description{})

diff --git a/x/gov/keeper/common_test.go b/x/gov/keeper/common_test.go
@@ -48,6 +48,7 @@ func createValidators(t *testing.T, ctx sdk.Context, app *simapp.SimApp, powers
 		app.AccountKeeper,
 		app.BankKeeper,
 		app.GetSubspace(stakingtypes.ModuleName),
+		0,
 	)
 
 	val1, err := stakingtypes.NewValidator(valAddrs[0], pks[0], stakingtypes.Description{})

diff --git a/x/staking/keeper/common_test.go b/x/staking/keeper/common_test.go
@@ -31,6 +31,7 @@ func createTestInput(t *testing.T) (*codec.LegacyAmino, *simapp.SimApp, sdk.Cont
 		app.AccountKeeper,
 		app.BankKeeper,
 		app.GetSubspace(types.ModuleName),
+		0,
 	)
 	return app.LegacyAmino(), app, ctx
 }

diff --git a/x/staking/keeper/grpc_query_test.go b/x/staking/keeper/grpc_query_test.go
@@ -796,6 +796,7 @@ func createValidators(t *testing.T, ctx sdk.Context, app *simapp.SimApp, powers
 		app.AccountKeeper,
 		app.BankKeeper,
 		app.GetSubspace(types.ModuleName),
+		0,
 	)
 
 	val1 := teststaking.NewValidator(t, valAddrs[0], pks[0])

diff --git a/x/staking/keeper/keeper.go b/x/staking/keeper/keeper.go
@@ -27,12 +27,15 @@ type Keeper struct {
 	bankKeeper types.BankKeeper
 	hooks      types.StakingHooks
 	paramstore paramtypes.Subspace
+
+	hardForkHeight int64
 }
 
 // NewKeeper creates a new staking Keeper instance
 func NewKeeper(
 	cdc codec.BinaryCodec, key storetypes.StoreKey, ak types.AccountKeeper, bk types.BankKeeper,
 	ps paramtypes.Subspace,
+	hardForkHeight int64,
 ) Keeper {
 	// set KeyTable if it has not already been set
 	if !ps.HasKeyTable() {
@@ -55,6 +58,8 @@ func NewKeeper(
 		bankKeeper: bk,
 		paramstore: ps,
 		hooks:      nil,
+
+		hardForkHeight: hardForkHeight,
 	}
 }
 

diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go
@@ -250,17 +250,56 @@ func (k Keeper) SlashRedelegation(ctx sdk.Context, srcValidator types.Validator,
 		slashAmount := slashAmountDec.TruncateInt()
 		totalSlashAmount = totalSlashAmount.Add(slashAmount)
 
+		valDstAddr, err := sdk.ValAddressFromBech32(redelegation.ValidatorDstAddress)
+		if err != nil {
+			panic(err)
+		}
+
+		if k.hardForkHeight > ctx.BlockHeight() {
+			// Handle undelegation after redelegation
+			// Prioritize slashing unbondingDelegation than delegation
+			unbondingDelegation, found := k.GetUnbondingDelegation(ctx, sdk.MustAccAddressFromBech32(redelegation.DelegatorAddress), valDstAddr)
+			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):
+						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)
+					}
+				}
+			}
+
+			if slashAmount.IsZero() {
+				continue
+			}
+		}
+
+		// Slash the moved delegation
+
 		// Unbond from target validator
 		sharesToUnbond := slashFactor.Mul(entry.SharesDst)
 		if sharesToUnbond.IsZero() {
 			continue
 		}
 
-		valDstAddr, err := sdk.ValAddressFromBech32(redelegation.ValidatorDstAddress)
-		if err != nil {
-			panic(err)
-		}
-
 		delegatorAddress := sdk.MustAccAddressFromBech32(redelegation.DelegatorAddress)
 
 		delegation, found := k.GetDelegation(ctx, delegatorAddress, valDstAddr)