6.1 A persisted log cursor already beyond the switch block causes deposits of the new contract to be skipped permanently
The execution service rebuilds the deposit Merkle tree by scanning DepositEvent logs and persists the last scanned execution block in LastRequestedBlock. On every startup, and whenever the node falls behind, the processPastLogs(...) function resumes scanning from that persisted position and only moves forward. The deposit contract switch selects the retired contract for blocks below DEPOSIT_CONTRACT_SWITCH_BLOCK and the current contract at or above it, based solely on the block being scanned.
func (s *Service) processPastLogs(ctx context.Context) error {
// @audit-issue The scan resumes from the persisted cursor regardless of the switch.
currentBlockNum := s.latestEth1Data.LastRequestedBlock
deploymentBlock := params.BeaconNetworkConfig().ContractDeploymentBlock
// ...
currentBlockNum = max(currentBlockNum, deploymentBlock)
// ...
}
A node that first synchronised (fresh database or restored snapshot) with a binary or configuration that did not know about the switch persists a cursor that may already be beyond the switch block, because it kept scanning only the retired contract. When such a node is upgraded and reconfigured, processPastLogs(...) resumes at that cursor and queries the current contract from there on. Any DepositEvent emitted by the current contract between the switch block and the persisted cursor is never requested. The –clear-deposit-contract flag introduced as the migration mechanism does not help, because the clearDepositContractAddress(...) function removes only the stored contract address and leaves LastRequestedBlock, the deposit containers and the tree snapshot untouched.
func (b *BeaconNode) clearDepositContractAddress() error {
// ...
// @audit Only the address key is deleted; the scan cursor and deposit state remain.
if err := b.db.ClearDepositContractAddress(b.ctx); err != nil {
return errors.Wrap(err, "could not clear deposit contract address")
}
// ...
}
Once indices are missing, the next deposit of the current contract fails the sequential index check in ProcessDepositLog(...) ( received incorrect merkle index), the cursor is rolled back and the same range is retried indefinitely. The affected node stops following deposits, votes stale Eth1Data, and cannot build blocks that must include deposits once the chain state adopts them. On the Vana mainnet the cursor is persisted only at a node's first start and every 1000 deposits, so the exposed population is limited to nodes whose first start happened after the switch with the old setup, and to snapshots created from such nodes.
Recommendation
Consider treating the first activation of a switch as an explicit, persisted migration step: when the stored cursor is at or beyond DEPOSIT_CONTRACT_SWITCH_BLOCK and the switch has not been applied before, rewind the cursor to the switch block (or verify that the local tree already contains the current contract's deposits) before resuming. Consider also documenting that snapshots must be regenerated after the switch, and adding a test that starts from a persisted cursor beyond the switch with deposits inside the skipped range.
Update from Nethermind Security
The fix is incomplete: the applied-switch marker is persisted immediately, while the rewound cursor exists only in memory. The cursor is written to disk only by savePowchainData(...), which runs at every 1000th deposit index, at chain start, and in a first-start branch gated on chainStartData.GenesisBlock == 0 that cannot fire again on Vana (its genesis eth1 block is 47). For exactly the population this fix targets, a node whose persisted cursor is above the switch, the following sequence re-opens the hole: the first patched start rewinds in memory, durably records the switch as applied, and rescans the missed deposits into memory only; on any later restart before one of the rare persistence points fires, the stale cursor is restored from disk while the migration is skipped because the marker is already present. The deposits between the switch block and the stale cursor are then missed permanently, since the one-shot protection has consumed itself. We recommend persisting the execution chain data with the rewound cursor in the same step that records the applied switch, or recording the marker only once the cursor has durably passed the switch block again. Additionally, we recommend that applyDepositContractSwitchMigration(...) refuse to start when a recorded switch differs from the configured one ( applied != switchBlock) instead of migrating again. A database that already scanned under one switch block cannot in general be made consistent with a different one by rewinding: blocks between the two values keep their old contract attribution, and current contract deposits between a raised switch and its predecessor pass the deposit-height refusal unnoticed. Since a legitimate second contract switch is not representable in the current configuration (a single retired address and switch block), any mismatch indicates a misconfiguration or a deliberate correction, and both warrant an explicit stop with instructions rather than a silent re-migration. Such a check would also need an escape hatch, for example extending –clear-deposit-contract to clear the recorded switch as well.
6.2 Deposit logs in the switch block are permanently skipped when a historical scan ends at the switch block
The beacon node builds its deposit trie by scanning the execution chain for DepositEvent logs emitted by the deposit contract. Because the chain replaced its deposit contract at a configured switch block, exactly one contract is authoritative for any given block: the depositContractForBlock(...) function from the Service contract returns the retired contract for blocks below depositContractSwitchBlock and the current contract at or above it, together with the first block beyond that contract's range (the boundary).
Catching up on history is done by the processPastLogs(...) function, which walks forward in batches by calling the processBlockInBatch(...) function. Each batch reports back the block it got to, and that number is used as a bookmark meaning *"everything up to here has been scanned"*. When the walk is over, the bookmark is saved as LastRequestedBlock:
for currentBlockNum < latestFollowHeight {
currentBlockNum, batchSize, err = s.processBlockInBatch(ctx, currentBlockNum,
latestFollowHeight, batchSize, additiveFactor, logCount, headersMap)
if err != nil {
return err
}
}
s.latestEth1DataLock.Lock()
// @audit-issue The bookmark the loop ends on is saved as the resume point, and no code
// path ever goes back below it.
s.latestEth1Data.LastRequestedBlock = currentBlockNum
s.latestEth1DataLock.Unlock()
Normally the bookmark is honest, because a batch reports end, the last block it actually asked the execution client about. The code path added for the contract switch is the exception, because two rules meet there:
- a batch must never cross the switch block, since the contract to query differs on either side of it, so
end is trimmed down to switchBlock - 1; - a batch must always move forward, otherwise the loop would repeat the same batch forever;
When a batch happens to start exactly at switchBlock - 1, the two rules collide: trimming leaves end equal to start, so nothing moved. The code resolves this by reporting switchBlock instead — a block it never asked about:
addr, boundary := s.depositContractForBlock(start)
clampedToBoundary := boundary != 0 && end >= boundary
if clampedToBoundary {
// @audit The batch is trimmed to the last block below the switch.
end = boundary - 1
}
// ...
currentBlockNum = end
// @audit-issue The bookmark is moved to the switch block, which this batch never
// queried against any contract.
if clampedToBoundary && currentBlockNum <= start {
currentBlockNum = boundary
}
Most of the time that inaccurate bookmark is harmless. The walk continues, the next batch starts at switchBlock, and the block does get scanned after all. It turns into a real hole only when the walk stops right at that point, which is the case when the follow height is exactly switchBlock: every batch is then trimmed to switchBlock - 1, so this forward jump is the only thing that can end the loop. It also does not depend on an unlucky batch size. With the follow height sitting on the switch, the trimming walks the position to switchBlock - 1, and the batch after that necessarily starts there and takes the jump.
The loop then ends, switchBlock is saved as if it had been scanned, and normal block-by-block processing picks up at the block after the bookmark. Nothing ever looks at the switch block:
// @audit-issue Normal processing resumes one block above the saved bookmark, so the
// switch block is never scanned.
for i := s.latestEth1Data.LastRequestedBlock + 1; i <= requestedBlock; i++ {
// ...
err = s.ProcessETH1Block(ctx, new(big.Int).SetUint64(i))
// ...
}
This situation is not hypothetical, because the follow height is not a fixed number. The followedBlockHeight(...) function derives it from block timestamps, so it climbs one block at a time and therefore passes through switchBlock, sitting there for about one block's worth of time, roughly one follow distance after the chain produced the switch block. Anything that starts a historical walk during that short window, from a position below the switch, hits the bug. Two things start such a walk: node startup, and the catch-up path taken by the requestBatchedHeadersAndLogs(...) function once the node has fallen more than maxTolerableDifference blocks behind. A node that restarts or catches up while the switch block is that fresh, for example during a restart or upgrade around the switch lands in this window.
If the switch block happens to contain a DepositEvent, that deposit is dropped without a trace: no error is returned and no warning is logged. The hole can still close by itself, but only while the bookmark still reads exactly switchBlock, because a restart or a catch-up at that point rescans starting from the bookmark, switch block included. Once normal processing has moved on to switchBlock + 1, the bookmark is past the hole and it can never be filled.
Deposits carry consecutive Merkle indices, and the node checks that each one follows the previous. The next deposit therefore arrives with an index one too high and is rejected by the ProcessDepositLog(...) function with received incorrect merkle index if one deposit is missed. That rejection also stops the bookmark from advancing, so the node keeps retrying the same block: deposits stop being ingested for good, the execution service stays in a permanent error state, and the only way out is to resync the deposit history.
The existing switch tests do not cover this, as they all use follow heights well above the switch block ( 200, 300 and 500 against a switch block of 100), so the walk never has to end on the forward jump.
Recommendation
In processPastLogs, after the loop and before storing LastRequestedBlock, adjust the resume point back when it equals the switch block:
if b := s.cfg.depositContractSwitchBlock; b != 0 && currentBlockNum == b {
currentBlockNum = b - 1
}
At loop exit the value can equal switchBlock via the boundary jump, so the adjustment persists a block that was scanned, and the per-block path then queries the switch block with the new contract's address.
Consider also extending the test suite with a case where the follow height equals the switch block, asserting that every block at or below the saved resume point was queried, so that this exit path is covered.
Resolution
Good point! It has been fixed in the commit 275b9ae. The idea of the fix is that we keep the query range (start, end) and determine the corresponding deposit contract address in the range if the switch block is in the range.
6.3 The deposit contract switch validation runs before the –contractdeploymentblock override is applied
During startup the beacon node applies its CLI overrides in sequence. The configureEth1Config(...) function applies the deposit contract flags and ends by calling validateDepositContractSwitch(...), which rejects a switch block that is not above the contract deployment block, because the historical scan starts at max(cursor, ContractDeploymentBlock) and a switch block at or below it would mean the retired contract is never scanned.
if err := configureEth1Config(cliCtx); err != nil {
return errors.Wrap(err, "could not configure eth1 config")
}
// @audit-issue The deployment block flag is applied only after the validation above ran.
configureNetwork(cliCtx)
The –contract-deployment-block flag is applied later, inside configureNetwork(...), so the validation compares DEPOSIT_CONTRACT_SWITCH_BLOCK against the pre-override deployment block.
if deployment := params.BeaconNetworkConfig().ContractDeploymentBlock; switchBlock <= deployment {
// @audit `deployment` is the value before the CLI override is applied.
return fmt.Errorf(
"deposit contract switch block %d must be above the contract deployment block %d, ...",
switchBlock, deployment)
}
As a result a valid configuration can be rejected when the default deployment block is higher than the effective one, and an invalid configuration is accepted when the flag raises the deployment block to or above the switch block; in the latter case the node silently starts scanning at the raised deployment block and never reads the retired contract's history. The unit test TestValidateDepositContractSwitch sets the network config directly, so it does not exercise the real startup order. With Vana's current invocation ( –chain-config-file and –contract-deployment-block=0) both values are 0 and the issue has no effect, but the check does not protect against the configuration it was written for.
Recommendation
Consider applying configureNetwork(...) before the switch validation, or moving the validation to a final step that runs after every chain and network override has been applied, and adding a startup-level test that supplies all three flags together.
6.4 New source files are not registered in the Bazel build
Prysm builds its release binaries and container images with Bazel, and every Go package lists its source files explicitly in a BUILD.bazel file. The change adds deposit_contract_switch.go and deposit_contract_switch_test.go to the beacon-chain/execution package but does not add them to the srcs lists of the go_library and go_test targets. The go build and go test commands succeed because they discover files automatically, but the Bazel build of the execution package fails, since log_processing.go references the depositContractForBlock(...) method that Bazel never compiles.
bazel build //beacon-chain/execution:go_default_library
beacon-chain/execution/log_processing.go:63:15: s.depositContractForBlock undefined
beacon-chain/execution/log_processing.go:104:23: s.depositContractForBlock undefined
beacon-chain/execution/log_processing.go:403:22: s.depositContractForBlock undefined
The dependent //beacon-chain/node targets fail as well, and the new switch tests are not part of any Bazel test target. The repository's GitHub workflows run go test, so this is not caught by CI.
Recommendation
Consider regenerating the Bazel metadata (for example with Gazelle) so that both files appear in their respective srcs lists, and consider running the affected Bazel targets in CI so that a passing go test cannot mask missing build metadata.
6.5 The ConfigToYaml(...) function drops the deposit contract switch fields
The change adds RETIRED_DEPOSIT_CONTRACT_ADDRESS and DEPOSIT_CONTRACT_SWITCH_BLOCK to BeaconChainConfig and teaches UnmarshalConfig(...) to parse them. The ConfigToYaml(...) function, which serialises an active configuration back to YAML and is used by the end-to-end test components to write the config file handed to test nodes, was not updated and still emits only DEPOSIT_CONTRACT_ADDRESS.
lines := []string{
// ...
// @audit-issue The two switch keys are not emitted.
fmt.Sprintf("DEPOSIT_CONTRACT_ADDRESS: %s", cfg.DepositContractAddress),
// ...
}
Any configuration that passes through this function silently loses the switch, so an end-to-end test that configured a switch would exercise the no-switch path without failing. The config comparison helpers used in the params tests also do not compare the two new fields, so the omission is not detected.
Recommendation
Consider emitting both fields in ConfigToYaml(...), comparing them in the config equality helpers, and adding a round-trip test starting from non-default switch values.
6.6 The deposit count is always read from the current contract, even while the chain is below the switch block
The deposit contract switch makes log queries contract-aware: blocks below DEPOSIT_CONTRACT_SWITCH_BLOCK are queried against the retired contract and the rest against the current one. The historical scan in processPastLogs(...) additionally reads the total deposit count with an eth_call to size its batches. That call is not switch-aware: depositContractCaller is bound once to the current contract address in setupExecutionClientConnections(...), and processPastLogs(...) calls it unconditionally at every startup and whenever the node falls more than 50 blocks behind.
// @audit-issue Called at the latest block against the current contract, regardless of the switch.
rawLogCount, err := s.depositContractCaller.GetDepositCount(&bind.CallOpts{})
if err != nil {
return err
}
If the switch is configured before the current contract is deployed, or if a node points at a wrong address, the call returns no data, processPastLogs(...) fails, and initPOWService(...) retries indefinitely with the generic message "Unable to process past deposit contract logs, perhaps your execution client is not fully synced". The execution service never finishes initialising, so the node produces no Eth1Data votes and cannot include deposits. This is an operational ordering constraint (deploy the contract first, then roll out the configuration) that the code does not express.
Recommendation
Consider making the count lookup consistent with the switch, for example by reading it from the contract that is authoritative for the scanned range, or by tolerating a missing current contract while the followed block height is below the switch block, and consider emitting a specific error message when the call fails so operators can distinguish this case from an unsynced execution client.
Resolution
The deposit count should always be read from the current deposit contract. A more specific error message was added in the commit ee47e3c
