Lido

ZK Oracle

6.1 The cl_balance manipulation and oracle DOS through incorrect validator indices in lido_changed

Classification: Under-constrained.

The new validators in the all_added may be included also in the list of changed validators lido_changed. This could be exploited to artificially increase the reported sum of all validator balances in cl_balance. The validators in all_added represent all the new validators that were registered between the previous and current state. All validators in the lido_changed list are ensured to be in the current beacon state:

self.verify_validator_inclusion_proof(
    &format!("{vals_and_bals_prefix}.validator_inclusion_proofs.lido_changed"),
    total_validators,
    &beacon_state.validators,
    &input.validators_and_balances.validators_delta.lido_changed,
    proof,
)?;

However, there is no check to ensure that the lido_changed are the validators from the old state. This allows for including the validators from all_added also into lido_changed. The function merge_validator_delta(...) checks validators' status and adds them to the pending, deposited, or exited lists. From the exploit perspective, we are only interested in creating duplicates in deposited since those are used to calculate cl_balance. This can be achieved by adding from all_added to lido_changed only those validators that currently have "Deposited" status and were not registered in the previous state. The status is determined by the status(...) function:

fn status(&self, epoch: Epoch) -> ValidatorStatus {
    if epoch >= self.exit_epoch {
        ValidatorStatus::Exited
    } else if epoch >= self.activation_eligibility_epoch {
        ValidatorStatus::Deposited
    } else {
        ValidatorStatus::FutureDeposit
    }
}

The merge_validator_delta(...) first iterates over the all_added and gives our targeted validators status of Deposited and adds them to new_deposited:

for validator_with_index in &validator_delta.all_added {
    let validator = &validator_with_index.validator;
    if !validator.is_lido(lido_withdrawal_credentials) {
        continue;
    }
    match validator.status(epoch) {
        // @audit: our targeted validators would be added here
        ValidatorStatus::Deposited => new_deposited.push(validator_with_index.index),
        ValidatorStatus::FutureDeposit => {
            new_pending_deposit.insert(validator_with_index.index);
        }
        ValidatorStatus::Exited => {
            new_deposited.push(validator_with_index.index);
            new_exited.push(validator_with_index.index);
        }
    }
}

Then, the validators in the lido_changed are distributed by comparing the status at the old epoch and new epoch:

for validator_with_index in &validator_delta.lido_changed {
            let validator = &validator_with_index.validator;
            // It is expected that the caller will filter out non-Lido validators, but worth double-checking
            if !validator.is_lido(lido_withdrawal_credentials) {
                return Err(Error::NonLidoValidatorInDelta {
                    index: validator_with_index.index,
                    withdrawal_credentials: validator.withdrawal_credentials,
                });
            }

            let old_status = validator.status(self.epoch);
            let new_status = validator.status(epoch);

            match (&old_status, &new_status) {
                // @audit: our targeted validators did not exist in the old state, so its old status is determined as FutureDeposit.
                //               They currently have Deposited status, so those validators would be match this case
                (ValidatorStatus::FutureDeposit, ValidatorStatus::Deposited) => {
                    new_deposited.push(validator_with_index.index);
                    new_pending_deposit.remove(&validator_with_index.index);
                }
                ...
            }
        }

Our targeted validators in the current epoch have Deposited status and didn't exist in the old status. The function merge_validator_delta(...) assumes that the existence of the validators was checked before, so it treats non-existing validators as FutureDeposit. Our targeted validators would match the case: (ValidatorStatus::FutureDeposit, ValidatorStatus::Deposited). In this case, the validator is added to the new_deposited and is removed from new_pending_deposit, which contains all pending validators from the old state. Our targeted validators did not exist in the old state, so they won't be in new_pending_deposit. Removal from the HashSet results in removing the element if it exists and returning a boolean value that informs whether the element is present. The operation new_pending_deposit.remove(&validator_with_index.index) doesn't check the return boolean value, which for our targeted validator would be false. The deposited_lido_validator_indices would contain the duplicated values. The compute_from_state(...) function would iterate over duplicated validators and artificially increase the sum of balances stored in cl_balance.

pub fn compute_from_state(...) -> Self {
    let mut cl_balance: u64 = 0;

    let deposited_indices = &lido_validators_state.deposited_lido_validator_indices;
    // @audit: through duplicated validator indices, the sum of balances is artificially increased
    for index in deposited_indices {
        cl_balance += balances[u64_to_usize(*index)];
    }
   ...
}

Another similar scenario can happen with a validator that is both in all_added and lido_changed, but it currently has an Exited status. In the lido_changed loop, it would match the case (ValidatorStatus::FutureDeposit, ValidatorStatus::Exited). The effects would be similar to those described above, with an additional presence in the new_exited. This scenario, however, is less likely since the validator can be exited 9 days from being activated while the oracle should be run daily. Additionally, the duplicated balance would exist only around 1 day while it's in the exit queue waiting for withdrawal. After withdrawal, the balance duplication is not harmful since it's zero.

Apart from the artificially increased sum of balance, in both scenarios, the Lido state hash that is registered on the chain would be incorrect. This would cause the DOS since the next fair oracle would not be able to publish correct data because the old state hash from the Sp1LidoAccountingReportContract would not match. Submitting the old state that would match the on-chain state would be impossible since the deposited list contains duplicates for which the circuit throws an error. Therefore, the oracle is completely blocked without the possibility of continuation.

Recommendation

Consider ensuring that the validators in the lido_changed are not new validators but were registered in the old state. This could be done by adding a check that the last index in lido_changed is less or equal to the max_validator_index. Additionally, consider checking that the boolean value of new_pending_deposit.remove(...) is equal to true to ensure that it exists before removing it.

Resolution

Criticality of this issue could be discussed. According to LIP-23, the use case of ZK Oracle is only for the negative rebase. This means that increasing CL Balance during attack is useless to confirm the negative rebase. However, in general this issue is valid and fixed. Fixed in PR #12.

Update from Nethermind Security

After discussing with the Lido team to clarify the severity classification, they acknowledged its relevance in the context of zkOracle as a standalone project. Thus, the severity has been retained.

6.2 Arbitrary old state can be used

Classification: Under-constrained.

The report publication includes Report and ReportMetadata. Part of the ReportMetadata is the old_state which is the previous state to which the changes are compared. Expected scenario is that the old_state is the root hash of the state from the last published report.

However, there are no checks to ensure that. Moreover, the reports can be published in the past, or already published reports can be republished. This allows for many scenarios that would cause incorrect state on-chain that are confusing and not expected:

• the old state can be the future state: let's assume that there are reports published at x and x+2 slots. The report can be published at the x+1 slot with old_state pointing to the x+2. This is incorrect since the future state can't be an old state. Note that for this to happen between x+1 and x+2 states, there can't be any changes in new validators;

• the old state can be current state: let's assume that there is report published at x slot with old state at x-1. The report can be republished at x with old state at x. The old state should not be the current state since that does not prove the correctness of provided data;

The old state should be based on which the oracle verifies the correctness of changes and shows the history of state change. In both scenarios, the old state does not provide any information. The second scenario is the most dangerous since every report can be republished this way and show old_state as its own state. Note that this does not allow for publishing incorrect data, however the on-chain state reports could not be trusted since they would show that the old state is equal to the current state, which does not prove correct verified change.

Recommendation

Consider adding a requirement in the circuit program that the new state epoch is greater than the old state epoch.

Resolution

Fixed by adding check to the contract. PR #14

6.3 No checks on new state

Classification: Under-constrained.

The proof is to ensure that the new state is correctly constructed (from the old state). Currently this would potentially allow a new invalid state with duplicates to have a proof constructed.

Recommendation

Check that the new state is correctly computed by performing the sorted and unique checks on the new state and also checking the hash tree root of the new state is equivalent to the one passed in.

