Renzo - fyamf's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L326-L335

Vulnerability details

Impact

An incorrect implementation of the getTokenBalanceFromStrategy function in the OperatorDelegator contract can result in inaccurate TVL (Total Value Locked) calculation, which opens up an opportunity for a sandwich attack.

Proof of Concept

The calculateTVLs function in the RestakeManager contract calculates TVL by calling the getTokenBalanceFromStrategy function for each operator delegator.

 function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
    //......
    uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
        collateralTokens[j]
    );
    //......
 }

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

The getTokenBalanceFromStrategy function checks if queuedShares[address(this)] is zero. If zero, it only considers the underlying token amount from the amount of shares. If not, it considers both the underlying token amount from the amount of shares and queued withdrawal shares.

    /// @dev Gets the underlying token amount from the amount of shares + queued withdrawal shares
    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)]
                    );
    }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L326-L335

The issue arises from using queuedShares[address(this)], which is always zero. This should be replaced with queuedShares[token].

Sandwich Attack Opportunity:

1. The attacker deposits into the protocol when there is ERC20 withdrawal request (but not yet completed). In this case, TVL will not takes into account the queued withdrawal value.
2. Admin calls completeQueuedWithdrawal to complete the queued withdrawal. This causes that the queued ERC20 be trasnferred to WithdrawQueue to fill the buffer, and the remaining will be deposited into the strategy again. All in all, this leads to increase in the TVL, because in the calculation of TVL, both the ERC20 balance of WithdrawQueue and ERC20 deposited in the strategy are taken into account.
3. The attacker requests to withdraw by burning his ezETH balance. Since, TVL is now increased, each ezETH is worthier than before. So, the attacker gains by receiving higher underlying token than before.

