From 2c49839cba52844af532d86d82887e120b09dd76 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 15 Dec 2022 19:26:31 +0100 Subject: [PATCH 1/4] check x/bank send enabled before escrowing fees --- modules/apps/29-fee/keeper/msg_server.go | 8 ++++ modules/apps/29-fee/keeper/msg_server_test.go | 45 +++++++++++++++++++ modules/apps/29-fee/types/expected_keepers.go | 1 + 3 files changed, 54 insertions(+) diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go index eba43c563b8..7475b3a2910 100644 --- a/modules/apps/29-fee/keeper/msg_server.go +++ b/modules/apps/29-fee/keeper/msg_server.go @@ -92,6 +92,10 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) return nil, err } + if err = k.bankKeeper.IsSendEnabledCoins(ctx, msg.Fee.Total()...); err != nil { + return nil, err + } + if k.bankKeeper.BlockedAddr(refundAcc) { return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to escrow fees", refundAcc) } @@ -133,6 +137,10 @@ func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacket return nil, err } + if err = k.bankKeeper.IsSendEnabledCoins(ctx, msg.PacketFee.Fee.Total()...); err != nil { + return nil, err + } + if k.bankKeeper.BlockedAddr(refundAcc) { return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to escrow fees", refundAcc) } diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 7f8f08744d8..fb0d73171a9 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -2,6 +2,7 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" "github.com/cosmos/ibc-go/v6/modules/apps/29-fee/types" transfertypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" @@ -197,6 +198,17 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { }, true, }, + { + "bank send enabled for fee denom", + func() { + suite.chainA.GetSimApp().BankKeeper.SetParams(suite.chainA.GetContext(), + banktypes.Params{ + SendEnabled: []*banktypes.SendEnabled{{Denom: sdk.DefaultBondDenom, Enabled: true}}, + }, + ) + }, + true, + }, { "refund account is module account", func() { @@ -244,6 +256,17 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { }, false, }, + { + "bank send disabled for fee denom", + func() { + suite.chainA.GetSimApp().BankKeeper.SetParams(suite.chainA.GetContext(), + banktypes.Params{ + SendEnabled: []*banktypes.SendEnabled{{Denom: sdk.DefaultBondDenom, Enabled: false}}, + }, + ) + }, + false, + }, { "acknowledgement fee balance not found", func() { @@ -347,6 +370,17 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { }, true, }, + { + "bank send enabled for fee denom", + func() { + suite.chainA.GetSimApp().BankKeeper.SetParams(suite.chainA.GetContext(), + banktypes.Params{ + SendEnabled: []*banktypes.SendEnabled{{Denom: sdk.DefaultBondDenom, Enabled: true}}, + }, + ) + }, + true, + }, { "fee module is locked", func() { @@ -432,6 +466,17 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { }, false, }, + { + "bank send disabled for fee denom", + func() { + suite.chainA.GetSimApp().BankKeeper.SetParams(suite.chainA.GetContext(), + banktypes.Params{ + SendEnabled: []*banktypes.SendEnabled{{Denom: sdk.DefaultBondDenom, Enabled: false}}, + }, + ) + }, + true, + }, { "acknowledgement fee balance not found", func() { diff --git a/modules/apps/29-fee/types/expected_keepers.go b/modules/apps/29-fee/types/expected_keepers.go index c7168eef370..06f67f4cdcd 100644 --- a/modules/apps/29-fee/types/expected_keepers.go +++ b/modules/apps/29-fee/types/expected_keepers.go @@ -33,4 +33,5 @@ type BankKeeper interface { SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error BlockedAddr(sdk.AccAddress) bool + IsSendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error } From 4e9a28f367012c8067c1dd9f4700b3b5e14688eb Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 15 Dec 2022 19:36:19 +0100 Subject: [PATCH 2/4] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a9da1f226a..0333484265a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (06-solomachine) [\#2744](https://github.com/cosmos/ibc-go/pull/2744) `Misbehaviour.ValidateBasic()` now only enforces that signature data does not match when the signature paths are different. * (06-solomachine) [\#2748](https://github.com/cosmos/ibc-go/pull/2748) Adding sentinel value for header path in 06-solomachine. +* (apps/29-fee) [\#2942](https://github.com/cosmos/ibc-go/pull/2942) Check `x/bank` send enabled before escrowing fees. ### Improvements From c40a0b35ad5108019cea0056a0101fc5532cd7b1 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 16 Dec 2022 10:47:32 +0100 Subject: [PATCH 3/4] review comment --- modules/apps/29-fee/keeper/msg_server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go index 7475b3a2910..5e2587b48e4 100644 --- a/modules/apps/29-fee/keeper/msg_server.go +++ b/modules/apps/29-fee/keeper/msg_server.go @@ -92,7 +92,7 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) return nil, err } - if err = k.bankKeeper.IsSendEnabledCoins(ctx, msg.Fee.Total()...); err != nil { + if err := k.bankKeeper.IsSendEnabledCoins(ctx, msg.Fee.Total()...); err != nil { return nil, err } @@ -137,7 +137,7 @@ func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacket return nil, err } - if err = k.bankKeeper.IsSendEnabledCoins(ctx, msg.PacketFee.Fee.Total()...); err != nil { + if err := k.bankKeeper.IsSendEnabledCoins(ctx, msg.PacketFee.Fee.Total()...); err != nil { return nil, err } From 0fe6723b82dcbd1ecefc6ff8a1a095792a34026a Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 16 Dec 2022 21:03:34 +0100 Subject: [PATCH 4/4] fix test condition --- modules/apps/29-fee/keeper/msg_server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index fb0d73171a9..d767b3f4361 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -475,7 +475,7 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { }, ) }, - true, + false, }, { "acknowledgement fee balance not found",