fix: prevent double spends after upgrades for transfer channels

modules/core/04-channel/keeper/keeper.go

@@ -194,12 +194,6 @@ func (k Keeper) SetPacketReceipt(ctx sdk.Context, portID, channelID string, sequ
 	store.Set(host.PacketReceiptKey(portID, channelID, sequence), []byte{byte(1)})
 }
 
-// deletePacketReceipt deletes a packet receipt from the store
-func (k Keeper) deletePacketReceipt(ctx sdk.Context, portID, channelID string, sequence uint64) {
-	store := ctx.KVStore(k.storeKey)
-	store.Delete(host.PacketReceiptKey(portID, channelID, sequence))
-}
-
 // GetPacketCommitment gets the packet commitment hash from the store
 func (k Keeper) GetPacketCommitment(ctx sdk.Context, portID, channelID string, sequence uint64) []byte {
 	store := ctx.KVStore(k.storeKey)
@@ -690,9 +684,6 @@ func (k Keeper) PruneAcknowledgements(ctx sdk.Context, portID, channelID string,
 		}
 
 		k.deletePacketAcknowledgement(ctx, portID, channelID, start)
-
-		// NOTE: packet receipts are only relevant for unordered channels.
-		k.deletePacketReceipt(ctx, portID, channelID, start)
 	}
 
 	// set pruning sequence start to the updated value

modules/core/04-channel/keeper/keeper_test.go

@@ -604,7 +604,7 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 				suite.Require().True(found)
 
 				// We expect nothing to be left and sequenceStart == sequenceEnd.
-				postPruneExpState(0, 0, sequenceEnd)
+				postPruneExpState(0, 10, sequenceEnd)
 
 				// We expect 10 to be pruned and 0 left.
 				suite.Require().Equal(uint64(10), pruned)
@@ -624,7 +624,7 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 			},
 			func(pruned, left uint64) {
 				// We expect 4 to be left and sequenceStart == 7.
-				postPruneExpState(4, 4, 7)
+				postPruneExpState(4, 10, 7)
 
 				// We expect 6 to be pruned and 4 left.
 				suite.Require().Equal(uint64(6), pruned)
@@ -644,7 +644,7 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 			},
 			func(pruned, left uint64) {
 				// We expect 0 to be left and sequenceStart == 11.
-				postPruneExpState(0, 0, 11)
+				postPruneExpState(0, 10, 11)
 
 				// We expect 10 to be pruned and 0 left.
 				suite.Require().Equal(uint64(10), pruned)
@@ -676,7 +676,7 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 				suite.Require().True(found)
 
 				// We expect nothing to be left and sequenceStart == sequenceEnd.
-				postPruneExpState(0, 0, sequenceEnd)
+				postPruneExpState(0, 15, sequenceEnd)
 
 				// We expect 15 to be pruned and 0 left.
 				suite.Require().Equal(uint64(15), pruned)
@@ -706,7 +706,7 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 				suite.Require().Equal(uint64(6), left)
 
 				// Check state post-prune
-				postPruneExpState(6, 6, 5)
+				postPruneExpState(6, 10, 5)
 
 				// Previous upgrade is complete, send additional packets and do yet another upgrade.
 				// This is _after_ the first upgrade.
@@ -720,7 +720,7 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 			},
 			func(pruned, left uint64) {
 				// Expected state should be 6 acks/receipts left, sequenceStart == 15.
-				postPruneExpState(6, 6, 15)
+				postPruneExpState(6, 20, 15)
 
 				// We expect 10 to be pruned and 6 left.
 				suite.Require().Equal(uint64(10), pruned)
@@ -747,8 +747,8 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 				suite.UpgradeChannel(path, upgradeFields)
 			},
 			func(pruned, left uint64) {
-				// After pruning 10 sequences we should be left with 5 acks and zero receipts.
-				postPruneExpState(5, 0, 11)
+				// After pruning 10 sequences we should be left with 5 acks and 5 receipts.
+				postPruneExpState(5, 5, 11)
 
 				// We expect 10 to be pruned and 5 left.
 				suite.Require().Equal(uint64(10), pruned)
@@ -784,7 +784,7 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 				suite.Require().Equal(uint64(0), left)
 
 				// we _do not_ expect error, simply a fast return
-				postPruneExpState(12, 12, 6)
+				postPruneExpState(12, 17, 6)
 			},
 			nil,
 		},
