Renzo - Aymen0909's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L299-L303 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L134-L137

Vulnerability details

Issue Description

When the OperatorDelegator::completeQueuedWithdrawal function is invoked to finalize a withdrawal from EL, it attempts to utilize the accumulated ERC20 tokens to fill the ERC20 withdrawal buffer, as demonstrated in the code snippet below:

function completeQueuedWithdrawal(
    IDelegationManager.Withdrawal calldata withdrawal,
    IERC20[] calldata tokens,
    uint256 middlewareTimesIndex
) external nonReentrant onlyNativeEthRestakeAdmin {
    uint256 gasBefore = gasleft();
    if (tokens.length != withdrawal.strategies.length) revert MismatchedArrayLengths();

    // Complete the queued withdrawal from EigenLayer with receiveAsToken set to true
    delegationManager.completeQueuedWithdrawal(withdrawal, tokens, middlewareTimesIndex, true);

    IWithdrawQueue withdrawQueue = restakeManager.depositQueue().withdrawQueue();
    for (uint256 i; i < tokens.length; ) {
        if (address(tokens[i]) == address(0)) revert InvalidZeroInput();

        // Deduct queued shares for tracking TVL
        queuedShares[address(tokens[i])] -= withdrawal.shares[i];

        // Check if the token is not Native ETH
        if (address(tokens[i]) != IS_NATIVE) {
            // Check the withdrawal buffer and fill if below buffer target
            uint256 bufferToFill = withdrawQueue.getBufferDeficit(address(tokens[i]));

            // Get the balance of this contract
            uint256 balanceOfToken = tokens[i].balanceOf(address(this));
            if (bufferToFill > 0) {
                bufferToFill = (balanceOfToken <= bufferToFill) ? balanceOfToken : bufferToFill;

                // Update the amount to send to the operator Delegator
                balanceOfToken -= bufferToFill;

                // Safely approve for depositQueue
                tokens[i].safeApprove(address(restakeManager.depositQueue()), bufferToFill);

                // Fill the Withdraw Buffer via depositQueue
                restakeManager.depositQueue().fillERC20withdrawBuffer(
                    address(tokens[i]),
                    bufferToFill
                );
            }

            // Deposit remaining tokens back to EigenLayer
            if (balanceOfToken > 0) {
                _deposit(tokens[i], balanceOfToken);
            }
        }
        unchecked {
            ++i;
        }
    }

    // Emit the Withdraw Completed event with withdrawalRoot
    emit WithdrawCompleted(
        delegationManager.calculateWithdrawalRoot(withdrawal),
        withdrawal.strategies,
        withdrawal.shares
    );
    // Record the current spent gas
    _recordGas(gasBefore);
}

The function iterates over the withdrawn tokens array and, for each token, checks if the withdrawal buffer needs filling. If required, the function attempts to call the depositQueue::fillERC20withdrawBuffer function, which is responsible for directing the ERC20 to the withdrawal queue contract to fill the buffer.

The issue arises because the depositQueue::fillERC20withdrawBuffer function can only be accessed by the RestakeManager contract, as it enforces the onlyRestakeManager modifier, as depicted below:

/// @dev Allows only the RestakeManager address to call functions
modifier onlyRestakeManager() {
    if (msg.sender != address(restakeManager)) revert NotRestakeManager();
    _;
}

function fillERC20withdrawBuffer(
    address _asset,
    uint256 _amount
) external nonReentrant onlyRestakeManager {
    ...
}

Consequently, when the completeQueuedWithdrawal function attempts this call, it reverts because OperatorDelegator lacks access to the depositQueue::fillERC20withdrawBuffer function. This results in the entire withdrawal completion call reverting, rendering it impossible for the admin to retrieve funds from EL.

In summary, this issue triggers a persistent DOS of the OperatorDelegator::completeQueuedWithdrawal function, preventing the protocol and users from withdrawing funds from EL and resulting in a loss of funds.

Impact

Persistent DOS of the OperatorDelegator::completeQueuedWithdrawal function, preventing the protocol from withdrawing funds from EL and leading to fund losses for the protocol and users.

Tools Used

Manual review, VS Code

Recommended Mitigation

The simplest resolution is to grant access to the depositQueue::fillERC20withdrawBuffer function to everyone by removing the onlyRestakeManager modifier. This adjustment introduces no vulnerabilities to the protocol since any user calling it effectively donates funds to the protocol (to the withdrawal queue).

Assessed type

DoS

Lines of code

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

Vulnerability details

Issue Description

The calculateTVLs() function shown in the snippet below is used by the RestakeManager and other protocol contracts to calculate the TVL of the entire protocol denominated in ETH. The function is supposed to return 3 pieces of information as stated in the function Natspec:

/// @dev This function calculates the TVLs for each operator delegator by individual token, total for each OD, and total for the protocol.
/// @return operatorDelegatorTokenTVLs Each OD's TVL indexed by operatorDelegators array by collateralTokens array
/// @return operatorDelegatorTVLs Each OD's Total TVL in order of operatorDelegators array
/// @return totalTVL The total TVL across all operator delegators.

We will only be concentrating on the global total TVL of the protocol represented by totalTVL (which is denominated in ETH). This value is supposed to include all the value of funds given to operator delegators (either ERC20 or ETH), plus the value of the ones kept in the deposit queue contract (mainly ETH), and finally, the value of funds reserved for withdrawal which are kept in the withdrawal queue contract.

The function calculates correctly the first two values, but for the latter, there is an error. As it can be seen below, the ERC20 withdrawal queue TVL value is calculated only in the first iteration of the loops, to avoid accounting it multiple times (this is dictated by the withdrawQueueTokenBalanceRecorded flag). However, when trying to call the renzoOracle.lookupTokenValue function to get the value of ERC20 tokens held in the withdrawal queue denominated in ETH, the wrong collateral token address is provided:

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 tokens and get the value of each
    uint256 tokenLength = collateralTokens.length;
    for (uint256 j = 0; j < tokenLength; ) {
        // Get the value of this token

        uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
            collateralTokens[j]
        );

        // Set the value in the array for this OD
        operatorValues[j] = renzoOracle.lookupTokenValue(
            collateralTokens[j],
            operatorBalance
        );

        // Add it to the total TVL for this OD
        operatorTVL += operatorValues[j];

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

        unchecked {
            ++j;
        }
    }
    ...

    // Add native ETH help in withdraw Queue and totalWithdrawalQueueValue to totalTVL
    totalTVL += (address(withdrawQueue).balance + totalWithdrawalQueueValue);
}

Indeed, we see that the collateral address given to renzoOracle.lookupTokenValue (first parameter of the function) is equal to collateralTokens[i], but the index i is used for the operator delegator loop and will always be equal to 0 in the first iteration. The correct index that must be used is index j representing the different tokens.

This issue means that all tokens from j == 1 will be calculated using the wrong price feed (using the price feed of the first token), and thus all their values will be wrong. This will result in the overall withdrawal queue TVL value totalWithdrawalQueueValue to be wrong, and thus the global TVL totalTVL returned by the function will be inaccurate (maybe lesser or higher than in reality).

This issue will have a negative impact on many critical parts of the protocol that rely on the value of totalTVL in their logic. For example, the calculation of the ezETH minted/burned shares when depositing or withdrawing from the protocol or the calculation of the ezETH/ETH rate which also relies on that value.

In the end, this issue will compromise the correct functioning of the protocol and could potentially result in a loss of funds for the protocol or its users.

Impact

Wrong calculation of the ERC20 withdrawal queue TVL value totalWithdrawalQueueValue in calculateTVLs() will result in inaccurate protocol TVL calculation, which can compromise the correct functioning of the protocol and could potentially result in a loss of funds for the protocol or its users.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, use the correct token index when calling lookupTokenValue function. calculateTVLs() must be modified as follows:

function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
    ...

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

    ...

}

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L135-L149 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L206 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L279

Vulnerability details

Issue Description

The WithdrawQueue contract implements the OZ PausableUpgradeable contract, which adds pausing functionalities to the contract that can be used by the admin:

/**
 * @notice  Pause the contract
 * @dev     Permissioned call (onlyWithdrawQueueAdmin)
 */
function pause() external onlyWithdrawQueueAdmin {
    _pause();
}

/**
 * @notice  Unpause the contract
 * @dev     Permissioned call (onlyWithdrawQueueAdmin)
 */
function unpause() external onlyWithdrawQueueAdmin {
    _unpause();
}

The issue is that although the admin can change the paused state using either pause() or unpause() functions, these functionalities are not utilized within the contract, the contract does not implement the whenNotPaused nor whenPaused modifiers in any function. As a result, withdrawals from the protocol cannot be paused (stopped).

This could pose significant problems, especially in emergency situations or if the protocol is exploited. In such cases, the admin will have no means to prevent users from withdrawing their funds from the withdrawal queue contract.

It's worth noting that although the RestakeManager contract also has pausing functionality, which can be triggered by the onlyDepositWithdrawPauserAdmin admin, this will only be applied to new deposits and will not affect withdrawals (as the admin name would suggest).

Impact

The inability to pause withdrawals from the WithdrawQueue contract could lead to potential loss of funds if the protocol is exploited or enters an emergency state.

Tools Used

Manual review, VS Code

Recommended Mitigation

To implement the pausing functionalities in WithdrawQueue, the whenNotPaused modifier must be added to the main functions of the contract: withdraw and claim.

Assessed type

Context

Lines of code

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

Vulnerability details

Issue Description

When a user deposits an ERC20 collateral token into the protocol using the RestakeManager::deposit function, a protion of the deposited amount will be used to fill the withdrawal queue buffer if it need to be filled as shown below:

function deposit(
    IERC20 _collateralToken,
    uint256 _amount,
    uint256 _referralId
) public nonReentrant notPaused {
    ...

    // Transfer the collateral token to this address
    _collateralToken.safeTransferFrom(msg.sender, address(this), _amount);

    // Check the withdraw buffer and fill if below buffer target
    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);

    //@audit will revert if _amount == 0 after buffer got filled
    // Call deposit on the operator delegator
    operatorDelegator.deposit(_collateralToken, _amount);

    // Calculate how much ezETH to mint
    uint256 ezETHToMint = renzoOracle.calculateMintAmount(
        totalTVL,
        collateralTokenValue,
        ezETH.totalSupply()
    );

    // Mint the ezETH
    ezETH.mint(msg.sender, ezETHToMint);

    // Emit the deposit event
    emit Deposit(msg.sender, _collateralToken, _amount, ezETHToMint, _referralId);
}

As it can be seen, the actual amount used to fill the buffer bufferToFill is calculated using the formula:

bufferToFill = min(_amount, bufferToFill)

There is an edge case that can occur when bufferToFill >= _amount. In that case, the value will be capped bufferToFill = _amount, and this will result in the remaining amount _amount (to be deposited) being null after this decrement: _amount -= bufferToFill.

In this case, after filling the buffer, the function will try to deposit that remaining zero amount _amount into the operator delegator contract by calling OperatorDelegator::deposit function:

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

    ...
}

The issue is that this function reverts when the provided token amount tokenAmount is zero, as shown in the check above. Thus, in our edge case where the provided amount _amount is in fact zero, the deposit call will revert, preventing the user from depositing in the protocol.

Even though this issue doesn't cause direct fund loss for the users who deposit an amount smaller than the withdrawal buffer deficit, it will prevent a lot of users from depositing, which will, in turn, prevent the correct refill of the withdrawal buffer. This will force the protocol to initiate EL withdrawals to meet the withdrawal requests made, resulting in a loss of potential yield and rewards that could've been generated by EL for the protocol and its users.

A simple example to illustrate this issue would be:

• Let's consider the withdrawal buffer for stETH token is 100 stETH (equivalent to $200,000 if 1 ETH = $2,000) and the current buffer deficit is 50 stETH (equivalent to $100,000).

• In this scenario, all users depositing less than 50 stETH will see their deposit transaction reverted due to the mentioned issue. Only deposits exceeding this threshold will be accepted without reversion.

• Consequently, the protocol will miss out on many smaller deposited funds that could have accumulated into a larger amount, potentially generating a relatively high yield and rewards from EL.

• Additionally, since there will always be withdrawal requests that must be fulfilled and the fact that the buffer is not getting filled by those reverted deposits, this issue will force the protocol to withdraw its staked funds from EL to satisfy those withdrawal requests. This action will result in the protocol and its users losing potential yield and rewards that could have been generated from EL.

Impact

The RestakeManager::deposit function will prevent users from depositing in certain conditions, preventing the refill of the withdrawal buffer. This will force the protocol to initiate EL withdrawals to meet the withdrawal requests made, resulting in a loss of potential yield and rewards for the protocol and its users.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, the OperatorDelegator::deposit function should only be invoked in RestakeManager::deposit function when the amount _amount is not null, the following modification should be introduced:

function deposit(
    IERC20 _collateralToken,
    uint256 _amount,
    uint256 _referralId
) public nonReentrant notPaused {
    ...

    // Transfer the collateral token to this address
    _collateralToken.safeTransferFrom(msg.sender, address(this), _amount);

    // Check the withdraw buffer and fill if below buffer target
    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);
++  if (_amount > 0) {
++      operatorDelegator.deposit(_collateralToken, _amount);
++  }

    // Calculate how much ezETH to mint
    uint256 ezETHToMint = renzoOracle.calculateMintAmount(
        totalTVL,
        collateralTokenValue,
        ezETH.totalSupply()
    );

    // Mint the ezETH
    ezETH.mint(msg.sender, ezETHToMint);

    // Emit the deposit event
    emit Deposit(msg.sender, _collateralToken, _amount, ezETHToMint, _referralId);
}

Assessed type

Context