Renzo - GalloDaSballo's results

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:

• Socialize the loss to other depositors
• Make the withdrawer avoid the loss completely

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

• See stETH getting slashed
• Realized oracle in Renzo has yet to update with the slashing
• Queue a withdrawal
• I successfully avoided the slashing
• If a lot of people do this, then ezETH may be unable to repay all withdrawals

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

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L245-L246

Vulnerability details

Impact

xRenzoDeposit uses the L2 Oracle Data to determine how much xezETH to Mint

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L244-L245

        // Fetch price and timestamp of ezETH from the configured price feed
        (uint256 _lastPrice, uint256 _lastPriceTimestamp) = getMintRate();

It will then receive a WETH amount to deposit into ezETH and it will then proceed to mint ezETH for the corresponding amount

This logic has flaws, which will cause undercollateralization of xezETH in the Lockbox, this is because:

• xezETH to ezETH is 1:1
• L2 will mint a bit more xezETH using a stale exchange rate
• L1 will mint a bit less ezETH using the current exchange rate
• Some people will be unable to withdraw their ezETH due to this

Investigation

From my review, the lockbox is currently over-collateralized

https://docs.google.com/spreadsheets/d/11YceC1tdweSHn4zqsVJoCsOBYp8X_bmQ6-Z6_5usCeM/edit?usp=sharing

I attribute this to the fact that the stETH Feed is a Market Rate Feed

Which means that over time, the stETH feed is causing a slight reduction in the price of ezETH, making it so that the lockbox is minting more than intended

Empirically we have prove that the accounting is incorrect

That said the incorrect accounting currently ensures that withdrawers will be able to exit, while some ETH will be indefinitely stuck in the Renzo Contracts

Rationale around risk of insolvency

To keep things simple, we can think of ezETH as a stETH Wrapper

Depositing 1 stETH over time, will entitle to more stETH

The rate between ezETH and stETH is not dictated by it's TVL but rather the conversion of stETH to ETH via a Chainlink Price feed, which has at most 1 day of delay

The same logic applies between ezETH and xezETH, a feed with another day of delay is used.

This means that we can have 3 scenarios:

• All feeds are synchronized (Optimistic case)
• Sometimes one of the feed is delayed (Realistic case)
• Sometimes both feeds are delayed, and minting on L2 gives more xezETH than intended (Pessimistic Case)

From this, assuming the pessimistic case can happen, we derive the following POC

POC

Let's assume a 1% Price change to simplify (a more likely price change would be betwee 1.5 BPS and 3 BPS)

• Oracle is slow by definition
• Minting on L1 will require 1 ETH
• Minting on L2 will require 0.99 ETH
• Mint on L2 -> For size that is above 5 times the 32 ETH cap
• You pay less there
• Go back to L1
• You receive less than intended
• Someone else will be unable to withdraw as the ratio of xEZETH to ezETH is incorrect

Mitigation

I believe that it would be best for Renzo to use fees as part of a mechanism to ensure solvency.

At 4% per year, we get 4/365 = 0.0109589041 APR

Meaning that in the average case, xezETH should lose around 1 BPS to oracle drift

Given that xezETH fees are 5 BPS, the fees should be able to cover this in most cases

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Oracle/RenzoOracle.sol#L70-L81

Vulnerability details

Impact

The stETH/ETH oracle is not a exchange rate feed, it's a Market Rate Feed

While other feeds are exchange rate feeds

This opens up ezETH to be vulnerable to:

• Market Rate Manipulations
• Sentiment based Price Action
• Duration based discounts

POC

This opens up to arbitrage anytime stETH trades at a discount (see Liquidations on the 13th of April)

Had withdrawals been open, the following could have been possible:

• Deposit stETH before the Depeg (front-run oracle update)
• Get ezETH
• Withdraw stETH after the depeg (1% a day, around 365% APR)

As well as:

• stETH market depegs
• Deposit ETH for ezETH
• Withdraw stETH at premium (about 1% arbitrage, around 365% APR)

Mitigation

I believe the withdrawal logic needs to be rethought to be denominated in ETH

The suggested architecture would look like the following:

• Deposit of ETH or LSTs, estimated via a pessimistic exchange rate
• Withdraw exclusively ETH, while pricing in slashing, discounts and operative costs

Assessed type

