RestakeManager.calculateTVLs function does not increase totalTVL by value of DepositQueue contract's collateral tokens

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Deposits/DepositQueue.sol#L254-L277
https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L647-L668
https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L274-L358
https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L592-L616
https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Withdraw/WithdrawQueue.sol#L206-L263

Vulnerability details

Impact

It is possible that collateral tokens are sent to the DepositQueue contract as rewards. To ensure that these tokens are properly used, the following DepositQueue.sweepERC20 function is implemented for depositing these tokens into EigenLayer through calling the RestakeManager.depositTokenRewardsFromProtocol function below.

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

    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);
                ...
            }

            // 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;
            ...
        }
    }

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

    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);
    }

Since there is no guarantee that the DepositQueue.sweepERC20 function would be called in the timely manner, these collateral tokens can stay in the DepositQueue contract temporarily. Yet, because these collateral tokens of the DepositQueue contract can be eventually deposited into EigenLayer, the value of these tokens should count towards the total TVL. However, the following RestakeManager.calculateTVLs function only executes totalTVL += address(depositQueue).balance so the total TVL is not increased by the value of the collateral tokens that stay in the DepositQueue contract temporally. In this case, the total TVL becomes lower than what it should be.

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L274-L358

    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;

        // flag for withdrawal queue balance set
        bool withdrawQueueTokenBalanceRecorded = false;
        address withdrawQueue = address(depositQueue.withdrawQueue());

        // withdrawalQueue total value
        uint256 totalWithdrawalQueueValue = 0;

        for (uint256 i = 0; i < odLength; ) {
            // Track the TVL for this OD
            uint256 operatorTVL = 0;

            // Track the individual token TVLs for this OD - native ETH will be last item in the array
            uint256[] memory operatorValues = new uint256[](collateralTokens.length + 1);
            operatorDelegatorTokenTVLs[i] = operatorValues;

            // 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],
                        collateralTokens[j].balanceOf(withdrawQueue)
                    );
                }

                unchecked {
                    ++j;
                }
            }

            // Get the value of native ETH staked for the OD
            uint256 operatorEthBalance = operatorDelegators[i].getStakedETHBalance();

            // Save it to the array for the OD
            operatorValues[operatorValues.length - 1] = operatorEthBalance;

            // Add it to the total TVL for this OD
            operatorTVL += operatorEthBalance;

            // Add it to the total TVL for the protocol
            totalTVL += operatorTVL;

            // Save the TVL for this OD
            operatorDelegatorTVLs[i] = operatorTVL;

            // Set withdrawQueueTokenBalanceRecorded flag to true
            withdrawQueueTokenBalanceRecorded = true;

            unchecked {
                ++i;
            }
        }

        // Get the value of native ETH held in the deposit queue and add it to the total TVL
        totalTVL += address(depositQueue).balance;

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

        return (operatorDelegatorTokenTVLs, operatorDelegatorTVLs, totalTVL);
    }

