Insufficient timestamp validation in `validateWithdrawalCredentials` will lead to erroneous user balances, allowing validators to steal rewards

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L181-L187

Vulnerability details

Impact

The timestamp validation of beaconStateRootProof.timestamp in validateWithdrawalCredentials is insufficient and may cause validators to be minted more shares than intended.

Proof of Concept

This is the validation of the beaconStateRootProof timestamp:

    if (beaconStateRootProof.timestamp == block.timestamp) {
        revert BeaconTimestampIsCurrent();
    }
    if (
        beaconStateRootProof.timestamp < node.lastSnapshotTimestamp
            || beaconStateRootProof.timestamp < node.currentSnapshotTimestamp
    ) revert BeaconTimestampTooOld();

beaconStateRootProof is specified by node owners and is used to validate the provided validatorFieldsProofs against the beacon block root. The issue is that this validation is insufficient and may allow stale validatorFieldsProofs to be valid.

As we can see from EigenLayer's implementation, it is essential that the withdrawal credential proof is not stale as the validator may have been slashed in a more recent beacon state root: https://github.com/Layr-Labs/eigenlayer-contracts/blob/bb5ae1822e0444dde1fe95f45cb0d77bf0983436/src/contracts/pods/EigenPod.sol#L331-L334

/**
         * Withdrawal credential proof should not be "stale" (older than VERIFY_BALANCE_UPDATE_WINDOW_SECONDS) as we are doing a balance check here
         * The validator container persists as the state evolves and even after the validator exits. So we can use a more "fresh" credential proof within
         * the VERIFY_BALANCE_UPDATE_WINDOW_SECONDS window, not just the first proof where the validator container is registered in the state.
         */
        require(
            oracleTimestamp + VERIFY_BALANCE_UPDATE_WINDOW_SECONDS >= block.timestamp,
            "EigenPod.verifyWithdrawalCredentials: specified timestamp is too far in past"
        );

In the EigenLayer implementation the oracleTimestamp variable is the beaconStateRootProof.timestamp. It is validated that it is not older than VERIFY_BALANCE_UPDATE_WINDOW_SECONDS: https://github.com/Layr-Labs/eigenlayer-contracts/blob/bb5ae1822e0444dde1fe95f45cb0d77bf0983436/src/contracts/pods/EigenPod.sol#L43-L47

    /**
     * @notice Maximum "staleness" of a Beacon Chain state root against which `verifyBalanceUpdate` or `verifyWithdrawalCredentials` may be proven.
     * We can't allow "stale" roots to be used for restaking as the validator may have been slashed in a more updated beacon state root.
     */
    uint256 internal constant VERIFY_BALANCE_UPDATE_WINDOW_SECONDS = 4.5 hours;

This validation is missing in the NativeVault.sol contract. Furthermore, beaconStateRootProof.timestamp in some cases is not even validated as node.lastSnapshotTimestamp and node.currentSnapshotTimestamp can even be zero if a snapshot is yet to start.

As a result, stale roots may be used to acquire the validator's withdrawal credentials. If a slash on a validator has happened in a more recent beacon state root, the validator will receive more shares of the Native Vault than expected. With more shares, they will be gaining a higher number of rewards unfairly.

Furthermore, it is also important to be aware that in validateSnapshotProofs the snapshot.parentBeaconBlockRoot that is used to validate the balanceContainer may also be stale.

Tools Used

Manual review

Recommended Mitigation Steps

Include a check that beaconStateRootProof.timestamp is not older than 4.5 hours in validateWithdrawalCredentials.

Assessed type

Invalid Validation

[trachevgeorgi]

This issue has been incorrectly excluded. In the report, I have raised the concern that beaconStateRootProof.timestamp has not been validated thoroughly enough. As a result, stale data will be used to acquire the validator's withdrawal credentials, which is critical for all Native Vaults as a validator can receive more shares than expected. Furthermore, I have presented an example from EigenLayer's similar implementation, further noting the importance of the validation that is currently missing.

[MiloTruck]

With reference to EIP-4788, the beacon roots contract has the following check:

    timestamp_idx = to_uint256_be(evm.calldata) % HISTORY_BUFFER_LENGTH
    timestamp = storage.get(timestamp_idx)

    if timestamp != evm.calldata:
        evm.revert()

If you read on, you'll realize that the contract only stores the most recent 8191 block roots, which is about ~27.3 hours:

The ring buffer data structures are sized to hold 8191 roots from the consensus layer. Using a prime number as the ring buffer size ensures that no value is overwritten until the entire ring buffer has been saturated and thereafter, each value will be updated once per iteration. This also means that even if the slot times were to change, we would continue to use at most 8191 storage slots.

Given the current mainnet values, 8191 roots provides about a day of coverage. This gives users plenty of time to make a transaction with a verification against a specific root and get the transaction included on-chain.

If beaconStateRootProof.timestamp passed to _getParentBlockRoot() is more than ~27.3 hours ago, the function will revert.

Karak allows the beacon chain state root to be stale for a day or so, which is a design choice. This value is 4.5 hours for EigenLayer, which is why they have the check.