Skip to content

Latest commit

 

History

History
888 lines (637 loc) · 37.5 KB

2024-05-Renzo.md

File metadata and controls

888 lines (637 loc) · 37.5 KB
Raw

Findings Summary

ID Description Severity
H-01 OperatorDelegator::getTokenBalanceFromStrategy never account queuedShares High
H-02 Eigen rewards can be sandwiched to earn more ezETH High
H-03 WithdrawQueue::claim rebasing tokens can block the last claim temporarily High
H-04 RestakeManager::calculateTvls always account withdraw queue balance for wrong token High
M-01 No slippage in RestakeManger::deposit and WithdrawQueue::withdraw Medium
M-02 RestakeManager::deposit will revert always Medium
M-03 Possible arbitrage due to the long heartbeat of stETH/ETH price feed Medium
L-01 WithdrawQueue::claim should expect the recipient Low
L-02 No way to cancel a withdrawal request Low
L-03 chooseOperatorDelegatorForWithdraw should fetch the TVL Low
L-04 Inaccuracy of shares between Eigen withdrawal request and claim Low

[H-01] OperatorDelegator::getTokenBalanceFromStrategy never account queuedShares

Impact

Due to a wrong token check in the getTokenBalanceFromStrategy, RestakeManager::calculateTVLs will return less balance when there are queued withdrawals.

Proof of Concept

RestakeManager::calculateTVLs is used to calculate the TVL of OperatorDelegator, divided into 3 separate variables:

  • by individual collateral tokens
  • a sum of all collateral tokens
  • total for the protocol (previous 2 + withdraw and deposit queue balances).

For each operator getTokenBalanceFromStrategy is called and the returned amount of this function is added to all 3 sources mentioned above:

OperatorDelegator.sol#L326-L335

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

From the snippet we can see that queuedShares, which is responsible for tracking the queued withdrawals from EigenLayer for given token, uses address(this) instead.

Important note is that there is no place in the code which increases this particular key of the mapping and it will always be 0, even when there are active queued withdrawals.

As result the above mentioned function will not account the queued shares for all tokens and will return highly reduced balance.

Then when depositing 3 major issues will be observed:

  1. In deposit tokenTVL limit will be bypassed indicating more withdraw needs.
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);

    // Get the TVLs for each operator delegator and the total TVL
    (
        uint256[][] memory operatorDelegatorTokenTVLs,
        uint256[] memory operatorDelegatorTVLs,
        uint256 totalTVL
    ) = calculateTVLs();

    // Get the value of the collateral token being deposited
    uint256 collateralTokenValue = renzoOracle.lookupTokenValue(_collateralToken, _amount);

    // Enforce TVL limit if set, 0 means the check is not enabled
    if (maxDepositTVL != 0 && totalTVL + collateralTokenValue > maxDepositTVL) {
        revert MaxTVLReached();
    }

    // Enforce individual token TVL limit if set, 0 means the check is not enabled
    if (collateralTokenTvlLimits[_collateralToken] != 0) {
        // Track the current token's TVL
        uint256 currentTokenTVL = 0;

        // For each OD, add up the token TVLs
        uint256 odLength = operatorDelegatorTokenTVLs.length;
        for (uint256 i = 0; i < odLength; ) {
            currentTokenTVL += operatorDelegatorTokenTVLs[i][tokenIndex];
            unchecked {
                ++i;
            }

        // Check if it is over the limit
        if (currentTokenTVL + collateralTokenValue > collateralTokenTvlLimits[_collateralToken])//@audit limit will be filled slower
            revert MaxTokenTVLReached();
    }
  1. totalTVL when depositing in any of the available collateral tokens will be way less than the actual balance, minting more ezETH tokens than needed.

RestakeManager.sol#L592-L616

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

  // Calculate how much ezETH to mint
  uint256 ezETHToMint = renzoOracle.calculateMintAmount(
      totalTVL, //@audit wrong value used
      msg.value,
      ezETH.totalSupply()
  );

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

  // Emit the deposit event
  emit Deposit(msg.sender, IERC20(address(0x0)), msg.value, ezETHToMint, _referralId);
}