When the totalTVL returned by the RestakeManager.calculateTVLs function is lower than what it should be, functions like RestakeManager.depositETH and WithdrawQueue.withdraw that call the RestakeManager.calculateTVLs function would operate inaccurately. For instance, when calling the WithdrawQueue.withdraw function below, amountToRedeem that equals renzoOracle.calculateRedeemAmount(_amount, ezETH.totalSupply(), totalTVL) would be less than what it should be, which forces the user to redeem a collateral token amount that is less than what the user deserves for giving up the same amount of ezETH; hence, such user loses the collateral token amount difference that the user is entitled to.

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L592-L616

    function depositETH(uint256 _referralId) public payable nonReentrant notPaused {
        // Get the total TVL
        (, , uint256 totalTVL) = calculateTVLs();

        // Enforce TVL limit if set
        if (maxDepositTVL != 0 && totalTVL + msg.value > maxDepositTVL) {
            revert MaxTVLReached();
        }

        // Deposit the remaining ETH into the DepositQueue
        depositQueue.depositETHFromProtocol{ value: msg.value }();

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

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

        ...
    }

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/Withdraw/WithdrawQueue.sol#L206-L263

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
        // check for 0 values
        if (_amount == 0 || _assetOut == address(0)) revert InvalidZeroInput();

        // check if provided assetOut is supported
        if (withdrawalBufferTarget[_assetOut] == 0) revert UnsupportedWithdrawAsset();

        // transfer ezETH tokens to this address
        IERC20(address(ezETH)).safeTransferFrom(msg.sender, address(this), _amount);

        // calculate totalTVL
        (, , uint256 totalTVL) = restakeManager.calculateTVLs();

        // Calculate amount to Redeem in ETH
        uint256 amountToRedeem = renzoOracle.calculateRedeemAmount(
            _amount,
            ezETH.totalSupply(),
            totalTVL
        );

        // update amount in claim asset, if claim asset is not ETH
        if (_assetOut != IS_NATIVE) {
            // Get ERC20 asset equivalent amount
            amountToRedeem = renzoOracle.lookupTokenAmountFromValue(
                IERC20(_assetOut),
                amountToRedeem
            );
        }

        // revert if amount to redeem is greater than withdrawBufferTarget
        if (amountToRedeem > getAvailableToWithdraw(_assetOut)) revert NotEnoughWithdrawBuffer();

        // increment the withdrawRequestNonce
        withdrawRequestNonce++;

        // add withdraw request for msg.sender
        withdrawRequests[msg.sender].push(
            WithdrawRequest(
                _assetOut,
                withdrawRequestNonce,
                amountToRedeem,
                _amount,
                block.timestamp
            )
        );

        // add redeem amount to claimReserve of claim asset
        claimReserve[_assetOut] += amountToRedeem;

        ...
    }

Proof of Concept

Please add the following test file. Using forge, this test_totalTVLThatIsNotIncreasedByValueOfDepositQueueCollateralTokens and test_totalTVLThatIsIncreasedByValueOfDepositQueueCollateralTokens tests will pass to demonstrate the described scenario.

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.19;

import { Test } from "forge-std/Test.sol"; // solhint-disable-line no-unused-import

library balanceOfMock {
  // balanceOf mock for testing purpose
  function balanceOf(address _token, address) internal pure returns (uint256) {
    if (_token == address(1)) {
      return 100;
    }
    if (_token == address(2)) {
      return 200;
    }
  }
}

