search

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

0 stars 0 forks source link

[M-12] mitigation error: `getRate()` should not revert in all cases #38

Closed c4-bot-5 closed 3 months ago

c4-bot-5 commented 3 months ago

Lines of code

https://github.com/Renzo-Protocol/Contracts/blob/consistent-price-fetch/contracts/Bridge/L2/xRenzoDeposit.sol#L293 https://github.com/Renzo-Protocol/Contracts/blob/consistent-price-fetch/contracts/Bridge/L2/Oracle/RenzoOracleL2.sol#L52 https://github.com/Renzo-Protocol/Contracts/blob/consistent-price-fetch/contracts/Bridge/L2/Oracle/RenzoOracleL2.sol#L55

Vulnerability details

The applied mitigation of calling getMintRate() does not take into account that the RenzoOracleL2 contract was tailored to provide a price used to facilitate deposits into the protocol. This is evidenced by the fact that the RenzoOracleL2.getMintRate() function reverts in the following scenarios:

However, these two cases do not apply in providing a price to Balancer pools, and:

Impact

In Balancer's ComposableStablePools such as the one in use for ezETH/WETH on mainnet, provider.getRate() is called before all swaps, joins and exits once the configured cache duration is expired via _beforeSwapJoinExit() -> _updateTokenRateCache().

This means all of these operations will revert leading to temporary DOS until Recovery Mode is enabled on the pool. From a Balancer governance proposal:

TL;DR on recovery mode:

  • Pools in Recovery Mode do not collect protocol fees(and therefore do not accumulate core pools incentives)
  • Pools in Recovery Mode can only be joined and exited proportionally.
  • Recovery Mode may interfere with the compatibility of pools.

Disabling Recovery Mode requires governance intervention.

Proof of Concept

  1. The price of ezETH falls below 1 ETH due to a loss in one of the collateral assets used by Renzo. The current price feed reflects this sub-1 ETH price.
  2. A user attempts to swap, join or exit an ezETH Balancer pool on L2.
  3. The pool calls xRenzoDeposit.getRate() to fetch the current price of ezETH as part of the _beforeSwapJoinExit() hook.
  4. Inside xRenzoDeposit.getRate():
    • getMintRate() is called which fetches the price from RenzoOracleL2.
    • RenzoOracleL2.getMintRate() reverts because the price is below 1 ETH.
    • This causes xRenzoDeposit.getRate() to revert.
  5. The revert bubbles up and causes the user's swap, join or exit transaction on the Balancer pool to fail.
  6. All swaps, joins and exits on the pool start reverting due to the same reason.

Tools Used

Manual review

Recommended Mitigation Steps

Move the checks and reverts in the RenzoOracleL2 contract to xRenzoDeposit.getMintRate() and implement separate logic in getRate() that does not revert in the same cases. This will also allow reverting in getMintRate() in case the price falls below 1 ETH even if it was reported via updatePrice() or updatePriceByOwner(), as reported in this finding.

Assessed type

Other

jatinj615 commented 3 months ago

If any incorrect/corrupted price is reported on L2 then protocol would want to pause the balancer until fixed.

also with the raising concerns/confusions around using Oracle and CCIP on L2. Protocol will be rebuilding logic to make it more simplified and not over complicate it. For Current status, Oracle Feed on L2 will not be configured and enabled util proper changes have been made.

alcueca commented 3 months ago

The finding linked by the warden is the same one that specifies that the sponsor won't be using the chainlink oracle, but a custom one where they won't be reporting prices below 1.

Under those conditions, it would make sense for the sponsor to pause the protocol if they can't provide a price below 1 in their oracle.

c4-judge commented 3 months ago

alcueca marked the issue as unsatisfactory: Invalid

0xEVom commented 3 months ago

@alcueca @jatinj615 another way of framing this finding (which might help in conveying its intention) is that if xRenzoDeposit is to function as a price provider to Balancer pools, then RenzoOracleL2 must be able to provide prices below 1 ETH.

The problem I see with using the oracle revert as a pausing mechanism is that then there is no way of "unpausing" deposits and/or the pool, other than disabling the RenzoOracleL2 feed and falling back to CCIP updates. This does not invalidate the finding, as the RenzoOracleL2 would still be bricked. Aside from that, the sponsor had indicated their intention to fully migrate to said feed.

In the sponsor's comment in the finding I linked as well as here, they seem to assume that the ezETH/TVL rate falling below 1 implies the feed must be stale or incorrect. However, the ezETH.totalSupply() / TVL ratio is currently below 1 (not just the market price, but the values from onchain data) and it is not at all unreasonable to think that it could fall again below 1 once it rises above. If it falls because of an extraordinary event such as a depeg of a collateral asset, it can stay there for a long time. However, the current RenzoOracleL2 implementation strictly does not allow any functionality on L2s for prices below 1 ETH.