Oracle

I'd like to point that for https://github.com/code-423n4/2024-04-renzo-findings/issues/13 the arbitrage can be avoided, by underpricing stETH on deposit (market value), and over pricing it (redemption value) on withdrawal.

Pricing in any other way, due to lack of fees will create arbitrage, while pricing in the way above will protect against it

There is a factually better way to price stETH which is to use it's redemption value, and to be on the safer side, use it's market value on deposit and the redemption value on withdrawal

The in scope configuration is objectively leaking value to arbitrage when compare to the suggest mitigation

QA Review

The following are notes I wrote while doing the security review for Renzo

They are separate by topic and include:

• Gas considerations

• Small Refactorings

• Economic Considerations

• Blast Integration specifics

• Invariants

• QA Review

• Operator Delegator Gas Refund Logic Flaws

  • Gas Refunds are not possible
  • Gas Refunds are gameable
  • Gas Refunds are flawed for Contracts

• Deposit Queue

  • totalEarned logic ignores gas refunds

• BalancerRateProvider

• xRenzoBridge

• Economic Considerations

  • stETH Arbitrage Via Oracle Drift to gain daily rebase from LRTs
  • Withdrawal Implementation will force a discount due to unknown duration risk
  • In case of slashing, xRenzo Bridge can have accounting errors

• Basic Refactorings

  • withdrawRequestNonce is unused

• Blast Integration

  • Missed opportunity to collect gas refunds

• Suggested Invariants

  • Deposit and Withdrawals should not create an arbitrage opportunity
    • A withdrawal shouldn't cause a Loss -> PPFS doesn't decrease at time of withdrawal
  • xERC20 CrossChain Total Supply matches the ezETH in the Lockbox
  • Arbitrage Doomsday Invariant

• Oracle

  • Oracle will stop working for new LST that get's slashed
  • Oracle may not update once it goes past min and max

Operator Delegator Gas Refund Logic Flaws

Gas Refunds are not possible

We can see 0x0B1981a9Fcc24A445dE15141390d3E46DA0e425c.adminGasSpentInWei(address(0x3b8C27038848592a51384334D8090DD869a816CB))

As well as the rest of the operatorDelegators

Have almost 1ETH of gas refund tied to their accounts

But there seems to be no way to pay these out

And paying them out would require causing a loss of yield to he rest of the depositors

Which doesn't seem to be accounted for

Gas Refunds are gameable

Instead of capping the refund to block.basefee + premium

The refunds are using tx.gasprice, which is directly manipulatable by the eoa performing the calls

Gas Refunds are flawed for Contracts

Since the logic is as follows:

adminGasSpentInWei[tx.origin]

Deposit Queue

