From e4ee61d071a8b71043c616c320710676a9daaed6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 3 Aug 2022 13:13:29 +0200 Subject: [PATCH 1/4] fix bug, add test Ensures that a sender account isn't a blocked address Added test cases for MsgTransfer handling --- modules/apps/transfer/keeper/msg_server.go | 6 ++ .../apps/transfer/keeper/msg_server_test.go | 71 +++++++++++++++++++ testing/chain.go | 6 ++ 3 files changed, 83 insertions(+) create mode 100644 modules/apps/transfer/keeper/msg_server_test.go diff --git a/modules/apps/transfer/keeper/msg_server.go b/modules/apps/transfer/keeper/msg_server.go index 219990c957d..6f4ec1157ae 100644 --- a/modules/apps/transfer/keeper/msg_server.go +++ b/modules/apps/transfer/keeper/msg_server.go @@ -4,6 +4,7 @@ import ( "context" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types" ) @@ -20,6 +21,11 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types. if err != nil { return nil, err } + + if k.bankKeeper.BlockedAddr(sender) { + return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", sender) + } + if err := k.SendTransfer( ctx, msg.SourcePort, msg.SourceChannel, msg.Token, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp, ); err != nil { diff --git a/modules/apps/transfer/keeper/msg_server_test.go b/modules/apps/transfer/keeper/msg_server_test.go new file mode 100644 index 00000000000..9d838a99f11 --- /dev/null +++ b/modules/apps/transfer/keeper/msg_server_test.go @@ -0,0 +1,71 @@ +package keeper_test + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types" +) + +func (suite *KeeperTestSuite) TestMsgTransfer() { + var msg *types.MsgTransfer + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", + func() {}, + true, + }, + { + "invalid sender", + func() { + msg.Sender = "address" + }, + false, + }, + { + "sender is a blocked address", + func() { + msg.Sender = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(types.ModuleName).String() + }, + false, + }, + { + "channel does not exist", + func() { + msg.SourceChannel = "channel-100" + }, + false, + }, + } + + for _, tc := range testCases { + suite.SetupTest() + + path := NewTransferPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + coin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + msg = types.NewMsgTransfer( + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + coin, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), + suite.chainB.GetTimeoutHeight(), 0, // only use timeout height + ) + + tc.malleate() + + res, err := suite.chainA.GetSimApp().TransferKeeper.Transfer(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) + + if tc.expPass { + suite.Require().NoError(err) + suite.Require().NotNil(res) + } else { + suite.Require().Error(err) + suite.Require().Nil(res) + } + } +} diff --git a/testing/chain.go b/testing/chain.go index ab7f707007c..83401d7700c 100644 --- a/testing/chain.go +++ b/testing/chain.go @@ -575,3 +575,9 @@ func (chain *TestChain) GetChannelCapability(portID, channelID string) *capabili return cap } + +// GetTimeoutHeight is a convenience function which returns a IBC packet timeout height +// to be used for testing. It returns the current IBC height + 100 blocks +func (chain *TestChain) GetTimeoutHeight() clienttypes.Height { + return clienttypes.NewHeight(clienttypes.ParseChainID(chain.ChainID), uint64(chain.GetContext().BlockHeight())+100) +} From 58bd0f7f3a6a9687a18dde374f80ced858977712 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 3 Aug 2022 13:16:24 +0200 Subject: [PATCH 2/4] update documentation --- modules/apps/transfer/keeper/msg_server.go | 2 -- modules/apps/transfer/keeper/relay.go | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/transfer/keeper/msg_server.go b/modules/apps/transfer/keeper/msg_server.go index 6f4ec1157ae..a3a0ae0562c 100644 --- a/modules/apps/transfer/keeper/msg_server.go +++ b/modules/apps/transfer/keeper/msg_server.go @@ -11,8 +11,6 @@ import ( var _ types.MsgServer = Keeper{} -// See createOutgoingPacket in spec:https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer#packet-relay - // Transfer defines a rpc handler method for MsgTransfer. func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.MsgTransferResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 6747ac5cbd7..13864d7e620 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -48,6 +48,8 @@ import ( // 4. A -> C : sender chain is sink zone. Denom upon receiving: 'C/B/denom' // 5. C -> B : sender chain is sink zone. Denom upon receiving: 'B/denom' // 6. B -> A : sender chain is sink zone. Denom upon receiving: 'denom' +// +// Note: An IBC Transfer must be initiated using a MsgTransfer via the Transfer rpc handler func (k Keeper) SendTransfer( ctx sdk.Context, sourcePort, From baf8f2c156ea062f8314857a11e9e798f0bc4ca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 3 Aug 2022 14:00:28 +0200 Subject: [PATCH 3/4] move blocked address check to SendTransfer --- modules/apps/transfer/keeper/msg_server.go | 5 ----- modules/apps/transfer/keeper/relay.go | 4 ++++ modules/apps/transfer/keeper/relay_test.go | 13 +++++++++++-- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/modules/apps/transfer/keeper/msg_server.go b/modules/apps/transfer/keeper/msg_server.go index a3a0ae0562c..f899f70d229 100644 --- a/modules/apps/transfer/keeper/msg_server.go +++ b/modules/apps/transfer/keeper/msg_server.go @@ -4,7 +4,6 @@ import ( "context" sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types" ) @@ -20,10 +19,6 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types. return nil, err } - if k.bankKeeper.BlockedAddr(sender) { - return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", sender) - } - if err := k.SendTransfer( ctx, msg.SourcePort, msg.SourceChannel, msg.Token, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp, ); err != nil { diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 13864d7e620..8821c911c33 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -64,6 +64,10 @@ func (k Keeper) SendTransfer( return types.ErrSendDisabled } + if k.bankKeeper.BlockedAddr(sender) { + return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", sender) + } + sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel) if !found { return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", sourcePort, sourceChannel) diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index 7cfb0cae2fc..844549c7628 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/modules/apps/transfer/keeper/relay_test.go @@ -20,6 +20,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() { var ( amount sdk.Coin path *ibctesting.Path + sender sdk.AccAddress err error ) @@ -68,7 +69,14 @@ func (suite *KeeperTestSuite) TestSendTransfer() { amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) }, true, false, }, - + { + "transfer failed - sender account is blocked", + func() { + suite.coordinator.CreateTransferChannels(path) + amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + sender = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(types.ModuleName) + }, true, false, + }, // createOutgoingPacket tests // - source chain { @@ -106,6 +114,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() { suite.SetupTest() // reset path = NewTransferPath(suite.chainA, suite.chainB) suite.coordinator.SetupConnections(path) + sender = suite.chainA.SenderAccount.GetAddress() tc.malleate() @@ -133,7 +142,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() { err = suite.chainA.GetSimApp().TransferKeeper.SendTransfer( suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, amount, - suite.chainA.SenderAccount.GetAddress(), suite.chainB.SenderAccount.GetAddress().String(), clienttypes.NewHeight(0, 110), 0, + sender, suite.chainB.SenderAccount.GetAddress().String(), clienttypes.NewHeight(0, 110), 0, ) if tc.expPass { From 6a8e27c62c896f4d7b1fc732fe1f25b9df4a5fe7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 8 Aug 2022 13:11:13 +0200 Subject: [PATCH 4/4] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6aff1fcc83e..0e9f637e45e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking +* (apps/transfer) [\#1907](https://github.com/cosmos/ibc-go/pull/1907) Blocked module account addresses are no longer allowed to send IBC transfers. * (apps/27-interchain-accounts) [\#1882](https://github.com/cosmos/ibc-go/pull/1882) Explicitly check length of interchain account packet data in favour of nil check. ### Improvements