Renzo - 0xCiphky's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Delegation/OperatorDelegator.sol#L327 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Delegation/OperatorDelegator.sol#L193 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L274

Vulnerability details

Vulnerability Details:

The getTokenBalanceFromStrategy function in the OperatorDelegator contract retrieves the underlying token amount plus queued withdrawal shares for an operator's specified token. This calculation is used by the calculateTVLs function to determine the TVLs for each operator delegator by individual token, total for each OD, and total for the protocol.

The issue with the current implementation is that the function uses queuedShares[address(this)] to check if there are currently any queued shares for the specified token. If there are no queued shares, it returns just the underlying token amount.

    function getTokenBalanceFromStrategy(IERC20 token) external view returns (uint256) {
        return queuedShares[address(this)] == 0
            ? tokenStrategyMapping[token].userUnderlyingView(address(this))
            : tokenStrategyMapping[token].userUnderlyingView(address(this))
                + tokenStrategyMapping[token].sharesToUnderlyingView(queuedShares[address(token)]);
    }

However, if we look at the queueWithdrawals function, the queuedShares mapping saves queued shares for a specific token using the token address as the key. Therefore, the getTokenBalanceFromStrategy function should also be using the token address as the key to correctly perform the check.

    function queueWithdrawals(IERC20[] calldata tokens, uint256[] calldata tokenAmounts)
        external
        nonReentrant
        onlyNativeEthRestakeAdmin
        returns (bytes32)
    {
        ...
        for (uint256 i; i < tokens.length;) {
            ...
            // track shares of tokens withdraw for TVL
            queuedShares[address(tokens[i])] += queuedWithdrawalParams[0].shares[i];
            unchecked {
                ++i;
            }
        }
	...
        return withdrawalRoot;
    }

Impact:

• Severity: High. queuedShares will always be missed when the calculateTVL function is called, meaning the TVL values will never be accurate.
• Likelihood: High. Since the TVL values are integral to the protocol's main functions, this problem will occur frequently.

Tools Used:

• Manual analysis

Recommendation:

To address this issue, the getTokenBalanceFromStrategy function in the OperatorDelegator contract can be modified to perform the correct check as shown below.

    function getTokenBalanceFromStrategy(IERC20 token) external view returns (uint256) {
        return queuedShares[address(token)] == 0 // Change from address(this) to address(token)
            ? tokenStrategyMapping[token].userUnderlyingView(address(this))
            : tokenStrategyMapping[token].userUnderlyingView(address(this))
                + tokenStrategyMapping[token].sharesToUnderlyingView(queuedShares[address(token)]);
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Oracle/Mantle/METHShim.sol#L69

Vulnerability details

Vulnerability Details:

The METHShim contract uses the _getMETHData function to retrieve the price of 1 mETH in ETH from the mETH staking contract. Although the Meth token is not yet supported, "it can technically be supported using the METHShim contract if required". However, there is currently an unhandled edge case that can cause losses for both users and the protocol.

As mentioned in the Mantle docs:

• If a major slashing event occurs for the protocol's validators, which causes a large loss of ETH controlled by the protocol, the exchange rate will not reflect this slashing until the next Oracle update. As the Oracle is configured to run every 8 hours, it is possible for another party to detect the slashing event, and request an unstake of all of their ETH at an elevated exchange rate before the oracle data "catches up" and the correct rate is quoted.
• This leads to a loss of ETH for all remaining stakers, and would cause a rush to exit the protocol.

To mitigate this, the mantle staking contract implements the following mechanism:

• All unstake requests have a property called "finalization" which determines whether or not they can be claimed by the user. Requests will only finalize when an Oracle update has been posted which covers the block in which they were requested, plus some buffer.

However, in the RestakeManager contract, users can create a withdraw request at any time as long as there are currently enough funds for one of the collateral tokens or ETH in the withdrawal queue contract. This withdraw request will lock in the amount at the current exchange rate and be able to be claimed once a cooldown period has passed.

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
        ...
        // 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)
        );
				...
    }