totalEarned logic ignores gas refunds

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L163-L179

    receive() external payable nonReentrant {
        uint256 feeAmount = 0;
        // Take protocol cut of rewards if enabled
        if (feeAddress != address(0x0) && feeBasisPoints > 0) {
            feeAmount = (msg.value * feeBasisPoints) / 10000;
            (bool success, ) = feeAddress.call{ value: feeAmount }("");
            if (!success) revert TransferFailed();

            emit ProtocolFeesPaid(IERC20(address(0x0)), feeAmount, feeAddress);
        }
        // update remaining rewards
        uint256 remainingRewards = msg.value - feeAmount;
        // Check and fill ETH withdraw buffer if required
        _checkAndFillETHWithdrawBuffer(remainingRewards);

        // Add to the total earned
        totalEarned[address(0x0)] = totalEarned[address(0x0)] + remainingRewards; /// @audit THIS IS WRONG DUE TO FEES

Since the system pays out gas refunds, and totalEarned is ignoring them, it will over-report by a bit

BalancerRateProvider

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RateProvider/BalancerRateProvider.sol#L29-L41

    function getRate() external view returns (uint256) { /// TODO: I feel the TVL math is wrong post-withdrawal, esp post slashing
        // Get the total TVL priced in ETH from restakeManager
        (, , uint256 totalTVL) = restakeManager.calculateTVLs(); /// @audit 100% Arbitrage here due to delay + oracle swings

        // Get the total supply of the ezETH token
        uint256 totalSupply = ezETHToken.totalSupply();

        // Sanity check
        if (totalSupply == 0 || totalTVL == 0) revert InvalidZeroInput();

        // Return the rate
        return (10 ** 18 * totalTVL) / totalSupply; /// @audit this breaks if total supply is below 1e18
    }

xRenzoBridge

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L1/xRenzoBridge.sol#L139-L150

    function xReceive(
        bytes32 _transferId,
        uint256 _amount,
        address _asset,
        address _originSender,
        uint32 _origin,
        bytes memory
    ) external nonReentrant returns (bytes memory) {
        // Only allow incoming messages from the Connext contract
        if (msg.sender != address(connext)) { /// @audit TODO: Connext integration unclear
            revert InvalidSender(address(connext), msg.sender);
        }

xezETH can be minted and lost by anyone, meaning that the total ezETH in the lockbox can be greater than what is claimable

This doesn't seem to cause issues

Economic Considerations

stETH Arbitrage Via Oracle Drift to gain daily rebase from LRTs

• LRTs are repriced up, they will raise by let's say 0.1% as a simple example
• The price is clear and the change will happen
• We deposit early
• All LRTs are underpriced
• The TVL math will be lower by X%

-> NOTE: as long as the oracles used are the exchange rate oracles, this can be performed both for depositing and withdrawing

Withdrawal Implementation will force a discount due to unknown duration risk

This line shows the logic to queue a withdrawal:

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

        if (amountToRedeem > getAvailableToWithdraw(_assetOut)) revert NotEnoughWithdrawBuffer();

This is strictly first-in first-out

As any new withdrawal queued will reduce claimReserve

The problem with this approach is that while the duration of a withdrawal is guaranteed to be coolDownPeriod

The time to wait for getAvailableToWithdraw(_assetOut) to be increased and the amount by which it will grow, is undefined

This is because:

• People have no way to signal withdrawals in the Smart Contract System (undefine amount to withdraw from Eigenlayer)
• People that do not signal their intentions to withdraw may still elect to do so

In case of slashing, xRenzo Bridge can have accounting errors

-> Oracle on L2 reports pre-slashing amount -> Oracle on Mainnet has post-slashing amount -> xRenzoBridge mints ezETH amount -> That's a higher amount

• Extra tokens are lost and can never be claimed

The extra tokens would be minted but not backed

That said the tokens wouldn't be redeemable

Basic Refactorings

withdrawRequestNonce is unused

Just use the previous length of the array

Blast Integration

Missed opportunity to collect gas refunds

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/Connext/integration/LockboxAdapterBlast.sol#L30-L47

contract LockboxAdapterBlast {
    address immutable blastStandardBridge;
    address immutable registry;

    // ERRORS
    error InvalidRemoteToken(address _remoteToken);
    error AmountLessThanZero();
    error InvalidAddress();

    constructor(address _blastStandardBridge, address _registry) {
        // Sanity check
        if (_blastStandardBridge == address(0) || _registry == address(0)) {
            revert InvalidAddress();
        }

        blastStandardBridge = _blastStandardBridge;
        registry = _registry;
    }

See: https://docs.blast.io/building/guides/gas-fees

Without explicitly enabling gas mode, you will not be able to claim the gas back

Suggested Invariants

Deposit and Withdrawals should not create an arbitrage opportunity

A withdrawal shouldn't cause a Loss -> PPFS doesn't decrease at time of withdrawal

PPFS (Price Per Full Share) reduction would indicate that the Withdrawal Mechanism is leaking value against other user operations, this is the case in the current code

xERC20 CrossChain Total Supply matches the ezETH in the Lockbox

This invariant is currently safe, however, inaccuracies will cause loss of collateral (inability to withdraw ezETH) Or may cause insolvency of the Lockbox

Arbitrage Doomsday Invariant

No single token pair should allow for an immediate arbitrage

By trying various combinations of tokens in and out, you should ensure that no direct arbitrage can be achieved

It's important you consider any direct arbitrage as a potential risk as your system is factually giving away value to the caller

Oracle

Oracle will stop working for new LST that get's slashed

While unlikely, it is possible for new LSTs to get slashed below parity, this will cause the oracle to stop working

If you plan on adding a lot more LSTs you may want to allow them to be valued slightly below parity

Oracle may not update once it goes past min and max

This is a common issue in validating Chainlink Oracles, you should ensure that the value from the aggregator is not at the bound of min, and max