From 73374df0b22b80c5c9727e2254ad01f5cee35bb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 17 Jan 2024 18:06:59 +0100 Subject: [PATCH 1/2] fix: prevent double spends after upgrades --- modules/apps/29-fee/transfer_test.go | 63 ++++++++++- modules/core/04-channel/keeper/keeper.go | 9 -- modules/core/04-channel/keeper/keeper_test.go | 22 ++-- modules/core/integration_test.go | 105 ++++++++++++++++++ 4 files changed, 178 insertions(+), 21 deletions(-) create mode 100644 modules/core/integration_test.go diff --git a/modules/apps/29-fee/transfer_test.go b/modules/apps/29-fee/transfer_test.go index d5163f68657..634091e52ad 100644 --- a/modules/apps/29-fee/transfer_test.go +++ b/modules/apps/29-fee/transfer_test.go @@ -1,11 +1,15 @@ package fee_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" ) @@ -106,7 +110,50 @@ func (suite *FeeTestSuite) TestTransferFeeUpgrade() { tc.malleate() - err := path.EndpointA.ChanUpgradeInit() + // 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() @@ -121,6 +168,20 @@ func (suite *FeeTestSuite) TestTransferFeeUpgrade() { 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") + expPass := tc.expError == nil if expPass { channelA := path.EndpointA.GetChannel() diff --git a/modules/core/04-channel/keeper/keeper.go b/modules/core/04-channel/keeper/keeper.go index b1e856608f4..018bb66fb67 100644 --- a/modules/core/04-channel/keeper/keeper.go +++ b/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 diff --git a/modules/core/04-channel/keeper/keeper_test.go b/modules/core/04-channel/keeper/keeper_test.go index ccfe44a9a39..e241e0150ab 100644 --- a/modules/core/04-channel/keeper/keeper_test.go +++ b/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) diff --git a/modules/core/integration_test.go b/modules/core/integration_test.go new file mode 100644 index 00000000000..5f76bb8a653 --- /dev/null +++ b/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") +} From 705a7d42c1e92ed81932a7559890f876d98ead07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 17 Jan 2024 18:14:00 +0100 Subject: [PATCH 2/2] cleanup: remove unnecessary diffs --- modules/apps/29-fee/transfer_test.go | 63 +--------------------------- 1 file changed, 1 insertion(+), 62 deletions(-) diff --git a/modules/apps/29-fee/transfer_test.go b/modules/apps/29-fee/transfer_test.go index 634091e52ad..d5163f68657 100644 --- a/modules/apps/29-fee/transfer_test.go +++ b/modules/apps/29-fee/transfer_test.go @@ -1,15 +1,11 @@ package fee_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" ) @@ -110,50 +106,7 @@ func (suite *FeeTestSuite) TestTransferFeeUpgrade() { tc.malleate() - // 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() + err := path.EndpointA.ChanUpgradeInit() suite.Require().NoError(err) err = path.EndpointB.ChanUpgradeTry() @@ -168,20 +121,6 @@ func (suite *FeeTestSuite) TestTransferFeeUpgrade() { 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") - expPass := tc.expError == nil if expPass { channelA := path.EndpointA.GetChannel()