Renzo - TECHFUND's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L11-L16 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L64-L99 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L139-L149

Vulnerability details

Impact

WithdrawQueue::pause() and unpause() are not functional as the modifier that restrict the functionality as not assigned to any of the functions of the contract.

As a result, the WithdrawQueue cannot be effectively paused under required circumstances.

Proof of Concept

WithdrawQueue contract derives from PausableUpgradeable contract. The contract initialize function properly initialises the PausableUpgradeable contract.

   __Pausable_init();

The contract also exposes pause() and unpause() function for WithdrawQueueAdmin to pause or unpause.

    function pause() external onlyWithdrawQueueAdmin {
        _pause();
    }

    
    function unpause() external onlyWithdrawQueueAdmin {
        _unpause();
    }

But, the pause and unpause is enabled for a function by attaching the below to modifiers.

a) whenPaused: allows the function to operate when paused b) whenNotPaused: allows the function to operate when not paused

As these modifiers are not attached to any of the functions in the WithdrawQueue contract, the pause feature is not effectively implemented.

hence, the functions will continue to work even when paused or unpaused.

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant 
    {
       ...
    } 

    function claim(uint256 withdrawRequestIndex) external nonReentrant {
       ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

Attached the modifier to the functions to effectively implement the pause functionality.

example: Attach whenNotPaused() modifier to withdraw function as below. By attaching the modifier, when the contract is paused, the withdraw function will stop working.

  function withdraw(uint256 _amount, address _assetOut) external whenNotPaused nonReentrant 
  {
       ...
  }  

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L244-L263 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L274-L358

Vulnerability details

Impact

The TVL calculation could result in incorrect TVL if the Admin removes the collateral from CollateralList using removeCollateralToken() function.

Since the calculateTVLs() function loops through the CollateralList to arrive at the TVL, if a token is removed from the list with out checking for balance in the strategies, then the TVL computed will be incorrect.

The removal of token from CollateralList will also result in loosing of rewards earned.

Proof of Concept

As we can see from the below code snippet, to compute TVL, for each of the tokens in the list, the balance with each strategy is checked and accounted for TVL. If token is remove from this list with out checking for any balance in strategies, then potentially, the TVL value is under accounted and hence incorrect.

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
      ...
 ===>       // 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
                );
      ... 
    }

This also impacts the totalRewards earned.

    function getTotalRewardsEarned() external view returns (uint256) {
        uint256 totalRewards = 0;

        // First get the ETH rewards tracked in the deposit queue
        totalRewards += depositQueue.totalEarned(address(0x0));

        // For each token, get the total rewards earned from the deposit queue and price it in ETH
        uint256 tokenLength = collateralTokens.length;
        for (uint256 i = 0; i < tokenLength; ) {
            // Get the amount
            uint256 tokenRewardAmount = depositQueue.totalEarned(address(collateralTokens[i]));

            // Convert via the price oracle
            totalRewards += renzoOracle.lookupTokenValue(collateralTokens[i], tokenRewardAmount);

            unchecked {
                ++i;
            }
        }

        // For each OperatorDelegator, get the balance (these are rewards from staking that have not been restaked)
        // Funds in OD's EigenPod are assumed to be rewards in M1 until exiting validators or withdrawals are supported
        // Pending unstaked delayed withdrawal amounts are pending being routed into the DepositQueue after a delay
        uint256 odLength = operatorDelegators.length;
        for (uint256 i = 0; i < odLength; ) {
            totalRewards += address(operatorDelegators[i].eigenPod()).balance;
            unchecked {
                ++i;
            }
        }

        return totalRewards;
    }

Tools Used

Manual

Recommended Mitigation Steps

Before removing the token from the CollateralList, the function should check against all strategies to ensure there is no balance invested in any of the strategies. This check will ensure that token being removed does not have any funds locked in strategies.

Assessed type

Other