paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.com/
1.89k stars 696 forks source link

Fix border condition in Snowbridge free consensus Updates #5671

Closed claravanstaden closed 1 month ago

claravanstaden commented 2 months ago

Description

A fix for a border condition introduced with new feature https://github.com/paritytech/polkadot-sdk/pull/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: https://github.com/Snowfork/polkadot-sdk/pull/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.

claravanstaden commented 2 months ago

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

claravanstaden commented 2 months ago

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

acatangiu commented 2 months ago

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

claravanstaden commented 1 month ago

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?

claravanstaden commented 1 month ago

@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 commented 1 month ago

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?

claravanstaden commented 1 month ago

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 commented 1 month ago

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

paritytech-cmd-bot-polkadot-sdk[bot] commented 1 month ago

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
paritytech-cmd-bot-polkadot-sdk[bot] commented 1 month ago

Successfully created backport PR for stable2409: