From 45c7eea95d371c166c15bc773899c1c4f6985e49 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Thu, 30 May 2024 11:35:39 +0100 Subject: [PATCH 1/2] Add more unit tests for ExtractDenomFromFullPath (#6431) * chore: adding additional unit tests for ExtractDenomFromFullPath * chore: adding additial tests based on PR feedback --- modules/apps/transfer/types/trace_test.go | 27 +++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/modules/apps/transfer/types/trace_test.go b/modules/apps/transfer/types/trace_test.go index 40ad3037586..9e8378ebf8d 100644 --- a/modules/apps/transfer/types/trace_test.go +++ b/modules/apps/transfer/types/trace_test.go @@ -154,3 +154,30 @@ func TestValidateIBCDenom(t *testing.T) { require.NoError(t, err, tc.name) } } + +func TestExtractDenomFromFullPath(t *testing.T) { + testCases := []struct { + name string + fullPath string + expDenom types.Denom + }{ + {"base denom no slashes", "atom", types.Denom{Base: "atom"}}, + {"base denom with trailing slash", "atom/", types.Denom{Base: "atom/"}}, + {"base denom multiple trailing slash", "foo///bar//baz/atom/", types.Denom{Base: "foo///bar//baz/atom/"}}, + {"ibc denom one hop", "transfer/channel-0/atom", types.Denom{Base: "atom", Trace: []string{"transfer/channel-0"}}}, + {"ibc denom one hop trailing slash", "transfer/channel-0/atom/", types.Denom{Base: "atom/", Trace: []string{"transfer/channel-0"}}}, + {"ibc denom two hops", "transfer/channel-0/transfer/channel-60/atom", types.Denom{Base: "atom", Trace: []string{"transfer/channel-0", "transfer/channel-60"}}}, + {"ibc denom two hops trailing slash", "transfer/channel-0/transfer/channel-60/atom/", types.Denom{Base: "atom/", Trace: []string{"transfer/channel-0", "transfer/channel-60"}}}, + {"empty prefix", "/uatom", types.Denom{Base: "/uatom"}}, + {"empty identifiers", "//uatom", types.Denom{Base: "//uatom"}}, + {"base denom with single '/'", "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", types.Denom{Base: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA"}}, + {"single trace identifier", "transfer/", types.Denom{Base: "transfer/"}}, + } + + for _, tc := range testCases { + tc := tc + + denom := types.ExtractDenomFromFullPath(tc.fullPath) + require.Equal(t, tc.expDenom, denom, tc.name) + } +} From 80b0c9a4a179ff7f6fd30e5786c37a085972c8bd Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 30 May 2024 15:21:32 +0300 Subject: [PATCH 2/2] 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 +}