Specifically, the new state must be verified. In main.rs, after computing the new lido state we propose to perform the following check:

if !Self::is_sorted_and_unique(&mut new_state.deposited_lido_validator_indices.iter()) {
      return Err(Error::ConditionCheck(
           ConditionCheckFailure::DepositedValidatorsNotSorted, 
      ));
}
// no need to check pending since it is a hash set which would not allow duplicates and also gets sorted once it is converted into a list.

Finally, the code must check that the computed new state root equals the input new state root. This is correctly performed in the code.

Resolution

Accepted and fixed. Fixed at PR #17.

6.4 Inefficient checks for old lido state

To verify the validity of the old lido state passed as the private input the following checks are performed in input_verification.rs:

1. Check correct epoch value;
2. Check if the deposited validator indices are unique and sorted;
3. Check if the pending deposited validator indices are unique and sorted;

This is performed as follows in the code:

pub fn prove_input(&self, input: &ProgramInput) -> Result<(), Error> {
        ...
        self.cycle_tracker.start_span("prove_input.old_state");
                let epoch_from_slot = input.old_lido_validator_state.slot.epoch();
        let actual_epoch = input.old_lido_validator_state.epoch;
        if actual_epoch != epoch_from_slot {
            return Err(Error::ConditionCheck(ConditionCheckFailure::OldStateEpochMismatch {
                epoch: actual_epoch,
                epoch_from_slot,
            }));
        }
        self.cycle_tracker.start_span("prove_input.old_state.deposited.sorted");
        if !Self::is_sorted_and_unique(&mut input.old_lido_validator_state.deposited_lido_validator_indices.iter()) {
            return Err(Error::ConditionCheck(
                ConditionCheckFailure::DepositedValidatorsNotSorted, 
            ));
        }
        self.cycle_tracker.end_span("prove_input.old_state.deposited.sorted");

        self.cycle_tracker.start_span("prove_input.old_state.pending.sorted");
        if !Self::is_sorted_and_unique(
            &mut input
                .old_lido_validator_state
                .pending_deposit_lido_validator_indices
                .iter(),
        ) {
            return Err(Error::ConditionCheck(
                ConditionCheckFailure::PendingDepositValidatorsNotSorted,
            ));
        }
        self.cycle_tracker.end_span("prove_input.old_state.pending.sorted");
        self.cycle_tracker.end_span("prove_input.old_state");
        ...
}

This requires looping through all the indices of two lists and can be expensive depending on the size of lists. Since the old state is trusted these checks do not need to occur for this value.

Recommendation

Since we trust the old state is correctly computed, these checks are redundant. We recommend to pass the old state root as an input in order to replace the checks described above for simplicity and efficiency. We can pass the old state hash in the inputs and do the check they are equal, similarly to the new state hash, and then finally add in the report the calculated value. Specifically, these checks above can be avoided by adding as an additional value in the ProgramInput struct which is the old lido state's hash tree root:

#[derive(PartialEq, Debug, Clone, Serialize, Deserialize)]
pub struct ProgramInput {
    ...
    pub validators_and_balances: ValsAndBals,
    pub old_lido_validator_state: LidoValidatorState,
    pub old_lido_validator_state_root: Hash256, // added this
    pub new_lido_validator_state_hash: Hash256,
    pub withdrawal_vault_data: WithdrawalVaultData,
}

Remove the above checks in input_verification.rs, and in main.rs add the following check:

input.old_lido_validator_state.tree_hash_root() == input.old_lido_validator_state_root

Why this works? As we trust the first hash of the lido state we can thus trust the previous root is correct. Additionally, merkelization results in a different root if validator indices are in incorrect order and/or are not unique so this implicitly checks the sorted and uniqueness of the indices. This provides stronger guarantees of correctness.

Resolution

