Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L279-L312

Vulnerability details

Proof Of Concept

First see H-04 from the previous contest here: https://github.com/code-423n4/2024-04-renzo-findings/issues/326

The TLDR of this report is the fact that the previous withdrawal logic allows MEV exploits of TVL changes due to it's zero-slippage zero-fee swaps, that's to say if there would be a massive change in the TVL or what not, an attacker can front run this change to place a withdrawal which would allow them game the system considering no fees are applied.

Now protocol applied a sufficient mitigation to that bug case, from this pull request, so now an arbitraguer can't blindly frontrun a call to game the system since the profit is unsure as they do not know what the rate would be after 7 days of their requests in the withdrawal queue.

Now whereas this mitigates the previous H-04, this also leads to loss of funds for honest users, raesons explained below.

First note that the highlighted diff additions are what've been added to the previous claim() function, i.e https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L279-L312

    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;

+         // calculate the amount to redeem
+         uint256 claimAmountToRedeem = _calculateAmountToRedeem(
+             _withdrawRequest.ezETHLocked,
+             _withdrawRequest.collateralToken
+         );
+         // update withdraw request amount to redeem if lower at claim time.
+         if (claimAmountToRedeem < _withdrawRequest.amountToRedeem) {
+             _withdrawRequest.amountToRedeem = claimAmountToRedeem;
+         }
+
        // 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);
        } else {
            IERC20(_withdrawRequest.collateralToken).transfer(
                msg.sender,
                _withdrawRequest.amountToRedeem
            );
        }
        // emit the event
        emit WithdrawRequestClaimed(_withdrawRequest);
    }

And the below is the new function that gets called whenever withdraw() or claim() is queried:

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

Would be key to note that this function just does the calculation that was previously in withdraw() as can be seen in these snippets from the previous contest: https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L215-L233.

Now, after the mitigation, withdraw() has been truncated to a few lines and it now delegates getting the amount to redeem to the _calculateAmountToRedeem() function shown below uint256 amountToRedeem = _calculateAmountToRedeem(_amount, _assetOut);.

Now back to why this leads to a new problem, the new if (claimAmountToRedeem < _withdrawRequest.amountToRedeem) check from claim() would now mean that users would lose out on their tokens cause their is only a downside to it, and the current approach of withdraw() <-> claim() doesn't even have slippage provision so users can't reject the withdrawal requests they've placed in the case where a very unfair rate is now being given to them.

A rough worded POC as to why this approach is wrong is the fact that protocol did not consider honest users who innocently placed their withdrawal requests and are just waiting for the 7 days to pass inorder to claim.

Now, while placing their requests, the renzoOracle.calculateRedeemAmount() is being queried with the amount of ezETH users would like to burn/withdraw.

This funcion calculates the final amount to redeem as such https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Oracle/RenzoOracle.sol#L159-L161

        uint256 redeemAmount = (_currentValueInProtocol * _ezETHBeingBurned) / _existingEzETHSupply;

Where _currentValueInProtocol is the totalTVL, _ezETHBeingBurned is the amount of ezETH users would like to withdraw & _existingEzETHSupply is the current total supply of ezETH.

This means that after their withdrawal requests are ripe for claim, protocol unfairly queries this function again, and uses the current totalTVL and total supply of ezETH to determine the amount of final redemption,

Note that in our case here, within this 7 days timeframe the totalTVL can have a massive drop and since protocol recalculates the amount to redeem for users using the current tvl, the if (claimAmountToRedeem < _withdrawRequest.amountToRedeem) check would be triggered and users would be sent the current calculated amount for them to redeem instead of what they are rightfully owed, would be key to note that the massive drop in the tvl could be from any factor, in our case it could even be as simple as the fact a lot of withdrawal requests were placed prior to a user placing theirs, and after these withdrawal requests get claimed the tvl drops, so by the time the user is trying to claim he uses a way lower tvl.

Impact

Users would unfairly lose their assets since protocol always uses the bad rate when exchanging ezETH for collateral assets.

This also breaks the first main Invariant hinted https://github.com/code-423n4/2024-04-renzo/tree/1c7cc4e632564349b204b4b5e5f494c9b0bc631d?tab=readme-ov-file#main-invariants:

ezETH should be minted or redeemed based on current supply and TVL.

Considering, not only is ezETH not redeemed by the current supply and TVL when the withdrawal request is placed. Protocol also cherry picks if having the invariant hold if it's to be applied when claiming, cause in the case the amountToRedeem increases at the time of claim() it's not being used and instead the amount attached to the withdrawal request from when it was placed is used, however if the amountToRedeem decreases at the time of claim() it's then being used instead of the amount attached to the withdrawal request from when it was placed. And all these cost the user as they can't decline whatever drop happens to the assets they are to receive.

Would be key to note that for the invariant to hold, protocol has to chose one of the two:

Either always using supply and TVL as at the time the request was placed, or
Using the supply and TVL when the request is being claimed

Recommended Mitigation Steps.

Advisably the if (claimAmountToRedeem < _withdrawRequest.amountToRedeem) check should be scrapped and there should be only one instance of calculating amountToRedeem when users are withdrawing, pending protocol's decision as whether the rate should be from when the requests are being placed or when the requests are being finalized.

If the latter is going to be implemented then there is subtly a need to bring in a slippage param so users can reject the drop in their withdrawal amount if it's not applicable to them.

Also this can be easily done while still fixing the H-04 from the previous contest, in this case and as in multiple arbitrage cases, a fee should instead be applied to withdrawals, this way MEV attacks would become way more unprofitable, and this way not only does the invariant hold that ezETH is being minted/redeemed based on current supply and TVL, users are also sure that the amount they calculate and what gets emitted for via the WithdrawRequestCreated() event is what they end up claiming.

Assessed type

Context

alcueca marked the issue as primary issue

The calculateRedeemAmount was placed inside the claim as well to price in the slashing for the withdrawing users. Also related to fee issue we believe enforcing a dedicated fee to user is not favourable. Instead we can see this withdraw process as, withdrawing users do not get any APY from the point they started withdraw but will be sharing pooled loss if any while claim. Also, related to the MEV exploits the explanation is given here - https://github.com/code-423n4/2024-04-renzo-findings/issues/424#issuecomment-2137550998 With that in mind, I think that only stETH feed is volatile to have unexpected rise/drop in price impacting TVL to certainly rise/drop for which the long term plan is shared in the above mentioned explanation.

Another discussion on Slippage on deposit and withdraw - https://github.com/code-423n4/2024-04-renzo-findings/issues/484

Hi Judge and Sponsor,

We will leave a comment on this since it's the main issue to keep the discussion nice and organized, although our issue is:

https://github.com/code-423n4/2024-06-renzo-mitigation-findings/issues/39

We have read the reply of the Sponsor and we agree with the long-term solution provided in said comment:

Instead we think a long term solution can be implemented by utilizing the true price and exchange rate price of stETH. i.e. consider lowest of both at time of deposit and highest of both at withdraw.

Furthermore, we have read the discussion on slippage on deposit and withdraw.

However, as stated by the Sponsor in their reply, their proposed solution in their comment is a long term solution. At the current moment, the lowest of both are considered during the time of withdrawal, not the highest, as mentioned as the long term strategy:

+         // calculate the amount to redeem
+         uint256 claimAmountToRedeem = _calculateAmountToRedeem(
+             _withdrawRequest.ezETHLocked,
+             _withdrawRequest.collateralToken
+         );
+         // update withdraw request amount to redeem if lower at claim time.
+         if (claimAmountToRedeem < _withdrawRequest.amountToRedeem) {
+             _withdrawRequest.amountToRedeem = claimAmountToRedeem;
+         }

With regards to the discussion on slippage on deposit and withdraw, this is not applicable to this issue. The discussion is about the exchange rate fluctuations when solely calling withdraw(), which was a borderline Medium.

This High issue however describes that, in the very least, honest users can lose funds, due to the additional logic added in claim() and especially this if-statement:

+         if (claimAmountToRedeem < _withdrawRequest.amountToRedeem) {
+             _withdrawRequest.amountToRedeem = claimAmountToRedeem;
+         }

which greatly magnifies the amount of loss a user can suffer without the interference of a malicious actor. If a malicious actor would interfere, the exploitable surface increases even more.

To summarize, we agree with the long-term solution, but the short-term patch that has been deployed with the mitigation greatly increases the risk of honest users losing funds.

I don't quite see what the issue is here. This finding just describes the mitigation that was implemented, which was precisely to give users the worse of both prices.

As such I don't think this can be considered a valid finding as there is no vulnerability or loss of funds other than the intended behaviour explicitly introduced by the mitigation.

