From f18fd1a409be6cee453afeaec79e098be0f849a4 Mon Sep 17 00:00:00 2001 From: Matthew Witkowski Date: Thu, 22 Dec 2022 15:53:23 -0500 Subject: [PATCH] Added Arnab's changes in for the custom handler. Also made SendTransfer public again so that we can use it. --- modules/apps/transfer/keeper/keeper.go | 4 +- modules/apps/transfer/keeper/msg_server.go | 8 +- modules/apps/transfer/keeper/relay.go | 36 ++-- modules/apps/transfer/keeper/relay_test.go | 182 ++++++++++++++++++ .../apps/transfer/types/expected_keepers.go | 1 + 5 files changed, 214 insertions(+), 17 deletions(-) diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 9688d57aff2..84955679167 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -26,7 +26,7 @@ type Keeper struct { channelKeeper types.ChannelKeeper portKeeper types.PortKeeper authKeeper types.AccountKeeper - bankKeeper types.BankKeeper + BankKeeper types.BankKeeper scopedKeeper exported.ScopedKeeper } @@ -54,7 +54,7 @@ func NewKeeper( channelKeeper: channelKeeper, portKeeper: portKeeper, authKeeper: authKeeper, - bankKeeper: bankKeeper, + BankKeeper: bankKeeper, scopedKeeper: scopedKeeper, } } diff --git a/modules/apps/transfer/keeper/msg_server.go b/modules/apps/transfer/keeper/msg_server.go index 74812ecd1a9..099f8f7d4c0 100644 --- a/modules/apps/transfer/keeper/msg_server.go +++ b/modules/apps/transfer/keeper/msg_server.go @@ -24,17 +24,17 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types. return nil, err } - if !k.bankKeeper.IsSendEnabledCoin(ctx, msg.Token) { + if !k.BankKeeper.IsSendEnabledCoin(ctx, msg.Token) { return nil, sdkerrors.Wrapf(types.ErrSendDisabled, "%s transfers are currently disabled", msg.Token.Denom) } - if k.bankKeeper.BlockedAddr(sender) { + if k.BankKeeper.BlockedAddr(sender) { return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", sender) } - sequence, err := k.sendTransfer( + sequence, err := k.SendTransfer( ctx, msg.SourcePort, msg.SourceChannel, msg.Token, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp, - msg.Memo) + msg.Memo, nil) if err != nil { return nil, err } diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 39410ad3e35..29e6fefb856 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -16,6 +16,9 @@ import ( coretypes "github.com/cosmos/ibc-go/v6/modules/core/types" ) +// CheckRestrictionsHandler checks if the ibc transfer should happen. +type CheckRestrictionsHandler func(ctx sdk.Context, k Keeper, sender sdk.AccAddress, token sdk.Coin) (canTransfer bool, err error) + // sendTransfer handles transfer sending logic. There are 2 possible cases: // // 1. Sender chain is acting as the source zone. The coins are transferred @@ -48,7 +51,7 @@ 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' -func (k Keeper) sendTransfer( +func (k Keeper) SendTransfer( ctx sdk.Context, sourcePort, sourceChannel string, @@ -58,7 +61,18 @@ func (k Keeper) sendTransfer( timeoutHeight clienttypes.Height, timeoutTimestamp uint64, memo string, + // check restrictions condition met for this address and coin. + checkRestrictionsHandler CheckRestrictionsHandler, + ) (uint64, error) { + // if nil then apply default checks + if checkRestrictionsHandler != nil { + res, err := checkRestrictionsHandler(ctx, k, sender, token) + if !res { + return 0, err + } + } + channel, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel) if !found { return 0, sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", sourcePort, sourceChannel) @@ -104,7 +118,7 @@ func (k Keeper) sendTransfer( escrowAddress := types.GetEscrowAddress(sourcePort, sourceChannel) // escrow source tokens. It fails if balance insufficient. - if err := k.bankKeeper.SendCoins( + if err := k.BankKeeper.SendCoins( ctx, sender, escrowAddress, sdk.NewCoins(token), ); err != nil { return 0, err @@ -113,13 +127,13 @@ func (k Keeper) sendTransfer( labels = append(labels, telemetry.NewLabel(coretypes.LabelSource, "false")) // transfer the coins to the module account and burn them - if err := k.bankKeeper.SendCoinsFromAccountToModule( + if err := k.BankKeeper.SendCoinsFromAccountToModule( ctx, sender, types.ModuleName, sdk.NewCoins(token), ); err != nil { return 0, err } - if err := k.bankKeeper.BurnCoins( + if err := k.BankKeeper.BurnCoins( ctx, types.ModuleName, sdk.NewCoins(token), ); err != nil { // NOTE: should not happen as the module account was @@ -215,13 +229,13 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t } token := sdk.NewCoin(denom, transferAmount) - if k.bankKeeper.BlockedAddr(receiver) { + if k.BankKeeper.BlockedAddr(receiver) { return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", receiver) } // unescrow tokens escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel()) - if err := k.bankKeeper.SendCoins(ctx, escrowAddress, receiver, sdk.NewCoins(token)); err != nil { + if err := k.BankKeeper.SendCoins(ctx, escrowAddress, receiver, sdk.NewCoins(token)); err != nil { // NOTE: this error is only expected to occur given an unexpected bug or a malicious // counterparty module. The bug may occur in bank or any part of the code that allows // the escrow address to be drained. A malicious counterparty module could drain the @@ -276,14 +290,14 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t voucher := sdk.NewCoin(voucherDenom, transferAmount) // mint new tokens if the source of the transfer is the same chain - if err := k.bankKeeper.MintCoins( + if err := k.BankKeeper.MintCoins( ctx, types.ModuleName, sdk.NewCoins(voucher), ); err != nil { return err } // send to receiver - if err := k.bankKeeper.SendCoinsFromModuleToAccount( + if err := k.BankKeeper.SendCoinsFromModuleToAccount( ctx, types.ModuleName, receiver, sdk.NewCoins(voucher), ); err != nil { return err @@ -357,7 +371,7 @@ func (k Keeper) refundPacketToken(ctx sdk.Context, packet channeltypes.Packet, d if types.SenderChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), data.Denom) { // unescrow tokens back to sender escrowAddress := types.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel()) - if err := k.bankKeeper.SendCoins(ctx, escrowAddress, sender, sdk.NewCoins(token)); err != nil { + if err := k.BankKeeper.SendCoins(ctx, escrowAddress, sender, sdk.NewCoins(token)); err != nil { // NOTE: this error is only expected to occur given an unexpected bug or a malicious // counterparty module. The bug may occur in bank or any part of the code that allows // the escrow address to be drained. A malicious counterparty module could drain the @@ -369,13 +383,13 @@ func (k Keeper) refundPacketToken(ctx sdk.Context, packet channeltypes.Packet, d } // mint vouchers back to sender - if err := k.bankKeeper.MintCoins( + if err := k.BankKeeper.MintCoins( ctx, types.ModuleName, sdk.NewCoins(token), ); err != nil { return err } - if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, sender, sdk.NewCoins(token)); err != nil { + if err := k.BankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, sender, sdk.NewCoins(token)); err != nil { panic(fmt.Sprintf("unable to send coins from module to account despite previously minting coins to module account: %v", err)) } diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index 80d57b33415..3e7d67e6d69 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/modules/apps/transfer/keeper/relay_test.go @@ -5,14 +5,196 @@ import ( "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + "github.com/cosmos/ibc-go/v6/modules/apps/transfer/keeper" "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types" + host "github.com/cosmos/ibc-go/v6/modules/core/24-host" ibctesting "github.com/cosmos/ibc-go/v6/testing" "github.com/cosmos/ibc-go/v6/testing/simapp" ) +// test sending from chainA to chainB using both coin that orignate on +// chainA and coin that orignate on chainB +func (suite *KeeperTestSuite) TestSendTransferOriginal() { + var ( + amount sdk.Coin + path *ibctesting.Path + sender sdk.AccAddress + err error + ) + + testCases := []struct { + msg string + malleate func() + sendFromSource bool + expPass bool + checkRestrictionsHandler keeper.CheckRestrictionsHandler + }{ + { + "successful transfer from source chain", + func() { + suite.coordinator.CreateTransferChannels(path) + amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + }, true, true, nil, + }, + { + "successful transfer with coin from counterparty chain", + func() { + // send coin from chainA back to chainB + suite.coordinator.CreateTransferChannels(path) + amount = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, sdk.DefaultBondDenom, sdk.NewInt(100)) + }, false, true, nil, + }, + { + "source channel not found", + func() { + // channel references wrong ID + suite.coordinator.CreateTransferChannels(path) + path.EndpointA.ChannelID = ibctesting.InvalidID + amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + }, true, false, nil, + }, + { + "next seq send not found", + func() { + path.EndpointA.ChannelID = "channel-0" + path.EndpointB.ChannelID = "channel-0" + // manually create channel so next seq send is never set + suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, + channeltypes.NewChannel(channeltypes.OPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID), []string{path.EndpointA.ConnectionID}, ibctesting.DefaultChannelVersion), + ) + suite.chainA.CreateChannelCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + }, true, false, nil, + }, + { + "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, nil, + }, + // createOutgoingPacket tests + // - source chain + { + "send coin failed", + func() { + suite.coordinator.CreateTransferChannels(path) + amount = sdk.NewCoin("randomdenom", sdk.NewInt(100)) + }, true, false, nil, + }, + // - receiving chain + { + "send from module account failed", + func() { + suite.coordinator.CreateTransferChannels(path) + amount = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, " randomdenom", sdk.NewInt(100)) + }, false, false, nil, + }, + { + "channel capability not found", + func() { + suite.coordinator.CreateTransferChannels(path) + cap := suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + // Release channel capability + suite.chainA.GetSimApp().ScopedTransferKeeper.ReleaseCapability(suite.chainA.GetContext(), cap) + amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + }, true, false, nil, + }, + { + "send coin success,even though send coin is disabled, by passing in custom checkRestrictionHandler(for restricted marker) ", + func() { + suite.coordinator.CreateTransferChannels(path) + amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + + // Disable SendEnabled + params := suite.chainA.GetSimApp().BankKeeper.GetParams(suite.chainA.GetContext()) + params.SendEnabled = append(params.SendEnabled, banktypes.NewSendEnabled(sdk.DefaultBondDenom, false)) + suite.chainA.GetSimApp().BankKeeper.SetParams(suite.chainA.GetContext(), params) + }, true, true, func(ctx sdk.Context, k keeper.Keeper, sender sdk.AccAddress, token sdk.Coin) (canTransfer bool, err error) { + if !k.GetSendEnabled(ctx) { + return false, types.ErrSendDisabled + } + if k.BankKeeper.BlockedAddr(sender) { + return false, sdkerrors.ErrUnauthorized.Wrapf("%s is not allowed to send funds", sender) + } + return true, nil + }, + }, + { + "send coin failed, send coin is disabled, pass in customHandler that checks sendEnabled ", + func() { + suite.coordinator.CreateTransferChannels(path) + amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + + // Disable SendEnabled + params := suite.chainA.GetSimApp().BankKeeper.GetParams(suite.chainA.GetContext()) + params.SendEnabled = append(params.SendEnabled, banktypes.NewSendEnabled(sdk.DefaultBondDenom, false)) + suite.chainA.GetSimApp().BankKeeper.SetParams(suite.chainA.GetContext(), params) + }, true, false, func(ctx sdk.Context, k keeper.Keeper, sender sdk.AccAddress, token sdk.Coin) (canTransfer bool, err error) { + if err := k.BankKeeper.IsSendEnabledCoins(ctx, sdk.NewCoins(token)...); err != nil { + return false, err + } + return true, nil + }, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { + suite.SetupTest() // reset + path = NewTransferPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + sender = suite.chainA.SenderAccount.GetAddress() + + tc.malleate() + + if !tc.sendFromSource { + // send coin from chainB to chainA + coinFromBToA := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + transferMsg := types.NewMsgTransfer(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, coinFromBToA, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), clienttypes.NewHeight(0, 110), 0, "") + _, err = suite.chainB.SendMsgs(transferMsg) + suite.Require().NoError(err) // message committed + + // receive coin on chainA from chainB + fungibleTokenPacket := types.NewFungibleTokenPacketData(coinFromBToA.Denom, coinFromBToA.Amount.String(), suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), "") + packet := channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, clienttypes.NewHeight(0, 110), 0) + + // get proof of packet commitment from chainB + err = path.EndpointA.UpdateClient() + suite.Require().NoError(err) + packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + proof, proofHeight := path.EndpointB.QueryProof(packetKey) + + recvMsg := channeltypes.NewMsgRecvPacket(packet, proof, proofHeight, suite.chainA.SenderAccount.GetAddress().String()) + _, err = suite.chainA.SendMsgs(recvMsg) + suite.Require().NoError(err) // message committed + } + + _, err = suite.chainA.GetSimApp().TransferKeeper.SendTransfer( + suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, amount, + sender, suite.chainB.SenderAccount.GetAddress().String(), suite.chainB.GetTimeoutHeight(), 0, "", tc.checkRestrictionsHandler, + ) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } +} + // test sending from chainA to chainB using both coin that orignate on // chainA and coin that orignate on chainB func (suite *KeeperTestSuite) TestSendTransfer() { diff --git a/modules/apps/transfer/types/expected_keepers.go b/modules/apps/transfer/types/expected_keepers.go index 0edfa4016fb..b267cff7953 100644 --- a/modules/apps/transfer/types/expected_keepers.go +++ b/modules/apps/transfer/types/expected_keepers.go @@ -25,6 +25,7 @@ type BankKeeper interface { SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error BlockedAddr(addr sdk.AccAddress) bool IsSendEnabledCoin(ctx sdk.Context, coin sdk.Coin) bool + IsSendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error } // ChannelKeeper defines the expected IBC channel keeper