This optimizes the amount of data and checks to be done in the proof - at a cost of capturing issues much later in the process (at contract). Since this is an info, we would like to keep the checks in place for (1) added security, (2) added convenience in testing (tests failing proof don't need to hit the prover network).

6.5 TimestampOutOfRange error reports parameters in the wrong order

The TimestampOutOfRange error is declared as:

error TimestampOutOfRange(uint256 target_slot, uint256 target_timestamp, uint256 earliest_available_timestamp);

However, in the _findBeaconBlockHash function, the earliest and target timestamps are swapped. This can:

function _findBeaconBlockHash(uint256 slot) internal view virtual returns (bytes32) {
    uint256 targetBlockTimestamp = _slotToTimestamp(slot + 1);

    uint256 earliestBlockTimestamp = block.timestamp - (BEACON_ROOTS_HISTORY_BUFFER_LENGTH * SECONDS_PER_SLOT);
    if (targetBlockTimestamp <= earliestBlockTimestamp) {
        // @audit the timestamps order are mismatched
        revert TimestampOutOfRange(slot, earliestBlockTimestamp, targetBlockTimestamp);
    }
    ...
}

This could produce error data that leads to misinterpretation of error messages.

Recommendation

Consider applying the following change:

-revert TimestampOutOfRange(slot, earliestBlockTimestamp, targetBlockTimestamp);
+revert TimestampOutOfRange(slot, targetBlockTimestamp, earliestBlockTimestamp);

Resolution

Fixed a61db08

6.6 Redundant merkle proof check

There is a merkle multiproof check that verifies that the validator hash and the balances hash from the inputted beacon state. This can be seen below:

let vals_and_bals_multiproof_leaves = [beacon_state.validators, beacon_state.balances];
beacon_state
    .verify_serialized(
        &input.validators_and_balances.validators_and_balances_proof,
        &[BeaconStateFields::validators, BeaconStateFields::balances],
        &vals_and_bals_multiproof_leaves,
    )
    .map_err(|e| Error::MerkleProofError {
        operation: "Verify Inclusion Proof for validators and balances",
        error: e,
    })?;

As can be seen in the code, the balances and validators hashes are already taken from the beacon state itself which makes proving their inclusion in their own struct redundant.

Recommendation

We recommend to remove this proof from the inputs and not perform this check.

Why can we remove this check since we need to prove the validators and balances changed and added are included in the beacon state? This will not break the chain of proofs leading from validators/balances to beacon root since those hashes in the beacon state are used to verify the inclusion of the validators and balances in question and since the beacon header and root are checked against the public beacon root. This means that we know the validators are included in the validators list which is included in the beacon state which is included in the beacon root (equivalently for the balances).

Existing check for the added validators inclusion proof that proves validators ∈ validator list:

let proof =
    merkle_proof::serde::deserialize_proof(&input.validators_and_balances.added_validators_inclusion_proof)
        .map_err(|err| Error::MerkleProofError {
            operation: "Deserialize all_added Inclusion Proof",
            error: err,
        })?;
self.verify_validator_inclusion_proof(
    &format!("{vals_and_bals_prefix}.validator_inclusion_proofs.all_added"),
    total_validators,
    &beacon_state.validators,
    &input.validators_and_balances.validators_delta.all_added,
    proof,

Existing check that beacon state ∈ beacon root:

let beacon_block_header = &input.beacon_block_header;
let beacon_state = &input.beacon_state;

// Beacon Block root == merkle_tree_root(BeaconBlockHeader)
self.cycle_tracker.start_span("prove_input.beacon_block_header");
let bh_root = beacon_block_header.tree_hash_root();
if bh_root != input.beacon_block_hash {
    return Err(Error::ConditionCheck(ConditionCheckFailure::BeaconBlockHashMismatch {
        actual: bh_root,
        expected: input.beacon_block_hash,
    }));
}
self.cycle_tracker.end_span("prove_input.beacon_block_header");

// merkle_tree_root(BeaconState) is included into BeaconBlockHeader
self.cycle_tracker.start_span("prove_input.beacon_state");
let bs_root = beacon_state.tree_hash_root();
if bs_root != beacon_block_header.state_root {
    return Err(Error::ConditionCheck(ConditionCheckFailure::BeaconStateHashMismatch {
        actual: bs_root,
        expected: beacon_block_header.state_root,
    }));
}
self.cycle_tracker.end_span("prove_input.beacon_state");

On the other hand, should this check be kept we recommend to add the latest_execution_payload_header to the values proven for completeness since it is also used later in the circuit.

Resolution

Fixed. PR #17

6.7 Unnecessary data in lido_changed

The data preparation script in validator_delta.rs::compute_changed(...) gathers the validators to lido_changed. All the pending validators from the old state are added. Then, it iterates over the deposited validators in the old state and checks if the activation_epoch has changed:

for index in &self.old_state.deposited_lido_validator_indices {
    // for already deposited validators, we want to check if something material has changed:
    // this can only be an activation epoch or exist epoch. Theoretically "slashed" can also be
    // relevant, but for now we have no use for it
    let index_usize = u64_to_usize(*index);
    let old_validator = &self.old_bs.validators[index_usize];
    let new_validator = &self.new_bs.validators[index_usize];

    if !self.skip_verification && old_validator.pubkey != new_validator.pubkey {
        return Err(Error::ValidatorPubkeyMismatch {
            index: *index,
            old: old_validator.pubkey.clone(),
            new: new_validator.pubkey.clone(),
        });
    }
    if check_epoch_based_change(
        old_bs_epoch,
        new_bs_epoch,
        old_validator.exit_epoch,
        new_validator.exit_epoch,
    ) {
        lido_changed_indices.insert(*index);
    }
    if check_epoch_based_change(
        old_bs_epoch,
        new_bs_epoch,
        old_validator.activation_epoch,
        new_validator.activation_epoch,
    ) {
        lido_changed_indices.insert(*index);
    }
}

In case the activation_epoch is changed for a validator, then it is included in the lido_changed. The circuit handles lido_changed in the lido.rs::merge_validator_delta(...). In the presented scenario, our validator previously had Deposited since it was in the deposited validators. The merge_validator_delta(...) would check its status, and the new status would also be Deposited. It would match the (ValidatorStatus::Deposited, ValidatorStatus::Deposited) case, which would result in no operation.

Recommendation

Consider not including the validators that had Deposited status in the old state to the lido_changed if their activation epoch changed. This would save computation and proving time.

Resolution

Fixed. PR #17

6.8 Update crates/program rust-toolchain

Originally, the channel had to be 1.82 because of SP1. As specified in the code below:

> ==== cargo edition and rustc compatibility ====
>  These dependencies are pinned to a concrete version as a compatibility mechanism between new version of the crates
>  (moving on oo edition=2024 and rust-version=1.85) and sp1 compiler that's based on rustc=1.82
>  This can be unpinned when sp1 rustc moves to at least 1.85
>  1.2.0 requires rust >= 1.85, sp1 uses 1.82-dev
>  Note: there are overrides in script and service Cargo.toml in dev-dependencies - needs to be updated there as well

sp1 rustc has moved to at least 1.85, so the value can be updated to maintain consistency, reduce maintenance overheads and access the bug fixes included in 1.85

Recommendation

Change channel = "1.82" to channel = "1.85".

Resolution

Fixed. PR #11

6.9 Unchecked integer overflows

Some arithmetic operations in the codebase are performed without explicit overflow checks. Since SP1 operates over release mode, overflow behavior does not panic no immediate security vulnerability has been identified, as overflows would require unrealistically large inputs. However, the absence of explicit checks could lead to unexpected behavior if malicious input is used.

Recommendation

Consider integrating checks like checked_add() to the calculations e.g. of the accumulated balance and total validators. Using overflow-checks = true on Cargo.toml won't be enough on the SP1 platform.

Resolution

Fixed. PR #16 and PR #22