Consider the following scenario:

• There is a major slashing event for the mantle protocol's validators, causing a large loss of ETH controlled by the protocol. However, the exchange rate does not reflect this slashing until the next Oracle update.
• mETH holders notice this and deposit tokens into the Renzo protocol, creating a withdrawal request with a token other than mETH to lock in their withdrawal at the current exchange rate before the slashing is reflected in the rate.
• Once the oracle is updated, the exchange rate is lowered, and the dumped mETH in the protocol causes losses through the total TVL for current users.

Impact:

• Severity: High. This could result in significant losses for users and the protocol
• Likelihood: Low. This issue is dependent on specific conditions related to the slashing event and the timing of Oracle updates.

Tools Used:

• Manual analysis

Recommendation:

Given that this issue is specific to the mETH exchange rate, consider implementing a mechanism to handle this edge case when retrieving the price. One approach could be to add functionality that allows disabling the Oracle, preventing deposits and withdrawals using mETH, until manually re-enabled by an admin. This would help mitigate the risk of users taking advantage of the exchange rate discrepancy caused by major slashing events.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L168 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L227 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L1/xRenzoBridge.sol#L139 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L592

Vulnerability details

Vulnerability Details:

The xRenzoDeposit contract facilitates the minting of xezETH tokens on Layer 2 (L2) in exchange for ETH or WETH tokens.

Specifically, the internal _deposit function exchanges the deposit tokens for nextWETH and mints xezETH tokens to the user. Subsequently, the funds in the contract are aggregated and swept by an admin down to Layer 1.

Upon reaching the L1 contract, the collateral is unwrapped and deposited into Renzo. The ezETH from the deposit is then sent to the lockbox to be wrapped into xezETH, and the xezETH is burned, as it is already minted on L2.

    function sweep() public payable nonReentrant {
	...
        // Get the balance of nextWETH in the contract
        uint256 balance = collateralToken.balanceOf(address(this));
	...
        // Approve it to the connext contract
        collateralToken.safeApprove(address(connext), balance);

        // Need to send some calldata so it triggers xReceive on the target
        bytes memory bridgeCallData = abi.encode(balance);

        connext.xcall{value: msg.value}(
            bridgeDestinationDomain,
            bridgeTargetAddress,
            address(collateralToken),
            msg.sender,
            balance,
            0, // Asset is already nextWETH, so no slippage will be incurred
            bridgeCallData
        );
	...
    }

Two potential problems exist:

1. Deposits are not immediately swept, leading to a delay where funds sit in the L2 contract until the sweep function is called. This delay can result in differing exchange rates between the time shares are minted on L2 and the time funds are minted on L1, potentially leaving the L1 lockbox for xezETH undercollateralized.
2. The L2 price is updated every six hours. This infrequency could lead to differing exchange rates between L1 and L2, also resulting in fewer shares being minted on L1 than on L2.

Impact:

• Severity: High. Minting fewer shares on the L1 could leave the L1 lockbox undercollateralized for the L2 shares.
• Likelihood: Medium. The likelihood will depend on the frequency of sweeping deposits and L2 price updates.

Tools Used:

• Manual analysis

Recommendation:

To reduce the discrepancy in price updates and deposits, increase the frequency of updates and deposits. A more permanent solution could involve minting and bridging for each deposit to the L1 and returning the minted shares' value on the L2, allowing users to mint the exact amount. However, this approach may incur higher fees.

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability Details:

The calculateTVL function in RestakeManager.sol calculates the TVLs for each operator delegator by individual tokens, total for each OD, and total for the protocol. However, it currently incorrectly calculates the totalWithdrawalQueueValue.

