Asymmetry contest - nemveer's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L68-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L98 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L93-L95

Vulnerability details

Impact

Users may receive 0 SafETH in exchange for their deposit if the attacker has inflated the derivatives balances, which leads to the loss of their principal due to rounding off in solidity. 

Proof of Concept

Steps to reproduce: -- When TotalSupply() of SafEth = 0 1. Attacker stakes minAmount(0.5 ether) in SafETH.  2. Attacker then unstakes all but 1 wei of SafETH.  3. Attacker then inflates all of the derivatives by directly transferring Z amounts of the respective underlyings (reth, sFRXEth, wStETH in our case) to the respective derivatives address. Thus increasing the balances of all the respective derivatives.  4. Now 1 wei of SafETH is equivalent to Z amount of underlying staked derivatives, thus increasing the preDepositPrice in the stake function.  5. Now if users stake less than Z amount, they will be minted 0 SafETH due to rounding off, while their deposited ETH will be lost to the attacker.  6. Attacker won't have any issues making Z amount as big as possible since 1 wei of SafEth is owned by him. 

Tools Used

Manual Review

Recommended Mitigation Steps 

• Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case, i.e. when totalSupply() == 0, send the first SafETH to the zero address to enable share dilution.
• In stake(), ensure the number of shares to be minted is non-zero: require(mintAmount != 0, "No shares minted");

Lines of code

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

Vulnerability details

Vulnerability Details:

When: reth.balance() increases up to a certain limit such that poolCanDeposit(reth.balance()) is always false.   

Where:  Stake function

Under certain condition mentioned above, reth.ethPerDerivative(reth.balance())  will always return poolPrice() rather than getEthValue, while deposit will still happen in reth pool rather than through uniswap swapping.  Attacker can use this schema to deflate underlying using flashloan in stake function and increase totalStakedValueEth more than deserved. 

Here, derivatives[i].balance() is used, instead of derivatives[i].ethPerDerivative((msg.value*weights[i])/totalWeights)

            underlyingValue +=

                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *

                    derivatives[i].balance()) /

                10 ** 18;

Proof of Concept

Assuming reth.balance()(balance of reth in the derivative) increases to a certain threshold, such poolCanDeposit() will always return false.  

In the below code,  _amount is reth.balance() sent through here.  

        return

            rocketDepositPool.getBalance() + _amount <=

            rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() 

-1. A malicious user will take a huge flashloan of RETH from some other pool and swap it for ETH in the RETH-ETH Uniswap V3 Pool that is used by rethDerivative. 

-2. This will result into RETH price being drastically drop in the pool.  -3. Now, this malicious user will call  stake function in SafETH with high eth. In the stake function, underlying is calculated using totalDerivative[i].balance() rather than _amount or msg.value. Thus ethPerDerivative will return a deflated price of RETH.  -4. This will result in a reduced underlying -> which will in turn reduce preDepositPrice. -5. While on the other hand reth.deposit() will deposit in the rethPool directly rather than using the RETH-ETH Uniswap Pool as PoolCanDeposit() will return true here. 

if (!poolCanDeposit(msg.value)) {

-6. Similarily, totalStakedValueEth will remain normal as ethPerDerivative() will return correct values. 

            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(

                depositAmount

            ) * depositAmount) / 10 ** 18;

            totalStakeValueEth += derivativeReceivedEthValue;

-7. Hence, mintAmount will be inflated as preDepositPrice is deflated and totalStakedValueEth is correct.  -8. The attacker can easily swap and return the remaining flash loan, and the profit from this attack will be much higher than fees. 

Impact

Malicious users are prone to get more shares than deserved. 

Tools Used

Manuel Review

Recommended Mitigation Steps 

Underlying is using ethPerDerivative(derivative[i].balance()) but only reth uses this arguement and that too in a vulnerable fashion. I would suggest to use derivatives[i].ethPerDerivative((msg.value*weights[i])/totalWeights)

-             underlyingValue +=

                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *

                    derivatives[i].balance()) /

                10 ** 18;

Lines of code

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

Vulnerability details

Vulnerability Details:

Where : withdraw() - SfrxEth derivative

Slippage is calculated using the spot price of the curve pool from which swap is being done, which can be easily manipulated for sandwich attacks.

Proof of Concept

• Possibility of sandwich attacks, as when user submits high value transaction in mempool.
• Attacker keeps track and sells massive amount of FRX from FRX-ETH Curve Pool.
• As minOut is calculated from the same inflated pool, the slippage holds no value under this circumstance.

        uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
            (10 ** 18 - maxSlippage)) / 10 ** 18;

        IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
            1,
            0,
            frxEthBalance,
            minOut
        );

• The user gets less amount of Eth then deserved.
• Attacker immediately buys FRX in the next transaction or directly above the user's transaction and makes profit from the arbitrage.

Impact

These attack has two implications:

1. User get less Eth than deserved.
2. Huge losses could occur during rebalanceToWeights()

Tools Used

Manual Review

Recommended Mitigation Steps

In such scenarios, it is recommended that slippage should not be calculated from the spot price of the pool.

Lines of code

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

Vulnerability details

Impact

When the safEth pool is initialised, no derivatives are added in the initialization.  Derivatives are added in a separate function, addDerivative(). Any users trying to stake()  during that gap will lose their ETH as they will get minted 0 safEth in return.

Proof of Concept

