From 9051f8bce6c1ec482ec4ee76b7076b8c28ccfb59 Mon Sep 17 00:00:00 2001 From: khanh <50263489+catShaark@users.noreply.github.com> Date: Tue, 27 Feb 2024 23:28:05 +0700 Subject: [PATCH 1/3] 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 --- CHANGELOG.md | 2 ++ simapp/app.go | 2 +- x/auth/migrations/v043/store_test.go | 1 + x/gov/keeper/common_test.go | 1 + x/staking/keeper/common_test.go | 1 + x/staking/keeper/grpc_query_test.go | 1 + x/staking/keeper/keeper.go | 5 +++ x/staking/keeper/slash.go | 49 +++++++++++++++++++++++++--- 8 files changed, 56 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70a47d009bd8..5a678cba8ca3 100644 --- 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 index 5261b552827c..7e7cf33d8efb 100644 --- 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 index df7902c191bd..3b29947d419a 100644 --- 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 index e5e5bf183109..d770c2986c8c 100644 --- 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 index ad31204ba43b..c755d4d3f07f 100644 --- 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 index 09cdb5e47cea..fd255350cbcd 100644 --- 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 index b3d6a2a317b1..d82c2f0b3591 100644 --- 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 index 40f5d85e8195..e6aecd4a9a23 100644 --- 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) From 0498c4fdbf0dbf592c25b854bb81ae0be83375dc Mon Sep 17 00:00:00 2001 From: yihuang Date: Wed, 28 Feb 2024 18:18:44 +0800 Subject: [PATCH 2/3] Update x/staking/keeper/slash.go Signed-off-by: yihuang --- x/staking/keeper/slash.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go index e6aecd4a9a23..4824600ac4f4 100644 --- a/x/staking/keeper/slash.go +++ b/x/staking/keeper/slash.go @@ -255,7 +255,7 @@ func (k Keeper) SlashRedelegation(ctx sdk.Context, srcValidator types.Validator, panic(err) } - if k.hardForkHeight > ctx.BlockHeight() { + if k.hardForkHeight >= ctx.BlockHeight() { // Handle undelegation after redelegation // Prioritize slashing unbondingDelegation than delegation unbondingDelegation, found := k.GetUnbondingDelegation(ctx, sdk.MustAccAddressFromBech32(redelegation.DelegatorAddress), valDstAddr) From 3ec552f42144f20b383d950b5115ad727add733c Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 29 Feb 2024 10:39:33 +0800 Subject: [PATCH 3/3] fix test --- x/staking/common_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/staking/common_test.go b/x/staking/common_test.go index 948cb3df5fc7..eb9f5a390426 100644 --- a/x/staking/common_test.go +++ b/x/staking/common_test.go @@ -49,6 +49,7 @@ func getBaseSimappWithCustomKeeper(t *testing.T) (*codec.LegacyAmino, *simapp.Si app.AccountKeeper, app.BankKeeper, app.GetSubspace(types.ModuleName), + 0, ) app.StakingKeeper.SetParams(ctx, types.DefaultParams())