The issue arises from the parameter passed to renzoOracle.lookupTokenValue, where collateralTokens[i] is used throughout the inside loop instead of collateralTokens[j] , causing the same collateral token to be used for all iterations.

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        ...
        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;) {
                ...
                // record token value of withdraw queue
                if (!withdrawQueueTokenBalanceRecorded) {
                    totalWithdrawalQueueValue +=
                        renzoOracle.lookupTokenValue(collateralTokens[i], collateralTokens[j].balanceOf(withdrawQueue));
                }
                unchecked {
                    ++j;
                }
            }
            ...
            unchecked {
                ++i;
            }
        }
        ...
        return (operatorDelegatorTokenTVLs, operatorDelegatorTVLs, totalTVL);
    }

Consequently, the totalWithdrawalQueueValue will be incorrectly calculated as the same collateral token will be used throughout the loop with different collateral token balances.

Impact:

• Severity: High. The calculateTVLs function will be incorrectly calculated, affecting deposit and withdrawal amounts throughout the protocol.
• Likelihood: High. Since the function is integral to the protocol's main functions, this problem will occur frequently.

Tools Used:

• Manual analysis

Recommendation:

Modify the following code to pass collateralTokens[j] instead of collateralTokens[i].

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

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L491 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L206 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L279

Vulnerability details

Vulnerability Details:

The Renzo protocol documentation mentions the DEPOSIT_WITHDRAW_PAUSER role as one of the trusted roles in the protocol with the ability to pause/unpause deposits and withdrawals.

• Reference

This functionality is correctly implemented in the deposit function, which uses the notPaused modifier controlled by the DEPOSIT_WITHDRAW_PAUSER role. However, although the PausableUpgradeable contract is inherited by the WithdrawQueue contract and initialized, the notPaused modifier is not added to the withdraw or claim functions.

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

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

Consequently, neither the withdraw nor the claim functions will be able to be paused if needed. This could be problematic if the functionality needs to be paused due to the identification of a vulnerability or during a contract upgrade.

Additionally, the pausing ability is currently configured to another role contrary to the docs. However, this does not have any harmful impact.

Impact:

• Severity: Medium. The missing functionality could be costly if immediate pausing is required for security or upgrade purposes.
• Likelihood: Medium. While the need to pause these functions may not be frequent, it is essential for risk management and emergency situations.

Tools Used:

• Manual analysis

Recommendation:

To address this issue, add the notPaused modifier to the withdraw and claim functions in the WithdrawQueue contract to allow the protocol to pause these functions if needed.

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

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

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L274 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L491 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L206

Vulnerability details

Vulnerability Details:

The deposit and withdraw functions in the RestakeManager contract utilize the totalTVL (Total Value Locked) to determine the mint and redeem amounts for a user. However, the totalTVL may change between the transaction's submission and fulfillment due to various factors, including fluctuations in collateral/ETH balances or price changes.

    function calculateMintAmount(uint256 _currentValueInProtocol, uint256 _newValueAdded, uint256 _existingEzETHSupply)
        external
        pure
        returns (uint256)
    {
        // For first mint, just return the new value added.
        // Checking both current value and existing supply to guard against gaming the initial mint
        if (_currentValueInProtocol == 0 || _existingEzETHSupply == 0) {
            return _newValueAdded; // value is priced in base units, so divide by scale factor
        }
        // Calculate the percentage of value after the deposit
        uint256 inflationPercentaage = (SCALE_FACTOR * _newValueAdded) / (_currentValueInProtocol + _newValueAdded);

        // Calculate the new supply
        uint256 newEzETHSupply = (_existingEzETHSupply * SCALE_FACTOR) / (SCALE_FACTOR - inflationPercentaage);
        // Subtract the old supply from the new supply to get the amount to mint
        uint256 mintAmount = newEzETHSupply - _existingEzETHSupply;

        // Sanity check
        if (mintAmount == 0) revert InvalidTokenAmount();

        return mintAmount;
    }

Therefore, the lack of slippage control on these functions could lead to users receiving a much worse rate then anticipated. This lack of control could lead to user losses or at the very least discourage users from interacting with the protocol.

Impact:

