diff --git a/x/ibc/04-channel/keeper/timeout.go b/x/ibc/04-channel/keeper/timeout.go index ca7161077c..4ef456b927 100644 --- a/x/ibc/04-channel/keeper/timeout.go +++ b/x/ibc/04-channel/keeper/timeout.go @@ -77,11 +77,6 @@ func (k Keeper) TimeoutPacket( return nil, sdkerrors.Wrap(types.ErrPacketTimeout, "packet timeout has not been reached for height or timestamp") } - // check that packet has not been received - if nextSequenceRecv >= packet.GetSequence() { - return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet already received") - } - commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) // verify we sent the packet and haven't cleared it out yet @@ -91,6 +86,11 @@ func (k Keeper) TimeoutPacket( switch channel.Ordering { case exported.ORDERED: + // check that packet has not been received + if nextSequenceRecv > packet.GetSequence() { + return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet already received") + } + // check that the recv sequence is as claimed err = k.connectionKeeper.VerifyNextSequenceRecv( ctx, connectionEnd, proofHeight, proof, diff --git a/x/ibc/20-transfer/keeper/keeper_test.go b/x/ibc/20-transfer/keeper/keeper_test.go index f5405e4eac..b3a0c512fd 100644 --- a/x/ibc/20-transfer/keeper/keeper_test.go +++ b/x/ibc/20-transfer/keeper/keeper_test.go @@ -63,6 +63,10 @@ func (suite *KeeperTestSuite) SetupTest() { suite.chainA = NewTestChain(testClientIDA) suite.chainB = NewTestChain(testClientIDB) + // reset prefixCoins at each setup + prefixCoins = sdk.NewCoins(sdk.NewCoin("bank/firstchannel/atom", sdk.NewInt(100))) + prefixCoins2 = sdk.NewCoins(sdk.NewCoin("testportid/secondchannel/atom", sdk.NewInt(100))) + suite.cdc = suite.chainA.App.Codec() } diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index b0c1e04389..e0d66e458b 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -209,7 +209,7 @@ func (k Keeper) refundPacketAmount(ctx sdk.Context, packet channel.Packet, data } // check the denom prefix - prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) + prefix := types.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel()) source := strings.HasPrefix(data.Amount[0].Denom, prefix) // decode the sender address @@ -229,7 +229,7 @@ func (k Keeper) refundPacketAmount(ctx sdk.Context, packet channel.Packet, data } // unescrow tokens back to sender - escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel()) + escrowAddress := types.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel()) return k.bankKeeper.SendCoins(ctx, escrowAddress, sender, coins) } diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index 8684c5bf31..16df8202c3 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -130,7 +130,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { // - receiving chain {"incorrect dest prefix on coin denom", func() { - data.Amount = prefixCoins2 + data.Amount = prefixCoins }, false}, {"success receive from external chain", func() { @@ -164,8 +164,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { // TestOnAcknowledgementPacket tests that successful acknowledgement is a no-op // and failure acknowledment leads to refund func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() { - data := types.NewFungibleTokenPacketData(prefixCoins, testAddr1.String(), testAddr2.String()) - testCoins2 := sdk.NewCoins(sdk.NewCoin("testportid/secondchannel/atom", sdk.NewInt(100))) + data := types.NewFungibleTokenPacketData(prefixCoins2, testAddr1.String(), testAddr2.String()) successAck := types.FungibleTokenPacketAcknowledgement{ Success: true, @@ -186,13 +185,13 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() { func() {}, true, true}, {"successful refund from source chain", failedAck, func() { - escrow := types.GetEscrowAddress(testPort2, testChannel2) + escrow := types.GetEscrowAddress(testPort1, testChannel1) _, err := suite.chainA.App.BankKeeper.AddCoins(suite.chainA.GetContext(), escrow, sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(100)))) suite.Require().NoError(err) }, true, false}, {"successful refund from external chain", failedAck, func() { - data.Amount = testCoins2 + data.Amount = prefixCoins }, false, false}, } @@ -204,18 +203,18 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() { suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { suite.SetupTest() // reset - preCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, prefixCoins[0].Denom) - tc.malleate() var denom string if tc.source { - prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) - denom = prefixCoins[0].Denom[len(prefix):] + prefix := types.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel()) + denom = prefixCoins2[0].Denom[len(prefix):] } else { denom = data.Amount[0].Denom } + preCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, denom) + err := suite.chainA.App.TransferKeeper.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, data, tc.ack) suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) @@ -225,7 +224,7 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() { if tc.success { suite.Require().Equal(sdk.ZeroInt(), deltaAmount, "successful ack changed balance") } else { - suite.Require().Equal(prefixCoins[0].Amount, deltaAmount, "failed ack did not trigger refund") + suite.Require().Equal(prefixCoins2[0].Amount, deltaAmount, "failed ack did not trigger refund") } }) } @@ -233,8 +232,8 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() { // TestOnTimeoutPacket test private refundPacket function since it is a simple wrapper over it func (suite *KeeperTestSuite) TestOnTimeoutPacket() { - data := types.NewFungibleTokenPacketData(prefixCoins, testAddr1.String(), testAddr2.String()) - testCoins2 := sdk.NewCoins(sdk.NewCoin("testportid/secondchannel/atom", sdk.NewInt(100))) + data := types.NewFungibleTokenPacketData(prefixCoins2, testAddr1.String(), testAddr2.String()) + testCoins2 := sdk.NewCoins(sdk.NewCoin("bank/firstchannel/atom", sdk.NewInt(100))) testCases := []struct { msg string @@ -244,7 +243,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() { }{ {"successful timeout from source chain", func() { - escrow := types.GetEscrowAddress(testPort2, testChannel2) + escrow := types.GetEscrowAddress(testPort1, testChannel1) _, err := suite.chainA.App.BankKeeper.AddCoins(suite.chainA.GetContext(), escrow, sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(100)))) suite.Require().NoError(err) }, true, true}, @@ -261,7 +260,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() { }, true, false}, {"mint failed", func() { - data.Amount[0].Denom = prefixCoins[0].Denom + data.Amount[0].Denom = prefixCoins2[0].Denom data.Amount[0].Amount = sdk.ZeroInt() }, true, false}, } @@ -274,18 +273,18 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() { suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { suite.SetupTest() // reset - preCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, prefixCoins[0].Denom) - tc.malleate() var denom string if tc.source { - prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) - denom = prefixCoins[0].Denom[len(prefix):] + prefix := types.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel()) + denom = prefixCoins2[0].Denom[len(prefix):] } else { denom = data.Amount[0].Denom } + preCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, denom) + err := suite.chainA.App.TransferKeeper.OnTimeoutPacket(suite.chainA.GetContext(), packet, data) postCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, denom) @@ -293,7 +292,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() { if tc.expPass { suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) - suite.Require().Equal(prefixCoins[0].Amount.Int64(), deltaAmount.Int64(), "failed ack did not trigger refund") + suite.Require().Equal(prefixCoins2[0].Amount.Int64(), deltaAmount.Int64(), "successful timeout did not trigger refund") } else { suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg) } diff --git a/x/ibc/20-transfer/module.go b/x/ibc/20-transfer/module.go index dd0758a640..cb50f0d25d 100644 --- a/x/ibc/20-transfer/module.go +++ b/x/ibc/20-transfer/module.go @@ -324,7 +324,7 @@ func (am AppModule) OnTimeoutPacket( packet channeltypes.Packet, ) (*sdk.Result, error) { var data FungibleTokenPacketData - if err := types.ModuleCdc.UnmarshalBinaryBare(packet.GetData(), &data); err != nil { + if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error()) } // refund tokens