From 323cd78ebf2f89f4d9c1ed968c473fd3c37b872a Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 30 Sep 2024 08:58:07 +0200 Subject: [PATCH 1/3] refactor(x/bank): swap sendrestriction check in InputOutputCoins --- x/bank/keeper/keeper_test.go | 12 ++++++------ x/bank/keeper/send.go | 29 +++++++++++++++++++++-------- x/group/keeper/abci.go | 4 ++-- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index e625712f08b8..ddb432200726 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -878,7 +878,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { }, expErr: "restriction test error", expBals: expBals{ - from: sdk.NewCoins(newFooCoin(959), newBarCoin(412)), + from: sdk.NewCoins(newFooCoin(959), newBarCoin(500)), to1: sdk.NewCoins(newFooCoin(15)), to2: sdk.NewCoins(newFooCoin(26)), }, @@ -907,7 +907,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { }, }, expBals: expBals{ - from: sdk.NewCoins(newFooCoin(948), newBarCoin(400)), + from: sdk.NewCoins(newFooCoin(948), newBarCoin(488)), to1: sdk.NewCoins(newFooCoin(26)), to2: sdk.NewCoins(newFooCoin(26), newBarCoin(12)), }, @@ -937,8 +937,8 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { }, expErr: "second restriction error", expBals: expBals{ - from: sdk.NewCoins(newFooCoin(904), newBarCoin(400)), - to1: sdk.NewCoins(newFooCoin(38)), + from: sdk.NewCoins(newFooCoin(948), newBarCoin(488)), + to1: sdk.NewCoins(newFooCoin(26)), to2: sdk.NewCoins(newFooCoin(26), newBarCoin(12)), }, }, @@ -966,8 +966,8 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { }, }, expBals: expBals{ - from: sdk.NewCoins(newFooCoin(904), newBarCoin(365)), - to1: sdk.NewCoins(newFooCoin(38), newBarCoin(25)), + from: sdk.NewCoins(newFooCoin(948), newBarCoin(453)), + to1: sdk.NewCoins(newFooCoin(26), newBarCoin(25)), to2: sdk.NewCoins(newFooCoin(26), newBarCoin(22)), }, }, diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index 063417b6ca03..929ff2e50285 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -148,14 +148,15 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, return err } - err = k.subUnlockedCoins(ctx, inAddress, input.Coins) - if err != nil { - return err + // ensure all coins can be sent + type toSend struct { + AddressStr string + Address []byte + Coins sdk.Coins } - - var outAddress sdk.AccAddress + sending := make([]toSend, 0) for _, out := range outputs { - outAddress, err = k.ak.AddressCodec().StringToBytes(out.Address) + outAddress, err := k.ak.AddressCodec().StringToBytes(out.Address) if err != nil { return err } @@ -165,13 +166,25 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, return err } - if err := k.addCoins(ctx, outAddress, out.Coins); err != nil { + sending = append(sending, toSend{ + Address: outAddress, + AddressStr: out.Address, + Coins: out.Coins, + }) + } + + if err := k.subUnlockedCoins(ctx, inAddress, input.Coins); err != nil { + return err + } + + for _, out := range sending { + if err := k.addCoins(ctx, out.Address, out.Coins); err != nil { return err } if err := k.EventService.EventManager(ctx).EmitKV( types.EventTypeTransfer, - event.NewAttribute(types.AttributeKeyRecipient, out.Address), + event.NewAttribute(types.AttributeKeyRecipient, out.AddressStr), event.NewAttribute(types.AttributeKeySender, input.Address), event.NewAttribute(sdk.AttributeKeyAmount, out.Coins.String()), ); err != nil { diff --git a/x/group/keeper/abci.go b/x/group/keeper/abci.go index 6de580de74ba..c8d980375496 100644 --- a/x/group/keeper/abci.go +++ b/x/group/keeper/abci.go @@ -3,7 +3,7 @@ package keeper import ( "context" - "cosmossdk.io/x/gov/types" + "cosmossdk.io/x/group" "github.com/cosmos/cosmos-sdk/telemetry" ) @@ -12,7 +12,7 @@ import ( // prunes expired proposals. func (k Keeper) EndBlocker(ctx context.Context) error { start := telemetry.Now() - defer telemetry.ModuleMeasureSince(types.ModuleName, start, telemetry.MetricKeyEndBlocker) + defer telemetry.ModuleMeasureSince(group.ModuleName, start, telemetry.MetricKeyEndBlocker) if err := k.TallyProposalsAtVPEnd(ctx); err != nil { return err From 88dbd51649a755e667fbf0432e7c729cba7cfcdf Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 30 Sep 2024 09:14:38 +0200 Subject: [PATCH 2/3] cl --- x/bank/CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/x/bank/CHANGELOG.md b/x/bank/CHANGELOG.md index f226eac88503..7cfd8ed0410b 100644 --- a/x/bank/CHANGELOG.md +++ b/x/bank/CHANGELOG.md @@ -35,13 +35,14 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. * [#20517](https://github.com/cosmos/cosmos-sdk/pull/20517) `SendCoins` now checks for `SendRestrictions` before instead of after deducting coins using `subUnlockedCoins`. * [#20354](https://github.com/cosmos/cosmos-sdk/pull/20354) Reduce the number of `ValidateDenom` calls in `bank.SendCoins`. +* [#21976](https://github.com/cosmos/cosmos-sdk/pull/21976) Resolve a footgun by swapping send restrictions check in `InputOutputCoins` before coin deduction. ### Bug Fixes * [#21407](https://github.com/cosmos/cosmos-sdk/pull/21407) Fix handling of negative spendable balances. - * The `SpendableBalances` query now correctly reports spendable balances when one or more denoms are negative (used to report all zeros). Also, this query now looks up only the balances for the requested page. - * The `SpendableCoins` keeper method now returns the positive spendable balances even when one or more denoms have more locked than available (used to return an empty `Coins`). - * The `SpendableCoin` keeper method now returns a zero coin if there's more locked than available (used to return a negative coin). + * The `SpendableBalances` query now correctly reports spendable balances when one or more denoms are negative (used to report all zeros). Also, this query now looks up only the balances for the requested page. + * The `SpendableCoins` keeper method now returns the positive spendable balances even when one or more denoms have more locked than available (used to return an empty `Coins`). + * The `SpendableCoin` keeper method now returns a zero coin if there's more locked than available (used to return a negative coin). ### API Breaking Changes From a0518d0cac38cc572ac620a6a5457b803de445e2 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 2 Oct 2024 13:26:24 +0200 Subject: [PATCH 3/3] updates --- x/bank/keeper/keeper_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index ddb432200726..4fe765039638 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -790,6 +790,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { suite.Require().NoError(err) fromAcc := authtypes.NewBaseAccountWithAddress(fromAddr) inputAccs := []sdk.AccountI{fromAcc} + suite.authKeeper.EXPECT().GetAccount(suite.ctx, inputAccs[0].GetAddress()).Return(inputAccs[0]).AnyTimes() toAddr1 := accAddrs[1] toAddr1Str, err := suite.authKeeper.AddressCodec().BytesToString(toAddr1) suite.Require().NoError(err) @@ -980,7 +981,6 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { actualRestrictionArgs = nil suite.bankKeeper.SetSendRestriction(tc.fn) ctx := suite.ctx - suite.mockInputOutputCoins(inputAccs, tc.outputAddrs) input := banktypes.Input{ Address: fromStrAddr, Coins: tc.inputCoins,