Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix border condition in Snowbridge free consensus Updates #5671

Merged

Conversation

claravanstaden
Copy link
Contributor

Description

A fix for a border condition introduced with new feature #5201. A malicious relayer could spam the Ethereum client with sync committee updates that have already been imported for the period. This PR adds a storage item to track the last imported sync committee period, so that subsequent irrelevant updates are not free.

Original PR: Snowfork#172

Integration

Downstream projects are not affected. Relayers will not be able to spam the Ethereum client with irrelevant sync committee updates for free.

Review Notes

Adds a storage item to track the last free sync committee update period, so that duplicate imports are not free.

* illustrates border condition

* adds test payloads

* cleanup

* update comment

* fmt

* shuffle things around

* a few more test checks

* Update bridges/snowbridge/pallets/ethereum-client/src/lib.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@claravanstaden claravanstaden changed the title Free consensus updates border condition Snowbridge free consensus updates border condition fix Sep 10, 2024
@claravanstaden
Copy link
Contributor Author

@acatangiu may you please add the T15-bridges label. :)

@ggwpez ggwpez added T15-bridges This PR/Issue is related to bridges. A4-needs-backport Pull request must be backported to all maintained releases. labels Sep 10, 2024
@claravanstaden claravanstaden marked this pull request as ready for review September 12, 2024 13:41
@paritytech-review-bot paritytech-review-bot bot requested a review from a team September 12, 2024 13:43
@claravanstaden
Copy link
Contributor Author

@vgeddes @alistair-singh @yrong ready to review. :)

@claravanstaden claravanstaden changed the title Snowbridge free consensus updates border condition fix Fix border condition in Snowbridge free consensus Updates Sep 13, 2024
@acatangiu
Copy link
Contributor

@bkontur @serban300 please review - we also need to backport this fix.

Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add another check after this that updates with a lower number also are not free

@claravanstaden
Copy link
Contributor Author

I'd add another check after this that updates with a lower number also are not free

@franciscoaguirre do you mean another test check or a check in the light client?

…te-fix' into snowbridge-free-consensus-update-fix
@claravanstaden
Copy link
Contributor Author

@franciscoaguirre more test conditions added.

@acatangiu @yrong @vgeddes @alistair-singh I added an interesting test case here. It is an update with a sync committee that has already been imported. The update contains a newer finalized header, so the update is free. However, the sync committee merkle proof is verified, but not stored, because it is already known and stored. So technically the sync committee merkle proof computation is unnecessary, but the update is still free because it contains a new finalized header update. I think it is OK, but just highlighting it is an odd case.

@yrong
Copy link
Contributor

yrong commented Sep 23, 2024

but the update is still free because it contains a new finalized header update

a new finalized header update with slot advanced more than FreeHeadersInterval which can't be spammed, right?

Copy link
Contributor

@serban300 serban300 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as far as I can understand !

bridges/snowbridge/pallets/ethereum-client/src/lib.rs Outdated Show resolved Hide resolved
@claravanstaden
Copy link
Contributor Author

a new finalized header update with slot advanced more than FreeHeadersInterval which can't be spammed, right?

@yrong correct. So it is a valid, free update. The only weird thing is the sync committee will also be verified and prepared but not imported, which is unnecessary.

@acatangiu
Copy link
Contributor

@claravanstaden

error[E0412]: cannot find type `LatestFreeSyncCommitteeUpdatePeriod` in this scope
   --> bridges/snowbridge/pallets/ethereum-client/src/lib.rs:479:6
    |
184 |     pub type LatestSyncCommitteeUpdatePeriod<T: Config> = StorageValue<_, u64, ValueQuery>;
    |     --------------------------------------------------------------------------------------- similarly named type alias `LatestSyncCommitteeUpdatePeriod` defined here
...
479 |                 <LatestFreeSyncCommitteeUpdatePeriod<T>>::set(update_finalized_period);
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: a type alias with a similar name exists: `LatestSyncCommitteeUpdatePeriod`

error[E0433]: failed to resolve: use of undeclared type `LatestFreeSyncCommitteeUpdatePeriod`
   --> bridges/snowbridge/pallets/ethereum-client/src/lib.rs:667:36
    |
667 |             let latest_free_update_period = LatestFreeSyncCommitteeUpdatePeriod::<T>::get();
    |                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                                             |
    |                                             use of undeclared type `LatestFreeSyncCommitteeUpdatePeriod`
    |                                             help: a type alias with a similar name exists: `LatestSyncCommitteeUpdatePeriod`

quick find+replace to rename across the whole repo required.

auto-merge was automatically disabled September 24, 2024 09:48

Head branch was pushed to by a user without write access

@acatangiu acatangiu added this pull request to the merge queue Sep 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 24, 2024
@acatangiu acatangiu added this pull request to the merge queue Sep 24, 2024
Merged via the queue into paritytech:master with commit 1790e42 Sep 24, 2024
206 of 209 checks passed
@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2407:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-5671-to-stable2407
git worktree add --checkout .worktree/backport-5671-to-stable2407 backport-5671-to-stable2407
cd .worktree/backport-5671-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 1790e4272b9fd8832f1ab9e3bf5aaaae9cf5ada7
git push --force-with-lease

github-actions bot pushed a commit that referenced this pull request Sep 24, 2024
# Description

A fix for a border condition introduced with new feature
#5201. A malicious
relayer could spam the Ethereum client with sync committee updates that
have already been imported for the period. This PR adds a storage item
to track the last imported sync committee period, so that subsequent
irrelevant updates are not free.

Original PR: Snowfork#172

## Integration

Downstream projects are not affected. Relayers will not be able to spam
the Ethereum client with irrelevant sync committee updates for free.

## Review Notes

Adds a storage item to track the last free sync committee update period,
so that duplicate imports are not free.

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
(cherry picked from commit 1790e42)
@paritytech-cmd-bot-polkadot-sdk

Successfully created backport PR for stable2409:

@claravanstaden claravanstaden deleted the snowbridge-free-consensus-update-fix branch September 24, 2024 13:02
acatangiu added a commit that referenced this pull request Sep 24, 2024
Backport #5671 into `stable2409` from claravanstaden.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A4-needs-backport Pull request must be backported to all maintained releases. T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants