Renzo - gesha17's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L274 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L289 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L299 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L316 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L510 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L565 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L123

Vulnerability details

Impact

During TVL calculation the total value stored in the withdrawal queue is accumulated. This is done by iterating over each token and adding the value(in ETH) to an accumulator variable totalWithdrawalQueueValue. This is done by passing the token and token amount to the RenzoOracle contract, however the wrong token index is passed, leading to wrong token/amount pairs. As a result the value of the totalWithdrawalQueueValue variable is incorrect and the totalTVL variable passed back is incorrect as well.

This impacts every function that relies on the totalTVL returned being correct, which includes the following core:

• deposit() in RestakeManager contract,
• calculateMintAmount() in RenzoOracle.

As a result of this flawed calculation, the deposit TVL limit is incorrect, but more importantly the mint/redeem amounts for ezETH are incorect which would lead to loss of funds.

Proof of Concept

Lets look at how the calculateTVLs() function works. As we can see, the function is responsible for calculating the total value locked in the protocol, which includes any value deposited through operator delegators(ODs), the deposit queue and the withdrawal queue.

For values in the OD's there are two arrays:

• operatorDelegatorTokenTVLs[][] : for the value locked for each OD for each token
• operatorDelegatorTVLs[] : for the total value locked for each OD for all tokens

These do not include value in the deposit or withdrawal queue contracts. Those are only included in the third returned parameter - totalTVL.

The withdrawal queue contains value for each token, so there must be a loop that iterates over all tokens and accumulates the value locked in the withdrawal queue.

The calculateTVLs() function contains a nested loop. One outer loop, to iterate over all OD's:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L289

function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        uint256[][] memory operatorDelegatorTokenTVLs = new uint256[][](operatorDelegators.length);
        uint256[] memory operatorDelegatorTVLs = new uint256[](operatorDelegators.length);
        uint256 totalTVL = 0;

        // Iterate through the ODs
        uint256 odLength = operatorDelegators.length;
        ...
        for (uint256 i = 0; i < odLength; ) {
            ...
            unchecked {
                ++i;
            }
        }
        ...
    }

And an inner loop to iterate over the tokens:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L299

function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        uint256[][] memory operatorDelegatorTokenTVLs = new uint256[][](operatorDelegators.length);
        uint256[] memory operatorDelegatorTVLs = new uint256[](operatorDelegators.length);
        uint256 totalTVL = 0;

        // Iterate through the ODs
        uint256 odLength = operatorDelegators.length;
        ...
        for (uint256 i = 0; i < odLength; ) {
            ...
            // Iterate through the tokens and get the value of each
            uint256 tokenLength = collateralTokens.length;
            for (uint256 j = 0; j < tokenLength; ) {
                ...
                // record token value of withdraw queue
                if (!withdrawQueueTokenBalanceRecorded) {
                    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
                        collateralTokens[i],
                        collateralTokens[j].balanceOf(withdrawQueue)
                    );
                }
                unchecked {
                    ++j;
                }
            }
            ...
            
            // Set withdrawQueueTokenBalanceRecorded flag to true
            withdrawQueueTokenBalanceRecorded = true;
            unchecked {
                ++i;
            }
        }
        ...
    }

We need the inner loop to add the value for each token to the totalWithdrawalQueueValue only once, not for every OD. This is why there is a bool withdrawQueueTokenBalanceRecorded = false; that is set to true after the first iteration.

However, notice that the index used to access token and token balance for withdrawal queue do not match:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L316

    // record token value of withdraw queue
    if (!withdrawQueueTokenBalanceRecorded) {
        totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
            collateralTokens[i],
            collateralTokens[j].balanceOf(withdrawQueue)
        );
    }

The value is looked up with collater token i but balance for collateralToken j. Only j should be used to denote token index. Since this is only accumulated for the first iteration over the ODs, the renzo oracle will return values for token with index 0, and balance of each other token. This is an incorrect calculation.

At the end of the calculateTVLs() function execution, the totalWithdrawalQueueValue is added to the totalTVL, which makes that variable incorrect.

As a result any function that relies on the totalTVL returned being correct will be impacted.

This includes the deposit() function in RestakeManager, which checks if the totalTVL will go above the tvl limit after deposit - link. But more imporantly, in this deposit() function the amount of ezETH mint amount is impacted since the renzoOracle will calculate amount of ezETH to mint based on the totalTVL - link and link.

This can lead to loss of funds, since abnormal amounts of ezETH will be minted. This can be exploited by an attacker that chooses to deposit funds from the type of the token at index 0, specifically inflating the totalTVL returned by the calculateTVLs() function.

Tools Used

Manual Review

Recommended Mitigation Steps

Access the collateralTokens index correctly:

    if (!withdrawQueueTokenBalanceRecorded) {
        totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
            collateralTokens[j],
            collateralTokens[j].balanceOf(withdrawQueue)
        );
    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L143 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L543 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L547 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L148

Vulnerability details

Impact

During a deposit the restaking manager first tries to fill the buffer deficit in the withdrawal queue. The remaining amount is forwarded to the operator delegator. However, if the buffer deficit is larger than the deposited amount, the remianing amount for the operator delegator will be 0 and the transaction will revert because the operator delegator does not accept 0 amount deposits.

As a result of this user transactions will revert unexpectedly whenever an amount smaller than the buffer deficit is being deposited.

Proof of Concept

First notice that operator delegator deposit function reverts if the input deposit tokenAmount is 0:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L143

    function deposit(
        IERC20 token,
        uint256 tokenAmount
    ) external nonReentrant onlyRestakeManager returns (uint256 shares) {
        if (address(tokenStrategyMapping[token]) == address(0x0) || tokenAmount == 0)
            revert InvalidZeroInput();

        // Move the tokens into this contract
        token.safeTransferFrom(msg.sender, address(this), tokenAmount);

        return _deposit(token, tokenAmount);
    }

Now, suppose a user wants to deposit an amount say 1e18 and there is a buffer deficit in the withdrawal queue of 2e18 tokens. The restaking manager checks how much the withdrawal queue buffer deficit is, and first tries to fill it up before forwarding the amount to the operator delegator:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L543

    uint256 bufferToFill = depositQueue.withdrawQueue().getBufferDeficit(
        address(_collateralToken)
    );
    if (bufferToFill > 0) {
        bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
        // update amount to send to the operator Delegator
        _amount -= bufferToFill;

        // safe Approve for depositQueue
        _collateralToken.safeApprove(address(depositQueue), bufferToFill);

        // fill Withdraw Buffer via depositQueue
        depositQueue.fillERC20withdrawBuffer(address(_collateralToken), bufferToFill);
    }

    // Approve the tokens to the operator delegator
    _collateralToken.safeApprove(address(operatorDelegator), _amount);

    // Call deposit on the operator delegator
    operatorDelegator.deposit(_collateralToken, _amount);

So, with the bufferToFill being at 2e18 and _amount at 1e18, the lines:

        bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
        // update amount to send to the operator Delegator
        _amount -= bufferToFill;

Will set the bufferToFill to 1e18 and _amount to 0.

So when the amount is deposited in the operator delegator, it will be passed in as 0, and since the deposit function in the OperatorDelegator contract reverts if the amount is 0, the transaction will fail.

Tools Used

Manual Review

Recommended Mitigation Steps

Only call deposit on the operator delegator if the remaining amount is larger than 0.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L352 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L275 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L254 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L647 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L152

Vulnerability details

Impact

The calculateTVLs() function is responsible for returning the total value locked in the protocol. This is mainly used for determining a correct ezETH/ETH price. The function collects token values from ODs, the deposit queue and the withdrawal queue converts them to ETH and accumulates them in the totalTVL variable.

