From 0c47e45619dc4604f5e7fa364e08d40f2fa588c1 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Thu, 8 Aug 2024 12:15:51 +0100 Subject: [PATCH] chore(api)!: move checks from Transfer to OnSendPacket (#7068) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * deps: bump cosmos-sdk to v0.50.9 (#6828) * deps: bump cosmos-sdk to v0.50.8 * chore: update changelog * deps: bump cosmossdk.io/client to v2.0.0-beta.3. bump x/upgrade to v0.1.4 * chore: make tidy-all * test: bump to 3f6796fba413cca for testing purposes. * deps: bump cosmos sdk to 0.50.9 * Update CHANGELOG.md * chore: update CHANGELOG for submodules. --------- Co-authored-by: DimitrisJim * chore(api!): move checks from Transfer to OnSendPacket * Fix merge error * fix to test * Add Require() back * Update modules/apps/transfer/ibc_module.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * Linter * Removed wrong merge results * reintroduce test * Move check for isSendEnabledCoins to keeper * new tests * fixed test * linter * Removed checks and added comment * removed extra parentheses * Revert "Merge remote-tracking branch 'origin' into bznein/6949/msgTransferWrapperToSendPacket" This reverts commit be9f5583c22f4c31e767e7baaaa6ecbce9079d99, reversing changes made to 36e4b05adfd7746899d96f25590b192747673fe9. * Delete commented out code --------- Co-authored-by: Damian Nolan Co-authored-by: DimitrisJim Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- modules/apps/transfer/ibc_module.go | 12 +++ modules/apps/transfer/ibc_module_test.go | 87 +++++++++++++++++++++ modules/apps/transfer/keeper/export_test.go | 5 -- modules/apps/transfer/keeper/keeper.go | 2 +- modules/apps/transfer/keeper/msg_server.go | 41 ++-------- modules/apps/transfer/keeper/relay.go | 8 +- 6 files changed, 112 insertions(+), 43 deletions(-) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index e624761bd57..14830a35770 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -172,6 +172,13 @@ func (im IBCModule) OnSendPacket( dataBz []byte, signer sdk.AccAddress, ) error { + if !im.keeper.GetParams(ctx).SendEnabled { + return types.ErrSendDisabled + } + if im.keeper.IsBlockedAddr(signer) { + return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", signer) + } + ics20Version, found := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, portID, channelID) if !found { return errorsmod.Wrapf(ibcerrors.ErrNotFound, "app version not found for port %s and channel %s", portID, channelID) @@ -182,6 +189,11 @@ func (im IBCModule) OnSendPacket( return err } + // If the ics20version is V1, we can't have multiple tokens nor forwarding info. + // However, we do not need to check it here, as a packet containing that data would + // fail the unmarshaling above, where if ics20version == types.V1 we first unmarshal + // into a V1 packet and then convert. + if data.Sender != signer.String() { return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "invalid signer address: expected %s, got %s", data.Sender, signer) } diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 39b23e0945c..7162cebd967 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -9,6 +9,7 @@ import ( "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" sdk "github.com/cosmos/cosmos-sdk/types" + minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" "github.com/cosmos/ibc-go/v9/modules/apps/transfer" "github.com/cosmos/ibc-go/v9/modules/apps/transfer/types" @@ -883,3 +884,89 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { }) } } + +func (suite *TransferTestSuite) TestOnSendPacket() { + var ( + packetData types.FungibleTokenPacketDataV2 + path *ibctesting.Path + signer sdk.AccAddress + ) + + testCases := []struct { + name string + malleate func() + expError error + }{ + { + "success", + func() {}, + nil, + }, + { + "failure: send disabled", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false, true)) + }, + types.ErrSendDisabled, + }, + { + "failure: blocked address", + func() { + signer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(minttypes.ModuleName) + }, + ibcerrors.ErrUnauthorized, + }, + { + "failure: sender is not signer", + func() { + packetData.Sender = suite.chainB.SenderAccount.GetAddress().String() + }, + ibcerrors.ErrInvalidAddress, + }, + { + "failure: V2 packet can't be unmarshaled if version is V1 and it contains V2 data", + func() { + path.EndpointA.UpdateChannel(func(channel *channeltypes.Channel) { + channel.Version = types.V1 + }) + packetData.Forwarding = types.NewForwardingPacketData("", types.NewHop(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + }, + ibcerrors.ErrInvalidType, + }, + } + for _, tc := range testCases { + suite.Run(tc.name, func() { + suite.SetupTest() + signer = sdk.MustAccAddressFromBech32(suite.chainA.SenderAccount.GetAddress().String()) + + path = ibctesting.NewTransferPath(suite.chainA, suite.chainB) + path.Setup() + + packetData = types.NewFungibleTokenPacketDataV2( + []types.Token{{Denom: types.NewDenom(sdk.DefaultBondDenom), Amount: ibctesting.TestCoin.Amount.String()}}, + suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.String(), "", types.ForwardingPacketData{}, + ) + + tc.malleate() + + dataBytes := packetData.GetBytes() + + cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRoute(path.EndpointA.ChannelConfig.PortID) + suite.Require().True(ok) + + err := cbs[0].OnSendPacket( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, + 0, clienttypes.ZeroHeight(), 0, + dataBytes, + signer, + ) + + if tc.expError == nil { + suite.Require().NoError(err) + } else { + suite.Require().ErrorIs(err, tc.expError) + } + }) + } +} diff --git a/modules/apps/transfer/keeper/export_test.go b/modules/apps/transfer/keeper/export_test.go index 5efd6a85910..dedfd560b9b 100644 --- a/modules/apps/transfer/keeper/export_test.go +++ b/modules/apps/transfer/keeper/export_test.go @@ -54,11 +54,6 @@ func (k Keeper) GetAllForwardedPackets(ctx sdk.Context) []types.ForwardedPacket return k.getAllForwardedPackets(ctx) } -// IsBlockedAddr is a wrapper around isBlockedAddr for testing purposes -func (k Keeper) IsBlockedAddr(addr sdk.AccAddress) bool { - return k.isBlockedAddr(addr) -} - // CreatePacketDataBytesFromVersion is a wrapper around createPacketDataBytesFromVersion for testing purposes func CreatePacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) ([]byte, error) { return createPacketDataBytesFromVersion(appVersion, sender, receiver, memo, tokens, hops) diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index c63d90a7ab2..17aef2757d9 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -391,7 +391,7 @@ func (k Keeper) iterateForwardedPackets(ctx sdk.Context, cb func(packet types.Fo // IsBlockedAddr checks if the given address is allowed to send or receive tokens. // The module account is always allowed to send and receive tokens. -func (k Keeper) isBlockedAddr(addr sdk.AccAddress) bool { +func (k Keeper) IsBlockedAddr(addr sdk.AccAddress) bool { moduleAddr := k.authKeeper.GetModuleAddress(types.ModuleName) if addr.Equals(moduleAddr) { return false diff --git a/modules/apps/transfer/keeper/msg_server.go b/modules/apps/transfer/keeper/msg_server.go index e74e60a8dad..9f8457481b6 100644 --- a/modules/apps/transfer/keeper/msg_server.go +++ b/modules/apps/transfer/keeper/msg_server.go @@ -19,41 +19,11 @@ var _ types.MsgServer = (*Keeper)(nil) func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.MsgTransferResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) - if !k.GetParams(ctx).SendEnabled { - return nil, types.ErrSendDisabled - } - sender, err := sdk.AccAddressFromBech32(msg.Sender) if err != nil { return nil, err } - coins := msg.GetCoins() - if err := k.bankKeeper.IsSendEnabledCoins(ctx, coins...); err != nil { - return nil, errorsmod.Wrapf(types.ErrSendDisabled, err.Error()) - } - - if k.isBlockedAddr(sender) { - return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", sender) - } - - appVersion, found := k.ics4Wrapper.GetAppVersion(ctx, msg.SourcePort, msg.SourceChannel) - if !found { - return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "application version not found for source port: %s and source channel: %s", msg.SourcePort, msg.SourceChannel) - } - - if appVersion == types.V1 { - // ics20-1 only supports a single coin, so if that is the current version, we must only process a single coin. - if len(msg.Tokens) > 1 { - return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot transfer multiple coins with %s", types.V1) - } - - // ics20-1 does not support forwarding, so if that is the current version, we must reject the transfer. - if msg.HasForwarding() { - return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot forward coins with %s", types.V1) - } - } - if msg.Forwarding.GetUnwind() { msg, err = k.unwindHops(ctx, msg) if err != nil { @@ -61,6 +31,7 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types. } } + coins := msg.GetCoins() tokens := make([]types.Token, 0, len(coins)) for _, coin := range coins { token, err := k.tokenFromCoin(ctx, coin) @@ -71,15 +42,15 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types. tokens = append(tokens, token) } + appVersion, found := k.ics4Wrapper.GetAppVersion(ctx, msg.SourcePort, msg.SourceChannel) + if !found { + return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "application version not found for source port: %s and source channel: %s", msg.SourcePort, msg.SourceChannel) + } packetDataBz, err := createPacketDataBytesFromVersion(appVersion, sender.String(), msg.Receiver, msg.Memo, tokens, msg.Forwarding.GetHops()) if err != nil { return nil, err } - // packetData := types.NewFungibleTokenPacketData( - // fullDenomPath, msg.Token.Amount.String(), sender.String(), msg.Receiver, msg.Memo, - // ) - msgSendPacket := &channeltypes.MsgSendPacket{ PortId: msg.SourcePort, ChannelId: msg.SourceChannel, @@ -149,7 +120,7 @@ func (k Keeper) unwindHops(ctx sdk.Context, msg *types.MsgTransfer) (*types.MsgT msg.Forwarding.Hops = append(unwindHops[1:], msg.Forwarding.Hops...) msg.Forwarding.Unwind = false - // Message is validate again, this would only fail if hops now exceeds maximum allowed. + // Message is validated again, this would only fail if hops now exceeds maximum allowed. if err := msg.ValidateBasic(); err != nil { return nil, err } diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 213ca9bc732..3e328ab9063 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -73,6 +73,10 @@ func (k Keeper) OnSendPacket( coins = append(coins, sdk.NewCoin(token.Denom.IBCDenom(), transferAmount)) } + if err := k.bankKeeper.IsSendEnabledCoins(ctx, coins...); err != nil { + return errorsmod.Wrapf(types.ErrSendDisabled, err.Error()) + } + destinationPort := channel.Counterparty.PortId destinationChannel := channel.Counterparty.ChannelId @@ -155,7 +159,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t return err } - if k.isBlockedAddr(receiver) { + if k.IsBlockedAddr(receiver) { return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to receive funds", receiver) } @@ -323,7 +327,7 @@ func (k Keeper) refundPacketTokens(ctx sdk.Context, packet channeltypes.Packet, if err != nil { return err } - if k.isBlockedAddr(sender) { + if k.IsBlockedAddr(sender) { return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to receive funds", sender) }