chore(api)!: move checks from Transfer to OnSendPacket (#7068)

* 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 <d.f.hilliard@gmail.com> * 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 be9f558, reversing changes made to 36e4b05. * Delete commented out code --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
cosmos · Aug 8, 2024 · 0c47e45 · 0c47e45
1 parent 63dd289
commit 0c47e45
Show file tree

Hide file tree

Showing 6 changed files with 112 additions and 43 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -19,48 +19,19 @@ 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 {
 			return nil, err
 		}
 	}
 
+	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
@@ -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)
 	}