Let’s review two examples, one with the right totalTVL and one when half of it is queued for withdrawal from EigenLayer and deposited amount 10e18:

For sake of simplicity we will assume that ezETH and ETH price to be at 1:1 ratio.

RenzoOracle.sol#L123-L149

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

Right calculation:

  • totalTVL/_currentValueInProtocol - 1000e18
  • _newValueAdded - 10e18
  • ezETH.totalSupply/_existingEzETHSupply - 1000e18

Then we receive:

  • inflationPercentage - 9900990099009900
  • newEzETHSupply - 1009999999999999998990 ≈ 1009e18
  • mintAmount - 9999999999999998990 ≈ 9e18

User is minted 9 ezETH for 10 tokens deposit.

Wrong calculation:

  • totalTVL/_currentValueInProtocol - 500e18 (as we mentioned half of the totalTVL is queued for withdrawal from EigenLayer and wrongly won’t be added to this calculation)
  • _newValueAdded - 10e18
  • ezETH.totalSupply/_existingEzETHSupply - 1000e18

Then we receive:

  • inflationPercentage - 19607843137254901
  • newEzETHSupply - 1019999999999999999000 ≈ 1019e18
  • mintAmount - 19999999999999999000 ≈ 19e18

User is minted 19 ezETH for 10 tokens deposit.

After the actual withdrawal from EigenLayer is executed, queuedShares will be zeroed, buffer will be filled (if needed) and the rest will be redeposited in the Strategy, totalTVL will now return the real value held in the protocol.

Then user can redeem his 19 ezETH tokens and get 19 ETH back:

RenzoOracle.sol#L152-L164

  function calculateRedeemAmount(
      uint256 _ezETHBeingBurned,
      uint256 _existingEzETHSupply,
      uint256 _currentValueInProtocol
  ) external pure returns (uint256) {
      // This is just returning the percentage of TVL that matches the percentage of ezETH being burned
      uint256 redeemAmount = (_currentValueInProtocol * _ezETHBeingBurned) / _existingEzETHSupply;

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

      return redeemAmount;
  }
  • redeemAmount = (1010e18 * 19999999999999999000) / 1019999999999999999000 ≈ 19e18

In summary, he deposited 10 ETH when there were pending withdrawal and redeemed roughly 19 ETH back when withdrawal from EigenLayer has been completed.

  1. When picking operatorDelegator in selectOperatorDelegatorForDeposit(), it will pick the wrong one because of the lowered totalTVL.

Tools Used

Manual Review

Recommended Mitigation Steps

Replace address(this) with the token that is being verified.

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

[H-02] Eigen rewards can be sandwiched to earn more ezETH

Impact

Native rewards initiated from OperatorDelegator::verifyAndProcessWithdrawals and OperatorDelegator::withdrawNonBeaconChainETHBalanceWei can be sandwiched from users to steal assets when the exchange rate of ezETH increases.

Proof of Concept

The protocol handles ETH rewards by sending them to the OperatorDelegator’s. There are at least 3 flows that end up sending funds there:

  1. When the function withdrawNonBeaconChainETHBalanceWei is called to scrape non-beacon chain ETH from an Eigenpod.
  2. When a validator receives rewards via partial withdrawals after the function EigenPod::verifyAndProcessWithdrawals is called.
  3. When a validator exists and has more than 32ETH the excess will be sent as rewards after the function EigenPod::verifyAndProcessWithdrawals is called.

All of these 3 flows end up queuing a withdrawal to the OperatorDelegator. After a delay the rewards can claimed by calling the permissionless function DelayedWithdrawalRouter::claimDelayedWithdrawals this call will instantly increase the totalTVL of the protocol.

Following the mint and redeem flows in Renzo we can see that the exchange rate of ezETH is dependent on the totalTVL, which will be increased when rewards are sent to the system:

  • Minting:

RenzoOracle.sol#L123-L149

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

