fix: add replay protection on upgraded channels #5651

colin-axner · 2024-01-18T10:44:52Z

Description

After performing a channel upgrade, we have reached a new checkpoint state where we should not process any old packets. Replay protection is currently given by packet receipts on unordered channels and next sequence recv on ordered channels. Because upgrading may result in invalid packet receipt state (either by pruning old packet receipts or upgrading from an ordered to unordered channel) we need to add an additional defensive check which ensures packets are only being received after the next checkpoint sequence, otherwise replay attacks (such as double spends) are possible

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

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.

Targeted PR against the correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

…nd-option2

damiannolan

Looks great, lgtm! I just left a minor suggestion on godoc.
Thanks @colin-axner!

damiannolan · 2024-01-22T12:32:32Z

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

@@ -623,17 +623,17 @@ func (k Keeper) HasInflightPackets(ctx sdk.Context, portID, channelID string) bo
 	return iterator.Valid()
 }

-// SetPruningSequenceEnd sets the channel's pruning sequence end to the store.
-func (k Keeper) SetPruningSequenceEnd(ctx sdk.Context, portID, channelID string, sequence uint64) {
+// SetRecvStartSequence sets the channel's recv start sequence to the store.


should we add an extra note, with something like:

NOTE: RecvStartSequence is only set for channels which have undergone an upgrade. This sequences acts as a checkpoint for pruning historical channel data

could also add a note saying its used as the upper bound when pruning

or maybe something specific like "This sequence indicates the first packet sequence which should be processed after an upgrade"

I thought about this, but none of the other set functions for nextSequenceRecv nextSequenceAck indicate their purpose. I wasn't sure where we should be documenting their behaviour (presumably at least in the spec). I can add a comment in you'd like?

damiannolan · 2024-01-22T12:36:51Z

modules/core/integration_test.go

what a juicy test 🚀 🥇

chatton

LGTM really nice job with this one, left a few minor wording suggestions but fix looks solid, nice job 🥇

chatton · 2024-01-22T12:35:54Z

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

+	// rejected.
+	recvStartSequence, _ := k.GetRecvStartSequence(ctx, packet.GetDestPort(), packet.GetDestChannel())
+	if packet.GetSequence() < recvStartSequence {
+		return errorsmod.Wrap(types.ErrPacketReceived, "packet already processed in previous channel upgrade")


Does previous version sound a bit better here?

Suggested change

return errorsmod.Wrap(types.ErrPacketReceived, "packet already processed in previous channel upgrade")

return errorsmod.Wrap(types.ErrPacketReceived, "packet already processed in a previous channel version")

I wanted to stay away from "version" here since you can upgrade a channel without changing its version

chatton · 2024-01-22T12:45:59Z

modules/core/integration_test.go

+	ibcmock "github.com/cosmos/ibc-go/v8/testing/mock"
+)
+
+// If packet receipts are pruned, it is possible to double spend via a


Suggested change

// If packet receipts are pruned, it is possible to double spend via a

// If packet receipts are pruned, it should not be possible to double spend via a

i understood colin's comment to mean that double spending works if the proof is submitted, that's why we added the check against the packet recv seq start in addition to the proof verification? but im not super opinionated, i think it makes sense this way as well.

yea exactly, I can update the comment to be more specific

f235734 7d5f6e8

crodriguezvega

Nice work, @colin-axner!

crodriguezvega · 2024-01-22T12:47:41Z

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

+	// rejected.
+	recvStartSequence, _ := k.GetRecvStartSequence(ctx, packet.GetDestPort(), packet.GetDestChannel())
+	if packet.GetSequence() < recvStartSequence {
+		return errorsmod.Wrap(types.ErrPacketReceived, "packet already processed in previous channel upgrade")


Would it be possible to do return types.ErrNoOpMsg instead of returning an error? Just the same as we do in case the packet receipt for the sequence already exists, so that we prevent consuming fees. We could log the error message instead to give this information.

No, in my opinion this is an error. This would mean someone is attempting to receive packets on a channel that has been upgraded to different fields. We added ErrNoOpMsg so that relayers in a race wouldn't cause each other to fail other relays, but in this situation, I'd say a relayer is either malfunctioning or a user is acting maliciously, in which case I'd much prefer to return an error than to silently fail

There's a possibility one relayer is attempting to flush while another relayer completes the upgrade, but I think this would be very rare and in such cases, I'd still prefer to reject the entire transaction (the relayer is probably flushing stale packets in the tx)

charleenfei

super nice work @colin-axner on the find and the fix!

AdityaSripal

Excellent work @colin-axner !!

colin-axner · 2024-01-22T15:29:50Z

I made setRecvStartSequence private to avoid any accidental pitfalls

DimitrisJim

ignore brain-fart now deleted review, lgtm!

* test: add integration test for double spend attack * refactor: draft alternative approach to fixing double spend * refactor: cleanup tests, deduplicate key storage, add documentation * godoc * test: add packet already recevied unit test case * satisfy the linter * imp: add additional comment to integration test * imp: add a little more info to the test comment * review suggestions + make setRecvStartSeqeuence private (cherry picked from commit 8b6932b)

* test: add integration test for double spend attack * refactor: draft alternative approach to fixing double spend * refactor: cleanup tests, deduplicate key storage, add documentation * godoc * test: add packet already recevied unit test case * satisfy the linter * imp: add additional comment to integration test * imp: add a little more info to the test comment * review suggestions + make setRecvStartSeqeuence private (cherry picked from commit 8b6932b) Co-authored-by: colin axnér <[email protected]> Co-authored-by: Damian Nolan <[email protected]>

colin-axner added 3 commits January 18, 2024 11:07

test: add integration test for double spend attack

3539f25

Merge branch 'main' of github.com:cosmos/ibc-go into colin/double-spe…

94a98f5

…nd-option2

refactor: draft alternative approach to fixing double spend

4406000

colin-axner added audit Feedback from implementation audit priority PRs that need prompt reviews channel-upgradability Channel upgradability feature backport-to-v8.1.x labels Jan 18, 2024

colin-axner mentioned this pull request Jan 18, 2024

fix: prevent double spends after upgrades for transfer channels #5646

Closed

9 tasks

colin-axner self-assigned this Jan 18, 2024

colin-axner added this to the 04-channel upgrades RC milestone Jan 18, 2024

colin-axner added 4 commits January 22, 2024 12:04

Merge branch 'main' of github.com:cosmos/ibc-go into colin/double-spe…

5f1085c

…nd-option2

refactor: cleanup tests, deduplicate key storage, add documentation

7969ba8

godoc

74fed9e

test: add packet already recevied unit test case

1a5ef9b

colin-axner changed the title ~~fix: double spend solution on channel upgrades using counterparty next sequence send~~ fix: add replay protection on upgraded channels Jan 22, 2024

colin-axner marked this pull request as ready for review January 22, 2024 12:17

colin-axner requested review from AdityaSripal and damiannolan as code owners January 22, 2024 12:17

satisfy the linter

48e546b

damiannolan approved these changes Jan 22, 2024

View reviewed changes

chatton approved these changes Jan 22, 2024

View reviewed changes

crodriguezvega reviewed Jan 22, 2024

View reviewed changes

charleenfei approved these changes Jan 22, 2024

View reviewed changes

colin-axner added 2 commits January 22, 2024 16:14

imp: add additional comment to integration test

f235734

imp: add a little more info to the test comment

7d5f6e8

AdityaSripal approved these changes Jan 22, 2024

View reviewed changes

colin-axner added 2 commits January 22, 2024 16:28

review suggestions + make setRecvStartSeqeuence private

b2a1f50

Merge branch 'main' into colin/double-spend-option2

12f721b

DimitrisJim approved these changes Jan 22, 2024

View reviewed changes

colin-axner merged commit 8b6932b into main Jan 22, 2024
63 checks passed

colin-axner deleted the colin/double-spend-option2 branch January 22, 2024 16:08

mergify bot mentioned this pull request Jan 22, 2024

fix: add replay protection on upgraded channels (backport #5651) #5683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add replay protection on upgraded channels #5651

fix: add replay protection on upgraded channels #5651

colin-axner commented Jan 18, 2024 •

edited

Loading

damiannolan left a comment

damiannolan Jan 22, 2024

charleenfei Jan 22, 2024

colin-axner Jan 22, 2024

colin-axner Jan 22, 2024

damiannolan Jan 22, 2024

chatton left a comment

chatton Jan 22, 2024

colin-axner Jan 22, 2024

chatton Jan 22, 2024

charleenfei Jan 22, 2024

colin-axner Jan 22, 2024

colin-axner Jan 22, 2024

crodriguezvega left a comment

crodriguezvega Jan 22, 2024

colin-axner Jan 22, 2024

colin-axner Jan 22, 2024

charleenfei left a comment

AdityaSripal left a comment

colin-axner commented Jan 22, 2024

DimitrisJim left a comment •

edited

Loading

	return errorsmod.Wrap(types.ErrPacketReceived, "packet already processed in previous channel upgrade")
	return errorsmod.Wrap(types.ErrPacketReceived, "packet already processed in a previous channel version")

	// If packet receipts are pruned, it is possible to double spend via a
	// If packet receipts are pruned, it should not be possible to double spend via a

fix: add replay protection on upgraded channels #5651

fix: add replay protection on upgraded channels #5651

Conversation

colin-axner commented Jan 18, 2024 • edited Loading

Description

Commit Message / Changelog Entry

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleenfei left a comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

colin-axner commented Jan 22, 2024

DimitrisJim left a comment • edited Loading

Choose a reason for hiding this comment

colin-axner commented Jan 18, 2024 •

edited

Loading

DimitrisJim left a comment •

edited

Loading