Renzo - BiasedMerc's results

Lines of code

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

Vulnerability details

Impact

The current code that calculates the value of totalWithdrawalQueueValue incorrectly passes the same token to each call to lookupTokenValue() with balances of the withdrawQueue of other tokens, as the passed balance accesses collateralTokens[j].balanceOf(withdrawQueue) rather than collateralTokens[i].balanceOf(withdrawQueue).

This will cause the calculated totalWithdrawalQueueValue to be incorrect, which will cause the calculation of BalancerRateProvider::getRate() to calculate the incorrect rate of ezETH in ETH which will cause protocol to utilise an incorrect price for ezETH which can make the protocol mint too much ezETH on deposits which will devalue ezETH or to mint less ezETH than intended and users will lose funds.

Proof of Concept

RestakeManager::calculateTVLs()

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
...SKIP!...
                // record token value of withdraw queue
                if (!withdrawQueueTokenBalanceRecorded) {
                    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
                        collateralTokens[i],
                        collateralTokens[j].balanceOf(withdrawQueue)
                    );
                }

totalWithdrawalQueueValue is calculated by summing the value of collateralTokens[j].balanceOf(withdrawQueue) of each token collateralTokens[i] however the balance being passed to renzoOracle.lookupTokenValue is for collateralTokens[j] rather than collateralTokens[i] meaning the returned value will be incorrect.

RenzoOracle::lookupTokenValue()

    function lookupTokenValue(IERC20 _token, uint256 _balance) public view returns (uint256) {
        AggregatorV3Interface oracle = tokenOracleLookup[_token];
        if (address(oracle) == address(0x0)) revert OracleNotFound();

        (, int256 price, , uint256 timestamp, ) = oracle.latestRoundData();
        if (timestamp < block.timestamp - MAX_TIME_WINDOW) revert OraclePriceExpired();
        if (price <= 0) revert InvalidOraclePrice();

        // Price is times 10**18 ensure value amount is scaled
        return (uint256(price) * _balance) / SCALE_FACTOR;
    }

As seen, lookUpTokenValue is meant to be passed the token and the balance of the same token.

BalancerRateProvider::getRate()

    function getRate() external view returns (uint256) {
        // Get the total TVL priced in ETH from restakeManager
        (, , uint256 totalTVL) = restakeManager.calculateTVLs();

        // Get the total supply of the ezETH token
        uint256 totalSupply = ezETHToken.totalSupply();

        // Sanity check
        if (totalSupply == 0 || totalTVL == 0) revert InvalidZeroInput();

        // Return the rate
        return (10 ** 18 * totalTVL) / totalSupply;
    }

getRate() utilises the calculated TVL from restakeManager.calculateTVLs() to calculate the exchange rate between ezETH and ETH. Therfore if the returned TVL is deflated due to the incorrect calculations mentioned above, the returned ezETH exchange rate will be lower, which will lead to more ezETH being minted. This will cause the the protocol and users to become diluted and their redeems of ezETH in the future will redeem less ETH than intended, which will lead to a loss of funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Make the following changes:

• Remove the withdrawQueueTokenBalanceRecorded variable
• Move the totalWithdrawalQueueValue calculation outside of the nested j loop
• Change collateralTokens[j].balanceOf(withdrawQueue) to collateralTokens[i].balanceOf(withdrawQueue).

RestakeManager.sol#L282-L284

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

RestakeManager.sol#L289-L326

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

+           totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
+               collateralTokens[i],
+               collateralTokens[i].balanceOf(withdrawQueue)
+           );

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

This now accesses the correct token and balance for that token.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When funds are deposited using RestakeManager::deposit() the current withdraw buffer is calculated to check if it needs to be refilled, bufferToFill. If there is an amount that needs to be refilled then the deposit is used to fully or partiall refill the deposit (as much as the deposit funds allow). However if the _amount that the user is depositing is less than bufferToFill the whole _amount will be used, setting _amount to 0.

This will then be passed to operatorDelegator.deposit(_collateralToken, _amount) which will revert if _amount is 0. Meaning that all deposits of less than bufferToFill will revert, breaking core functionality of the protocol.

Proof of Concept

User deposits can be used to refill buffer. If _amount is less than the buffer then the full _amount will be used, reducing _amount to 0.

RestakeManager.sol#L542-L549

        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;

RestakeManager.sol#L558-L562

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

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

If 0 is passed to operatorDelegator.deposit() the call will revert:

OperatorDelegator::deposit()

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

This will cause all deposits worth less than bufferToFill to revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure that if _amount is 0 after refilling the buffer, then the approve and deposit calls are skipped to avoid 0 amount deposit attempts.

-        // 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) {
+            // Approve the tokens to the operator delegator
+            _collateralToken.safeApprove(address(operatorDelegator), _amount);

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

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/L2/xRenzoDeposit.sol#L247-L250

Vulnerability details

Impact

xRenzoDeposit.sol utilises RenzoOracleL2::getMintRate() to retrieve the price of ezETH however the implementation of the staleness check differs between the two contracts, which means that a valid price from the oracle can result a revert in xRenzoDeposit.sol.

Proof of Concept

xRenzoDeposit.sol#L247-L250

        // Verify the price is not stale
        if (block.timestamp > _lastPriceTimestamp + 1 days) {
            revert OraclePriceExpired();
        }

The staleness check ensures that the _lastPriceStamp is not older than 1 day, however in the RenzoOracleL2.sol contract prices are only stale after 1 day + 60 seconds, which can cause a valid price from the oracle to be unaccepted by this contract.

RenzoOracleL2::getMintRate()

    function getMintRate() public view returns (uint256, uint256) {
        (, int256 price, , uint256 timestamp, ) = oracle.latestRoundData();
        if (timestamp < block.timestamp - MAX_TIME_WINDOW) revert OraclePriceExpired();
        // scale the price to have 18 decimals
        uint256 _scaledPrice = (uint256(price)) * 10 ** (18 - oracle.decimals());
        if (_scaledPrice < 1 ether) revert InvalidOraclePrice();
        return (_scaledPrice, timestamp);
    }

RenzoOracleL2.sol#L12-L13

uint256 public constant MAX_TIME_WINDOW = 86400 + 60; // 24 hours + 60 seconds

As seen above, in RenzoOracleL2.sol the price is only stale after 24 hours + 60 seconds, whilst in xRenzoDeposit.sol a price is stale after 24 hours. This small difference can cause a valid to be rejected due to the implementation difference.

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure that staleness checks are consistent throughout the whole codebase to avoid issues with oracle prices being accepted in certain contracts and not in others.

Assessed type

Invalid Validation