diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index 889e852938f..bfbca027d87 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -1,9 +1,10 @@ package ibccallbacks_test import ( - "encoding/json" "fmt" + "github.com/cosmos/gogoproto/proto" + errorsmod "cosmossdk.io/errors" storetypes "cosmossdk.io/store/types" @@ -507,7 +508,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() { s.Require().NoError(err) s.Require().NotNil(packet) - err = json.Unmarshal(packet.Data, &packetData) + err = proto.Unmarshal(packet.Data, &packetData) s.Require().NoError(err) ctx = s.chainA.GetContext() diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 5ffce8a7860..5607ce91f42 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -334,11 +334,11 @@ func (suite *TransferTestSuite) TestOnRecvPacket() { sdk.NewAttribute(types.AttributeKeyMemo, ""), sdk.NewAttribute(types.AttributeKeyForwardingHops, "null"), sdk.NewAttribute(types.AttributeKeyAckSuccess, "false"), - sdk.NewAttribute(types.AttributeKeyAckError, "cannot unmarshal ICS20-V2 transfer packet data: invalid character 'i' looking for beginning of value: invalid type: invalid type"), + sdk.NewAttribute(types.AttributeKeyAckError, "cannot unmarshal ICS20-V2 transfer packet data: errUnknownField \"*types.FungibleTokenPacketDataV2\": {TagNum: 13, WireType:\"fixed64\"}: invalid type: invalid type"), } }, channeltypes.NewErrorAcknowledgement(ibcerrors.ErrInvalidType), - "cannot unmarshal ICS20-V2 transfer packet data: invalid character 'i' looking for beginning of value: invalid type: invalid type", + "cannot unmarshal ICS20-V2 transfer packet data: unexpected EOF: invalid type: invalid type", }, { "failure: receive disabled", diff --git a/modules/apps/transfer/internal/packet.go b/modules/apps/transfer/internal/packet.go index cd6a2638954..cc5885523a8 100644 --- a/modules/apps/transfer/internal/packet.go +++ b/modules/apps/transfer/internal/packet.go @@ -3,8 +3,12 @@ package internal import ( "encoding/json" + "github.com/cosmos/gogoproto/proto" + errorsmod "cosmossdk.io/errors" + "github.com/cosmos/cosmos-sdk/codec/unknownproto" + "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" ) @@ -22,7 +26,11 @@ func UnmarshalPacketData(bz []byte, ics20Version string) (types.FungibleTokenPac return packetDataV1ToV2(datav1) case types.V2: var datav2 types.FungibleTokenPacketDataV2 - if err := json.Unmarshal(bz, &datav2); err != nil { + if err := unknownproto.RejectUnknownFieldsStrict(bz, &datav2, unknownproto.DefaultAnyResolver{}); err != nil { + return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error()) + } + + if err := proto.Unmarshal(bz, &datav2); err != nil { return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error()) } diff --git a/modules/apps/transfer/internal/packet_test.go b/modules/apps/transfer/internal/packet_test.go index 30202ed8218..f9ed72aab42 100644 --- a/modules/apps/transfer/internal/packet_test.go +++ b/modules/apps/transfer/internal/packet_test.go @@ -8,6 +8,12 @@ import ( errorsmod "cosmossdk.io/errors" "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" + ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" +) + +const ( + sender = "sender" + receiver = "receiver" ) var emptyForwardingPacketData = types.ForwardingPacketData{} @@ -37,7 +43,7 @@ func TestUnmarshalPacketData(t *testing.T) { Denom: types.NewDenom("atom", types.NewHop("transfer", "channel-0")), Amount: "1000", }, - }, "sender", "receiver", "", emptyForwardingPacketData) + }, sender, receiver, "", emptyForwardingPacketData) packetDataBz = packetData.GetBytes() version = types.V2 @@ -55,7 +61,7 @@ func TestUnmarshalPacketData(t *testing.T) { for _, tc := range testCases { - packetDataV1 := types.NewFungibleTokenPacketData("transfer/channel-0/atom", "1000", "sender", "receiver", "") + packetDataV1 := types.NewFungibleTokenPacketData("transfer/channel-0/atom", "1000", sender, receiver, "") packetDataBz = packetDataV1.GetBytes() version = types.V1 @@ -73,6 +79,61 @@ func TestUnmarshalPacketData(t *testing.T) { } } +// TestV2ForwardsCompatibilityFails asserts that new fields being added to a future proto definition of +// FungibleTokenPacketDataV2 fail to unmarshal with previous versions. In essence, permit backwards compatibility +// but restrict forward one. +func TestV2ForwardsCompatibilityFails(t *testing.T) { + var ( + packet types.FungibleTokenPacketDataV2 + packetDataBz []byte + ) + + testCases := []struct { + name string + malleate func() + expError error + }{ + { + "success", + func() {}, + nil, + }, + { + "failure: new field present in packet data", + func() { + // packet data containing extra field unknown to current proto file. + packetDataBz = append(packet.GetBytes(), []byte("22\tnew_value")...) + }, + ibcerrors.ErrInvalidType, + }, + } + + for _, tc := range testCases { + packet = types.NewFungibleTokenPacketDataV2( + []types.Token{ + { + Denom: types.NewDenom("atom", types.NewHop("transfer", "channel-0")), + Amount: "1000", + }, + }, "sender", "receiver", "", emptyForwardingPacketData, + ) + + packetDataBz = packet.GetBytes() + + tc.malleate() + + packetData, err := UnmarshalPacketData(packetDataBz, types.V2) + + expPass := tc.expError == nil + if expPass { + require.NoError(t, err) + require.NotEqual(t, types.FungibleTokenPacketDataV2{}, packetData) + } else { + require.ErrorIs(t, err, tc.expError) + } + } +} + func TestPacketV1ToPacketV2(t *testing.T) { const ( sender = "sender" diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 79bb359f0ae..926a1ff155a 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -4,6 +4,8 @@ import ( "fmt" "time" + "github.com/cosmos/gogoproto/proto" + sdkmath "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" @@ -277,7 +279,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithMemo() { // Check that the memo is stored correctly in the packet sent from A var tokenPacketOnA types.FungibleTokenPacketDataV2 - err = suite.chainA.Codec.UnmarshalJSON(packetFromAtoB.Data, &tokenPacketOnA) + err = proto.Unmarshal(packetFromAtoB.Data, &tokenPacketOnA) suite.Require().NoError(err) suite.Require().Equal("", tokenPacketOnA.Memo) suite.Require().Equal(testMemo, tokenPacketOnA.Forwarding.DestinationMemo) @@ -299,7 +301,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithMemo() { // Check that the memo is stored correctly in the packet sent from B var tokenPacketOnB types.FungibleTokenPacketDataV2 - err = suite.chainB.Codec.UnmarshalJSON(packetFromBtoC.Data, &tokenPacketOnB) + err = proto.Unmarshal(packetFromBtoC.Data, &tokenPacketOnB) suite.Require().NoError(err) suite.Require().Equal(testMemo, tokenPacketOnB.Memo) suite.Require().Equal("", tokenPacketOnB.Forwarding.DestinationMemo) @@ -324,7 +326,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithMemo() { // Check that the memo is stored directly in the memo field on C var tokenPacketOnC types.FungibleTokenPacketDataV2 - err = suite.chainC.Codec.UnmarshalJSON(packetOnC.Data, &tokenPacketOnC) + err = proto.Unmarshal(packetOnC.Data, &tokenPacketOnC) suite.Require().NoError(err) suite.Require().Equal("", tokenPacketOnC.Forwarding.DestinationMemo) suite.Require().Equal(testMemo, tokenPacketOnC.Memo) @@ -412,7 +414,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithNonCosmosAccAddress() { // Check that the token sent from A has final receiver intact var tokenPacketOnA types.FungibleTokenPacketDataV2 - err = suite.chainA.Codec.UnmarshalJSON(packetFromAtoB.Data, &tokenPacketOnA) + err = proto.Unmarshal(packetFromAtoB.Data, &tokenPacketOnA) suite.Require().NoError(err) suite.Require().Equal(nonCosmosReceiver, tokenPacketOnA.Receiver) @@ -432,7 +434,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithNonCosmosAccAddress() { // Check that the token sent from B has final receiver intact var tokenPacketOnB types.FungibleTokenPacketDataV2 - err = suite.chainB.Codec.UnmarshalJSON(packetFromBtoC.Data, &tokenPacketOnB) + err = proto.Unmarshal(packetFromBtoC.Data, &tokenPacketOnB) suite.Require().NoError(err) suite.Require().Equal(nonCosmosReceiver, tokenPacketOnB.Receiver) diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index fd1d7f69320..9e2dacd35ff 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/modules/apps/transfer/keeper/relay_test.go @@ -1154,55 +1154,9 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() { expError error expAckError error }{ - { - "success: no new field with memo v2", - func() { - jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"base": "atom", "trace": []},"amount":"100"}],"sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String()) - packetData = []byte(jsonString) - }, - nil, - nil, - }, - { - "success: no new field without memo", - func() { - jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"base": "atom", "trace": []},"amount":"100"}],"sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String()) - packetData = []byte(jsonString) - }, - nil, - nil, - }, - { - "failure: invalid packet data v2", - func() { - packetData = []byte("invalid packet data") - }, - ibcerrors.ErrInvalidType, - ibcerrors.ErrInvalidType, - }, - { - "failure: 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) - }, - ibcerrors.ErrInvalidType, - ibcerrors.ErrInvalidType, - }, - { - "failure: missing 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()) - packetData = []byte(jsonString) - }, - types.ErrInvalidDenomForTransfer, - ibcerrors.ErrInvalidType, - }, { "success: no new field with memo", 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"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String()) packetData = []byte(jsonString) }, @@ -1212,8 +1166,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() { { "success: no new field without memo", func() { - path.EndpointA.ChannelConfig.Version = types.V1 - path.EndpointB.ChannelConfig.Version = types.V1 jsonString := fmt.Sprintf(`{"denom":"denom","amount":"100","sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String()) packetData = []byte(jsonString) }, @@ -1223,8 +1175,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() { { "failure: invalid packet data", func() { - path.EndpointA.ChannelConfig.Version = types.V1 - path.EndpointB.ChannelConfig.Version = types.V1 packetData = []byte("invalid packet data") }, ibcerrors.ErrInvalidType, @@ -1233,8 +1183,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() { { "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) }, @@ -1244,8 +1192,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() { { "failure: missing field", func() { - path.EndpointA.ChannelConfig.Version = types.V1 - path.EndpointB.ChannelConfig.Version = types.V1 jsonString := fmt.Sprintf(`{"amount":"100","sender":%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String()) packetData = []byte(jsonString) }, @@ -1261,6 +1207,8 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() { packetData = nil path = ibctesting.NewTransferPath(suite.chainA, suite.chainB) + path.EndpointA.ChannelConfig.Version = types.V1 + path.EndpointB.ChannelConfig.Version = types.V1 tc.malleate() diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index 7a182b8598e..dadb354d28e 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -5,6 +5,8 @@ import ( "errors" "strings" + "github.com/cosmos/gogoproto/proto" + errorsmod "cosmossdk.io/errors" sdkmath "cosmossdk.io/math" @@ -152,9 +154,10 @@ func (ftpd FungibleTokenPacketDataV2) ValidateBasic() error { return nil } -// GetBytes is a helper for serialising +// GetBytes is a helper for serialising a FungibleTokenPacketDataV2. It uses protobuf to serialise +// the packet data and panics on failure. func (ftpd FungibleTokenPacketDataV2) GetBytes() []byte { - bz, err := json.Marshal(&ftpd) + bz, err := proto.Marshal(&ftpd) if err != nil { panic(errors.New("cannot marshal FungibleTokenPacketDataV2 into bytes")) }