or simplified:

$ezEthToMint = (ezEthSupply * (1 + amtToDeposit / totalTVL))$

  • Redeeming:

RenzoOracle.sol#L152-164

function calculateRedeemAmount(
      uint256 _ezETHBeingBurned,
      uint256 _existingEzETHSupply,
      uint256 _currentValueInProtocol
  ) external pure returns (uint256) {
      // This is just returning the percentage of TVL that matches the percentage of ezETH being burned
      uint256 redeemAmount = (_currentValueInProtocol * _ezETHBeingBurned) / _existingEzETHSupply;

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

      return redeemAmount;
  }

or simplified:

$redeemAmount = totalTVL * ezEthToBurn/ezEthSupply$

An attacker can take advantage of this to steal a part of the rewards:

  1. Mint a sensible amount of ezETH by depositing an accepted asset.
  2. Call DelayedWithdrawalRouter::claimDelayedWithdrawals, after which the value of the ezETHtokens just minted will immediately increase.
  3. Request a withdrawal for all the ezETHtokens via WithdrawQueue::withdraw

Tools Used

Manual Review

Recommended Mitigation Steps

It is hard to give a perfect solution for this issue because it is also affected the EigenLayer and the fact that DelayedWithdrawalRouter::claimDelayedWithdrawals is permissionless and everyone incentivized can call it.

One possible mitigation would be to add a time period after entering the system to not be able to initiate withdrawal requests, this will remove the possibility users to benefit from the stepwise jumps that rewards distribution causes.

[H-03] WithdrawQueue::claim rebasing tokens can block the last claim temporarily

Issue Description:

Due to explicitly mentioning that Renzo will support stETH (rebasing) token as collateral here, the following scenario can occur and block the last withdrawal request for at least 24 hours.

  1. User has 10 ezETH tokens which he wants to withdraw.
  2. WithdrawQueue::withdraw is called and these 10 restaking tokens result in 10 stETH tokens that he will receive when the delay period passes.
  3. In the meantime, all other withdrawals are claimed and there just enough tokens to fulfil this request.
  4. Rebasing of stETH occurs (once in 24 hours) and the balanceOf(address(this)) decreases to 9 tokens, whereas amountToRedeem, calculated at step 2, remains the same - 10 tokens.
  5. User wants to claim but the transfer will revert due to insufficient funds in the WithdrawQueue:

WithdrawQueue.sol

function claim(uint256 withdrawRequestIndex) external nonReentrant {
...MORE CODE
 else {
      IERC20(_withdrawRequest.collateralToken).transfer(
          msg.sender,
          _withdrawRequest.amountToRedeem //@audit doesn't apply the rebasing
      );
  }
}

Recommendation:

Consider using wstETH which is the non-rebasing version of the stETH.

Usage of rebasing tokens has not been a problem so far because Renzo’s withdrawal functionality is not live yet.

[H-04] RestakeManager::calculateTvls always account withdraw queue balance for wrong token

Impact

Due to the wrong token index being passed, when summing up totalWithdrawalQueueValue, calculateTVLs will always return the wrong value.

Proof of Concept

In calculateTVLs(), it goes through each OperatorDelegator and for each one, it loops through all supported tokens.totalWithdrawalQueueValue is calculated when iterating all the supported collateral tokens and uses withdrawQueueTokenBalanceRecorded as a flag to account them only one time - it will enter the if check only for the first OperatorDelegator.

In the first iteration of the OperatorDelegator loop, it calculates all available withdrawals for every token. However, it consistently passes the first token from the collateralTokens array, because using the OperatorDelegator loop index (i), which is always 0 in this scenario, instead of looping through all tokens by passing the collateralToken loop index (j).

// record token value of withdraw queue
if (!withdrawQueueTokenBalanceRecorded) {
    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
        collateralTokens[i],  // @ audit it will always pass the first token, instead of looping through all tokens
        collateralTokens[j].balanceOf(withdrawQueue)
    );
}

