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

Sync protocol implemented incorrectly when deciding if the update contains the next sync committee #370

Closed
4 tasks
srdtrk opened this issue Mar 7, 2025 · 0 comments · Fixed by #406
Closed
4 tasks
Labels
audit bug Something isn't working ethereum-light-client Issues related to the 08-wasm ethereum light client

Comments

@srdtrk
Copy link
Member

srdtrk commented Mar 7, 2025

Summary of Bug

Internal audit item: The sync-protocol implementation when determining whether an update contains a next sync committee is incorrect.

Expected Behaviour

Here is our code snippet:

if let (Some(next_sync_committee), Some(stored_next_sync_committee)) = (
&update.next_sync_committee,
trusted_consensus_state.next_sync_committee(),
) {
if update_attested_period == stored_period {
ensure!(
next_sync_committee == stored_next_sync_committee,
EthereumIBCError::NextSyncCommitteeMismatch {
expected: stored_next_sync_committee.aggregate_pubkey,
found: next_sync_committee.aggregate_pubkey,
}
);
}
// This validates the given next sync committee against the attested header's state root.
validate_merkle_branch(
next_sync_committee.tree_hash_root(),
update.next_sync_committee_branch.unwrap_or_default().into(),
NEXT_SYNC_COMMITTEE_BRANCH_DEPTH,
get_subtree_index(NEXT_SYNC_COMMITTEE_INDEX),
update.attested_header.beacon.state_root,
)
.map_err(|e| EthereumIBCError::ValidateNextSyncCommitteeFailed(Box::new(e)))?;
}

And here is the sync protocol specs:

https://github.com/ethereum/consensus-specs/blob/36d80adb44c21c66379c6207a9578f9b1dcc8a2d/specs/altair/light-client/sync-protocol.md?plain=1#L419-L429

Our implementation misses the case where:

  • the update contains a next_sync_committee
  • the next_sync_committee in our store is unknown

Version

main


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
  • Estimate provided
@srdtrk srdtrk added bug Something isn't working ethereum-light-client Issues related to the 08-wasm ethereum light client labels Mar 7, 2025
@srdtrk srdtrk changed the title Follow sync protocol specs when deciding if the update contains the next sync committee Sync protocol implemented incorrectly when deciding if the update contains the next sync committee Mar 7, 2025
@srdtrk srdtrk added the audit label Mar 7, 2025
@github-project-automation github-project-automation bot moved this to Backlog in IBC Mar 8, 2025
@github-project-automation github-project-automation bot moved this from Backlog to Done in IBC Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit bug Something isn't working ethereum-light-client Issues related to the 08-wasm ethereum light client
Projects
Status: Done
1 participant