Create a new file in test folder and replace the following content:

/* eslint-disable new-cap */
import { network, upgrades, ethers } from "hardhat";
import { expect } from "chai";
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
import { SafEth } from "../typechain-types";


import {
 SnapshotRestorer,
 takeSnapshot,
} from "@nomicfoundation/hardhat-network-helpers";
import { rEthDepositPoolAbi } from "./abi/rEthDepositPoolAbi";
import { RETH_MAX, WSTETH_ADRESS, WSTETH_WHALE } from "./helpers/constants";
import { derivativeAbi } from "./abi/derivativeAbi";
import { getDifferenceRatio } from "./SafEth-Integration.test";
import ERC20 from "@openzeppelin/contracts/build/contracts/ERC20.json";


const initialUpgradeableDeploy = async function () {
 const SafEth = await ethers.getContractFactory("SafEth");
 const safEth = await upgrades.deployProxy(SafEth, [
   "Asymmetry Finance ETH",
   "safETH",
 ]);
 await safEth.deployed();


 return safEth;
};


describe("Af Strategy", function () {
 let adminAccount: SignerWithAddress;
 let safEthProxy: SafEth;
 let initialHardhatBlock: number; // incase we need to reset to where we started


 const resetToBlock = async (blockNumber: number) => {
   await network.provider.request({
     method: "hardhat_reset",
     params: [
       {
         forking: {
           jsonRpcUrl: process.env.MAINNET_URL,
           blockNumber,
         },
       },
     ],
   });


   safEthProxy = (await initialUpgradeableDeploy()) as SafEth;
   const accounts = await ethers.getSigners();
   adminAccount = accounts[0];
 };


 before(async () => {
   const latestBlock = await ethers.provider.getBlock("latest");
   initialHardhatBlock = latestBlock.number;
   await resetToBlock(initialHardhatBlock);
 });


 describe("Loss of user's funds when there's no derivative", function () {
   it.only("Should be minted zero safEth", async function () {
     const depositAmount = ethers.utils.parseEther("200");
     const tx1 = await safEthProxy.connect(adminAccount).stake({ value: depositAmount });
     const mined1 = await tx1.wait();
     expect(await safEthProxy.balanceOf(adminAccount.address)).to.equal(0);
   });
 });


});

Tools Used

Manual Review

Recommended Mitigation Steps

• Check derivativeCount>0 in stake() function, i.e. require(derivativeCount>0,"No derivatives added yet")

1. Rounding off leads to dust being locked in safEth.sol forever

• In stake() function, msg.value is deposited in each derivatives respective to it's weight. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L88

uint256 ethAmount = (msg.value * weight) / totalWeight;

// This is slightly less than ethAmount because slippage
uint256 depositAmount = derivative.deposit{value: ethAmount}();

• But, in doing so, due to rounding off in solidity, there's some dust left in contract. Which should ideally be returned to msg.sender
• Over the time, the amount of dust can become significant and there's no way to get it back from contract.

2. totalStakeValueEth will always be wrong if poolCanDeposit == false

• In case of rETH, amount is deposited into rETH pool if poolCanDeposit == true. Otherwise it is swapped with rETH via uniswap pool. https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L91-L95

uint256 depositAmount = derivative.deposit{value: ethAmount}();
uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
          depositAmount
          ) * depositAmount) / 10 ** 18;
          totalStakeValueEth += derivativeReceivedEthValue;

• But as you can see in the snippet, price is fetched after swap happens, which will always be slightly lower, resulting in totalStakeValueEth being lesser.
• Which in turn, mints the lesser safEth to user

3. gas cost can be reduced by removing the derivative

• In safEth, owner can inactive a derivative market by changing it's weight to zero.
• But it's never removed from the array.
• So, anytime stake, unstake, rebalanceToWeights, adjustWeight or addDerivative is called, it uses unnecessary gas which can be prevented by removing the derivative from array

4. It's possible that amount is deposited into pool and yet price of swap will be fetched

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L92-L94 when: RocketPool's getMaximumDepositPoolSize() is reached right after user's deposit. Proof of Concept:

1. Rocket pool getMaximumDepositPoolSize is at 5000 eth currently.
2. Assuming that current pool deposit of rocket pool are at 4980 eth.
3. Now Alice stake amount is distributed in such a way that 20 eth is about to deposit() in reth derivative. https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L147-L149 As you can seen the derivative will process the deposit as poolCanDeposit function returns true. https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L211-L215 Now to fetch the totalStakedValueEth stake function calls the ethPerDerivative with 20 eth as arguement. But now poolCanDeposit will return false and Uniswap v3 pool prices will be fetched, which is incorrect.
   https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L92-L94

• This may increase or decrease the totalStakedValueEth, depending upon the pool's price, which can influence the users` SafEth proportion.

5. isStakingPaused of lido is not checked during deposit in WstEth.sol

https://github.com/lidofinance/lido-dao/blob/master/contracts/0.4.24/Lido.sol#L679

• As you can see, in _submit function of Lido, there's a check whether the staking is paused
• In such scenario when staking to Lido is paused, WstEth should use the swap method as done in case of rETH(when poolCanDeposit == false)

6. maxSlippage should have some upper limit

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

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

• While, changing the slippage of any derivative contract, there is no check on slippage
• Unintended or accidently set high slippage can lead to significant loss of user's funds while swapping