@@ -800,10 +800,10 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
 				limit = 7
 			},
 			func(pruned, left uint64) {
-				// After pruning 7 sequences we should be left with 5 acks and 5 receipts,
+				// After pruning 7 sequences we should be left with 5 acks and 7 receipts,
 				// because the 2 packets that timeout are still counted as pruned, even though
 				// there was nothing to prune since both packets timed out
-				postPruneExpState(5, 5, 8)
+				postPruneExpState(5, 10, 8)
 
 				// We expect 7 to be pruned and 5 left.
 				suite.Require().Equal(uint64(7), pruned)

modules/core/integration_test.go

@@ -0,0 +1,105 @@
+package ibc_test
+
+import (
+	sdkmath "cosmossdk.io/math"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types"
+	transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
+	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
+	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
+	host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
+	ibctesting "github.com/cosmos/ibc-go/v8/testing"
+)
+
+// If packet receipts are pruned, it is possible to double spend
+// by resubmitting the same proof used to process the original receive.
+func (suite *IBCTestSuite) TestDoubleSpendAttackOnPrunedReceipts() {
+	path := ibctesting.NewPath(suite.chainA, suite.chainB)
+
+	// configure the initial path to create a regular transfer channel
+	path.EndpointA.ChannelConfig.PortID = transfertypes.PortID
+	path.EndpointB.ChannelConfig.PortID = transfertypes.PortID
+	path.EndpointA.ChannelConfig.Version = transfertypes.Version
+	path.EndpointB.ChannelConfig.Version = transfertypes.Version
+
+	suite.coordinator.Setup(path)
+
+	// configure the channel upgrade to upgrade to an incentivized fee enabled transfer channel
+	upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version}))
+	path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+	path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
+
+	// setup double spend attack
+	amount, ok := sdkmath.NewIntFromString("1000")
+	suite.Require().True(ok)
+	coin := sdk.NewCoin(sdk.DefaultBondDenom, amount)
+
+	// send from chainA to chainB
+	timeoutHeight := clienttypes.NewHeight(1, 110)
+	msg := transfertypes.NewMsgTransfer(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, coin, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0, "")
+	res, err := suite.chainA.SendMsgs(msg)
+	suite.Require().NoError(err) // message committed
+
+	packet, err := ibctesting.ParsePacketFromEvents(res.Events)
+	suite.Require().NoError(err)
+
+	// relay
+	err = path.EndpointB.UpdateClient()
+	suite.Require().NoError(err)
+
+	// get proof of packet commitment on source
+	packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
+	proof, proofHeight := path.EndpointA.Chain.QueryProof(packetKey)
+
+	recvMsg := channeltypes.NewMsgRecvPacket(packet, proof, proofHeight, path.EndpointB.Chain.SenderAccount.GetAddress().String())
+
+	// receive on counterparty and update source client
+	res, err = path.EndpointB.Chain.SendMsgs(recvMsg)
+	suite.Require().NoError(err)
+
+	// check that voucher exists on chain B
+	voucherDenomTrace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom(packet.GetDestPort(), packet.GetDestChannel(), sdk.DefaultBondDenom))
+	ibcCoin := sdk.NewCoin(voucherDenomTrace.IBCDenom(), coin.Amount)
+	balance := suite.chainB.GetSimApp().BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), voucherDenomTrace.IBCDenom())
+	suite.Require().Equal(ibcCoin, balance)
+
+	err = path.EndpointA.UpdateClient()
+	suite.Require().NoError(err)
+
+	ack, err := ibctesting.ParseAckFromEvents(res.Events)
+	suite.Require().NoError(err)
+
+	err = path.EndpointA.AcknowledgePacket(packet, ack)
+	suite.Require().NoError(err)
+
+	err = path.EndpointA.ChanUpgradeInit()
+	suite.Require().NoError(err)
+
+	err = path.EndpointB.ChanUpgradeTry()
+	suite.Require().NoError(err)
+
+	err = path.EndpointA.ChanUpgradeAck()
+	suite.Require().NoError(err)
+
+	err = path.EndpointB.ChanUpgradeConfirm()
+	suite.Require().NoError(err)
+
+	err = path.EndpointA.ChanUpgradeOpen()
+	suite.Require().NoError(err)
+
+	// prune
+	msgPrune := channeltypes.NewMsgPruneAcknowledgements(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, 1, path.EndpointB.Chain.SenderAccount.GetAddress().String())
+	res, err = path.EndpointB.Chain.SendMsgs(msgPrune)
+	suite.Require().NoError(err)
+
+	recvMsg = channeltypes.NewMsgRecvPacket(packet, proof, proofHeight, path.EndpointB.Chain.SenderAccount.GetAddress().String())
+
+	// double spend
+	res, err = path.EndpointB.Chain.SendMsgs(recvMsg)
+	suite.Require().NoError(err)
+
+	balance = suite.chainB.GetSimApp().BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), voucherDenomTrace.IBCDenom())
+	suite.Require().Equal(ibcCoin, balance, "successfully double spent")
+}