diff --git a/CHANGELOG.md b/CHANGELOG.md index c41e42d710..42bf0cc57b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (crypto/ledger) [#24036](https://github.com/cosmos/cosmos-sdk/pull/24036) Improve error message when deriving paths using index > 100 * (gRPC) [#23844](https://github.com/cosmos/cosmos-sdk/pull/23844) Add debug log prints for each gRPC request. * (server) [#24072](https://github.com/cosmos/cosmos-sdk/pull/24072) Return BlockHeader by shallow copy in server Context. +* (x/bank) [#24053](https://github.com/cosmos/cosmos-sdk/pull/24053) Resolve a foot-gun by swapping send restrictions check in `InputOutputCoins` before coin deduction. + ### Bug Fixes diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index dee6c39d4c..b1ba26055a 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -675,11 +675,16 @@ func (suite *KeeperTestSuite) TestInputOutputCoins() { {Address: accAddrs[2].String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))}, } - require.Error(suite.bankKeeper.InputOutputCoins(ctx, input, []banktypes.Output{})) + // test that inputs with no outputs fails + require.ErrorContains(suite.bankKeeper.InputOutputCoins(ctx, input, []banktypes.Output{}), banktypes.ErrInputOutputMismatch.Error()) + // accounts has no funds, should error. suite.authKeeper.EXPECT().GetAccount(suite.ctx, accAddrs[0]).Return(acc0) - require.Error(suite.bankKeeper.InputOutputCoins(ctx, input, outputs)) + suite.authKeeper.EXPECT().HasAccount(suite.ctx, gomock.Any()).Return(true).Times(len(outputs)) + err := suite.bankKeeper.InputOutputCoins(ctx, input, outputs) + require.ErrorContains(err, "insufficient funds") + // fund account now. suite.mockFundAccount(accAddrs[0]) require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, accAddrs[0], balances)) @@ -692,15 +697,17 @@ func (suite *KeeperTestSuite) TestInputOutputCoins() { {Address: accAddrs[2].String(), Coins: sdk.NewCoins(newFooCoin(300), newBarCoin(100))}, } - require.Error(suite.bankKeeper.InputOutputCoins(ctx, insufficientInput, insufficientOutputs)) + // input: 300foo,100bar ==> output: 600foo,200bar. should fail + err = suite.bankKeeper.InputOutputCoins(ctx, insufficientInput, insufficientOutputs) + require.ErrorContains(err, banktypes.ErrInputOutputMismatch.Error()) + // should work with valid input/outputs. suite.mockInputOutputCoins([]sdk.AccountI{acc0}, accAddrs[1:3]) require.NoError(suite.bankKeeper.InputOutputCoins(ctx, input, outputs)) acc1Balances := suite.bankKeeper.GetAllBalances(ctx, accAddrs[0]) expected := sdk.NewCoins(newFooCoin(30), newBarCoin(10)) require.Equal(expected, acc1Balances) - acc2Balances := suite.bankKeeper.GetAllBalances(ctx, accAddrs[1]) require.Equal(expected, acc2Balances) @@ -708,6 +715,39 @@ func (suite *KeeperTestSuite) TestInputOutputCoins() { require.Equal(expected, acc3Balances) } +func (suite *KeeperTestSuite) TestInputOutputCoins_AccountCreated() { + ctx := suite.ctx + require := suite.Require() + balances := sdk.NewCoins(newFooCoin(90), newBarCoin(30)) + + acc0 := authtypes.NewBaseAccountWithAddress(accAddrs[0]) + input := banktypes.Input{ + Address: accAddrs[0].String(), Coins: sdk.NewCoins(newFooCoin(60), newBarCoin(20)), + } + outputs := []banktypes.Output{ + {Address: accAddrs[1].String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))}, + {Address: accAddrs[2].String(), Coins: sdk.NewCoins(newFooCoin(30), newBarCoin(10))}, + } + + suite.mockFundAccount(accAddrs[0]) + require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, accAddrs[0], balances)) + + // the input account should be retrieved. + suite.authKeeper.EXPECT().GetAccount(suite.ctx, accAddrs[0]).Return(acc0) + + // creates output account 1 + suite.authKeeper.EXPECT().HasAccount(suite.ctx, accAddrs[1].Bytes()).Return(false) + suite.authKeeper.EXPECT().NewAccountWithAddress(suite.ctx, accAddrs[1].Bytes()) + suite.authKeeper.EXPECT().SetAccount(suite.ctx, gomock.Any()) + + // creates output account 2 + suite.authKeeper.EXPECT().HasAccount(suite.ctx, accAddrs[2].Bytes()).Return(false) + suite.authKeeper.EXPECT().NewAccountWithAddress(suite.ctx, accAddrs[2].Bytes()) + suite.authKeeper.EXPECT().SetAccount(suite.ctx, gomock.Any()) + + require.NoError(suite.bankKeeper.InputOutputCoins(ctx, input, outputs)) +} + func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { type restrictionArgs struct { ctx context.Context @@ -774,6 +814,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { fromAddr := accAddrs[0] fromAcc := authtypes.NewBaseAccountWithAddress(fromAddr) inputAccs := []sdk.AccountI{fromAcc} + suite.authKeeper.EXPECT().GetAccount(suite.ctx, inputAccs[0].GetAddress()).Return(inputAccs[0]).AnyTimes() toAddr1 := accAddrs[1] toAddr2 := accAddrs[2] @@ -858,7 +899,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)), }, @@ -887,7 +928,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)), }, @@ -917,8 +958,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)), }, }, @@ -946,8 +987,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)), }, }, @@ -960,7 +1001,6 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { actualRestrictionArgs = nil suite.bankKeeper.SetSendRestriction(tc.fn) ctx := suite.ctx - suite.mockInputOutputCoins(inputAccs, tc.outputAddrs) input := banktypes.Input{ Address: fromAddr.String(), Coins: tc.inputCoins, @@ -970,6 +1010,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() { testFunc := func() { err = suite.bankKeeper.InputOutputCoins(ctx, input, tc.outputs) } + suite.authKeeper.EXPECT().HasAccount(gomock.Any(), gomock.Any()).Return(true).Times(len(tc.outputAddrs)) suite.Require().NotPanics(testFunc, "InputOutputCoins") if len(tc.expErr) > 0 { suite.Assert().EqualError(err, tc.expErr, "InputOutputCoins error") @@ -1404,10 +1445,12 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() { } suite.authKeeper.EXPECT().GetAccount(suite.ctx, accAddrs[0]).Return(acc0) + suite.authKeeper.EXPECT().HasAccount(gomock.Any(), gomock.Any()).Return(true).Times(len(outputs)) + require.Error(suite.bankKeeper.InputOutputCoins(ctx, input, outputs)) events := ctx.EventManager().ABCIEvents() - require.Equal(0, len(events)) + require.Equal(1, len(events)) // Set addr's coins but not accAddrs[1]'s coins suite.mockFundAccount(accAddrs[0]) @@ -1417,7 +1460,7 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() { require.NoError(suite.bankKeeper.InputOutputCoins(ctx, input, outputs)) events = ctx.EventManager().ABCIEvents() - require.Equal(12, len(events)) // 12 events because account funding causes extra minting + coin_spent + coin_recv events + require.Equal(13, len(events)) // 13 events because account funding causes extra minting + coin_spent + coin_recv events event1 := sdk.Event{ Type: sdk.EventTypeMessage, @@ -1442,7 +1485,7 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() { require.NoError(suite.bankKeeper.InputOutputCoins(ctx, input, outputs)) events = ctx.EventManager().ABCIEvents() - require.Equal(30, len(events)) // 27 due to account funding + coin_spent + coin_recv events + require.Equal(31, len(events)) // 31 due to account funding + coin_spent + coin_recv events event2 := sdk.Event{ Type: banktypes.EventTypeTransfer, @@ -1464,10 +1507,9 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() { abci.EventAttribute{Key: banktypes.AttributeKeySender, Value: accAddrs[0].String()}, abci.EventAttribute{Key: sdk.AttributeKeyAmount, Value: newCoins2.String()}, ) - // events are shifted due to the funding account events - require.Equal(abci.Event(event1), events[25]) - require.Equal(abci.Event(event2), events[27]) - require.Equal(abci.Event(event3), events[29]) + require.Contains(events, abci.Event(event1)) + require.Contains(events, abci.Event(event2)) + require.Contains(events, abci.Event(event3)) } func (suite *KeeperTestSuite) TestSpendableCoins() { diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index 9b2f22fc39..a4a35c14fa 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -152,11 +152,6 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, return err } - err = k.subUnlockedCoins(ctx, inAddress, input.Coins) - if err != nil { - return err - } - sdkCtx := sdk.UnwrapSDKContext(ctx) sdkCtx.EventManager().EmitEvent( sdk.NewEvent( @@ -165,9 +160,16 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, ), ) - var outAddress sdk.AccAddress + // ensure all coins can be sent + type toSend struct { + AddressStr string + Address []byte + Coins sdk.Coins + } + 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 } @@ -177,18 +179,11 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, return err } - if err := k.addCoins(ctx, outAddress, out.Coins); err != nil { - return err - } - - sdkCtx.EventManager().EmitEvent( - sdk.NewEvent( - types.EventTypeTransfer, - sdk.NewAttribute(types.AttributeKeyRecipient, outAddress.String()), - sdk.NewAttribute(types.AttributeKeySender, input.Address), - sdk.NewAttribute(sdk.AttributeKeyAmount, out.Coins.String()), - ), - ) + sending = append(sending, toSend{ + Address: outAddress, + AddressStr: out.Address, + Coins: out.Coins, + }) // Create account if recipient does not exist. // @@ -201,6 +196,24 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, } } + 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 + } + sdkCtx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeTransfer, + sdk.NewAttribute(types.AttributeKeyRecipient, out.AddressStr), + sdk.NewAttribute(types.AttributeKeySender, input.Address), + sdk.NewAttribute(sdk.AttributeKeyAmount, out.Coins.String()), + ), + ) + } + return nil }