Skip to content

Commit

Permalink
fix: prevent blocked addresses from sending ICS 20 transfers (#1907)
Browse files Browse the repository at this point in the history
* 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

(cherry picked from commit f891c29)

# Conflicts:
#	CHANGELOG.md
#	modules/apps/transfer/keeper/relay_test.go
  • Loading branch information
colin-axner authored and mergify[bot] committed Aug 9, 2022
1 parent 90a5e15 commit 25e555c
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 2 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

<<<<<<< HEAD
=======
* (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.

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

* (modules/light-clients/07-tendermint) [\#1713](https://github.com/cosmos/ibc-go/pull/1713) Allow client upgrade proposals to update `TrustingPeriod`. See ADR-026 for context.
Expand Down
3 changes: 1 addition & 2 deletions modules/apps/transfer/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,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)
Expand All @@ -19,6 +17,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 {
Expand Down
71 changes: 71 additions & 0 deletions modules/apps/transfer/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
6 changes: 6 additions & 0 deletions modules/apps/transfer/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -63,6 +65,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)
Expand Down
19 changes: 19 additions & 0 deletions modules/apps/transfer/keeper/relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
var (
amount sdk.Coin
path *ibctesting.Path
sender sdk.AccAddress
err error
)

Expand Down Expand Up @@ -58,8 +59,21 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
)
suite.chainA.CreateChannelCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
<<<<<<< HEAD
}, true, false},

=======
}, 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,
},
>>>>>>> f891c29 (fix: prevent blocked addresses from sending ICS 20 transfers (#1907))
// createOutgoingPacket tests
// - source chain
{"send coin failed",
Expand Down Expand Up @@ -91,6 +105,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()

Expand Down Expand Up @@ -118,7 +133,11 @@ func (suite *KeeperTestSuite) TestSendTransfer() {

err = suite.chainA.GetSimApp().TransferKeeper.SendTransfer(
suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, amount,
<<<<<<< HEAD
suite.chainA.SenderAccount.GetAddress(), suite.chainB.SenderAccount.GetAddress().String(), clienttypes.NewHeight(0, 110), 0,
=======
sender, suite.chainB.SenderAccount.GetAddress().String(), suite.chainB.GetTimeoutHeight(), 0,
>>>>>>> f891c29 (fix: prevent blocked addresses from sending ICS 20 transfers (#1907))
)

if tc.expPass {
Expand Down
6 changes: 6 additions & 0 deletions testing/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -524,3 +524,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)
}

0 comments on commit 25e555c

Please sign in to comment.