However, the the calculateTVLs() function access directly the balance of the DepositQueue contract, and it does not check for any ERC20 balances. Whenever an admin calls the sweepERC20() on the DepostQueue contract, these tokens are swept into the RestakeManager and then into an operator delegator - instantly increasing the TVL in the protocol and thus impacting the price of ezETH. As such, the price of ezETH while there are ERC20's in the deposit queue is incorrect.

This opens an attack vector for malicios users can frontrun calls to sweepERC20(), and take advantage of lower ezETH price. Additionally, this impacts the tvl deposit limits for ERC20s, as it also does not account for DepositQueue contract balances.

Proof of Concept

The calculateTVLs() function only accounts for the DepositQueue contract ETH balance:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L352

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        ...
        // Get the value of native ETH held in the deposit queue and add it to the total TVL
        totalTVL += address(depositQueue).balance;
        ...
    }

However, it does account for the balances of OD's, as it iterates over them and the ERC20's and accumulates those balances in the variables:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L275C1-L277C30

    uint256[][] memory operatorDelegatorTokenTVLs = new uint256[][](operatorDelegators.length);
    uint256[] memory operatorDelegatorTVLs = new uint256[](operatorDelegators.length);
    uint256 totalTVL = 0;

With this in mind, let's look at the sweepERC20() function in the DepositQueue:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L254

    /// @dev Sweeps any accumulated ERC20 tokens in this contract to the RestakeManager
    /// Only callable by a permissioned account
    function sweepERC20(IERC20 token) external onlyERC20RewardsAdmin {
        uint256 balance = IERC20(token).balanceOf(address(this));
        if (balance > 0) {
            uint256 feeAmount = 0;

            // Sweep fees if configured
            if (feeAddress != address(0x0) && feeBasisPoints > 0) {
                feeAmount = (balance * feeBasisPoints) / 10000;
                IERC20(token).safeTransfer(feeAddress, feeAmount);

                emit ProtocolFeesPaid(token, feeAmount, feeAddress);
            }

            // Approve and deposit the rewards
            token.approve(address(restakeManager), balance - feeAmount);
            restakeManager.depositTokenRewardsFromProtocol(token, balance - feeAmount);

            // Add to the total earned
            totalEarned[address(token)] = totalEarned[address(token)] + balance - feeAmount;

            // Emit the rewards event
            emit RewardsDeposited(IERC20(address(token)), balance - feeAmount);
        }
    }

As we can see, this function takes a fee from each ERC20 reward earned, and then forwards the rest to restakeManager.depositTokenRewardsFromProtocol(token, balance - feeAmount);

This function in turn forwards the amounts to the ODs:

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L647

    /// @dev Deposit ERC20 token rewards from the Deposit Queue
    /// Only callable by the deposit queue
    function depositTokenRewardsFromProtocol(
        IERC20 _token,
        uint256 _amount
    ) external onlyDepositQueue {
        // Get the TVLs for each operator delegator and the total TVL
        (, uint256[] memory operatorDelegatorTVLs, uint256 totalTVL) = calculateTVLs();

        // Determine which operator delegator to use
        IOperatorDelegator operatorDelegator = chooseOperatorDelegatorForDeposit(
            operatorDelegatorTVLs,
            totalTVL
        );

        // Transfer the tokens to this address
        _token.safeTransferFrom(msg.sender, address(this), _amount);

        // Approve the tokens to the operator delegator
        _token.safeApprove(address(operatorDelegator), _amount);

        // Deposit the tokens into EigenLayer
        operatorDelegator.deposit(_token, _amount);
    }

As a result, a call to calculateTVLs() after a transaction that calls sweepERC20() will return a larger totalTVL. And since the price of ezETH is proportional to totalTVL / ezETHsupply (link) it will be increased.

Thus, it is possible for malicios users to time their transactions right before calls to sweepERC20, to take advantage of underpriced ezETH.

Tools Used

Manual Review

Recommended Mitigation Steps

Include ERC20 balances(without the fees) in the totalTVL returned by the calculateTVLs() function.

Assessed type

Math