fix: prevent blocked addresses from sending ICS 20 transfers (#1907)

* fix bug, add test

Ensures that a sender account isn't a blocked address
Added test cases for MsgTransfer handling

* update documentation

* move blocked address check to SendTransfer

* add changelog entry

Author: colin-axner

CHANGELOG.md

@@ -77,6 +77,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

modules/apps/transfer/keeper/msg_server.go

@@ -10,8 +10,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)
@@ -20,6 +18,7 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
 	if err != nil {
 		return nil, err
 	}
+
 	if err := k.SendTransfer(
 		ctx, msg.SourcePort, msg.SourceChannel, msg.Token, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
 	); err != nil {

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)
+		}
+	}
+}

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,
@@ -62,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)

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(1, 110), 0,
+				sender, suite.chainB.SenderAccount.GetAddress().String(), suite.chainB.GetTimeoutHeight(), 0,
 			)
 
 			if tc.expPass {

testing/chain.go

@@ -593,3 +593,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)
+}