refactor(x/bank): swap sendrestriction check in InputOutputCoins (#21976

)
cosmos · Oct 3, 2024 · bb7d11d · bb7d11d
1 parent 9c646d8
commit bb7d11d
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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