Unhandled revert in `xRenzoBridge.xReceive()` can lead to stuck funds and xezETH insolvency

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/L1/xRenzoBridge.sol#L175 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L598

Vulnerability details

How L2 Native Restaking in Renzo works

1. User initiates the process by depositing ETH/wETH into the xRenzoDeposit.sol contract particularly using the function xRenzoDeposit::deposit() or the function xRenzoDeposit::depositETH(). If the user deposited ETH using the latter function, the ETH will then be wrapped into wETH.
2. xRenzoDeposit::_deposit() function will then be called, and it will do three things:
   1. Retrieve the bridge fee for the amount you deposited, using the function xRenzoDeposit::getBridgeFeeShare() and it'll subtract it from the amountIn (the amount you deposited).
   2. It'll then utilize Connext's swapping feature to swap the ETH or wETH you deposited to nextWETH.
   3. It'll then retrieve the price and last price timestamp of ezETH from the RenzoOracleL2.sol and calculate how much xezETH (XERC20) to mint you, and then it'll mint you that amount of xezETH.
   4. The resulted nextWETH from the swap will remain in the xRenzoDeposit contract for now.
3. Periodically, the xRenzoDeposit::sweep() function will be called by an admin. This function simply takes the balance of nextWETH in the contract and bridges it down to the L1.
4. After the bridging occurs and we're now in L1, the function xRenzoBridge::xReceive() will be triggered. It'll simply unwrap the received nextWETH to ETH and then deposit it into the RestakeManager using the function RestakeManager::depositETH()
5. The xRenzoBridge then sends the minted ezETH tokens to the Lockbox contract to be wrapped.
6. xRenzoBridge will then burn the amount of xezETH tokens minted since they were already minted on L2.

The vulnerability

The Connext documentation discusses how a receiver contract should be created. It explicitly mentions that any contract implementing the IXReceive interface, should be built defensively and any revert-susceptible code should always be handled properly. From the docs:

If the call on the receiver contract (also referred to as "target" contract) reverts, funds sent in with the call will end up on the receiver contract.

Ultimately, the goal should be to handle any revert-susceptible code and ensure that the logical owner of funds always maintains agency over them.

The xRenzoBridge::xReceive() function does not wrap the restakeManager.depositETH{value:ethAmount}() call in a try/catch block, which is dangerous given that the function RestakeManager::depositETH() can revert under certain circumstances.

xRenzoBridge::xReceive() :

    function xReceive(
        bytes32 _transferId,
        uint256 _amount,
        address _asset,
        address _originSender,
        uint32 _origin,
        bytes memory
    ) external nonReentrant returns (bytes memory) {

        ...............

        // Get the amonut of ezETH before the deposit
        uint256 ezETHBalanceBeforeDeposit = ezETH.balanceOf(address(this));

        // Deposit it into Renzo RestakeManager
------> restakeManager.depositETH{ value: ethAmount }();

        // Get the amount of ezETH that was minted
        uint256 ezETHAmount = ezETH.balanceOf(address(this)) - ezETHBalanceBeforeDeposit;

        ................

RestakeManager::depositETH() :

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

        .............

Looking at RestakeManager::depositETH(), it is evident that it can revert anytime xRenzoBridge tries to deposit an amount of ETH which would exceed the maxDepositTVL threshold.

Impact

When that happens, there are several consequences:

1. The continued execution flow in L1 will fail silently. The admin calling sweep() won't immediately notice that this has failed in L1.
2. The bridged WETH from L2 -> L1 will be stuck in the L1 xRenzoBridge contract.
3. The minted xezETH on L2 will become insolvent, as a corresponding amount to be redeemed for ezETH will not be minted on L1.

Tools Used

Manual Review

Recommended Mitigation Steps

Wrap the restakeManager.depositETH{}() call in a try/catch block and handle any potential reverts.

Assessed type

Other

[0xJuancito]

See #571

[0xJuancito]

@howlbot accept