Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Aug 9, 2022
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 @@ -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)
Expand All @@ -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 {
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(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do a followup pr to make this private for the next major release

ctx sdk.Context,
sourcePort,
Expand All @@ -62,6 +64,10 @@ func (k Keeper) SendTransfer(
return types.ErrSendDisabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit and no need to block the PR on this, but now that you added tests for the msg server, maybe we can also had a test case for the situation where send is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this test case is missing for receive as well. Would prefer it be done in a separate pr

}

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
13 changes: 11 additions & 2 deletions modules/apps/transfer/keeper/relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
var (
amount sdk.Coin
path *ibctesting.Path
sender sdk.AccAddress
err error
)

Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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()

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