Skip to content

Commit a934597

Browse files
rootulprootulp
authored
Merge pull request #1 from rootulp/rp/json-unmarshalling
fix: check JSON serialization/deserialization is deterministic
2 parents 6ce4d70 + 616fa05 commit a934597

File tree

2 files changed

+302
-0
lines changed

2 files changed

+302
-0
lines changed

‎modules/apps/transfer/ibc_module.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package transfer
22

33
import (
4+
"bytes"
45
"fmt"
56
"math"
67
"strings"
@@ -9,6 +10,8 @@ import (
910
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
1011
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
1112

13+
errorsmod "cosmossdk.io/errors"
14+
1215
"github.com/cosmos/ibc-go/v6/modules/apps/transfer/keeper"
1316
"github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
1417
channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
@@ -229,6 +232,13 @@ func (im IBCModule) OnAcknowledgementPacket(
229232
return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
230233
}
231234

235+
// Verify that the acknowledgmenet serializes and deserializes to JSON determinstically.
236+
// See https://github.com/cosmos/ibc-go/security/advisories/GHSA-jg6f-48ff-5xrw
237+
bz := types.ModuleCdc.MustMarshalJSON(&ack)
238+
if !bytes.Equal(bz, acknowledgement) {
239+
return errorsmod.Wrapf(sdkerrors.ErrInvalidType, "acknowledgement did not marshal to expected bytes: %X ≠ %X", bz, acknowledgement)
240+
}
241+
232242
if err := im.keeper.OnAcknowledgementPacket(ctx, packet, data, ack); err != nil {
233243
return err
234244
}

‎modules/apps/transfer/ibc_module_test.go

Lines changed: 292 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,18 @@
11
package transfer_test
22

33
import (
4+
"errors"
45
"math"
56

7+
sdk "github.com/cosmos/cosmos-sdk/types"
8+
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
69
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
710

811
"github.com/cosmos/ibc-go/v6/modules/apps/transfer"
912
"github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
1013
channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
1114
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
15+
"github.com/cosmos/ibc-go/v6/modules/core/exported"
1216
ibctesting "github.com/cosmos/ibc-go/v6/testing"
1317
)
1418

@@ -239,3 +243,291 @@ func (suite *TransferTestSuite) TestOnChanOpenAck() {
239243
})
240244
}
241245
}
246+
247+
func (suite *TransferTestSuite) TestOnRecvPacket() {
248+
// This test suite mostly covers the top-level logic of the ibc module OnRecvPacket function
249+
// The core logic is covered in keeper OnRecvPacket
250+
var (
251+
packet channeltypes.Packet
252+
path *ibctesting.Path
253+
)
254+
testCases := []struct {
255+
name string
256+
malleate func()
257+
expAck exported.Acknowledgement
258+
}{
259+
{
260+
"success", func() {}, channeltypes.NewResultAcknowledgement([]byte{byte(1)}),
261+
},
262+
{
263+
"failure: invalid packet data bytes",
264+
func() {
265+
packet.Data = []byte("invalid data")
266+
},
267+
channeltypes.NewErrorAcknowledgement(sdkerrors.ErrInvalidType),
268+
},
269+
{
270+
"failure: receive disabled",
271+
func() {
272+
suite.chainB.GetSimApp().TransferKeeper.SetParams(suite.chainB.GetContext(), types.Params{ReceiveEnabled: false})
273+
},
274+
channeltypes.NewErrorAcknowledgement(types.ErrReceiveDisabled),
275+
},
276+
}
277+
278+
for _, tc := range testCases {
279+
suite.Run(tc.name, func() {
280+
suite.SetupTest() // reset
281+
282+
path = NewTransferPath(suite.chainA, suite.chainB)
283+
suite.coordinator.Setup(path)
284+
285+
coin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
286+
287+
packetData := types.NewFungibleTokenPacketData(
288+
coin.Denom,
289+
coin.Amount.String(),
290+
suite.chainA.SenderAccount.GetAddress().String(),
291+
suite.chainB.SenderAccount.GetAddress().String(),
292+
"",
293+
)
294+
295+
seq := uint64(1)
296+
packet = channeltypes.NewPacket(packetData.GetBytes(), seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, suite.chainA.GetTimeoutHeight(), 0)
297+
298+
ctx := suite.chainB.GetContext()
299+
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Router.GetRoute(ibctesting.TransferPort)
300+
suite.Require().True(ok)
301+
302+
tc.malleate() // change fields in packet
303+
304+
ack := cbs.OnRecvPacket(ctx, packet, suite.chainB.SenderAccount.GetAddress())
305+
306+
suite.Require().Equal(tc.expAck, ack)
307+
})
308+
}
309+
}
310+
311+
func (suite *TransferTestSuite) TestOnAcknowledgePacket() {
312+
var (
313+
path *ibctesting.Path
314+
packet channeltypes.Packet
315+
ack []byte
316+
)
317+
318+
testCases := []struct {
319+
name string
320+
malleate func()
321+
expError error
322+
expRefund bool
323+
}{
324+
{
325+
"success",
326+
func() {},
327+
nil,
328+
false,
329+
},
330+
{
331+
"success: refund coins",
332+
func() {
333+
ack = channeltypes.NewErrorAcknowledgement(sdkerrors.ErrInsufficientFunds).Acknowledgement()
334+
},
335+
nil,
336+
true,
337+
},
338+
{
339+
"cannot refund ack on non-existent channel",
340+
func() {
341+
ack = channeltypes.NewErrorAcknowledgement(sdkerrors.ErrInsufficientFunds).Acknowledgement()
342+
343+
packet.SourceChannel = "channel-100"
344+
},
345+
errors.New("unable to unescrow tokens"),
346+
false,
347+
},
348+
{
349+
"invalid packet data",
350+
func() {
351+
packet.Data = []byte("invalid data")
352+
},
353+
sdkerrors.ErrUnknownRequest,
354+
false,
355+
},
356+
{
357+
"invalid acknowledgement",
358+
func() {
359+
ack = []byte("invalid ack")
360+
},
361+
sdkerrors.ErrUnknownRequest,
362+
false,
363+
},
364+
{
365+
"cannot refund already acknowledged packet",
366+
func() {
367+
ack = channeltypes.NewErrorAcknowledgement(sdkerrors.ErrInsufficientFunds).Acknowledgement()
368+
369+
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Router.GetRoute(ibctesting.TransferPort)
370+
suite.Require().True(ok)
371+
372+
suite.Require().NoError(cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, ack, suite.chainA.SenderAccount.GetAddress()))
373+
},
374+
errors.New("unable to unescrow tokens"),
375+
false,
376+
},
377+
{
378+
// See https://github.com/cosmos/ibc-go/security/advisories/GHSA-jg6f-48ff-5xrw
379+
"non-deterministic JSON ack serialization should return an error",
380+
func() {
381+
// Create a valid acknowledgement using deterministic serialization.
382+
ack = channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
383+
// Introduce non-determinism: insert an extra space after the first character '{'
384+
// This will deserialize correctly but fail to re-serialize to the expected bytes.
385+
if len(ack) > 0 && ack[0] == '{' {
386+
ack = []byte("{ " + string(ack[1:]))
387+
}
388+
},
389+
errors.New("acknowledgement did not marshal to expected bytes"),
390+
false,
391+
},
392+
}
393+
394+
for _, tc := range testCases {
395+
tc := tc
396+
suite.Run(tc.name, func() {
397+
suite.SetupTest() // reset
398+
399+
path = NewTransferPath(suite.chainA, suite.chainB)
400+
suite.coordinator.Setup(path)
401+
402+
timeoutHeight := suite.chainA.GetTimeoutHeight()
403+
msg := types.NewMsgTransfer(
404+
path.EndpointA.ChannelConfig.PortID,
405+
path.EndpointA.ChannelID,
406+
ibctesting.TestCoin,
407+
suite.chainA.SenderAccount.GetAddress().String(),
408+
suite.chainB.SenderAccount.GetAddress().String(),
409+
timeoutHeight,
410+
0,
411+
"",
412+
)
413+
res, err := suite.chainA.SendMsgs(msg)
414+
suite.Require().NoError(err) // message committed
415+
416+
packet, err = ibctesting.ParsePacketFromEvents(res.GetEvents())
417+
suite.Require().NoError(err)
418+
419+
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Router.GetRoute(ibctesting.TransferPort)
420+
suite.Require().True(ok)
421+
422+
ack = channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
423+
424+
tc.malleate() // change fields in packet
425+
426+
err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, ack, suite.chainA.SenderAccount.GetAddress())
427+
428+
if tc.expError == nil {
429+
suite.Require().NoError(err)
430+
431+
if tc.expRefund {
432+
escrowAddress := types.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel())
433+
escrowBalanceAfter := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), escrowAddress, sdk.DefaultBondDenom)
434+
suite.Require().Equal(sdk.NewInt(0), escrowBalanceAfter.Amount)
435+
}
436+
} else {
437+
suite.Require().Error(err)
438+
suite.Require().Contains(err.Error(), tc.expError.Error())
439+
}
440+
})
441+
}
442+
}
443+
444+
func (suite *TransferTestSuite) TestOnTimeoutPacket() {
445+
var path *ibctesting.Path
446+
var packet channeltypes.Packet
447+
448+
testCases := []struct {
449+
name string
450+
coinsToSendToB sdk.Coin
451+
malleate func()
452+
expError error
453+
}{
454+
{
455+
"success",
456+
ibctesting.TestCoin,
457+
func() {},
458+
nil,
459+
},
460+
{
461+
"non-existent channel",
462+
ibctesting.TestCoin,
463+
func() {
464+
packet.SourceChannel = "channel-100"
465+
},
466+
errors.New("unable to unescrow tokens"),
467+
},
468+
{
469+
"invalid packet data",
470+
ibctesting.TestCoin,
471+
func() {
472+
packet.Data = []byte("invalid data")
473+
},
474+
sdkerrors.ErrUnknownRequest,
475+
},
476+
{
477+
"already timed-out packet",
478+
ibctesting.TestCoin,
479+
func() {
480+
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Router.GetRoute(ibctesting.TransferPort)
481+
suite.Require().True(ok)
482+
483+
suite.Require().NoError(cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, suite.chainA.SenderAccount.GetAddress()))
484+
},
485+
errors.New("unable to unescrow tokens"),
486+
},
487+
}
488+
489+
for _, tc := range testCases {
490+
tc := tc
491+
suite.Run(tc.name, func() {
492+
suite.SetupTest() // reset
493+
494+
path = NewTransferPath(suite.chainA, suite.chainB)
495+
suite.coordinator.Setup(path)
496+
497+
timeoutHeight := suite.chainA.GetTimeoutHeight()
498+
msg := types.NewMsgTransfer(
499+
path.EndpointA.ChannelConfig.PortID,
500+
path.EndpointA.ChannelID,
501+
tc.coinsToSendToB,
502+
suite.chainA.SenderAccount.GetAddress().String(),
503+
suite.chainB.SenderAccount.GetAddress().String(),
504+
timeoutHeight,
505+
0,
506+
"",
507+
)
508+
res, err := suite.chainA.SendMsgs(msg)
509+
suite.Require().NoError(err) // message committed
510+
511+
packet, err = ibctesting.ParsePacketFromEvents(res.GetEvents())
512+
suite.Require().NoError(err)
513+
514+
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Router.GetRoute(ibctesting.TransferPort)
515+
suite.Require().True(ok)
516+
517+
tc.malleate() // change fields in packet
518+
519+
err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, suite.chainA.SenderAccount.GetAddress())
520+
521+
if tc.expError == nil {
522+
suite.Require().NoError(err)
523+
524+
escrowAddress := types.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel())
525+
escrowBalanceAfter := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), escrowAddress, sdk.DefaultBondDenom)
526+
suite.Require().Equal(sdk.NewInt(0), escrowBalanceAfter.Amount)
527+
} else {
528+
suite.Require().Error(err)
529+
suite.Require().Contains(err.Error(), tc.expError.Error())
530+
}
531+
})
532+
}
533+
}

0 commit comments

Comments
 (0)