Not handling the failure of cross chain messaging

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

If the xReceive function fails to handle reverts properly, it could result in funds getting stuck. This can happen for example when:

• Depositing into L2 when protocol on L1 is paused: because pausing is not defined on L2.
• Depositing large amount on L2: because it is not enforced to deposit less than a limit on L2
• Depositing too small amount on L2: because the ezETH rate on L1 and L2 could be different.

Proof of Concept

Failure to handle errors in the xReceive function as outlined by Connext can lead to funds becoming inaccessible. https://docs.connext.network/developers/guides/handling-failures#reverts-on-receiver-contract

This can happen in several scenarios within the xRenzoBridge contract:

1. If the deposited amount on L2 exceeds the maxDepositTVL limit set on L1, the deposit will fail when bridged to L1. This could occur if an attacker uses a large flash loan of WETH to deposit on L2 and then swaps the minted xezETH to repay the flash loan.

   function xReceive(
       bytes32 _transferId,
       uint256 _amount,
       address _asset,
       address _originSender,
       uint32 _origin,
       bytes memory
   ) external nonReentrant returns (bytes memory) {
       // ...
       restakeManager.depositETH{ value: ethAmount }();
       //....
   }

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

   function deposit(
       IERC20 _collateralToken,
       uint256 _amount,
       uint256 _referralId
   ) public nonReentrant notPaused {
       //....
       if (maxDepositTVL != 0 && totalTVL + collateralTokenValue > maxDepositTVL) {
           revert MaxTVLReached();
       }
       //....
   }

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

2. If the protocol on L1 is paused, the deposit made on L2 will be successful and equivalent xezETH will be minted, but when it is bridged to L1, it will fail. This is because no pausing mechanism is defined on L2 to be aligned with L1.

   function depositETH(
       uint256 _minOut,
       uint256 _deadline
   ) external payable nonReentrant returns (uint256) {
       //....
   }

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

   function deposit(
       uint256 _amountIn,
       uint256 _minOut,
       uint256 _deadline
   ) external nonReentrant returns (uint256) {
       //....
   }

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

3. If the deposited amount on L2 is too low so that when it is bridged to L1, minted amount of ezETH becomes zero, it will fail. This can happen if between the deposit on L2 and bridging to L1, the TVL increases while total supply of ezETH does not change. By doing so, each ezETH is worth more than before. So, when that small amount of WETH is going to be deposited into the protocol on L1, it mints less ezETH than the amount of xezETH minted on L2. If it is zero, it will revert.

   function xReceive(
       bytes32 _transferId,
       uint256 _amount,
       address _asset,
       address _originSender,
       uint32 _origin,
       bytes memory
   ) external nonReentrant returns (bytes memory) {
       // ...
       restakeManager.depositETH{ value: ethAmount }();
       //....
   }

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

   function deposit(
       IERC20 _collateralToken,
       uint256 _amount,
       uint256 _referralId
   ) public nonReentrant notPaused {
       //....
       uint256 ezETHToMint = renzoOracle.calculateMintAmount(
           totalTVL,
           collateralTokenValue,
           ezETH.totalSupply()
       );
       //....
   }

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

function calculateMintAmount(
        uint256 _currentValueInProtocol,
        uint256 _newValueAdded,
        uint256 _existingEzETHSupply
    ) external pure returns (uint256) {
        //....
        if (mintAmount == 0) revert InvalidTokenAmount();
        //....
    }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Oracle/RenzoOracle.sol#L146

Tools Used

Recommended Mitigation Steps

To address these potential failures, it is suggested to:

1. implement error handling in the xReceive function. Specifically, placing the depositETH function call within a try/catch block could help manage these failures. Additionally, only authenticated addresses should be allowed to handle these errors.

2. enforce the deposited amount on L2 to be less than maxDepositTVL.

3. define the pausing mechanism on L2.

4. allow the BrdigeSweepr to define the amount of token to be bridged to L1. By doing so, it can handle the situations where a large amount is deposited on L2.

   function sweep(uint256 _amount) public payable nonReentrant {
   //...
   }

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

Assessed type

Context

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[alcueca]

Mitigation of #372 is good as well.

[blckhv]

Hey @alcueca,

I think this issue should be of low severity because we can see that xRenzoBridge has functions to retrieve the funds that are "stuck", which is exactly what the first paragraph of the provided Connext documentation advises - https://docs.connext.network/developers/guides/handling-failures#reverts-on-receiver-contract. In all matters, manually distributing failed transactions funds is not an optimal solution, but in the end, funds are not really stuck, since the admin can still process them from the Connext directly:

Thanks!

[s1n1st3r01]

Hey @alcueca

https://github.com/code-423n4/2024-04-renzo-findings/issues/287 should be marked as a duplicate of this as well.

---

@blckhv I'm afraid this is factually incorrect. Connext allows you to re-submit / retry a failed TX only if it failed due to insufficient relayer fee (which is what the sponsor said in the screenshot).

In the case it failed due to the xReceive() on the destination chain reverting, then the funds will be stuck. Not to mention that this failure is silent. Meaning that admins may be notified that this call failed on the destination chain very late.

That's why the Connext docs says that the IXReceiver contract should be implemented defensively.

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. To avoid situations where user funds get stuck on the receivers, developers should build any contract implementing IXReceive defensively.

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

Bridging silently failing + admins having to manually recover funds and manually redistribute to other parts of the system is surely Medium severity worthy.

[JeffCX]

Hey @alcueca

287 should be marked as a duplicate of this as well.

@blckhv I'm afraid this is factually incorrect. Connext allows you to re-submit / retry a failed TX only if it failed due to insufficient relayer fee (which is what the sponsor said in the screenshot).

In the case it failed due to the xReceive() on the destination chain reverting, then the funds will be stuck. Not to mention that this failure is silent. Meaning that admins may be notified that this call failed on the destination chain very late.

That's why the Connext docs says that the IXReceiver contract should be implemented defensively.

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. To avoid situations where user funds get stuck on the receivers, developers should build any contract implementing IXReceive defensively.

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

Bridging silently failing + admins having to manually recover funds and manually redistribute to other parts of the system is surely Medium severity worthy.

Agree with this comments

my submission explains that there are several revert conditions

https://github.com/code-423n4/2024-04-renzo-findings/issues/374