Honest users can lose funds when calling the `claim()` function.

[c4-bot-8]

Lines of code

https://github.com/Renzo-Protocol/Contracts/blob/c5d62fc0319fca154f6e0e1c16c6c532f861e1c7/contracts/Withdraw/WithdrawQueue.sol#L290-L293

Vulnerability details

Original Issue Summary

• H-04: Withdrawals logic allows MEV exploits of TVL changes and zero-slippage zero-fee swaps

In the original issue, three scenarios are explained where MEV and/or malicious actors can exploit the fact that the withdrawal amount is calculated at withdrawal request submission time instead of at withdrawal claim time. Any small deviation of the amount from withdraw to claim can be exploited by MEV.

Mitigation

• Mitigation H-04

This mitigation proposes calculating the redeem value at the withdraw() function and at the claim() function.

Comments

This mitigation introduces another issue that harms honest users, which we will describe in the next few paragraphs.

Newly Introduced Vulnerability Description

The mitigation proposes the creation of the following function:

    function _calculateAmountToRedeem(
        uint256 _amount,
        address _assetOut
    ) internal view returns (uint256 _amountToRedeem) {
        // calculate totalTVL
        (, , uint256 totalTVL) = restakeManager.calculateTVLs();

        // Calculate amount to Redeem in ETH
        _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
            );
        }

and calling this function twice instead of once. _calculateAmountToRedeem() is called once during the withdraw() call and once during the claim() call. This mitigation aims to mitigate MEV/large arb opportunties.

However, this mitigation will introduce a problem which will impact honest users and make them lose (a portion of) their funds.

Proof of Concept

Assume Alice is a honest user.

• Alice calls withdraw() with 100e18 ezETH.
  • The amount that Alice should receive is calculated by calling the newly added _calculateAmountToRedeem() function.
• Due to the mitigation, when Alice calls claim(), _calculateAmountToRedeem() is called again.
  • Here lays the problem. Inside _calculateAmountToRedeem(), the amount to redeem gets calculated using the current total TVL by calling restakeManager.calculateTVLs().
  • However, this total TVL can be drastically different from the TVL that Alice initially calculated during the withdraw() call. Note that this does not need the interference of a malicious party for the TVL to drop. restakeManager.calculateTVLs() uses multiple factors to calculate the TVL and for example a newly added OperatorDelegator or a removed OperatorDelegator can heavily skew the TVL and reduce the amount that Alice can claim.
• Then, the following check is done inside the claim() function:

  // update withdraw request amount to redeem if lower at claim time.
      if (claimAmountToRedeem < _withdrawRequest.amountToRedeem) {
          _withdrawRequest.amountToRedeem = claimAmountToRedeem;
      }

• This is where Alice would lose funds. Let's assume Alice got 100e18 ETH for 100e18 ezETH during the withdraw(), but due to circumstances beyond her control, the TVL has dropped and she will now receive 50e18 ETH. Now, since (50e18 < 100e18) == true, the initial withdraw redeem amount will be overridden to 50e18, marking a loss of 50e18 for Alice.

Tools used

Manual Review

Recommended Mitigation Steps

Do not allow huge fluctuation that negatively impact honest users.

Assessed type

Timing

[c4-judge]

alcueca marked the issue as duplicate of #27

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid