search

code-423n4 / 2024-04-renzo-findings

6 stars 6 forks source link

Withdrawers can skip Slashing due to cached `amountToRedeem` #11

Closed c4-bot-8 closed 2 months ago

c4-bot-8 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L227-L250

Vulnerability details

Impact

ezETH calculates withdrawal value via the following:

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L227-L250

       if (_assetOut != IS_NATIVE) {
            // Get ERC20 asset equivalent amount
            amountToRedeem = renzoOracle.lookupTokenAmountFromValue(
                IERC20(_assetOut),
                amountToRedeem
            ); /// @audit Arbitrage here
        }

This is a spot value conversion of the ETH to LST value.

In the case of stETH, a slashing would reduce the balance of each token holder and as such it would reduce the total amount of tokens in Renzo.

Because the amount of stETH to withdraw is cached as amountToRedeem, in case of a slashing withdrawers stake will not be slashed, which will:

If this were to be done to a sufficient scale, this would cause ezETH to be undercollateralized, as all available withdrawals would be used to escape slashing mechanism

Code explanation

The logic to compute the ETH value of an LRT in Renzo is as follows:

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L227-L250

       if (_assetOut != IS_NATIVE) {
            // Get ERC20 asset equivalent amount
            amountToRedeem = renzoOracle.lookupTokenAmountFromValue(
                IERC20(_assetOut),
                amountToRedeem
            ); /// @audit Arbitrage here
        }

        // revert if amount to redeem is greater than withdrawBufferTarget
        if (amountToRedeem > getAvailableToWithdraw(_assetOut)) revert NotEnoughWithdrawBuffer();

        // increment the withdrawRequestNonce
        withdrawRequestNonce++;

        // add withdraw request for msg.sender
        withdrawRequests[msg.sender].push(
            WithdrawRequest(
                _assetOut,
                withdrawRequestNonce,
                amountToRedeem, /// @audit Can prevent slashing from here -> Insolvency
                _amount,
                block.timestamp
            )
        );

This amount is then subtracted in claim

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L279-L290

    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;

The issue with this logic is that in between the queueing of a withdrawal and the claiming

If a slashing happens, then the value of the LRT withdrawable would need to be decreased by a commesurate amount

In lack of that, withdrawers that are being slashed are protected from slashing as Renzo is taking on the Loss by socializing it to all depositors that do not withdraw

POC

Mitigation

I believe there's 2 ways to handle mitigation

One requires ad hoc customizations for each token, by computing the delta index in the exchange rate at the time of withdrawal

The alternative is to denominate withdrawals in ETH, and make changes to make all LSTs withdrawals be done back to ETH

Each token would need to have it's index tracked, and slashing must account for a discount that is reflects the loss that happened

While the codes bases are different, a good starting point is to check how Blast handles discount: https://github.com/blast-io/blast/blob/04ed82f54a627622f82bea2217a090106d1ca2c2/blast-optimism/packages/contracts-bedrock/src/mainnet-bridge/withdrawal-queue/WithdrawalQueue.sol#L411-L441

In my opinion, since Renzo separates different tokens at a conceptual level, then each token should have an index for the share value

e.g.

    uint256 withdrawalIndex = stETH.getPooledEthByShares(1e18); // Retrieve index at time of withdrawal
    uint256 currentIndex = stETH.getPooledEthByShares(1e18);
    uint256 impreciseDiscountFactor = currentIndex > withdrawalIndex 
        ? 0 /// @audit No discount
        : withdrawalIndex - currentIndex /// @audit Discount

    /// @audit TODO: Fuzz tests and invariant test for precision loss
    uint256 safeWithdrawalAmount = _withdrawRequest.amountToRedeem * (1e18 - impreciseDiscountFactor);

By doing this, the system should maintain the property that withdrawals do not lower the PPFS

Assessed type

MEV

jatinj615 commented 2 months ago

Proper fix will be implemented once EigenLayer Slashing is finalised.

C4-Staff commented 2 months ago

CloudEllie marked the issue as primary issue

alcueca commented 2 months ago

Related to #326 in that the root cause is calculating withdrawal amounts immediately allows front-running the TVL changes.

c4-judge commented 2 months ago

alcueca marked the issue as duplicate of #326

c4-judge commented 2 months ago

alcueca marked the issue as satisfactory