RestakeManager.sol#L274-L358

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; ) {
            // Get the value of this token

            uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
                collateralTokens[j]
            );

            // Set the value in the array for this OD
            operatorValues[j] = renzoOracle.lookupTokenValue(
                collateralTokens[j],
                operatorBalance
            );

            // Add it to the total TVL for this OD
            operatorTVL += operatorValues[j];

            // record token value of withdraw queue
            if (!withdrawQueueTokenBalanceRecorded) {
                totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
                    collateralTokens[i],  // @ audit it will always pass the first token, instead of looping through all tokens
                    collateralTokens[j].balanceOf(withdrawQueue)
                );
            }

            unchecked {
                ++j;
            }
        }
        
    ...
}

Because of this, it will always convert the collateral token available for withdrawal to the price of the token at index 0.

Tools Used

Manual Review

Recommended Mitigation Steps

In calculateTVLs(), collateralTokens ****should take tokens based on j index, not i index.

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; ) {
            // Get the value of this token

            uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
                collateralTokens[j]
            );

            // Set the value in the array for this OD
            operatorValues[j] = renzoOracle.lookupTokenValue(
                collateralTokens[j],
                operatorBalance
            );

            // Add it to the total TVL for this OD
            operatorTVL += operatorValues[j];

            // record token value of withdraw queue
            if (!withdrawQueueTokenBalanceRecorded) {
                totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
-                   collateralTokens[i],
+                   collateralTokens[j],
                    collateralTokens[j].balanceOf(withdrawQueue)
                );
            }

            unchecked {
                ++j;
            }
        }
        
    ...
}

[M-01] No slippage in RestakeManger::deposit and WithdrawQueue::withdraw

Impact

RestakeManager::deposit() and WithdrawQueue::withdraw() have no slippage protection. deposit() receives amount collateral from the user and mints them ezETH, and withdraw() does the opposite. Both calculations, collateral → ezETH and ezETH → collateral, rely on oracles to convert everything to ETH denomination, meaning that the amount received in return may vary indefinitely while the request is waiting to be executed.

Proof of Concept

As you can see, none of the functions have a slippage parameter that allows the user to specify a minimum amount they expect to receive. This means that the user has no control over how much ezETH or collateral they will receive in return.

RestakeManager.sol#L491-L576

function deposit(
    IERC20 _collateralToken,
    uint256 _amount,
    uint256 _referralId // @audit no minAmountOut
) public nonReentrant notPaused {
		...
}

WithdrawQueue.sol#L206-L263

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

Tools Used

Manual Review

Recommended Mitigation Steps

Add minAmountOut to deposit() and minAmountOut to withdraw() because it calculates amountToRedeem there.

[M-02] RestakeManager::deposit will revert always

Impact

All deposits under withdrawQueue.bufferToFill will always revert.

The first depositor must deposit at least the bufferToFill + 1 wei to enable smaller deposits.

Proof of Concept

When users deposit to the RestakeManager, first the withdrawQueue.bufferToFill must be filled, and then the deposits will go to the OperatorDelegaror and from there to EigenLayer.

Note: The sponsor said that bufferToFill will be around 1000ETH at the beginning.

In deposit() it will get _collateralToken.bufferToFill and move the token to the depositQueue, filling the buffer for that particular token and if it reaches the buffer limit the remainder is saved to _amount and deposited into the operatorDelegator.

RestakeManager.sol#L491-L576

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

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

However, until the buffer is full, _amount will be 0 and it will try to deposit 0 into the operatorDelegator, but operatorDelegator.deposit() reverts when the amount equals 0, causing all deposits to revert. It will only work if someone fills the entire buffer with one deposit and makes _amount at least 1, which is close to impossible.

OperatorDelegator.sol#L143-L154

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

Tools Used

Manual Review

Recommended Mitigation Steps

Until the buffer is reached, deposits to the operatorDelegator should only be made if _amount > 0.

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

    // 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) {		
			// Approve the tokens to the operator delegator
	    _collateralToken.safeApprove(address(operatorDelegator), _amount);
	
	    // Call deposit on the operator delegator
	    operatorDelegator.deposit(_collateralToken, _amount);
