chore: add packet data validation back (#6704)

modules/apps/transfer/ibc_module.go

@@ -191,6 +191,8 @@ func (im IBCModule) OnRecvPacket(
 
 	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 
+	// we are explicitly wrapping this emit event call in an anonymous function so that
+	// the packet data is evaluated after it has been assigned a value.
 	defer func() {
 		events.EmitOnRecvPacketEvent(ctx, data, ack, ackErr)
 	}()

modules/apps/transfer/keeper/export_test.go

@@ -39,6 +39,6 @@ func (k Keeper) UnwindHops(ctx sdk.Context, msg *types.MsgTransfer) (*types.MsgT
 }
 
 // CreatePacketDataBytesFromVersion is a wrapper around createPacketDataBytesFromVersion for testing purposes
-func CreatePacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) []byte {
+func CreatePacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) ([]byte, error) {
 	return createPacketDataBytesFromVersion(appVersion, sender, receiver, memo, tokens, hops)
 }

modules/apps/transfer/keeper/relay.go

@@ -147,7 +147,10 @@ func (k Keeper) sendTransfer(
 		tokens = append(tokens, token)
 	}
 
-	packetDataBytes := createPacketDataBytesFromVersion(appVersion, sender.String(), receiver, memo, tokens, forwarding.Hops)
+	packetDataBytes, err := createPacketDataBytesFromVersion(appVersion, sender.String(), receiver, memo, tokens, forwarding.Hops)
+	if err != nil {
+		return 0, err
+	}
 
 	sequence, err := k.ics4Wrapper.SendPacket(ctx, channelCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, packetDataBytes)
 	if err != nil {
@@ -438,8 +441,7 @@ func (k Keeper) tokenFromCoin(ctx sdk.Context, coin sdk.Coin) (types.Token, erro
 }
 
 // createPacketDataBytesFromVersion creates the packet data bytes to be sent based on the application version.
-func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) []byte {
-	var packetDataBytes []byte
+func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) ([]byte, error) {
 	switch appVersion {
 	case types.V1:
 		// Sanity check, tokens must always be of length 1 if using app version V1.
@@ -449,7 +451,12 @@ func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string,
 
 		token := tokens[0]
 		packetData := types.NewFungibleTokenPacketData(token.Denom.Path(), token.Amount, sender, receiver, memo)
-		packetDataBytes = packetData.GetBytes()
+
+		if err := packetData.ValidateBasic(); err != nil {
+			return nil, errorsmod.Wrapf(err, "failed to validate %s packet data", types.V1)
+		}
+
+		return packetData.GetBytes(), nil
 	case types.V2:
 		// If forwarding is needed, move memo to forwarding packet data and set packet.Memo to empty string.
 		var forwardingPacketData types.ForwardingPacketData
@@ -459,12 +466,15 @@ func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string,
 		}
 
 		packetData := types.NewFungibleTokenPacketDataV2(tokens, sender, receiver, memo, forwardingPacketData)
-		packetDataBytes = packetData.GetBytes()
+
+		if err := packetData.ValidateBasic(); err != nil {
+			return nil, errorsmod.Wrapf(err, "failed to validate %s packet data", types.V2)
+		}
+
+		return packetData.GetBytes(), nil
 	default:
 		panic(fmt.Errorf("app version must be one of %s", types.SupportedVersions))
 	}
-
-	return packetDataBytes
 }
 
 // burnCoin sends coins from the account to the transfer module account and then burn them.

modules/apps/transfer/keeper/relay_test.go

@@ -1287,34 +1287,61 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 
 func (suite *KeeperTestSuite) TestCreatePacketDataBytesFromVersion() {
 	var (
-		bz     []byte
-		tokens types.Tokens
+		bz               []byte
+		tokens           types.Tokens
+		sender, receiver string
 	)
 
 	testCases := []struct {
 		name        string
 		appVersion  string
 		malleate    func()
-		expResult   func(bz []byte)
+		expResult   func(bz []byte, err error)
 		expPanicErr error
 	}{
 		{
 			"success",
 			types.V1,
 			func() {},
-			func(bz []byte) {
-				expPacketData := types.NewFungibleTokenPacketData("", "", "", "", "")
+			func(bz []byte, err error) {
+				expPacketData := types.NewFungibleTokenPacketData(ibctesting.TestCoin.Denom, ibctesting.TestCoin.Amount.String(), sender, receiver, "")
 				suite.Require().Equal(bz, expPacketData.GetBytes())
+				suite.Require().NoError(err)
 			},
 			nil,
 		},
 		{
 			"success: version 2",
 			types.V2,
 			func() {},
-			func(bz []byte) {
-				expPacketData := types.NewFungibleTokenPacketDataV2(types.Tokens{types.Token{}}, "", "", "", emptyForwardingPacketData)
+			func(bz []byte, err error) {
+				expPacketData := types.NewFungibleTokenPacketDataV2(tokens, sender, receiver, "", emptyForwardingPacketData)
 				suite.Require().Equal(bz, expPacketData.GetBytes())
+				suite.Require().NoError(err)
+			},
+			nil,
+		},
+		{
+			"failure: fails v1 validation",
+			types.V1,
+			func() {
+				sender = ""
+			},
+			func(bz []byte, err error) {
+				suite.Require().Nil(bz)
+				suite.Require().ErrorIs(err, ibcerrors.ErrInvalidAddress)
+			},
+			nil,
+		},
+		{
+			"failure: fails v2 validation",
+			types.V2,
+			func() {
+				sender = ""
+			},
+			func(bz []byte, err error) {
+				suite.Require().Nil(bz)
+				suite.Require().ErrorIs(err, ibcerrors.ErrInvalidAddress)
 			},
 			nil,
 		},
@@ -1338,20 +1365,34 @@ func (suite *KeeperTestSuite) TestCreatePacketDataBytesFromVersion() {
 
 	for _, tc := range testCases {
 		suite.Run(tc.name, func() {
-			tokens = types.Tokens{types.Token{}}
+			suite.SetupTest()
+
+			path := ibctesting.NewTransferPath(suite.chainA, suite.chainB)
+			path.Setup()
+
+			tokens = types.Tokens{
+				{
+					Amount: ibctesting.TestCoin.Amount.String(),
+					Denom:  types.NewDenom(ibctesting.TestCoin.Denom),
+				},
+			}
+
+			sender = suite.chainA.SenderAccount.GetAddress().String()
+			receiver = suite.chainB.SenderAccount.GetAddress().String()
 
 			tc.malleate()
 
+			var err error
 			createFunc := func() {
-				bz = transferkeeper.CreatePacketDataBytesFromVersion(tc.appVersion, "", "", "", tokens, nil)
+				bz, err = transferkeeper.CreatePacketDataBytesFromVersion(tc.appVersion, sender, receiver, "", tokens, nil)
 			}
 
 			expPanic := tc.expPanicErr != nil
 			if expPanic {
 				suite.Require().PanicsWithError(tc.expPanicErr.Error(), createFunc)
 			} else {
 				createFunc()
-				tc.expResult(bz)
+				tc.expResult(bz, err)
 			}
 		})
 	}