From eb1ff79690f0ff58b33e9fb43cd795d4ed17e861 Mon Sep 17 00:00:00 2001 From: Sean King Date: Tue, 9 Aug 2022 21:01:21 +0200 Subject: [PATCH 1/2] fix: adding check for blocked addresses before escrowing fees (#1890) * fix: adding check for blocked addresses before escrowing fees * refactor: move check below isLocked check * refactor: use sdk error instead of fee error * feat: adding check to RegisterPayee * chore: format import * refactor: remove dist module from unblocked addr (cherry picked from commit 7694903af2990e7d814ece1016515e1279f6758d) # Conflicts: # modules/apps/29-fee/keeper/msg_server_test.go --- modules/apps/29-fee/keeper/msg_server.go | 27 +++++++++++++ modules/apps/29-fee/keeper/msg_server_test.go | 40 +++++++++++++++++++ testing/simapp/app.go | 2 +- 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go index c2beea28da3..461631d755f 100644 --- a/modules/apps/29-fee/keeper/msg_server.go +++ b/modules/apps/29-fee/keeper/msg_server.go @@ -20,6 +20,15 @@ var _ types.MsgServer = Keeper{} func (k Keeper) RegisterPayee(goCtx context.Context, msg *types.MsgRegisterPayee) (*types.MsgRegisterPayeeResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + payee, err := sdk.AccAddressFromBech32(msg.Payee) + if err != nil { + return nil, err + } + + if k.bankKeeper.BlockedAddr(payee) { + return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not authorized to be a payee", payee) + } + // only register payee address if the channel exists and is fee enabled if _, found := k.channelKeeper.GetChannel(ctx, msg.PortId, msg.ChannelId); !found { return nil, channeltypes.ErrChannelNotFound @@ -78,6 +87,15 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) return nil, types.ErrFeeModuleLocked } + refundAcc, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return nil, err + } + + if k.bankKeeper.BlockedAddr(refundAcc) { + return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to escrow fees", refundAcc) + } + // get the next sequence sequence, found := k.GetNextSequenceSend(ctx, msg.SourcePortId, msg.SourceChannelId) if !found { @@ -110,6 +128,15 @@ func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacket return nil, types.ErrFeeModuleLocked } + refundAcc, err := sdk.AccAddressFromBech32(msg.PacketFee.RefundAddress) + if err != nil { + return nil, err + } + + if k.bankKeeper.BlockedAddr(refundAcc) { + return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to escrow fees", refundAcc) + } + nextSeqSend, found := k.GetNextSequenceSend(ctx, msg.PacketId.PortId, msg.PacketId.ChannelId) if !found { return nil, sdkerrors.Wrapf(channeltypes.ErrSequenceSendNotFound, "channel does not exist, portID: %s, channelID: %s", msg.PacketId.PortId, msg.PacketId.ChannelId) diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 73c7caadc9c..d405541f9b2 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -2,12 +2,22 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" +<<<<<<< HEAD disttypes "github.com/cosmos/cosmos-sdk/x/distribution/types" "github.com/cosmos/ibc-go/v4/modules/apps/29-fee/types" clienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v4/testing" +======= + + "github.com/cosmos/ibc-go/v5/modules/apps/29-fee/types" + transfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types" + clienttypes "github.com/cosmos/ibc-go/v5/modules/core/02-client/types" + channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types" + ibctesting "github.com/cosmos/ibc-go/v5/testing" + ibcmock "github.com/cosmos/ibc-go/v5/testing/mock" +>>>>>>> 7694903 (fix: adding check for blocked addresses before escrowing fees (#1890)) ) func (suite *KeeperTestSuite) TestRegisterPayee() { @@ -39,6 +49,20 @@ func (suite *KeeperTestSuite) TestRegisterPayee() { suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) }, }, + { + "given payee is not an sdk address", + false, + func() { + msg.Payee = "invalid-addr" + }, + }, + { + "payee is a blocked address", + false, + func() { + msg.Payee = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(transfertypes.ModuleName).String() + }, + }, } for _, tc := range testCases { @@ -222,6 +246,14 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { }, false, }, + { + "refund account is a blocked address", + func() { + blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() + msg.Signer = blockedAddr.String() + }, + false, + }, { "acknowledgement fee balance not found", func() { @@ -401,6 +433,14 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { }, false, }, + { + "refund account is a blocked address", + func() { + blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() + msg.PacketFee.RefundAddress = blockedAddr.String() + }, + false, + }, { "acknowledgement fee balance not found", func() { diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 744e8c96b01..7ff42474038 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -655,7 +655,7 @@ func (app *SimApp) LoadHeight(height int64) error { func (app *SimApp) ModuleAccountAddrs() map[string]bool { modAccAddrs := make(map[string]bool) for acc := range maccPerms { - // do not add mock module to blocked addresses + // do not add the following modules to blocked addresses // this is only used for testing if acc == ibcmock.ModuleName { continue From c912ccd157183f2974a7c3d0fc0a5b22e7bd0235 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 9 Aug 2022 21:28:20 +0200 Subject: [PATCH 2/2] fix conflicts --- modules/apps/29-fee/keeper/msg_server_test.go | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index d405541f9b2..a7024abecc3 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -2,28 +2,17 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" -<<<<<<< HEAD - disttypes "github.com/cosmos/cosmos-sdk/x/distribution/types" - + "github.com/cosmos/ibc-go/v4/modules/apps/29-fee/types" + transfertypes "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v4/testing" -======= - - "github.com/cosmos/ibc-go/v5/modules/apps/29-fee/types" - transfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types" - clienttypes "github.com/cosmos/ibc-go/v5/modules/core/02-client/types" - channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types" - ibctesting "github.com/cosmos/ibc-go/v5/testing" - ibcmock "github.com/cosmos/ibc-go/v5/testing/mock" ->>>>>>> 7694903 (fix: adding check for blocked addresses before escrowing fees (#1890)) + ibcmock "github.com/cosmos/ibc-go/v4/testing/mock" ) func (suite *KeeperTestSuite) TestRegisterPayee() { - var ( - msg *types.MsgRegisterPayee - ) + var msg *types.MsgRegisterPayee testCases := []struct { name string @@ -211,7 +200,7 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { { "refund account is module account", func() { - msg.Signer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(disttypes.ModuleName).String() + msg.Signer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(ibcmock.ModuleName).String() expPacketFee := types.NewPacketFee(fee, msg.Signer, nil) expFeesInEscrow = []types.PacketFee{expPacketFee} },