+		}
    
    ...
}

[M-03] Possible arbitrage due to the long heartbeat of stETH/ETH price feed

Issue Description:

Renzo protocol relies on stETH/ETH price feed to convert stETH when used for deposit, withdrawal, etc. But this price feed has too long heartbeat - 24 hours, which can open arbitrage opportunities if the price is not updated accurately.

This is the price feed: stETH/ETH

Untitled

The 24-hour heartbeat and 0.5% deviation threshold means that price can move up to 0.5% or stay flat for 24 hours before triggering a price update, which is unlikely to be reached, but historically it is not impossible, you can check this graph for example:

287544459-d64c81e0-0e8a-4fe9-8063-166d9c5d9bcd.png

This allows you to deposit at these times to mint more ezETH or withdraw at a better rate.

The same issue was reported here and I used it as a guide - you can check it out as it has many more diagrams and explanations of possible situations. code-423n4/2023-11-kelp-findings#584

Recommendation:

It's hard to give a proper solution, one of them we also pointed out in our Kelp issue was to use multiple oracles, like this stETH/USD, but that will certainly make the system more complex, so can't suggest anything exactly.

[L-01] WithdrawQueue::claim should expect recipient

Issue Description:

When user claim their withdraw request it will always sent the tokens to him (msg.sender), consider allowing the user to specify who to receive the tokens.

Recommendation:

Add a new property to the WithdrawRequest struct:

WithdrawQueueStorage.sol

struct WithdrawRequest {
    address collateralToken;
    uint256 withdrawRequestID;
    uint256 amountToRedeem;
    uint256 ezETHLocked;
    uint256 createdAt;
+   address recipient;
}

As well as modify the code to use it: WithdrawQueue.sol

- function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
+ function withdraw(uint256 _amount, address _assetOut, address recipient) external nonReentrant {
...MORE CODE
      // add withdraw request for msg.sender
      withdrawRequests[msg.sender].push(
          WithdrawRequest(
              _assetOut,
              withdrawRequestNonce,
              amountToRedeem,
              _amount,
              block.timestamp,
+             recipient
          )
      );
...MORE CODE
  }
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();

    // subtract value from claim reserve for claim asset
    claimReserve[_withdrawRequest.collateralToken] -= _withdrawRequest.amountToRedeem;

    // delete the withdraw request
    withdrawRequests[msg.sender][withdrawRequestIndex] = withdrawRequests[msg.sender][
        withdrawRequests[msg.sender].length - 1
    ];
    withdrawRequests[msg.sender].pop();

    // burn ezETH locked for withdraw request
    ezETH.burn(address(this), _withdrawRequest.ezETHLocked);

    // send selected redeem asset to user
    if (_withdrawRequest.collateralToken == IS_NATIVE) {
-       payable(msg.sender).transfer(_withdrawRequest.amountToRedeem);
+       payable(_withdrawRequest.recipient).transfer(_withdrawRequest.amountToRedeem);
    } else {
-       IERC20(_withdrawRequest.collateralToken).transfer(
-           msg.sender,
-           _withdrawRequest.amountToRedeem
-       );
+       IERC20(_withdrawRequest.collateralToken).transfer(_withdrawRequest.recipient, _withdrawRequest.amountToRedeem);
    }
    // emit the event
    emit WithdrawRequestClaimed(_withdrawRequest);
}

[L-02] No way to cancel a withdrawal request

Issue Description:

There is no way for the user who requested a withdrawal to cancel it. Due to various factors, such as unfavorable conditions or increased yield on EigenLayer, user may wish to cancel their withdrawal request and request it later.

Recommendation:

Implement a function that allows users to cancel their withdrawal request by deleting their WithdrawRequest struct and returning them their ezETH back. But this should only be allowed after the withdrawal delay, because if it can be done immediately, other users can be gamed, by sandwich their withdraw request with request/cancel and lower the buffer. Also, if while the user is thinking about claiming or canceling their request, the buffer is refilled, the collateral that was reserved for them must be deposited into the EigenLayer, otherwise the buffer will overflow.

