From 80b0c9a4a179ff7f6fd30e5786c37a085972c8bd Mon Sep 17 00:00:00 2001
From: DimitrisJim <d.f.hilliard@gmail.com>
Date: Thu, 30 May 2024 15:21:32 +0300
Subject: [PATCH] fix(transfer): Don't allow unknown fields during json
 unmarshalling. (#6428)

* fix(transfer): Don't allow unknown fields during json unmarshalling.

Use json's DisallowUnknownFields flag on the decoder to control error emission for uknown fields.
Implement Unmarshaller interface for both version of the ics20 packet data.

* docs: expand on recursion protection.
---
 modules/apps/transfer/keeper/relay_test.go | 36 ++++++++---------
 modules/apps/transfer/types/encoding.go    | 46 ++++++++++++++++++++++
 2 files changed, 64 insertions(+), 18 deletions(-)
 create mode 100644 modules/apps/transfer/types/encoding.go

diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go
index 34ddaa54ffe..9a85c9d2d42 100644
--- a/modules/apps/transfer/keeper/relay_test.go
+++ b/modules/apps/transfer/keeper/relay_test.go
@@ -1088,15 +1088,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 		expError    error
 		expAckError error
 	}{
-		{
-			"success: new field v2",
-			func() {
-				jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"base": "atom", "trace": []},"amount":"100"}],"sender":"%s","receiver":"%s", "new_field":"value"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
-				packetData = []byte(jsonString)
-			},
-			nil,
-			nil,
-		},
 		{
 			"success: no new field with memo v2",
 			func() {
@@ -1124,24 +1115,22 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 			ibcerrors.ErrInvalidType,
 		},
 		{
-			"failure: missing field v2",
+			"failure: new field v2",
 			func() {
-				jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"trace": []},"amount":"100"}],"sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
+				jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"base": "atom", "trace": []},"amount":"100"}],"sender":"%s","receiver":"%s", "new_field":"value"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
 				packetData = []byte(jsonString)
 			},
-			types.ErrInvalidDenomForTransfer,
+			ibcerrors.ErrInvalidType,
 			ibcerrors.ErrInvalidType,
 		},
 		{
-			"success: new field",
+			"failure: missing field v2",
 			func() {
-				path.EndpointA.ChannelConfig.Version = types.V1
-				path.EndpointB.ChannelConfig.Version = types.V1
-				jsonString := fmt.Sprintf(`{"denom":"denom","amount":"100","sender":"%s","receiver":"%s","memo":"memo","new_field":"value"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
+				jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"trace": []},"amount":"100"}],"sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
 				packetData = []byte(jsonString)
 			},
-			nil,
-			nil,
+			types.ErrInvalidDenomForTransfer,
+			ibcerrors.ErrInvalidType,
 		},
 		{
 			"success: no new field with memo",
@@ -1175,6 +1164,17 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 			ibcerrors.ErrInvalidType,
 			ibcerrors.ErrInvalidType,
 		},
+		{
+			"failure: new field",
+			func() {
+				path.EndpointA.ChannelConfig.Version = types.V1
+				path.EndpointB.ChannelConfig.Version = types.V1
+				jsonString := fmt.Sprintf(`{"denom":"denom","amount":"100","sender":"%s","receiver":"%s","memo":"memo","new_field":"value"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
+				packetData = []byte(jsonString)
+			},
+			ibcerrors.ErrInvalidType,
+			ibcerrors.ErrInvalidType,
+		},
 		{
 			"failure: missing field",
 			func() {
diff --git a/modules/apps/transfer/types/encoding.go b/modules/apps/transfer/types/encoding.go
new file mode 100644
index 00000000000..f9073b37d0e
--- /dev/null
+++ b/modules/apps/transfer/types/encoding.go
@@ -0,0 +1,46 @@
+package types
+
+import (
+	"bytes"
+	"encoding/json"
+)
+
+// UnmarshalJSON implements the Unmarshaller interface for FungibleTokenPacketDataV2.
+func (ftpd *FungibleTokenPacketDataV2) UnmarshalJSON(bz []byte) error {
+	// Recursion protection. We cannot unmarshal into FungibleTokenPacketData directly
+	// else UnmarshalJSON is going to get invoked again, ad infinum. Create an alias
+	// and unmarshal into that, instead.
+	type ftpdAlias FungibleTokenPacketDataV2
+
+	d := json.NewDecoder(bytes.NewReader(bz))
+	// Raise errors during decoding if unknown fields are encountered.
+	d.DisallowUnknownFields()
+
+	var alias ftpdAlias
+	if err := d.Decode(&alias); err != nil {
+		return err
+	}
+
+	*ftpd = FungibleTokenPacketDataV2(alias)
+	return nil
+}
+
+// UnmarshalJSON implements the Unmarshaller interface for FungibleTokenPacketData.
+func (ftpd *FungibleTokenPacketData) UnmarshalJSON(bz []byte) error {
+	// Recursion protection. We cannot unmarshal into FungibleTokenPacketData directly
+	// else UnmarshalJSON is going to get invoked again, ad infinum. Create an alias
+	// and unmarshal into that, instead.
+	type ftpdAlias FungibleTokenPacketData
+
+	d := json.NewDecoder(bytes.NewReader(bz))
+	// Raise errors during decoding if unknown fields are encountered.
+	d.DisallowUnknownFields()
+
+	var alias ftpdAlias
+	if err := d.Decode(&alias); err != nil {
+		return err
+	}
+
+	*ftpd = FungibleTokenPacketData(alias)
+	return nil
+}