Note also that getRate() reverting will not only "pause" the associated pools. It will allow any user to put the pool in Recovery Mode, with the additional impact described in the finding.

The fact that any configured price provider reverting allows anyone to put the pool into recovery mode, which was designed "to provide a safe way to exit any pool during some kind of emergency, to avoid locking funds in the event the pool enters a non-functional state", shows that the getRate() function is not meant to revert unless the protocol finds itself in an unrecoverable state. However, the price of stETH falling below 1 ETH is far from an unrecoverable state, and the sponsor cannot ensure this will not happen either.

Here, the functionality of the RenzoOracleL2 contract under realistic (and current) conditions, as well as that of the xRenzoDeposit contract as a price provider to Balancer pools, is impacted, which in my opinion justifies medium Severity.

alcueca commented 3 months ago

@jatinj615, could you please provide your reasoning on why the ezETH/TVL won't drop under 1 under realistic scenarios, given that it is currently below 1?

jatinj615 commented 3 months ago

Hey @alcueca , the exchange rate of ezETH is = ETH/ezETH not ezETH/ETH I think that is where the confusion happened. And confirming with the same on chain data provided by warden 1 ezETH ~ 1.01 ETH .

In the realistic conditions Protocol will be making more rewards than accumulated minor slashing hence, incoming rewards will mostly be > 0. Which will compound the ezETH price over time and unless there is a mass slashing event. where protocol looses more than the accumulated rewards, ezETH price is expected to be > 1.

0xEVom commented 3 months ago

Apologies for the oversight. However, that is still very close to 1 ETH, and the rest of my comment still applies - it is not unreasonable to think that it could fall below 1, and in case of a mass slashing event, it would stay there for a long time. I assume in that scenario, you wouldn't want to indefinitely halt all functionality on L2s.

Correct me if I'm wrong, but wouldn't you choose to upgrade the RenzoOracleL2 contract in that case, addressing this finding?

bronzepickaxe commented 3 months ago

Hi Sponsor, Judge, Wardens,

We were looking into mass-slashing events during the original contest so I will adjust my notes a bit to the new information provided in this report and share some additional insights.

For the current ratio to go below 1, more than 1% needs to be wiped out.

Renzo uses Figment as their professional institutional grade operator(docs), which had 0 minor slashing event, major slashing events or catastrophic slashing events in 2023.

Due to the nature of ezETH the ratio of ETH/ezETH will only trend upwards. Any slashing events will be offset by the profit Renzo generates.

A catastrophic slashing event of more than 1%, spread over every validator used by Renzo, controlled by a professional institutional grade operator like [Figment] is not realistic - L2 contracts temporarily not working would be least of our issues because this will have a ripple effect throughout the industry.

If we look at the data of slashed validator:

it becomes evident that this is not realistic. The amount of validators needed to be slashed, simultaneously, operated by a professional operator, is not realistic. Also note that the maximum amount of ETH that can be slashed is around 1.08 ETH per validator. The rest of the ETH of the Validator will bleed when it is not operated correctly by Operator in the days after the slashing event.(more info here)

When we talk about mass-slashing events as security researchers, we often visualize 10000's of Validators being slashed. But, for example, this event has been classified as a mass-slashing event, which was a slashing event of only 17 validators.

This is without the fact that a happy-path has been provided by the Sponsor to immediately patch an issue regarding a slashing event & the fact that slashing events are insured, which would prop up the ratio back to what it was.

To summarize, the current ratio is not >1 as the report claims, it is 1<. Since we are looking to provide value for realistic scenario's, this ratio dropping below 1 with all the factors mentioned above, is not realistic.

Thanks!

alcueca commented 3 months ago

Today I learnt a fair deal about slashings. The risk that the ratio will fall below 1 is already low, and it will only decrease with time under normal operation. Not enough for a Medium severity.

0xEVom commented 3 months ago

Please note that slashing is by no means the only way this can happen (it hadn't even crossed my mind).

What's more relevant here is the risk that the price of any of the collateral assets in the TVL drops. How large that risk is is something I cannot assess, but this finding should certainly not be rejected on the grounds of the unlikelihood of a mass slashing event.

In any case, the price of a collateral asset dropping is not an unrealistic or unreasonable scenario, but one that the protocol should be equipped to deal with. It is certainly also a scenario that should be accounted for in this audit, as multiple findings related to the protocol's behaviour when the price of a collateral assets drops were deemed valid and mitigations applied, such as in the case of H-04.

And in such a scenario, getRate() is not meant to be used as a pausing mechanism for Balancer pools and it reverting essentially breaks an external integration.