All stETH withdrawls from the EigenLayer strategy will fail

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L302
https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L140
https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L144
https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L197

Vulnerability details

Background

Renzo is deploying a new set of smart contracts to support the withdrawals from EigenLayer. Which will mainly allow Renzo users to withdraw their already deposited assets such as ETH, stETH & wbETH from the renzo protocol / EL Strategies.

We are only concerned with the stETH because Renzo will have issues with withdrawing stETH from EL. Currently there are 20k stETH Tokens, worth more than $60 miilion, are deposited into Renzo, which of course are further deposited into EL Starategy.

Overview of the stETH's 1-2 wei Issue

stETH have intrinsic rounding down problems. It's a known issue, and the stETH balance of an account could be lower of 1-2 wei because of rounding down. The rounding down issue is not problematic only for when the balance is displayed, but also when transfers are performed. Transferring amount from accountA to accountB could lead accountB to receive less amount.

Overview of the Withdrawal Functionality

User's withdraw assets from the WithdrawQueue contract using withdraw() & claim(). The withdrawQueue contract get filled by 3 ways:

1. New Deposits
2. Daily Rewards (Coming in the Protocol)
3. Manual withdrawal from EigenLayer. Permissioned call from OperatorDelegator.

If options 1 and 2 are not sufficient to fulfil withdraw requests of Users then admin accounts will manually unstake from EigenLayer periodically through 2 step process (3 Steps in case of ETH)

1. OperatorDelegator.queueWithdrawals()
2. OperatorDelegator.completeWithdrawals()

This is what ERC20 tokens transfer flow looks like when admin execute completeQueuedWithdrawal():
-> EL Strategy transfers tokens to Renzo's OperatorDelegator Contract.
-> From their tokens are transferred to DepositQueue contract
-> From their tokens are transferred to WithdrawQueue contract

Another tokens tranfer can also happen in the same TRX, if the withdraw buffer gets filled and there are some remaining tokens left in the OperatorDelegator contract, then those token will be re-deposited into the EigenLayer strategy.

Note: There will be 3-4 tokens transfers will happen in one-single transaction.

The Vulnerability

Admin's Manual withdrawal from EigenLayer for stETH will fail due to stETH's 1-2 wei corner case because the code tries to transfer expected amounts instead of actual amounts available.
The OperatorDelegator.completeQueuedWithdrawal() function calls depositQueue's fillERC20withdrawBuffer() function with the amount:bufferToFill:

restakeManager.depositQueue().fillERC20withdrawBuffer(address(tokens[i]), bufferToFill);

Which transfer tokens from the OperatorDelegator to depositQueue and further calls withdrawQueue's fillERC20WithdrawBuffer(), which transfers EXACT GIVEN AMOUNT (bufferToFill) from depositQueue to withdrawQueue.

    function fillERC20withdrawBuffer(address _asset, uint256 _amount) external nonReentrant onlyRestakeManager {
        if (_amount == 0 || _asset == address(0)) revert InvalidZeroInput();
@>      IERC20(_asset).safeTransferFrom(msg.sender, address(this), _amount);
        IERC20(_asset).safeApprove(address(withdrawQueue), _amount);
        withdrawQueue.fillERC20WithdrawBuffer(_asset, _amount);
    }

    function fillERC20WithdrawBuffer(address _asset,uint256 _amount) external nonReentrant onlyDepositQueue {
        if (_asset == address(0) || _amount == 0) revert InvalidZeroInput();
@>      IERC20(_asset).safeTransferFrom(msg.sender, address(this), _amount);
        emit ERC20BufferFilled(_asset, _amount);
    }

Which is obviously bound to fail because of the stETH's 1-2 wei corner case.
The withdrawQueue's fillERC20WithdrawBuffer will try to transferFrom the _amount, which is the expected amount not actual amount, the actual balance of DepositQueue will be _amount - 1 wei, which will revert the whole transaction, resulting in OperatorDelegator inability to withdraw stETHs from the EL Strategy and funds will be frozen in the EL, as there is no other way to withdraw funds from EigenLayer Strategy.

As noted above, there will be 3-4 tokens transfers will happen in one-single transaction, which simply increases the likelihood of this issue.

Impact

Freezing of Funds

Tools Used

Shaheeniyat

Recommended Mitigation Steps

Transfer the actual token balance instead of expected amounts:

    function fillERC20withdrawBuffer(address _asset,uint256 _amount) external nonReentrant onlyRestakeManager {
         if (_amount == 0 || _asset == address(0)) revert InvalidZeroInput();
+        uint256 balanceBefore = IERC20(_asset).balanceOf(address(this));
         IERC20(_asset).safeTransferFrom(msg.sender, address(this), _amount);
+        uint256 balanceAfter = IERC20(_asset).balanceOf(address(this));
+        uint256 actualBalance = balanceAfter - balanceBefore;
+        require(actualBalance > 0, "No Tokens Received");
-        IERC20(_asset).safeApprove(address(withdrawQueue), _amount);
+        IERC20(_asset).safeApprove(address(withdrawQueue), actualBalance);
-        withdrawQueue.fillERC20WithdrawBuffer(_asset, _amount);
+        withdrawQueue.fillERC20WithdrawBuffer(_asset, actualBalance);
    }

Assessed type

Token-Transfer

[DadeKuma]

@howlbot accept