Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Withdrawal amount calculation timing causes yield leakage and asset imbalance #466

Closed
howlbot-integration bot opened this issue May 10, 2024 · 7 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-282 🤖_45_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L217-L233
https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L305-L308

Vulnerability details

Cause

The WithdrawQueue contract calculates the withdrawal redemption amount at the time of the withdrawal request but pays out the amount at the time of the claim.

  • For ETH and rebasing LSTs like stETH, this means that the withdrawal asset does not earn yield during the delay (as the ETH value of an amount is constant).
  • However, for accruing LSTs like wBETH, the withdrawal will continue earning yield during the delay period (because its price increases with the yield).

Impact

For accruing LSTs, this means that yield that can be accrued for ezETH holders, is instead accrued to withdrawals, and lost to the protocol.

This will also create an imbalance in deposit and withdrawal flows because users will prefer to redeem in accruing tokens to avoid losing 7 days of yield.

This, in turn, will skew the asset composition, causing the protocol to accumulate rebasing LSTs over time to reach their TVL cap and revert on subsequent deposits. This will result in an ongoing DoS of deposits for these assets.

While it's possible to keep increasing TVL caps for rebasing LSTs, it is unlikely to be a long term solution, because would not be responsible risk management and would reduce asset diversification.

Proof of Concept

  1. Users will deposit all supported LSTs.
  2. Users will withdraw predominantly accruing LSTs due to the yield accrued during withdrawal delay.
  3. The accruing LSTs' yield will be lost to ezETH holders.
  4. The rebasing LSTs will keep accumulating over time in the protocol due to imbalanced flows, hitting TVL caps required for risk management and diversification.
  5. Users will not be able to deposit rebasing LSTs when their TVL cap is reached and not increased.

Tools Used

Manual Review

Recommended Mitigation Steps

Calculate the redemption from ezETH to ETH value at the withdrawal request time, but calculate the final LST amount at claim time for all assets. This will keep the ETH value of a withdrawal fixed for the duration of the delay and divert the yield earned to the ezETH holders.

Alternatively, allow the admin to rebalance the asset mixture by swapping assets withdrawn from Eigenlayer before transferring them to the WithdrawQueue.

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
	    ...
	    
        // Calculate amount to Redeem in ETH
        uint256 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
-            );
        }

    function claim(uint256 withdrawRequestIndex) external nonReentrant {
	    ...

        WithdrawRequest memory _withdrawRequest = withdrawRequests[msg.sender][
            withdrawRequestIndex
        ];

+        // update amount in claim asset, if claim asset is not ETH
+        if (_withdrawRequest.collateralToken != IS_NATIVE) {
+            // Get ERC20 asset equivalent amount
+            _withdrawRequest.amountToRedeem = renzoOracle.lookupTokenAmountFromValue(
+                IERC20(_withdrawRequest.assetOut),
+                _withdrawRequest.amountToRedeem
+            );
        }

Assessed type

Other

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_45_group AI based duplicate group recommendation bug Something isn't working sufficient quality report This report is of sufficient quality labels May 10, 2024
howlbot-integration bot added a commit that referenced this issue May 10, 2024
@C4-Staff
Copy link

CloudEllie marked the issue as duplicate of #467

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label May 17, 2024
@c4-judge
Copy link
Contributor

alcueca marked the issue as satisfactory

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 17, 2024
@c4-judge
Copy link
Contributor

alcueca changed the severity to 3 (High Risk)

@c4-judge
Copy link
Contributor

alcueca marked the issue as duplicate of #326

@c4-judge
Copy link
Contributor

alcueca marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

alcueca marked the issue as duplicate of #282

@alcueca
Copy link

alcueca commented May 27, 2024

While this issue doesn't reveal the reverts caused by decreasing stEth balances in an slashing event, it does correctly point out that stETH shouldn't be treated as non-rebasing LSTs in the withdrawal queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-282 🤖_45_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

4 participants