c4-bot-6 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Delegation/OperatorDelegator.sol#L343

Vulnerability details

Impact

If the shares owned by an operator delegator, represented by podOwnerShares, become more negative than the sum of queuedShares[IS_NATIVE] and stakedButNotVerifiedEth, it causes an underflow. This leads to a failure whenever the calculateTVL() function is called, which stops any further deposits into the protocol.

Proof of Concept

The podOwnerShares indicate the amount of shares the owner possesses in the virtual beacon chain ETH strategy. If this value turns negative, which can happen if the owner's shares decrease between queuing and completing a withdrawal, it leads to problems. Specifically, when the function getStakedETHBalance is called, it may underflow and revert due to this negative value.

* @notice Mapping from Pod owner owner to the number of shares they have in the virtual beacon chain ETH strategy.
* @dev The share amount can become negative. This is necessary to accommodate the fact that a pod owner's virtual beacon chain ETH shares can
* decrease between the pod owner queuing and completing a withdrawal.
* When the pod owner's shares would otherwise increase, this "deficit" is decreased first _instead_.
* Likewise, when a withdrawal is completed, this "deficit" is decreased and the withdrawal amount is decreased; We can think of this
* as the withdrawal "paying off the deficit".

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/EigenLayer/interfaces/IEigenPodManager.sol#L111-L118

The issue arises when the absolute negative value of podOwnerShares exceeds the sum of queuedShares[IS_NATIVE] and stakedButNotVerifiedEth. In such a case, the function getStakedETHBalance underflows and reverts, preventing further action.

function getStakedETHBalance() external view returns (uint256) {
    // accounts for current podOwner shares + stakedButNotVerified ETH + queued withdraw shares
    int256 podOwnerShares = eigenPodManager.podOwnerShares(address(this));
    return
        podOwnerShares < 0
            ? queuedShares[IS_NATIVE] + stakedButNotVerifiedEth - uint256(-podOwnerShares)
            : queuedShares[IS_NATIVE] + stakedButNotVerifiedEth + uint256(podOwnerShares);
}

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Delegation/OperatorDelegator.sol#L343

This situation can escalate to the point where the function calculateTVLs can't be called anymore, as it relies on getStakedETHBalance, thus leading to a protocol blockage.

function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
    //.......
    uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
        collateralTokens[j]
        );
    //.....
}

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L302

To resolve this, an admin can inject ETH into the DepositQueue contract to boost its balance to at least 32 ETH. Then, they can trigger the stakeEthFromQueue function to transfer 32 ETH to the affected operator delegator, resolving the revert issue.

Recommended Mitigation Steps

A potential fix involves modifying the getStakedETHBalance function to return zero if the negative podOwnerShares surpass the sum of queuedShares[IS_NATIVE] and stakedButNotVerifiedEth. This prevents the underflow issue and allows the function to continue without reverting.

function getStakedETHBalance() external view returns (uint256) {
    // accounts for current podOwner shares + stakedButNotVerified ETH + queued withdraw shares
    int256 podOwnerShares = eigenPodManager.podOwnerShares(address(this));
    if(podOwnerShares < 0 && uint256(-podOwnerShares) >= queuedShares[IS_NATIVE] + stakedButNotVerifiedEth){
        return 0;
    }
    return
        podOwnerShares < 0
            ? queuedShares[IS_NATIVE] + stakedButNotVerifiedEth - uint256(-podOwnerShares)
            : queuedShares[IS_NATIVE] + stakedButNotVerifiedEth + uint256(podOwnerShares);
}

Assessed type

Context

raymondfam commented 4 months ago

@howlbot reject