diff --git a/packages/state-transition/src/lightClient/spec/processLightClientUpdate.ts b/packages/state-transition/src/lightClient/spec/processLightClientUpdate.ts index 2d127689bbed..a0fb6b27e4ab 100644 --- a/packages/state-transition/src/lightClient/spec/processLightClientUpdate.ts +++ b/packages/state-transition/src/lightClient/spec/processLightClientUpdate.ts @@ -5,7 +5,13 @@ import {pruneSetToMax} from "@lodestar/utils"; import {computeSyncPeriodAtSlot} from "../../util/epoch.js"; import {LightClientUpdateSummary, isBetterUpdate, toLightClientUpdateSummary} from "./isBetterUpdate.js"; import {type ILightClientStore, MAX_SYNC_PERIODS_CACHE, type SyncCommitteeFast} from "./store.js"; -import {deserializeSyncCommittee, getSafetyThreshold, isSyncCommitteeUpdate, sumBits} from "./utils.js"; +import { + deserializeSyncCommittee, + getSafetyThreshold, + isFinalityUpdate, + isSyncCommitteeUpdate, + sumBits, +} from "./utils.js"; import {validateLightClientUpdate} from "./validateLightClientUpdate.js"; export interface ProcessUpdateOpts { @@ -44,7 +50,13 @@ export function processLightClientUpdate( } // Update finalized header + // Defense-in-depth: require isFinalityUpdate(update) so a non-finality update can never + // overwrite store.finalizedHeader, regardless of any future loosening of the zeroed-header + // checks in validateLightClientUpdate. Spec-faithful behavior also holds without this guard + // once update.finalizedHeader is guaranteed fully-zero in the non-finality case, but the + // guard makes the invariant local to this write site. if ( + isFinalityUpdate(update) && syncCommitteeTrueBits * 3 >= SYNC_COMMITTEE_SIZE * 2 && update.finalizedHeader.beacon.slot > store.finalizedHeader.beacon.slot ) { diff --git a/packages/state-transition/src/lightClient/spec/utils.ts b/packages/state-transition/src/lightClient/spec/utils.ts index a571614e2e2e..ff97b1dd9901 100644 --- a/packages/state-transition/src/lightClient/spec/utils.ts +++ b/packages/state-transition/src/lightClient/spec/utils.ts @@ -29,7 +29,6 @@ import type {LightClientStore, SyncCommitteeFast} from "./store.js"; export const GENESIS_SLOT = 0; export const ZERO_HASH = new Uint8Array(32); -export const ZERO_PUBKEY = new Uint8Array(48); export const ZERO_SYNC_COMMITTEE = ssz.altair.SyncCommittee.defaultValue(); export const ZERO_HEADER = ssz.phase0.BeaconBlockHeader.defaultValue(); /** From https://notes.ethereum.org/@vbuterin/extended_light_client_protocol#Optimistic-head-determining-function */ @@ -106,13 +105,16 @@ export function isFinalityUpdate(update: LightClientUpdate): boolean { } export function isZeroedHeader(header: BeaconBlockHeader): boolean { - // Fast return for when constructing full LightClientUpdate from partial updates - return header === ZERO_HEADER || byteArrayEquals(header.bodyRoot, ZERO_HASH); + // Spec requires the whole header to equal LightClientHeader() in the non-finality case + // (see altair/light-client/sync-protocol.md `process_light_client_update`). Checking only + // bodyRoot would let an attacker smuggle arbitrary slot/proposerIndex/parentRoot/stateRoot + // through the non-finality branch and overwrite store.finalizedHeader. + return header === ZERO_HEADER || ssz.phase0.BeaconBlockHeader.equals(header, ZERO_HEADER); } export function isZeroedSyncCommittee(syncCommittee: SyncCommittee): boolean { - // Fast return for when constructing full LightClientUpdate from partial updates - return syncCommittee === ZERO_SYNC_COMMITTEE || byteArrayEquals(syncCommittee.pubkeys[0], ZERO_PUBKEY); + // Spec requires the whole SyncCommittee to equal SyncCommittee() in the non-sync-committee-update case. + return syncCommittee === ZERO_SYNC_COMMITTEE || ssz.altair.SyncCommittee.equals(syncCommittee, ZERO_SYNC_COMMITTEE); } export function isValidMerkleBranch(