/**
  function queueWithdrawals(
      IERC20[] calldata tokens,
      uint256[] calldata tokenAmounts
  ) external nonReentrant onlyNativeEthRestakeAdmin returns (bytes32) {
      // record gas spent
      uint256 gasBefore = gasleft();
      if (tokens.length != tokenAmounts.length) revert MismatchedArrayLengths();
      IDelegationManager.QueuedWithdrawalParams[]
          memory queuedWithdrawalParams = new IDelegationManager.QueuedWithdrawalParams[](1);
      // set strategies legth for 0th index only
      queuedWithdrawalParams[0].strategies = new IStrategy[](tokens.length);
      queuedWithdrawalParams[0].shares = new uint256[](tokens.length);

      // Save the nonce before starting the withdrawal
      uint96 nonce = uint96(delegationManager.cumulativeWithdrawalsQueued(address(this)));

      for (uint256 i; i < tokens.length; ) {
          if (address(tokens[i]) == IS_NATIVE) {
              // set beaconChainEthStrategy for ETH
              queuedWithdrawalParams[0].strategies[i] = eigenPodManager.beaconChainETHStrategy();

              // set shares for ETH
              queuedWithdrawalParams[0].shares[i] = tokenAmounts[i];
          } else {
              if (address(tokenStrategyMapping[tokens[i]]) == address(0))
                  revert InvalidZeroInput();

              // set the strategy of the token
              queuedWithdrawalParams[0].strategies[i] = tokenStrategyMapping[tokens[i]];

              // set the equivalent shares for tokenAmount
              queuedWithdrawalParams[0].shares[i] = tokenStrategyMapping[tokens[i]]
                  .underlyingToSharesView(tokenAmounts[i]);
          }

          // set withdrawer as this contract address
          queuedWithdrawalParams[0].withdrawer = address(this);

          // track shares of tokens withdraw for TVL
          queuedShares[address(tokens[i])] += queuedWithdrawalParams[0].shares[i];
          unchecked {
              ++i;
          }
      }

      // queue withdrawal in EigenLayer
      bytes32 withdrawalRoot = delegationManager.queueWithdrawals(queuedWithdrawalParams)[0];
      // Emit the withdrawal started event
      emit WithdrawStarted(
          withdrawalRoot,
          address(this),
          delegateAddress,
          address(this),
          nonce,
          block.number,
          queuedWithdrawalParams[0].strategies,
          queuedWithdrawalParams[0].shares
      );

      // update the gas spent for RestakeAdmin
      _recordGas(gasBefore);

      return withdrawalRoot;
  }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L186-L256

   function completeQueuedWithdrawal(
       IDelegationManager.Withdrawal calldata withdrawal,
       IERC20[] calldata tokens,
       uint256 middlewareTimesIndex
   ) external nonReentrant onlyNativeEthRestakeAdmin {
       uint256 gasBefore = gasleft();
       if (tokens.length != withdrawal.strategies.length) revert MismatchedArrayLengths();

       // complete the queued withdrawal from EigenLayer with receiveAsToken set to true
       delegationManager.completeQueuedWithdrawal(withdrawal, tokens, middlewareTimesIndex, true);

       IWithdrawQueue withdrawQueue = restakeManager.depositQueue().withdrawQueue();
       for (uint256 i; i < tokens.length; ) {
           if (address(tokens[i]) == address(0)) revert InvalidZeroInput();

           // deduct queued shares for tracking TVL
           queuedShares[address(tokens[i])] -= withdrawal.shares[i];

           // check if token is not Native ETH
           if (address(tokens[i]) != IS_NATIVE) {
               // Check the withdraw buffer and fill if below buffer target
               uint256 bufferToFill = withdrawQueue.getBufferDeficit(address(tokens[i]));

               // get balance of this contract
               uint256 balanceOfToken = tokens[i].balanceOf(address(this));
               if (bufferToFill > 0) {
                   bufferToFill = (balanceOfToken <= bufferToFill) ? balanceOfToken : bufferToFill;

                   // update amount to send to the operator Delegator
                   balanceOfToken -= bufferToFill;

                   // safe Approve for depositQueue
                   tokens[i].safeApprove(address(restakeManager.depositQueue()), bufferToFill);

                   // fill Withdraw Buffer via depositQueue
                   restakeManager.depositQueue().fillERC20withdrawBuffer(
                       address(tokens[i]),
                       bufferToFill
                   );
               }

               // Deposit remaining tokens back to eigenLayer
               if (balanceOfToken > 0) {
                   _deposit(tokens[i], balanceOfToken);
               }
           }
           unchecked {
               ++i;
           }
       }

       // emits the Withdraw Completed event with withdrawalRoot
       emit WithdrawCompleted(
           delegationManager.calculateWithdrawalRoot(withdrawal),
           withdrawal.strategies,
           withdrawal.shares
       );
       // record current spent gas
       _recordGas(gasBefore);
   }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L258-L324

Tools Used

Recommended Mitigation Steps

The following change is needed:

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

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

If the rate of ezETH changes on L1 during the time the deposited amount on L2 is being bridged to L1, there will be a difference between the quantity of xezETH minted on L2 and the quantity of ezETH locked in XERC20Lockbox on L1. This can happen by changing the TVL while total supply is remained constant, like: donation, or rewards being added. This can provide an opportunity for stealing fund from the protocol.

Proof of Concept

When the function sendPrice on L1 is called by PriceFeedSender, the price feed is sent to xRenzoDeposit to update the price on L2.`

function sendPrice(
        CCIPDestinationParam[] calldata _destinationParam,
        ConnextDestinationParam[] calldata _connextDestinationParam
    ) external payable onlyPriceFeedSender nonReentrant {
        // call getRate() to get the current price of ezETH
        uint256 exchangeRate = rateProvider.getRate();
        bytes memory _callData = abi.encode(exchangeRate, block.timestamp);
        // send price feed to renzo CCIP receivers
        for (uint256 i = 0; i < _destinationParam.length; ) {
            Client.EVM2AnyMessage memory evm2AnyMessage = Client.EVM2AnyMessage({
                receiver: abi.encode(_destinationParam[i]._renzoReceiver), // ABI-encoded xRenzoDepsot contract address
                data: _callData, // ABI-encoded ezETH exchange rate with Timestamp
                tokenAmounts: new Client.EVMTokenAmount[](0), // Empty array indicating no tokens are being sent
                extraArgs: Client._argsToBytes(
                    // Additional arguments, setting gas limit
                    Client.EVMExtraArgsV1({ gasLimit: 200_000 })
                ),
                // Set the feeToken  address, indicating LINK will be used for fees
                feeToken: address(linkToken)
            });

            // Get the fee required to send the message
            uint256 fees = linkRouterClient.getFee(
                _destinationParam[i].destinationChainSelector,
                evm2AnyMessage
            );

            if (fees > linkToken.balanceOf(address(this)))
                revert NotEnoughBalance(linkToken.balanceOf(address(this)), fees);

            // approve the Router to transfer LINK tokens on contract's behalf. It will spend the fees in LINK
            linkToken.approve(address(linkRouterClient), fees);

            // Send the message through the router and store the returned message ID
            bytes32 messageId = linkRouterClient.ccipSend(
                _destinationParam[i].destinationChainSelector,
                evm2AnyMessage
            );

            // Emit an event with message details
            emit MessageSent(
                messageId,
                _destinationParam[i].destinationChainSelector,
                _destinationParam[i]._renzoReceiver,
                exchangeRate,
                address(linkToken),
                fees
            );
            unchecked {
                ++i;
            }
        }

        // send price feed to renzo connext receiver
        for (uint256 i = 0; i < _connextDestinationParam.length; ) {
            connext.xcall{ value: _connextDestinationParam[i].relayerFee }(
                _connextDestinationParam[i].destinationDomainId,
                _connextDestinationParam[i]._renzoReceiver,
                address(0),
                msg.sender,
                0,
                0,
                _callData
            );

            emit ConnextMessageSent(
                _connextDestinationParam[i].destinationDomainId,
                _connextDestinationParam[i]._renzoReceiver,
                exchangeRate,
                _connextDestinationParam[i].relayerFee
            );

            unchecked {
                ++i;
            }
        }
    }

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

    function updatePrice(uint256 _price, uint256 _timestamp) external override {
        if (msg.sender != receiver) revert InvalidSender(receiver, msg.sender);
        _updatePrice(_price, _timestamp);
    }

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

Moreover, when a deposit is made on L2, the xezETH is minted based on the recent rate updated on L2.

function _deposit(
        uint256 _amountIn,
        uint256 _minOut,
        uint256 _deadline
    ) internal returns (uint256) {
        //...

        // Fetch price and timestamp of ezETH from the configured price feed
        (uint256 _lastPrice, uint256 _lastPriceTimestamp) = getMintRate();

        //...

        // Calculate the amount of xezETH to mint - assumes 18 decimals for price and token
        uint256 xezETHAmount = (1e18 * amountOut) / _lastPrice;

        //...

        // Mint xezETH to the user
        IXERC20(address(xezETH)).mint(msg.sender, xezETHAmount);

        //...
    }

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

Then, the deposited amount (which is traded from WETH to nextWETH on L2) is bridged to L1 through calling the function sweep by the allowedBridgeSweepers.

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

        // If there is no balance, return
        if (balance == 0) {
            revert InvalidZeroOutput();
        }

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

        // send collected bridge fee to sweeper
        _recoverBridgeFee();

        // Emit the event
        emit BridgeSwept(bridgeDestinationDomain, bridgeTargetAddress, msg.sender, balance);
    }

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

On L1, the bridged WETH by Connext is deposited in RestakeManager to mint ezETH. The amount of minted ezETH is based on the current rate of ezETH on L1. This amount of ezETH is deposited in XERC20Lockbox, and 1:1 xezETH will be minted. Then, the minted xezETH will be burned, because the xezETH is already minted on L2 to the user.

function xReceive(
        bytes32 _transferId,
        uint256 _amount,
        address _asset,
        address _originSender,
        uint32 _origin,
        bytes memory
    ) external nonReentrant returns (bytes memory) {
        // Only allow incoming messages from the Connext contract
        if (msg.sender != address(connext)) {
            revert InvalidSender(address(connext), msg.sender);
        }

        // Check that the token received is wETH
        if (_asset != address(wETH)) {
            revert InvalidTokenReceived();
        }

        // Check that the amount sent is greater than 0
        if (_amount == 0) {
            revert InvalidZeroInput();
        }

        // Get the balance of ETH before the withdraw
        uint256 ethBalanceBeforeWithdraw = address(this).balance;

        // Unwrap the WETH
        IWeth(address(wETH)).withdraw(_amount);

        // Get the amount of ETH
        uint256 ethAmount = address(this).balance - ethBalanceBeforeWithdraw;

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

        // Approve the lockbox to spend the ezETH
        ezETH.safeApprove(address(xezETHLockbox), ezETHAmount);

        // Get the xezETH balance before the deposit
        uint256 xezETHBalanceBeforeDeposit = xezETH.balanceOf(address(this));

        // Send to the lockbox to be wrapped into xezETH
        xezETHLockbox.deposit(ezETHAmount);

        // Get the amount of xezETH that was minted
        uint256 xezETHAmount = xezETH.balanceOf(address(this)) - xezETHBalanceBeforeDeposit;

        // Burn it - it was already minted on the L2
        IXERC20(address(xezETH)).burn(address(this), xezETHAmount);

        // Emit the event
        emit EzETHMinted(_transferId, _amount, _origin, _originSender, ezETHAmount);

        // Return 0 for success
        bytes memory returnData = new bytes(0);
        return returnData;
    }

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

If the rate of ezETH changes on L1 during the time the deposited amount on L2 is being bridged to L1, there will be a difference between the quantity of xezETH minted on L2 and the quantity of ezETH locked in XERC20Lockbox on L1.

1. Depositing on L2.
2. Donating on L1, before the amount is bridged to L1.
3. Bridge xezETH to L1, after the amount is bridged to L1.

Attack explanation

1. Suppose no one has deposited in the protocol yet, and the fees are ignored, just for simplicity.
2. Attacker deposits ETH on L1, and receives 1:1 ezETH.
3. Attacker deposits ezETH in XERC20Lockbox to wrap it into xezETH.
4. The sendPrice is called by priceFeedSender on L1 to send the price feed to L2.
5. Attacker deposits ETH on L2, and receives xezETH 1:1 (because the stpe 4 sets the rate of ezETH/eth = 1).
6. Attacker donates ETH to the protocol on L1 to increase the TVL, while the total supply of ezETH does not change. So, on L1, each ezETH worths more ETH than before.
7. The sweep is called on L2 by allowedBridgeSweeper to bridge the deposited amount in step 5 to L1.
8. On L1, when xReceive is called by Connext, the received WETH from the L2 is deposited in RestakeManager and mints ezETH. The amount of minted ezETH is lower than the amount of xezETH minted on L2, because on step 6 the donation increased the TVL, so the received WETH worths less ezETH.
9. The minted ezETH on L1 will be locked in the XERC20Lockbox, and xezETH will be minted 1:1 on L1, and then will be burned assuming the same amount of xezETH is minted on L2.
10. Now, the amount of minted xezETH on L2 is more than the amount of ezETH locked in XERC20Lockbox. This is because the ezETH rate on L2 is lower than the rate on L1. In other words, ezETH on L1 is worthier than ezETH on L2 due to the donation.
11. Attacker bridges the xezETH on L2 to receive ezETH on L1. Since on step 3, some ezETH is wrapped into xezETH, the XERC20Lockbox now has enough ezETH to redeem in exchange of bridged xezETH.
12. Now, the attacker has some ezETH and xezETH on L1.
13. By requesting to withdraw ezETH on RestakeManager, the depoisted ETH on step 2 will be redeemed.
14. Now the attacker has his own amount of ETH as well as some extra xezETH that are not backed with any ezETH.
15. If any user later deposits into the protocol on L1 or L2, resulting to nonzero ezETH balance of XERC20Lockbox, the attacker can steal them by burning his xezETH.

For better understanding, the numerical scenario is explained as follows:

1. Suppose the state is: On L1 Attacker balance: 20 ETH, 0 ezETH, 0 xezETH Protocol: TVL = 0, TotalSupply = 0, XERC20Lockbox = 0 ezETH On L2 Attacker balance: 20 ETH, 0 xezETH Protocol: 0 nextWETH

2. Attacker deposits 10 ETH on L1: On L1 Attacker balance: 10 ETH, 10 ezETH, 0 xezETH Protocol: TVL = 10, TotalSupply = 10, XERC20Lockbox = 0 ezETH On L2 Attacker balance: 20 ETH, 0 xezETH Protocol: 0 nextWETH

3. Attacker deposits 10 ezETH into XERC20Lockbox and receives 10 xezETH: On L1 Attacker balance: 10 ETH, 0 ezETH, 10 xezETH Protocol: TVL = 10, TotalSupply = 10, XERC20Lockbox = 10 ezETH On L2 Attacker balance: 20 ETH, 0 xezETH Protocol: 0 nextWETH

4. The sendPrice is called by priceFeedSender on L1 to send the price feed to L2.

5. Attacker deposits 20 ETH on L2, and receives 20 xezETH. On L1 Attacker balance: 10 ETH, 0 ezETH, 10 xezETH Protocol: TVL = 10, TotalSupply = 10, XERC20Lockbox = 10 ezETH On L2 Attacker balance: 0 ETH, 20 xezETH Protocol: 20 nextWETH

6. Attacker donates 10 ETH on L1. On L1 Attacker balance: 0 ETH, 0 ezETH, 10 xezETH Protocol: TVL = 20, TotalSupply = 10, XERC20Lockbox = 10 ezETH On L2 Attacker balance: 0 ETH, 20 xezETH Protocol: 20 nextWETH

7. The sweep is called on L2 by allowedBridgeSweeper to bridge the deposited amount in step 5 to L1. On L1 Attacker balance: 0 ETH, 0 ezETH, 10 xezETH Protocol: TVL = 20, TotalSupply = 10, XERC20Lockbox = 10 ezETH On L2 Attacker balance: 0 ETH, 20 xezETH Protocol: 0 nextWETH

8. xReceive is called by Connext to execute the bridging on the destination chain, so 20 WETH will be deposited into the protocol. Since, TVL is 20 and total supply is 10, only 10 ezETH will be minted and be locked in XERC20Lockbox. So, the TVL increases to 40, and total supply increases to 20. On L1 Attacker balance: 0 ETH, 0 ezETH, 10 xezETH Protocol: TVL = 40, TotalSupply = 20, XERC20Lockbox = 20 ezETH On L2 Attacker balance: 0 ETH, 20 xezETH Protocol: 0 nextWETH

9. Attacker bridges 20 xezETH on L2 to L1. Connext will transfer these 20 xezETH to XERC20Lockbox on L1 to be burned and redeem 20 ezETH to the attacker. On L1 Attacker balance: 0 ETH, 20 ezETH, 10 xezETH Protocol: TVL = 40, TotalSupply = 20, XERC20Lockbox = 0 ezETH On L2 Attacker balance: 0 ETH, 0 xezETH Protocol: 0 nextWETH

10. Attacker requests to withdraw the underlying token by burning his 20 ezETH. When the withdraw is completed, the attacker receives 40 ETH, because 20*40/20. On L1 Attacker balance: 40 ETH, 0 ezETH, 10 xezETH Protocol: TVL = 0, TotalSupply = 0, XERC20Lockbox = 0 ezETH On L2 Attacker balance: 0 ETH, 0 xezETH Protocol: 0 nextWETH

Now the attacker has his own 40 ETH as well as 10 xezETH extra. So, the attacker can use these extra xezETH to steal from next users.

11. If later an user deposits for example 50 ETH on L2, after the bridging and sweeping are executed, 50 ezETH will be locked in XERC20Lockbox. Now, the attacker burns his 10 xezETH and receives 10 ezETH which can be later burned to withdraw 10 underlying token (Like ETH). So, the attacker now owns 40 ETH as well as 10 stolen ETH.

The main root cause of this issue is that xezETH is minted immediately when depositing on L2 with the recent updated rate. While, when the deposited amount on L2 is bridged to L1, the rate on L1 could be different (for example by maliciously donating into the protocol), so an amount of ezETH locked on L1 will not be equal to the amount of minted xezETH on L2.

This attack is not limited to only first deposits scenarios, it can have critical impact even if the protocol is not in its zero state. For example, suppose the TVL of project and total supply is 100 already.

1. Suppose the state is: On L1 Attacker balance: 12 ETH, 0 ezETH, 0 xezETH Protocol: TVL = 100, TotalSupply = 100, XERC20Lockbox = 0 ezETH On L2 Attacker balance: 200 ETH, 0 xezETH Protocol: 0 nextWETH

2. Attacker deposits 8 ETH on L1: On L1 Attacker balance: 4 ETH, 8 ezETH, 0 xezETH Protocol: TVL = 108, TotalSupply = 108, XERC20Lockbox = 0 ezETH On L2 Attacker balance: 200 ETH, 0 xezETH Protocol: 0 nextWETH

3. Attacker deposits 8 ezETH into XERC20Lockbox and receives 8 xezETH: On L1 Attacker balance: 4 ETH, 0 ezETH, 8 xezETH Protocol: TVL = 108, TotalSupply = 108, XERC20Lockbox = 8 ezETH On L2 Attacker balance: 200 ETH, 0 xezETH Protocol: 0 nextWETH

4. The sendPrice is called by priceFeedSender on L1 to send the price feed to L2.

5. Attacker deposits 200 ETH on L2, and receives 200 xezETH. On L1 Attacker balance: 4 ETH, 0 ezETH, 8 xezETH Protocol: TVL = 108, TotalSupply = 108, XERC20Lockbox = 8 ezETH On L2 Attacker balance: 0 ETH, 200 xezETH Protocol: 200 nextWETH

6. Attacker donates 4 ETH on L1. On L1 Attacker balance: 0 ETH, 0 ezETH, 8 xezETH Protocol: TVL = 112, TotalSupply = 108, XERC20Lockbox = 8 ezETH On L2 Attacker balance: 0 ETH, 200 xezETH Protocol: 200 nextWETH

7. The sweep is called on L2 by allowedBridgeSweeper to bridge the deposited amount in step 5 to L1. On L1 Attacker balance: 0 ETH, 0 ezETH, 8 xezETH Protocol: TVL = 112, TotalSupply = 108, XERC20Lockbox = 8 ezETH On L2 Attacker balance: 0 ETH, 200 xezETH Protocol: 0 nextWETH

8. xReceive is called by Connext to execute the bridging on the destination chain, so 200 WETH will be deposited into the protocol. Since, TVL is 112 and total supply is 108, only 192 ezETH will be minted and locked in XERC20Lockbox. So, the TVL increases to 312, and total supply increases to 300. On L1 Attacker balance: 0 ETH, 0 ezETH, 8 xezETH Protocol: TVL = 312, TotalSupply = 300, XERC20Lockbox = 200 ezETH On L2 Attacker balance: 0 ETH, 200 xezETH Protocol: 0 nextWETH

9. Attacker bridges 200 xezETH on L2 to L1. Connext will transfer these 200 xezETH to XERC20Lockbox on L1 to be burned and redeem 200 ezETH to the attacker. On L1 Attacker balance: 0 ETH, 200 ezETH, 8 xezETH Protocol: TVL = 312, TotalSupply = 300, XERC20Lockbox = 0 ezETH On L2 Attacker balance: 0 ETH, 0 xezETH Protocol: 0 nextWETH

10. Attacker requests to withdraw the underlying token by burning his 200 ezETH. When the withdraw is completed, the attacker receives 208 ETH, because 200*312/300 On L1 Attacker balance: 208 ETH, 0 ezETH, 8 xezETH Protocol: TVL = 104, TotalSupply = 100, XERC20Lockbox = 0 ezETH On L2 Attacker balance: 0 ETH, 0 xezETH Protocol: 0 nextWETH

Now the attacker owns 208 ETH as well as 8 xezETH, while the attacker at first had only 212 ETH. So, the attacker can use these 8 xezETH to steal from next users.

11. If later an user deposits for example 50 ETH on L2, after the bridging and sweeping are executed, 48 ezETH will be locked in XERC20Lockbox, because 50*100/104. Now, the attacker burns his 8 xezETH in XERC20Lockbox and receives 8 ezETH which can be burned to withdraw 8 underlying token (Like ETH), because 8*(104+50)/(100+48). Now the attacker owns 208 ETH as well as 8 stolen ETH, in total 216 ETH, while at first he owned 212 ETH. So, the attacker could steal 4 ETH from the protocol.

Tools Used

Recommended Mitigation Steps

Two potential solutions are suggested:

1. One possible soultion is that xezETH should not be minted on L2 immediately. When the deposited amount is bridged to L1, then a message should be sent from L1 to L2 stating that how much ezETH is locked, so the same amount of xezETH should be minted on L2. In other words, delay minting xezETH on L2 until the corresponding amount of ezETH is confirmed to be locked on L1.

2. Second possible solution is that the value of minted xezETH on L2 should be sent to L1 as a message to declare that how much ezETH should be locked on L1 to match the minted xezETH on L2.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Using the wrong index for the collateral token array causes incorrect calculation of TVL or blocks deposits into the protocol.

Proof of Concept

When calculating TVL, the code includes ERC20 token balances from the WithdrawQueue. However, this isn't done correctly. Instead of getting the balance of collateralTokens[j], it uses the price of collateralTokens[i] for calculation. Since the prices of these tokens can differ, this leads to inaccurate calculations.

if (!withdrawQueueTokenBalanceRecorded) {
    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
        collateralTokens[i],
        collateralTokens[j].balanceOf(withdrawQueue)
    );
}

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

Additionally, if the number of operator delegators exceeds the number of collateral tokens, it causes an out-of-bounds error for collateralTokens[i]. As a result, the calculateTVLs function always reverts, preventing any deposits into the protocol.

Tools Used

Recommended Mitigation Steps

Update the code to use the correct index for collateral tokens:

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

Assessed type

Context

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

Lines of code

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

Vulnerability details

Impact

Depositing ERC20 collateral tokens below the withdrawal buffer-to-fill amount results in a revert. This means the deposited amount of collateral tokens must be at least equal to the buffer amount. It's an unexpected situation because the protocol doesn't define a limit for a minimum deposit amount.

Proof of Concept

When depositing collateral tokens into the protocol, it first checks if there's a withdrawal queue buffer to fill. If so, part of the deposited amount is transferred to the DepositQueue and then to the WithdrawQueue. However, if the deposited amount is less than the buffer-to-fill, the entire deposited amount is transferred to the WithdrawQueue, leaving nothing to be transferred to the OperatorDelegator.

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

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

The problem arises because the function deposit in the OperatorDelegator contract expects a non-zero amount. Since the remaining amount after transferring to the WithdrawQueue becomes zero, calling deposit reverts.

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

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L147

Tools Used

Recommended Mitigation Steps

To address this, before calling deposit on the OperatorDelegator contract, ensure that the remaining amount is greater than zero. Only if there's a non-zero amount remaining should it be deposited into the OperatorDelegator. This prevents reverts caused by deposits below the buffer-to-fill amount.

Modify the deposit function to check if the remaining amount is greater than zero before calling deposit on the OperatorDelegator:

function deposit(
    IERC20 _collateralToken,
    uint256 _amount,
    uint256 _referralId
) public nonReentrant notPaused {
    //....

    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

Context

Lines of code

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

Vulnerability details

Impact

Failure to store the fetched price from the oracle in the storage variables of the xRenzoDeposit contract can result in several important issues.

Proof of Concept

Price Fetching on Deposit: When a deposit is made in xRenzoDeposit, the getMintRate function fetches the price of ezETH.

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

    function _deposit(
        uint256 _amountIn,
        uint256 _minOut,
        uint256 _deadline
    ) internal returns (uint256) {
        //......
        // Fetch price and timestamp of ezETH from the configured price feed
        (uint256 _lastPrice, uint256 _lastPriceTimestamp) = getMintRate();
        //......
    }

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

Oracle vs. Storage: If an oracle is defined, the function retrieves the price from the oracle. Otherwise, it reads the price from the storage variable lastPrice, which is updated by the updatePrice function.

    function getMintRate() public view returns (uint256, uint256) {
        // revert if PriceFeedNotAvailable
        if (receiver == address(0) && address(oracle) == address(0)) revert PriceFeedNotAvailable();
        if (address(oracle) != address(0)) {
            (uint256 oraclePrice, uint256 oracleTimestamp) = oracle.getMintRate();
            return
                oracleTimestamp > lastPriceTimestamp
                    ? (oraclePrice, oracleTimestamp)
                    : (lastPrice, lastPriceTimestamp);
        } else {
            return (lastPrice, lastPriceTimestamp);
        }
    }

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

    function updatePrice(uint256 _price, uint256 _timestamp) external override {
        if (msg.sender != receiver) revert InvalidSender(receiver, msg.sender);
        _updatePrice(_price, _timestamp);
    }

    function _updatePrice(uint256 _price, uint256 _timestamp) internal {
        //.....

        lastPrice = _price;
        lastPriceTimestamp = _timestamp;

        //.....
    }

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

First Issue Inaccurate Price Usage: The first issue arises when the oracle is used to fetch the price but is not stored in the storage variables. Subsequent calls to getMintRate after the oracle is undefined may use the outdated price from storage instead of the most recent one fetched from the oracle. This leads to inaccurate pricing and potential financial losses. For example:

1. Suppose, the price is updated to 1 ether now, so the storage variables are lastPrice = 1 ether and lastPriceTimestamp = k (k is the timestamp of now)
2. Now, the oracle is defined by owner.
3. 3 hours later, a deposit is made, so getMintRate uses the oracle to fetch the price and returns the values from the oracle. For example the oracle returns oraclePrice = 1.02 ether and oracleTimestamp = k + 3 hours.
4. The owner sets the orcale undefined again by setting its address to zero.
5. After 2 hours, another deposit is made. So, getMintRate uses the storage variable again (because the oracle address is zero) to fetch the price and reads the storage variables. So, it returns lastPrice = 1 ether and lastPriceTimestamp = k, instead of the recent fetched price by the oracle 1.02 ether and k + 3 hours.

It shows that although the recent fetched price and timestamp is 1.02 ether and k + 3 hours, respectively, getMintRate returns the price of the time when they were stored in the storage variables during updating the price about 5 hourse before.

Second Issue Divergence Check: Another issue is that when the price is updated, the to-be-updated price is compared with the lastPrice. If, the oracle is used to fetch the price in between the updates, since the fetched price from the oracle is not stored in lastPrice variable, the to-be-updated price is compared with price stored in the storage variable before the oracle fetch. This undermines the trustworthiness of the divergence check mechanism.

function _updatePrice(uint256 _price, uint256 _timestamp) internal {
    //....
    
    // Check for price divergence - more than 10%
        if (
            (_price > lastPrice && (_price - lastPrice) > (lastPrice / 10)) ||
            (_price < lastPrice && (lastPrice - _price) > (lastPrice / 10))
        ) {
            revert InvalidOraclePrice();
        }

    //...
}

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

Third Issue Manual Price Updates: The third issue arises when the price update fails due to inaccurate divergence checks. For example:

1. Suppose the oracle is already defined and the price is updated to 1 ether now, so: lastPrice = 1 ether and lastPriceTimestamp = k (k is the timestamp now)
2. After 5 hours, a deposit is made, so getMintRate uses the oracle to fetch the price and returns the values from the oracle. For example: oraclePrice = 1.11 ether and oracleTimestamp = k + 5 hours.
3. After 1 hour, again the update price is called (because the on-chain transactions show that the interval for updates is 6 hours) to update the price. Suppose, this to-be-update price is 1.12 ether. But, since it compares 1.12 ether with 1 ether at time k, its divergence is %12 percent, and it will revert. While, in reality, the divergence is between 1.12 ether and 1.11 ether (the recent price fetched from the oracle) which is %0.9.

In such cases, manual intervention is required by the admin to update the price, which adds complexity and overhead.

function updatePriceByOwner(uint256 price) external onlyOwner {
       return _updatePrice(price, block.timestamp);
   }

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

Tools Used

Recommended Mitigation Steps

To address these issues, it's recommended to modify the getMintRate function to store the fetched price from the oracle in the storage variables. This ensures that the most recent price is used for calculations.

function getMintRate() public view returns (uint256, uint256) {
        // revert if PriceFeedNotAvailable
        if (receiver == address(0) && address(oracle) == address(0)) revert PriceFeedNotAvailable();
        if (address(oracle) != address(0)) {
            (uint256 oraclePrice, uint256 oracleTimestamp) = oracle.getMintRate();
            if(oracleTimestamp > lastPriceTimestamp){
                lastPrice = oraclePrice;
                lastPriceTimestamp = oracleTimestamp;
                return (oraclePrice, oracleTimestamp);
            } else {
                return (lastPrice, lastPriceTimestamp);
            }
        } else {
            return (lastPrice, lastPriceTimestamp);
        }
    }

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L283-L289

Vulnerability details

Impact

The administrator has the ability to drain the ETH balance of the DepositQueue contract by calling certain functions with a high gas price. This can lead to:

• TVL Waste: Draining the contract's balance wastes the Total Value Locked (TVL) in the protocol.
• Lower Token Value: This imbalance can lower the value of the ezETH token (even less than 1 ETH).
• Deficit in Operator Delegator Balance: Insufficient balance can lead to a deficit in supplying operator delegators, affecting protocol operations.

Proof of Concept

1. Gas Refund Mechanism: When the administrator, with the role NATIVE_ETH_RESTAKE_ADMIN, calls the stakeEthFromQueue or stakeEthFromQueueMulti functions, any consumed gas is refunded to the msg.sender.

    function stakeEthFromQueue(
        IOperatorDelegator operatorDelegator,
        bytes calldata pubkey,
        bytes calldata signature,
        bytes32 depositDataRoot
    ) external onlyNativeEthRestakeAdmin {
        uint256 gasBefore = gasleft();
        // Send the ETH and the params through to the restake manager
        restakeManager.stakeEthInOperatorDelegator{ value: 32 ether }(
            operatorDelegator,
            pubkey,
            signature,
            depositDataRoot
        );

        emit ETHStakedFromQueue(operatorDelegator, pubkey, 32 ether, address(this).balance);

        // Refund the gas to the Admin address if enough ETH available
        _refundGas(gasBefore);
    }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L187-L206

    function stakeEthFromQueueMulti(
        IOperatorDelegator[] calldata operatorDelegators,
        bytes[] calldata pubkeys,
        bytes[] calldata signatures,
        bytes32[] calldata depositDataRoots
    ) external onlyNativeEthRestakeAdmin nonReentrant {
        uint256 gasBefore = gasleft();
        // Verify all arrays are the same length
        if (
            operatorDelegators.length != pubkeys.length ||
            operatorDelegators.length != signatures.length ||
            operatorDelegators.length != depositDataRoots.length
        ) revert MismatchedArrayLengths();

        // Iterate through the arrays and stake each one
        uint256 arrayLength = operatorDelegators.length;
        for (uint256 i = 0; i < arrayLength; ) {
            // Send the ETH and the params through to the restake manager
            restakeManager.stakeEthInOperatorDelegator{ value: 32 ether }(
                operatorDelegators[i],
                pubkeys[i],
                signatures[i],
                depositDataRoots[i]
            );

            emit ETHStakedFromQueue(
                operatorDelegators[i],
                pubkeys[i],
                32 ether,
                address(this).balance
            );

            unchecked {
                ++i;
            }
        }

        // Refund the gas to the Admin address if enough ETH available
        _refundGas(gasBefore);
    }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L211-L250

2. Gas Refund Source: The refunded gas is paid from the balance of the DepositQueue contract and is calculated based on (initialGas - gasleft()) * tx.gasprice.

    function _refundGas(uint256 initialGas) internal {
        uint256 gasUsed = (initialGas - gasleft()) * tx.gasprice;
        uint256 gasRefund = address(this).balance >= gasUsed ? gasUsed : address(this).balance;
        (bool success, ) = payable(msg.sender).call{ value: gasRefund }("");
        if (!success) revert TransferFailed();
        emit GasRefunded(msg.sender, gasRefund);
    }

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L283-L289

3. Risk Scenario: If the admin sets a very high tx.gasprice, it can lead to a substantial drain on the contract's balance. For example the transaction https://etherscan.io/tx/0xc5ae7b766c0186590d4baf648a17b1e59f96c01a540e3b5aee675e87a882555f shows that calling the function stakeEthFromQueueMulti for two operator delegators consumes almost 286745 gas. If the admin sets the gas price in the transation to 1000 Gwei, almost 0.287 ETH will be wasted from the contract. The admin does not gain financially, but it can adversly impacts on the protocol.

Tools Used

Recommended Mitigation Steps

Modify the _refundGas function to allocate only a small percentage of the contract's balance for gas refunds:

function _refundGas(uint256 initialGas) internal {
        uint256 gasUsed = (initialGas - gasleft()) * tx.gasprice;

        uint256 refundPool = address(this).balance / 1000; // %0.1 of the balance is dedicated to the gas refund
        uint256 gasRefund = refundPool >= gasUsed ? gasUsed : refundPool;

        (bool success, ) = payable(msg.sender).call{ value: gasRefund }("");
        if (!success) revert TransferFailed();
        emit GasRefunded(msg.sender, gasRefund);
    }

Assessed type

Context