If the validator gets slashed before its credentials are verified then the stakedButNotVerifiedEth will be an incorrect value, misleading the TVL

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L364-L392 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L187-L206 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L592-L616

Vulnerability details

Impact

If the ETH deposited to a delegators validator is slashed before the verifyWithdrawalCredentials called, then the protocol will misaccount the total ether that the validator holds. It will always assume that there will be 32 ether however, in reality, depending on the slashing result the validator might even have 0 ether! This will result in Renzo calculating the TVL higher than it is.

Proof of Concept

When ETH is deposited by users to RestakingManager of Renzo, the ETH is sent directly to deposit queue as follows:

function depositETH(uint256 _referralId) public payable nonReentrant notPaused {
        .
        .
        // Deposit the remaining ETH into the DepositQueue
        -> depositQueue.depositETHFromProtocol{ value: msg.value }();
        .
        .
    }

ETH stands in deposit queue until it's pooled to 32 ether and then the admin can put the ether to a delegator calling stakeEthFromQueue as follows:

function stakeEthFromQueue(
        IOperatorDelegator operatorDelegator,
        bytes calldata pubkey,
        bytes calldata signature,
        bytes32 depositDataRoot
    ) external onlyNativeEthRestakeAdmin {
        uint256 gasBefore = gasleft();
        // Send the ETH and the params through to the restake manager
        -> restakeManager.stakeEthInOperatorDelegator{ value: 32 ether }(
            operatorDelegator,
            pubkey,
            signature,
            depositDataRoot
        );

function stakeEthInOperatorDelegator(
        IOperatorDelegator operatorDelegator,
        bytes calldata pubkey,
        bytes calldata signature,
        bytes32 depositDataRoot
    ) external payable onlyDepositQueue {
        .
        .
        // Call the OD to stake the ETH
        -> operatorDelegator.stakeEth{ value: msg.value }(pubkey, signature, depositDataRoot);
    }

and then the operator delegators function is finally triggered:

function stakeEth(
        bytes calldata pubkey,
        bytes calldata signature,
        bytes32 depositDataRoot
    ) external payable onlyRestakeManager {
        // Call the stake function in the EigenPodManager
        eigenPodManager.stake{ value: msg.value }(pubkey, signature, depositDataRoot);

        // Increment the staked but not verified ETH
        stakedButNotVerifiedEth += msg.value;
    }

As we can observe the stakedButNotVerifiedEth is increased. This is only will be decreased when the pods shares are verified via verifyWithdrawalCredentials function call. This function will convert the effective beacon chain balance to eigen pod shares in EigenLayer and inside the Renzo implementation it will decrease the queued balance so double accounting will not be the case.

function verifyWithdrawalCredentials(
        uint64 oracleTimestamp,
        BeaconChainProofs.StateRootProof calldata stateRootProof,
        uint40[] calldata validatorIndices,
        bytes[] calldata withdrawalCredentialProofs,
        bytes32[][] calldata validatorFields
    ) external onlyNativeEthRestakeAdmin {
        uint256 gasBefore = gasleft();
        eigenPod.verifyWithdrawalCredentials(
            oracleTimestamp,
            stateRootProof,
            validatorIndices,
            withdrawalCredentialProofs,
            validatorFields
        );

        // Decrement the staked but not verified ETH
        for (uint256 i = 0; i < validatorFields.length; ) {
            uint64 validatorCurrentBalanceGwei = BeaconChainProofs.getEffectiveBalanceGwei(
                validatorFields[i]
            );
            -> stakedButNotVerifiedEth -= (validatorCurrentBalanceGwei * GWEI_TO_WEI);
            unchecked {
                ++i;
            }
        }
        .
    }

However, if the validator is slashed between the very first deposit and the time when the verifyWithdrawalCredentials called, then the stakedButNotVerifiedEth will not be zeroed out.

For example, when 32 ether is deposited the beacon chain, effective balance of the validator is 32 ether. After some hours the validator gets slashed and have 30 ether now. Note that verifyWithdrawalCredentials is still not called yet. When the verifyWithdrawalCredentials is called, since the effective balance is 30 stakedButNotVerifiedEth will be 32-30 = 2 and there will be 30 eigen pod shares. Hence the TVL is 32 ether from that validator because the total eth hold by a operator delegator is queuedShares + stakedButNotVerifiedEth + eigen pod shares!. However, this is not true, the validator has exactly 30 ether but the stakedButNotVerifiedEth assumes there are 2 more that is awaiting the deposit, overaccounting the TVL of the validator.

Tools Used

Recommended Mitigation Steps

Add a special function to decrease stakedButNotVerifiedEth in case of such event described above happens. This function should be an only admin role and should be able to decrement the stakedButNotVerifiedEth without verifying a validators withdrawal credential

Assessed type

Other

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid