Skip to content

Latest commit

 

History

History
151 lines (128 loc) · 7.62 KB

2024-04-Renzo.md

File metadata and controls

151 lines (128 loc) · 7.62 KB
Raw

Renzo

Contest Summary

Code under review: 2024-04-renzo (3231 nSLOC)

Contest Page: Renzo-contest

Findings

[H-1] completeQueuedWithdrawal will always revert due to incorrect modifier implementation

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L300 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L137

Vulnerability details

Renzo has a buffer that acts as a pool of assets to support withdrawal requests. There is a need for buffer in order to support withdrawal of assets.

The withdrawQueue contract get filled by 3 ways ->

New Deposits Daily Rewards Coming in the Protocol. Manual withdrawal from EigenLayer. Permissioned call from OperatorDelegator. If options 1 and 2 are not sufficient to fulfill withdraw requests of Users then admin accounts will manually unstake from EigenLayer through 2 step process:

  1. OperatorDelegator.queueWithdrawals
  2. OperatorDelegator.completeWithdrawals
// OperatorDelegator.sol
    function completeQueuedWithdrawal(
        IDelegationManager.Withdrawal calldata withdrawal,
        IERC20[] calldata tokens,
        uint256 middlewareTimesIndex
    ) external nonReentrant onlyNativeEthRestakeAdmin {
    -- SNIP --
                    restakeManager.depositQueue().fillERC20withdrawBuffer( //@audit
                        address(tokens[i]),
                        bufferToFill
                    );
    -- SNIP --

The fillERC20withdrawBuffer is called to fill up ERC20 withdraw buffer. However the function call will revert because that function required to be called by the restake manager admin. In Renzo, each and every roles are unique and have their own functionalities to perform. This means that whenever there is a need to fill up the buffer, the 3rd option will not be able to do so, bricking the protocol if there isn't enough assets to cover the withdrawal requests.

Furthermore, the Operator will not be able to unstake delegated funds from Eigenlayer. Assets remain frozen due to the inability to complete the steps required for withdrawal.

// DepositQueue.sol
    /// @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 { //@audit
    -- SNIP --

Impact

Buffer will never be filled in the case of supporting withdrawal requests. Operator unable to unstake delegated assets from Eigenlayer, causing assets to be frozen.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the access control for only restake manager, leaving the function similar to a donation function.

    function fillERC20withdrawBuffer(
        address _asset,
        uint256 _amount
--    ) external nonReentrant onlyRestakeManager {
++    ) external nonReentrant {

Assessed type

Access Control

[H-2] Wrong amount of TVL is produced, causing unfair underlying asset conversion

Lines of code

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

Vulnerability details

The calculateTVLs() function in RestakeManager calculates TVLs for each operator delegator per token, total per OD, and total for the protocol.

It employs two for loops using elements i and j, where i is used for the outer loop and j for the inner loop. The function calls RenzoOracle::lookupTokenValue() to get the asset value in the underlying currency based on a single token and balance.

However, due to always using the outer loop element i, it only loops through the first collateral token, resulting in the return value for that token. This affects the totalWithdrawalQueueValue and the overall asset value locked.

For example, with whitelisted tokens [ezETH, stETH, wBETH], using i for ezETH means the loop always uses the ezETH price feed. This can lead to incorrect calculations when considering the amounts and price feeds of other tokens like stETH and wBETH.

Whenever user submit withdraw request, it will always produce the wrong amount converted to underlying asset.

Impact

This can produce false results in the total value locked calculations, impacting crucial state variables like the amount to redeem in ETH/LST.

Proof of Concept

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
-- SNIP --
                if (!withdrawQueueTokenBalanceRecorded) {
                    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
                        collateralTokens[i],
                        collateralTokens[j].balanceOf(withdrawQueue) //@audit redundant since it was already added in getTokenBalanceFromStrategy
                    );
                }
-- SNIP --
        // Add native ETH help in withdraw Queue and totalWithdrawalQueueValue to totalTVL
        totalTVL += (address(withdrawQueue).balance + totalWithdrawalQueueValue);

Tools Used

Manual Review

Recommended Mitigation Steps

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

                if (!withdrawQueueTokenBalanceRecorded) {
                    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
--                      collateralTokens[i],
++                      collateralTokens[j],
                        collateralTokens[j].balanceOf(withdrawQueue) //@audit redundant since it was already added in getTokenBalanceFromStrategy
                    );
                }

Assessed type

Loop

[M-1] User able to withdraw tokens when Renzo is paused

Lines of code

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

Vulnerability details

The protocol can be paused by the admin DEPOSIT_WITHDRAW_PAUSER and WITHDRAW_QUEUE_ADMIN for DepositQueue.sol and WithdrawalQueue.sol. It allows the admin to pause/unpause deposits and withdraws. Users are not allowed to deposit or withdraw when paused. However, the user is still able to withdraw even if protocol is paused.

The WithdrawalQueue::withdraw() and WithdrawalQueue::claim() did not enforce the Openzeppelins whenNotPaused modifier. This means that if the admin were to call the WithdrawalQueue::pause(), it does not affect the withdraw flow. Hence, users are still able to submit withdrawal request and withdraw their underlying assets after 7 days.

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
    function claim(uint256 withdrawRequestIndex) external nonReentrant {

Impact

Users can withdraw underlying assets even if the Renzo is paused.

Tools Used

Manual Review

Recommended Mitigation Steps

Include the whenNotPaused modifier to vital functions.

--    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
++    function withdraw(uint256 _amount, address _assetOut) external nonReentrant whenNotPaused{
--    function claim(uint256 withdrawRequestIndex) external nonReentrant {
++    function claim(uint256 withdrawRequestIndex) external nonReentrant whenNotPaused{

Assessed type

Access Control