• Severity: Medium. Users could receive a worse rate than anticipated due to fluctuations in the TVL.
• Likelihood: Medium. It is likely that users will encounter situations where the transaction price differs from their expectations.

Tools Used:

• Manual analysis

Recommendation:

Allows users to set a minimum amount out for deposits and withdrawals.

    function deposit(IERC20 _collateralToken, uint256 _amount, uint256 _minOut, uint256 _referralId) public nonReentrant notPaused {
        ...
        // Calculate how much ezETH to mint
        uint256 ezETHToMint = renzoOracle.calculateMintAmount(totalTVL, collateralTokenValue, ezETH.totalSupply());
        
        require(ezETHToMint >= _minOut) // ADD HERE

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

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

    function withdraw(uint256 _amount, address _assetOut, uint256 _minOut) external nonReentrant {
        ...
        // 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);
        }
        
        require(amountToRedeem >= _minOut) // ADD HERE

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

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/L2/xRenzoDeposit.sol#L414 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L592 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/xERC20/contracts/XERC20.sol#L328

Vulnerability details

Vulnerability Details:

The sweep function in the xRenzoDeposit contract takes the balance of nextWETH in the contract and bridges it down to the L1. The L1 contract will then unwrap it, deposit it in Renzo, and lock up the ezETH in the lockbox on L1.

A possible problem with the function is that it doesn’t let the user calling the function specify the amount being swept but will instead always sweep the full balance in the contract.

    function sweep() public payable nonReentrant {
        // Verify the caller is whitelisted
        if (!allowedBridgeSweepers[msg.sender]) {
            revert UnauthorizedBridgeSweeper();
        }
        // Get the balance of nextWETH in the contract
        uint256 balance = collateralToken.balanceOf(address(this));
	...
        connext.xcall{value: msg.value}(
            bridgeDestinationDomain,
            bridgeTargetAddress,
            address(collateralToken),
            msg.sender,
            balance,
            0, // Asset is already nextWETH, so no slippage will be incurred
            bridgeCallData
        );
	...
    }

The reason this is problematic is because there are restrictions on the L1 side which will make the call fail if not met.

• When tokens are received by the xRenzoBridge contract on L1, the xezETH is burned since it was already minted to the user. However, the XERC20 contract has a daily burning limit, so if the amount passes over that threshold, the bridged funds transaction will revert on the receiving contract's end.
• Another problem is that the RestakeManager contract has a TVL limit set when depositing ETH into Renzo. If the balance in the L2 side causes it to pass the threshold, the transaction will also revert.

Once a transaction fails in the receiving contract, it will have to be manually deposited, once possible by the team, as the full amount will still be deposited in the receiving contract but not to Renzo.

Note: If the Connext bridge transaction reverts on the receiving contract, the amount will still be deposited in the contract but not to Renzo.

This causes some more problems:

• Tokens would have been minted on the L2 side but not accounted for inside the protocol on the L1 side. This means that the value sitting idle could leave the L2 tokens undercollateralized.
• The protocol will be earning less rewards than it should, as the value will be sitting idle but shares have been minted for it, so all users will get less rewards.
• The team having to manually deposit tokens will mean this will not count towards the bridge's limits, and therefore the limit will be compromised as it can be bypassed for that duration until refilled.

Impact:

• Severity: High. This can lead to losses for users and the protocol, as well as operational difficulties for the team.
• Likelihood: Medium. This issue will occur every time the conditions on the L1 side are not met.

Tools Used:

• Manual analysis

Recommendation:

Modify the sweep function to allow the user to specify the amount being swept, rather than sweeping the full balance in the contract. This will allow for more control over the bridging process and reduce the risk of exceeding limits on the L1 side.

Additionally, consider implementing checks and limits on the L2 side to prevent the minting of amounts that could exceed daily burning limits or TVL thresholds.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/L2/xRenzoDeposit.sol#L168 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L592

Vulnerability details

Vulnerability Details:

The RestakeManager contract on the L1 side sets a maxDepositTVL limit on the depositETH function, causing it to revert when the cap is reached. However, this restriction is not applied to L2 deposits.

On the L2 side, the depositETH function in the xRenzoDeposit contract can establish daily minting limits, but there is currently no method to monitor or restrict deposits based on the maxDepositTVL limit set on the L1 side.

    function depositETH(uint256 _minOut, uint256 _deadline) external payable nonReentrant returns (uint256) {
        if (msg.value == 0) {
            revert InvalidZeroInput();
        }

        // Get the deposit token balance before
        uint256 depositBalanceBefore = depositToken.balanceOf(address(this));

        // Wrap the deposit ETH to WETH
        IWeth(address(depositToken)).deposit{value: msg.value}();

        // Get the amount of tokens that were wrapped
        uint256 wrappedAmount = depositToken.balanceOf(address(this)) - depositBalanceBefore;

        // Sanity check for 0
        if (wrappedAmount == 0) {
            revert InvalidZeroOutput();
        }

        return _deposit(wrappedAmount, _minOut, _deadline);
    }

As a result, any deposits that push the threshold over the limit will result in a transaction failure in the receiving contract after a sweep is performed. The team will then need to manually deposit this amount when feasible.

Note: If the Connext bridge transaction reverts on the receiving contract, the amount will still be deposited in the contract but not to Renzo.

Impact:

This scenario will result in tokens being minted on the L2 side without being recorded within the protocol on the L1 side. This discrepancy could potentially leave the L2 tokens undercollateralized. Additionally, the protocol would earn fewer rewards than expected, as the value would remain idle despite shares already being minted for it, resulting in losses for all users. Furthermore, the maxDepositTVL is compromised, as it is not enforced when minting tokens on the L2 side.

Tools Used:

• Manual analysis

Recommendation:

Implement a mechanism to track and limit deposits in relation to the maxDepositTVL limit on the L1 side.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Vulnerability Details:

The deposit function in the RestakeManager contract enables users to deposit ERC20 whitelisted collateral tokens into the protocol. It first checks the withdrawal buffer and fills it up using some or all of the deposited amount if it is below the buffer target. The remaining amount is then transferred to the operator delegator and deposited into EigenLayer.

The current issue with this implementation is that if the amount deposited is less than bufferToFill, the full amount will be used to fill the withdrawal buffer, leaving the amount value as zero.

    function deposit(IERC20 _collateralToken, uint256 _amount, uint256 _referralId) public nonReentrant notPaused {
        // Verify collateral token is in the list - call will revert if not found
        uint256 tokenIndex = getCollateralTokenIndex(_collateralToken);
	...

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

Subsequently, the function will approve the zero amount to the operator delegator and call deposit on the operator delegator. However, as seen in the OperatorDelegator contract's deposit function below, a zero deposit will be reverted.

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

Impact:

• Severity: Medium. User deposits will always revert if the amount being deposited is less than the bufferToFill value.
• Likelihood: High. Depending on the set amount for the withdrawal buffer, this could be a common occurrence.

Tools Used:

• Manual analysis

Recommendation:

To address this issue, the deposit function can be modified to only approve the amount to the operator delegator and call deposit on the operator delegator if the amount is greater than zero.

    function deposit(IERC20 _collateralToken, uint256 _amount, uint256 _referralId) public nonReentrant notPaused {
        // Verify collateral token is in the list - call will revert if not found
        uint256 tokenIndex = getCollateralTokenIndex(_collateralToken);
	...
        // 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);
        }
	if (_amount > 0) { // ADD HERE
            // Transfer the tokens to the operator delegator
            _collateralToken.safeApprove(address(operatorDelegator), _amount);

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

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/L1/xRenzoBridge.sol#L210 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RateProvider/BalancerRateProvider.sol#L29 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/L2/xRenzoDeposit.sol#L310

Vulnerability details

Vulnerability Details:

The sendPrice function in the xRenzoBridge contract calls the getRate function to retrieve the current price of ezETH to ETH and broadcasts it to Layer 2 networks. Subsequently, the price is received by either the ConnextReceiver or CCIPReceiver and invokes the updatePrice function in the xRenzoDeposit contract on L2. However, a potential opportunity exists where a user can monitor the L1 mempool for the sendPrice function call, observe the new price, and sandwich it if profitable.

    /**
     * @notice  Exposes the price via getRate()
     * @dev     This is required for a balancer pool to get the price of ezETH
     * @return  uint256  .
     */
    function getRate() external view override returns (uint256) {
        return lastPrice;
    }

Upon detecting a favourable price change, the user could mint xezETH on L2 before the price adjustment takes effect. Subsequently, when the price change is finalized, the user can sell the xezETH on a protocol that reads the price from the getRate function in the xRenzoDeposit contract, thus profiting from the price discrepancy.

Impact:

• Severity: Medium. This will allow users to exploit price changes by minting xezETH at a favourable rate before the price update is reflected.
• Likelihood: High. Given the visibility of transactions in the mempool and the potential for arbitrage opportunities, it is likely that users will attempt to exploit this.

Tools Used:

• Manual analysis

Recommendation:

Since there are two fees associated with L2 deposits, this should help minimize this problem. Additionally, as ezETH is more on the stable side, it is less prone to significant price fluctuations. However, continuous monitoring and adjustment of the update frequency may be necessary to prevent potential exploitation.

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability Details:

The protocol implements a two-step process for withdrawals: first, the withdraw function in the WithdrawQueue contract is called by a user to create a withdrawal request. Once created, a user can call the claim function to claim the withdrawal request after the cooldown period has passed.

The issue with the current implementation is that the claim function uses the coolDownPeriod state variable to check if a user's withdrawal request has passed the period and met the condition.

    function claim(uint256 withdrawRequestIndex) external nonReentrant {
        // check if provided withdrawRequest Index is valid
        if (withdrawRequestIndex >= withdrawRequests[msg.sender].length) {
            revert InvalidWithdrawIndex();
        }

        WithdrawRequest memory _withdrawRequest = withdrawRequests[msg.sender][withdrawRequestIndex];
        
        if (block.timestamp - _withdrawRequest.createdAt < coolDownPeriod) revert EarlyClaim();
	...
    }

This coolDownPeriod can be modified at any time by the WithdrawQueueAdmin, meaning that since the claim function uses the state variable, previous withdrawal requests that were finalized at the initial cooldown period will now have that time period changed and could have a longer wait.

    function updateCoolDownPeriod(uint256 _newCoolDownPeriod) external onlyWithdrawQueueAdmin {
        if (_newCoolDownPeriod == 0) revert InvalidZeroInput();
        emit CoolDownPeriodUpdated(coolDownPeriod, _newCoolDownPeriod);
        coolDownPeriod = _newCoolDownPeriod;
    }

Assume the following:

• User A calls the claim function to create a withdrawal request for funds needed before a specific date, and the cooldown period currently meets that date.
• The WithdrawQueueAdmin calls the updateCoolDownPeriod to increase the cooldown duration.
• User A now has to wait a longer period, even though their withdrawal request was finalized at the initial cooldown period.

Impact:

• Severity: Medium. The current implementation will lead to unexpected unfair delays for users who have already finalized their withdrawal requests.
• Likelihood: Low. The likelihood is dependent on the frequency of cooldown period changes

Tools Used:

• Manual analysis

Recommendation:

The protocol should calculate and add the release time to the specific withdrawal request. That way, any changes to the cooldown period will only affect future withdrawal requests.

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
        ...
        uint256 releaseTime = block.timestamp + coolDownPeriod; //ADD HERE
        // add withdraw request for msg.sender
        withdrawRequests[msg.sender].push(
            WithdrawRequest(_assetOut, withdrawRequestNonce, amountToRedeem, _amount, block.timestamp, releaseTime) //ADD HERE
        );
        ...
    }

Assessed type

Other