fix: disallow incentivizing packets which have not been sent or have …

…been already relayed (cosmos#1347)

## Description



closes: cosmos#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/<module>/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

@@ -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)

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
 	}

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

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)
 }
 

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")
 )