contract C4Test6 is Test {
  using balanceOfMock for address;

  // two mock collateral tokens
  address[] collateralTokens = [address(1), address(2)];

  // mock address for depositQueue
  address depositQueue = address(3);

  // two mock operator delegators
  uint256 odLength = 2;

  // simulate ezETH amount to be burned and existing ezETH supply
  uint256 ezETHBeingBurned = 0.5e18;
  uint256 existingEzETHSupply = 1e18;

  // mock function for RenzoOracle.lookupTokenValue
  function lookupTokenValueMock(address _token, uint256 _balance) public view returns (uint256) {
    if (_token == address(1)) {
      return _balance;
    }
    if (_token == address(2)) {
      return _balance * 2;
    }
  }

  // mock function for RenzoOracle.calculateRedeemAmount
  function calculateRedeemAmountMock(
      uint256 _ezETHBeingBurned,
      uint256 _existingEzETHSupply,
      uint256 _currentValueInProtocol
  ) public pure returns (uint256) {
      // This is just returning the percentage of TVL that matches the percentage of ezETH being burned
      uint256 redeemAmount = (_currentValueInProtocol * _ezETHBeingBurned) / _existingEzETHSupply;

      // Sanity check
      require(redeemAmount > 0, "InvalidTokenAmount");

      return redeemAmount;
  }

  function test_totalTVLThatIsNotIncreasedByValueOfDepositQueueCollateralTokens() external {
    deal(depositQueue, 100);

    // following code simulate RestakeManager.calculateTVLs function

    uint256 totalTVL = 0;

    totalTVL += address(depositQueue).balance;

    //////

    // totalTVL equals 100 for this test's conditions
    assertEq(totalTVL, 100);

    uint256 amountToRedeem = calculateRedeemAmountMock(
        ezETHBeingBurned,
        existingEzETHSupply,
        totalTVL
    );

    // amount to redeem in ETH equals 50 for this test's conditions
    assertEq(amountToRedeem, 50);
  }

  function test_totalTVLThatIsIncreasedByValueOfDepositQueueCollateralTokens() external {
    deal(depositQueue, 100);

    // following code simulate RestakeManager.calculateTVLs function
    //   except that totalTVL is increased by value of depositQueue's collateral tokens

    uint256 totalTVL = 0;

    bool depositQueueTokenBalanceRecorded = false;

    uint256 totalDepositQueueValue = 0;

    for (uint256 i = 0; i < odLength; ) {
      uint256 tokenLength = collateralTokens.length;

      for (uint256 j = 0; j < tokenLength; ) {
        if (!depositQueueTokenBalanceRecorded) {
          totalDepositQueueValue += lookupTokenValueMock(
              collateralTokens[j],
              collateralTokens[j].balanceOf(depositQueue)
          );
        }

        unchecked {
            ++j;
        }
      }

      depositQueueTokenBalanceRecorded = true;

      unchecked {
          ++i;
      }
    }

    totalTVL += address(depositQueue).balance + totalDepositQueueValue;

    //////

    // totalTVL equals 600, which is higher than 100, for same conditions used in test_totalTVLThatIsNotIncreasedByValueOfDepositQueueCollateralTokens
    assertEq(totalTVL, 600);

    uint256 amountToRedeem = calculateRedeemAmountMock(
        ezETHBeingBurned,
        existingEzETHSupply,
        totalTVL
    );

    // amount to redeem in ETH equals 300, which is higher than 50, for same conditions used in test_totalTVLThatIsNotIncreasedByValueOfDepositQueueCollateralTokens
    assertEq(amountToRedeem, 300);
  }
}

Tools Used

Manual Review

Recommended Mitigation Steps

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L283 can be updated to the following code.

        bool withdrawQueueTokenBalanceRecorded = false;
        bool depositQueueTokenBalanceRecorded = false;

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L287 can be updated to the following code.

        uint256 totalWithdrawalQueueValue = 0;
        uint256 totalDepositQueueValue = 0;

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L323-L325 can be updated to the following code.

		        if (!depositQueueTokenBalanceRecorded) {
		          totalDepositQueueValue += renzoOracle.lookupTokenValue(
		              collateralTokens[j],
		              collateralTokens[j].balanceOf(depositQueue)
		          );
		        }

                unchecked {
                    ++j;
                }

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L344 can be updated to the following code.

            withdrawQueueTokenBalanceRecorded = true;
            depositQueueTokenBalanceRecorded = true;

https://github.com/code-423n4/2024-04-renzo/blob/549f774626b71dd13a427561d4743535a9cc0dca/contracts/RestakeManager.sol#L352 can be updated to the following code.

        totalTVL += address(depositQueue).balance + totalDepositQueueValue;

Assessed type

Context

[C4-Staff]

CloudEllie marked the issue as duplicate of #28

[c4-judge]

alcueca marked the issue as not a duplicate

[jatinj615]

similar to #379

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[c4-judge]

alcueca marked the issue as grade-b

[rbserver]

Hi @alcueca, thanks for judging!

This report explains the same issue that is reported by #383. Furthermore, this report includes a coded POC that demonstrates the issue and coded recommended mitigation steps, which are not included in #383. Hence, would this report be more complete and better than the duplicates of #383 if not better than #383 itself and at least be a duplicate of #383 if #383 is still considered as the selected report?