The comment placed by the Warden is contradictory to the Mitigation Review Process.

A mitigation can lead to partly mitigating the original issue, while introducing a new issue. That's why we have the ability to link newly introduced issues to mitigations. Note that there is no such thing as a label partly-mitigated, which is why we resorted to unmitigated in our report.

For example, Mitigation A can be implemented to partly fix Issue B, but this will introduce Issue C, D.

Also note that mentioning something is intended behavior does not absolve it from being an issue. If an admin, for example, is able to steal all the tokens of a project and the claim is made that this is intended, that does not mean that it is not an issue.

The Sponsor has been clear in his comment:

With that in mind, I think that only stETH feed is volatile to have unexpected rise/drop in price impacting TVL to certainly rise/drop for which the long term plan is shared in the above mentioned explanation.

The correct way to remedy the mitigation, is by the long-term solution provided in the linked comment by the Sponsor. The long-term solution is not implemented yet. Therefore, the mitigation has not been successfully mitigated and the issue described in this report remains valid until the long-term strategy is correctly implemented.

Furthermore, the implementing the proposed mitigation breaks the main invariant that is foundational for this project:

ezETH should be minted or redeemed based on current supply and TVL

Now the question becomes:

Should a newly introduced issue, that impacts more (honest) users than the original issue, and which can be abused by malicious actors, be invalidated, even though it breaks a main invariant that acts as the foundation of the project, just because the mitigation partly fixes the original issue?

Our answer to that is it should not be invalidated. The original issue being partly mitigated(as per the comment of the Sponsor on the pull request) and a newly introduced issue can both be true at the same time, with the emphasis on this newly introduced issue having a greater impact.

Thanks!

The meaning of "unexpected losses by honest users" is not a clear one. From my point of view if the sponsor states that they are going to lock withdrawals for a week and that TVL fluctuations will affect the claimed amount, then users have been sufficiently warned of what might happen.

Other than that, the finding doesn't reveal anything that the sponsor wouldn't be expected to know: if you review downwards to current value the withdrawn assets upon claim, then the users might get less than was pointed out at withdrawal.

alcueca marked the issue as unsatisfactory: Invalid

Hi @alcueca,

Thanks for judging. However, I believe this issue to be valid. While this might be intended by the Sponsor, it would stall adoption when users find out that they can't really decide the rate at which they want to trade. Users will end up paying the cost of any deviation that occurs. Currently, the process of withdrawal/claiming functions with a reverse slippage idea. When users place requests, they are essentially signing off on the highest amount of value they can receive for their assets, whereas the downside has no limit. It could get as low as possible, and they also can't reject the attempted request even if they don't like the final rate. They can only wait for the rates to improve (which they are not sure will happen since there is no limit to how unfavorable the rates could get for them).

Also, the Sponsor has hinted that enforcing a dedicated fee to users is not favorable. But in my opinion, this is untrue, as every user would rather have a dedicated fee attached to their withdrawal attempts than a fixed highest rate they can exchange for, which they are not even sure they will even receive.

Additionally, as hinted in this report, I believe this bug case breaks the first main invariant Renzo attempts to uphold: https://github.com/code-423n4/2024-04-renzo/tree/1c7cc4e632564349b204b4b5e5f494c9b0bc631d?tab=readme-ov-file#main-invariants:

ezETH should be minted or redeemed based on current supply and TVL.

In the previous contest, issues that showcased how any of the main invariants were broken were deemed satisfactory H/M findings.

To finalize, I'd appreciate it if you could reassess this issue in light of the invariant being broken and the potential downside for users, and consider it a satisfactory finding. Thanks for your time.

@Bauchibred, you are just reiterating your previous point, and making arguments towards your preferred design and business strategy, which is not in the scope of this contest. While your comments would be valuable as an analysis finding, that is not what we are doing here.

Also, regarding the breaking of invariants. The sponsor is free to break invariants if he chooses to do so, as he is clearly doing in this case. This is not a contest on catching the sponsor on some faux pas to score points (and money). This is a contest in providing value to the sponsor in revealing flaws that the sponsor is not aware of. Revealing an unexpected breaking of an invariant is of value to any sponsor. Revealing an intentional breaking of an invariant is just stating the obvious, and a waste of time for everyone involved.

code-423n4 / 2024-06-renzo-mitigation-findings