From 78aa78c3d5b459d302dce737c28db92f7dd34acf Mon Sep 17 00:00:00 2001 From: Clara van Staden Date: Fri, 22 Mar 2024 11:25:28 +0200 Subject: [PATCH] Snowbridge Beacon header age check (#3727) ## Bug Explanation Adds a check that prevents finalized headers with a gap larger than the sync committee period being imported, which could cause execution headers in the gap being unprovable. The current version of the Ethereum client checks that there is a header at least every sync committee, but it doesn't check that the headers are within a sync period of each other. For example: Header 100 (sync committee period 1) Header 9000 (sync committee period 2) (8900 blocks apart) These headers are in adjacent sync committees, but more than the sync committee period (8192 blocks) apart. The reason we need a header every 8192 slots at least, is the header is used to prove messages within the last 8192 blocks. If we import header 9000, and we receive a message to be verified at header 200, the `block_roots` field of header 9000 won't contain the header in order to do the ancestry check. ## Environment While running in Rococo, this edge case was discovered after the relayer was offline for a few days. It is unlikely, but not impossible, to happen again and so it should be backported to polkadot-sdk 1.7.0 (so that [polkadot-fellows/runtimes](https://github.com/polkadot-fellows/runtimes) can be updated with the fix). Our Ethereum client has been operational on Rococo for the past few months, and this been the only major issue discovered so far. ### Unrelated Change An unrelated nit: Removes a left over file that should have been deleted when the `parachain` directory was removed. --------- Co-authored-by: claravanstaden --- .../pallets/ethereum-client/src/lib.rs | 15 +++++ .../pallets/ethereum-client/src/tests.rs | 57 ++++++++++++++++++- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/bridges/snowbridge/parachain/pallets/ethereum-client/src/lib.rs b/bridges/snowbridge/parachain/pallets/ethereum-client/src/lib.rs index a54d4a05ac58..fc2ab2fbb588 100644 --- a/bridges/snowbridge/parachain/pallets/ethereum-client/src/lib.rs +++ b/bridges/snowbridge/parachain/pallets/ethereum-client/src/lib.rs @@ -130,6 +130,10 @@ pub mod pallet { InvalidExecutionHeaderProof, InvalidAncestryMerkleProof, InvalidBlockRootsRootMerkleProof, + /// The gap between the finalized headers is larger than the sync committee period, + /// rendering execution headers unprovable using ancestry proofs (blocks root size is + /// the same as the sync committee period slots). + InvalidFinalizedHeaderGap, HeaderNotFinalized, BlockBodyHashTreeRootFailed, HeaderHashTreeRootFailed, @@ -398,6 +402,17 @@ pub mod pallet { Error::::IrrelevantUpdate ); + // Verify the finalized header gap between the current finalized header and new imported + // header is not larger than the sync committee period, otherwise we cannot do + // ancestry proofs for execution headers in the gap. + ensure!( + latest_finalized_state + .slot + .saturating_add(config::SLOTS_PER_HISTORICAL_ROOT as u64) >= + update.finalized_header.slot, + Error::::InvalidFinalizedHeaderGap + ); + // Verify that the `finality_branch`, if present, confirms `finalized_header` to match // the finalized checkpoint root saved in the state of `attested_header`. let finalized_block_root: H256 = update diff --git a/bridges/snowbridge/parachain/pallets/ethereum-client/src/tests.rs b/bridges/snowbridge/parachain/pallets/ethereum-client/src/tests.rs index 50b6a25c3428..4a7b7b458869 100644 --- a/bridges/snowbridge/parachain/pallets/ethereum-client/src/tests.rs +++ b/bridges/snowbridge/parachain/pallets/ethereum-client/src/tests.rs @@ -15,7 +15,7 @@ use crate::mock::{ pub use crate::mock::*; -use crate::config::{EPOCHS_PER_SYNC_COMMITTEE_PERIOD, SLOTS_PER_EPOCH}; +use crate::config::{EPOCHS_PER_SYNC_COMMITTEE_PERIOD, SLOTS_PER_EPOCH, SLOTS_PER_HISTORICAL_ROOT}; use frame_support::{assert_err, assert_noop, assert_ok}; use hex_literal::hex; use primitives::{ @@ -884,6 +884,61 @@ fn submit_execution_header_not_finalized() { }); } +/// Check that a gap of more than 8192 slots between finalized headers is not allowed. +#[test] +fn submit_finalized_header_update_with_too_large_gap() { + let checkpoint = Box::new(load_checkpoint_update_fixture()); + let update = Box::new(load_sync_committee_update_fixture()); + let mut next_update = Box::new(load_next_sync_committee_update_fixture()); + + // Adds 8193 slots, so that the next update is still in the next sync committee, but the + // gap between the finalized headers is more than 8192 slots. + let slot_with_large_gap = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 1; + + next_update.finalized_header.slot = slot_with_large_gap; + // Adding some slots to the attested header and signature slot since they need to be ahead + // of the finalized header. + next_update.attested_header.slot = slot_with_large_gap + 33; + next_update.signature_slot = slot_with_large_gap + 43; + + new_tester().execute_with(|| { + assert_ok!(EthereumBeaconClient::process_checkpoint_update(&checkpoint)); + assert_ok!(EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone())); + assert!(>::exists()); + assert_err!( + EthereumBeaconClient::submit(RuntimeOrigin::signed(1), next_update.clone()), + Error::::InvalidFinalizedHeaderGap + ); + }); +} + +/// Check that a gap of 8192 slots between finalized headers is allowed. +#[test] +fn submit_finalized_header_update_with_gap_at_limit() { + let checkpoint = Box::new(load_checkpoint_update_fixture()); + let update = Box::new(load_sync_committee_update_fixture()); + let mut next_update = Box::new(load_next_sync_committee_update_fixture()); + + next_update.finalized_header.slot = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64; + // Adding some slots to the attested header and signature slot since they need to be ahead + // of the finalized header. + next_update.attested_header.slot = + checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 33; + next_update.signature_slot = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 43; + + new_tester().execute_with(|| { + assert_ok!(EthereumBeaconClient::process_checkpoint_update(&checkpoint)); + assert_ok!(EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone())); + assert!(>::exists()); + assert_err!( + EthereumBeaconClient::submit(RuntimeOrigin::signed(1), next_update.clone()), + // The test should pass the InvalidFinalizedHeaderGap check, and will fail at the + // next check, the merkle proof, because we changed the next_update slots. + Error::::InvalidHeaderMerkleProof + ); + }); +} + /* IMPLS */ #[test]