From 10dc9297e250c857ac8617b7ad3ec5afca2a2358 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 12 May 2022 12:05:32 +0200 Subject: [PATCH] fix: disallow incentivizing packets which have not been sent or have been already relayed (#1347) ## Description closes: #1318 --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [x] Updated relevant documentation (`docs/`) or specification (`x//spec/`) - [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [x] Re-reviewed `Files changed` in the Github PR explorer - [x] Review `Codecov Report` in the comment section below once CI passes --- modules/apps/29-fee/keeper/keeper.go | 5 ++ modules/apps/29-fee/keeper/msg_server.go | 19 ++++++- modules/apps/29-fee/keeper/msg_server_test.go | 56 ++++++++++++++++++- modules/apps/29-fee/types/expected_keepers.go | 1 + modules/core/04-channel/types/errors.go | 1 + 5 files changed, 80 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/keeper/keeper.go b/modules/apps/29-fee/keeper/keeper.go index da24e5a2772..c7e31ad3d1a 100644 --- a/modules/apps/29-fee/keeper/keeper.go +++ b/modules/apps/29-fee/keeper/keeper.go @@ -64,6 +64,11 @@ func (k Keeper) GetChannel(ctx sdk.Context, portID, channelID string) (channelty return k.channelKeeper.GetChannel(ctx, portID, channelID) } +// GetPacketCommitment wraps IBC ChannelKeeper's GetPacketCommitment function +func (k Keeper) GetPacketCommitment(ctx sdk.Context, portID, channelID string, sequence uint64) []byte { + return k.channelKeeper.GetPacketCommitment(ctx, portID, channelID, sequence) +} + // GetNextSequenceSend wraps IBC ChannelKeeper's GetNextSequenceSend function func (k Keeper) GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool) { return k.channelKeeper.GetNextSequenceSend(ctx, portID, channelID) diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go index 69f6520c759..033fb67439a 100644 --- a/modules/apps/29-fee/keeper/msg_server.go +++ b/modules/apps/29-fee/keeper/msg_server.go @@ -4,6 +4,7 @@ import ( "context" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" @@ -58,7 +59,8 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) // PayPacketFee defines a rpc handler method for MsgPayPacketFee // PayPacketFee is an open callback that may be called by any module/user that wishes to escrow funds in order to -// incentivize the relaying of a known packet +// incentivize the relaying of a known packet. Only packets which have been sent and have not gone through the +// packet life cycle may be incentivized. func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacketFeeAsync) (*types.MsgPayPacketFeeAsyncResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) @@ -71,6 +73,21 @@ func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacket return nil, types.ErrFeeModuleLocked } + nextSeqSend, found := k.GetNextSequenceSend(ctx, msg.PacketId.PortId, msg.PacketId.ChannelId) + if !found { + return nil, sdkerrors.Wrapf(channeltypes.ErrSequenceSendNotFound, "channel does not exist, portID: %s, channelID: %s", msg.PacketId.PortId, msg.PacketId.ChannelId) + } + + // only allow incentivizing of packets which have been sent + if msg.PacketId.Sequence >= nextSeqSend { + return nil, channeltypes.ErrPacketNotSent + } + + // only allow incentivizng of packets which have not completed the packet life cycle + if bz := k.GetPacketCommitment(ctx, msg.PacketId.PortId, msg.PacketId.ChannelId, msg.PacketId.Sequence); len(bz) == 0 { + return nil, sdkerrors.Wrapf(channeltypes.ErrPacketCommitmentNotFound, "packet has already been acknowledged or timed out") + } + if err := k.escrowPacketFee(ctx, msg.PacketId, msg.PacketFee); err != nil { return nil, err } diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 8a927559a51..9de87d64121 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -4,6 +4,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types" + clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v3/testing" ) @@ -186,6 +187,7 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { var ( + packet channeltypes.Packet expEscrowBalance sdk.Coins expFeesInEscrow []types.PacketFee msg *types.MsgPayPacketFeeAsync @@ -234,6 +236,53 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { }, false, }, + { + "channel does not exist", + func() { + msg.PacketId.ChannelId = "channel-100" + + // to test this functionality, we must set the fee to enabled for this non existent channel + // NOTE: the channel doesn't exist in 04-channel keeper, but we will add a mapping within ics29 anyways + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), msg.PacketId.PortId, msg.PacketId.ChannelId) + }, + false, + }, + { + "packet not sent", + func() { + msg.PacketId.Sequence = msg.PacketId.Sequence + 1 + }, + false, + }, + { + "packet already acknowledged", + func() { + err := suite.path.RelayPacket(packet) + suite.Require().NoError(err) + }, + false, + }, + { + "packet already timed out", + func() { + // try to incentivze a packet which is timed out + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, msg.PacketId.Sequence+1) + packet = channeltypes.NewPacket(ibctesting.MockPacketData, packetID.Sequence, packetID.PortId, packetID.ChannelId, suite.path.EndpointB.ChannelConfig.PortID, suite.path.EndpointB.ChannelID, clienttypes.GetSelfHeight(suite.chainB.GetContext()), 0) + + err := suite.path.EndpointA.SendPacket(packet) + suite.Require().NoError(err) + + // need to update chainA's client representing chainB to prove missing ack + err = suite.path.EndpointA.UpdateClient() + suite.Require().NoError(err) + + err = suite.path.EndpointA.TimeoutPacket(packet) + suite.Require().NoError(err) + + msg.PacketId = packetID + }, + false, + }, { "invalid refund address", func() { @@ -278,7 +327,12 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { suite.SetupTest() suite.coordinator.Setup(suite.path) // setup channel + // send a packet to incentivize packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + packet = channeltypes.NewPacket(ibctesting.MockPacketData, packetID.Sequence, packetID.PortId, packetID.ChannelId, suite.path.EndpointB.ChannelConfig.PortID, suite.path.EndpointB.ChannelID, clienttypes.NewHeight(clienttypes.ParseChainID(suite.chainB.ChainID), 100), 0) + err := suite.path.EndpointA.SendPacket(packet) + suite.Require().NoError(err) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) @@ -288,7 +342,7 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { tc.malleate() - _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFeeAsync(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) + _, err = suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFeeAsync(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) if tc.expPass { suite.Require().NoError(err) // message committed diff --git a/modules/apps/29-fee/types/expected_keepers.go b/modules/apps/29-fee/types/expected_keepers.go index 1d9d3439b07..f725ff764b3 100644 --- a/modules/apps/29-fee/types/expected_keepers.go +++ b/modules/apps/29-fee/types/expected_keepers.go @@ -24,6 +24,7 @@ type ICS4Wrapper interface { // ChannelKeeper defines the expected IBC channel keeper type ChannelKeeper interface { GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channeltypes.Channel, found bool) + GetPacketCommitment(ctx sdk.Context, portID, channelID string, sequence uint64) []byte GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool) } diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 8c31cfbae18..efe90f165c0 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -38,4 +38,5 @@ var ( ErrNoOpMsg = sdkerrors.Register(SubModuleName, 23, "message is redundant, no-op will be performed") ErrInvalidChannelVersion = sdkerrors.Register(SubModuleName, 24, "invalid channel version") + ErrPacketNotSent = sdkerrors.Register(SubModuleName, 25, "packet has not been sent") )