[L-03] chooseOperatorDelegatorForWithdraw should fetch the TVL

Issue Description:

chooseOperatorDelegatorForWithdraw relies on providing the accurate data to the following arguments:

  • operatorDelegatorTokenTVLs
  • operatorDelegatorTVLs
  • totalTVL

instead of calculating them inside the function itself.

This will remove the possibilities when some transactions, for example deposits and withdrawals/claims, execute before chooseOperatorDelegatorForWithdraw and the values given as arguments becoming stale.

RestakeManager.sol

  function chooseOperatorDelegatorForWithdraw(
      uint256 tokenIndex,
      uint256 ezETHValue,
      uint256[][] memory operatorDelegatorTokenTVLs,
      uint256[] memory operatorDelegatorTVLs,
      uint256 totalTVL
  ) public view returns (IOperatorDelegator) {

Recommendation:

Apply the following changes to the chooseOperatorDelegatorForWithdraw function:

RestakeManager.sol

function chooseOperatorDelegatorForWithdraw(
    uint256 tokenIndex,
    uint256 ezETHValue,
-   uint256[][] memory operatorDelegatorTokenTVLs,
-   uint256[] memory operatorDelegatorTVLs,
-   uint256 totalTVL
) public view returns (IOperatorDelegator) {

+ (uint256[][] operatorDelegatorTokenTVLs, uint256[] operatorDelegatorTVLs, uint256 totalTVL) = calculateTVLs(); 

...code below remains unmodified
}

The drawback of this additions is the increased gas cost, but on the other hand it guarantees values will always to be up-to-date.

[L-04] Inaccuracy of shares between Eigen withdrawal request and claim

Issue Description:

Withdrawals from EigenLayer will not be up to date, because the shares for the given amount are calculated upon request and various price changes may occur until they are claimed.

When a withdrawal is queued, it calculates how many shares to withdraw based on the underlyingToShare exchange rate at the time of the request.

OperatorDelegator.sol#L193-L256

function queueWithdrawals(
    IERC20[] calldata tokens,
    uint256[] calldata tokenAmounts
) external nonReentrant onlyNativeEthRestakeAdmin returns (bytes32) {
    ..
          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]); // <----------- @audit it will convert the tokenAmount to shares at the time of request
                
        ....

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

    ...
}

And then the shares that were saved in queuedShares will be withdrawn, but what if the exchange rate changes during the withdrawal delay has passed and be able to call completeQueuedWithdrawal().

  1. Let's assume that 1 stETH equals 1 EigenLayer-stETH and the withdrawal buffer is full, so all deposits will be made to Eigen.
  2. Alice deposits 5e18 stETH.
  3. After some time, Bob deposits 100e18 stETH and then requests an immediate withdrawal, and at the time of the request, the amount of his withdrawal shares will be 100e18 EigenLayer-stETH.
  4. Now assume that the value of EigenLayer-stETH increases, meaning that 1 EigenLayer-stETH is now worth more stETH. This is expected behavior as EigenLayer-stETH is similar to an ERC4626 vault and value may increase over time.
  5. Let's say 1 EigenLayer-stETH is now worth 1.1 stETH.
  6. For the initial 100stETH that bob deposited, now only 90.9 EigenLayer-stETH can be exchanged and plus the 5stETH deposited by Alice gives us a total of 90.9 + 4.54 = 95.44, but bob's request was for 100.

This would mean that Bob's withdrawal request would not be fulfilled because there were changes in the exchange rate between the request and the claim.

A similar finding was reported here: sherlock-audit/2024-02-rio-network-core-protocol-judging#109

Recommendation:

It is hard to give appropriate solution to this, because this is how EigenLayer shares works, but it can be something to check in completeQueuedWithdrawal(), if the initial shares are still the same and if the exchange rate was updated.