diff --git a/x/bank/CHANGELOG.md b/x/bank/CHANGELOG.md index f226eac885..7cfd8ed041 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 diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index e625712f08..4fe7650396 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) @@ -878,7 +879,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 +908,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 +938,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 +967,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)), }, }, @@ -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, diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index 063417b6ca..929ff2e502 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 6de580de74..c8d9803754 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