Asymmetry contest - 7siech's results

Lines of code

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

Vulnerability details

Impact

Complete loss of user funds when staking with no active derivatives

Proof of Concept

Relevant code snippet from the contract

        uint256 totalStakeValueEth = 0; // total amount of derivatives worth of ETH in system
        for (uint i = 0; i < derivativeCount; i++) {
            uint256 weight = weights[i];
            IDerivative derivative = derivatives[i];

            // <---
            // @audit skips all derivatives when all zero weights
            if (weight == 0) continue;
            // -->

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

            // This is slightly less than ethAmount because slippage
            uint256 depositAmount = derivative.deposit{value: ethAmount}();
            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;
            totalStakeValueEth += derivativeReceivedEthValue;
        }
        // mintAmount represents a percentage of the total assets in the system
        uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
        _mint(msg.sender, mintAmount);

Relevant POC code snippet demonstrating user loss of funds

contract LossOfFunds is Test {

  SafEth safEth;
  SfrxEth sfrxEth;
  WstEth wstEth;
  Reth reth;
  address admin = address(0xcafe);
  address user = address(0xdeadbeef);

  function setUp() public {
    vm.createSelectFork("mainnet");

    // deploy contracts
    safEth = SafEth(payable(address(
      new ERC1967Proxy(address(
        new SafEth()),
        abi.encodeWithSelector(SafEth.initialize.selector, "safEth", "seth")))));
    safEth.transferOwnership(admin);

    sfrxEth = SfrxEth(payable(address(
      new ERC1967Proxy(address(
        new SfrxEth()),
        abi.encodeWithSelector(SfrxEth.initialize.selector, address(safEth))))));

    reth = Reth(payable(address(
      new ERC1967Proxy(address(
        new Reth()),
        abi.encodeWithSelector(Reth.initialize.selector, address(safEth))))));

    wstEth = WstEth(payable(address(
      new ERC1967Proxy(address(
        new WstEth()),
        abi.encodeWithSelector(WstEth.initialize.selector, address(safEth))))));

    // equal weights
    uint weight = 1e27;
    vm.startPrank(admin);
      safEth.addDerivative(address(sfrxEth), weight);
      safEth.addDerivative(address(reth), weight);
      safEth.addDerivative(address(wstEth), weight);
    vm.stopPrank();
  }

  function testStuckFunds() public {
    // admin accidentiall disables all derivatives
    for (uint i; i < safEth.derivativeCount(); ++i) {
      vm.prank(admin);
      safEth.adjustWeight(i, 0);
    }

    // ensure user balance of shares is zero
    uint balanceBefore = safEth.balanceOf(user);
    assertEq(balanceBefore, 0);

    // get some ETH
    uint value = 10 ether;
    vm.deal(user, value);

    // stake
    vm.prank(user);
    safEth.stake{value: value}();
    
    // get user balance shares
    uint balanceAfter = safEth.balanceOf(user);
    // convince ourselves user did not get any shares 
    assertEq(balanceAfter, 0);
    // and has no ETH
    assertEq(address(user).balance, 0);
    // and contract has ETH stuck in the contract
    assertEq(address(safEth).balance, 10 ether);
  }
}

Full POC https://gist.github.com/alpeware/41123fa62461bd5192d4bd430bc29499#file-lossoffunds-t-sol

POC logs https://gist.github.com/alpeware/41123fa62461bd5192d4bd430bc29499#file-log-txt

Tools Used

Foundry

Recommended Mitigation Steps

Add an additional require statement to stake function -

require(totalWeight > 0, "no active derivatives")

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L88

Vulnerability details

Impact

Loss of precision can result in lost user funds during staking.

Proof of Concept

Depending on the weights and msg.value, not the total value of ETH sent to stake will be deposited into derivatives and thus left over in the contract since there's no way to pull ETH out of the contract.

Here's the relevant code snippet from the contract -

        uint256 totalStakeValueEth = 0; // total amount of derivatives worth of ETH in system
        for (uint i = 0; i < derivativeCount; i++) {
            uint256 weight = weights[i];
            IDerivative derivative = derivatives[i];
            if (weight == 0) continue;

            // <---
            //   @audit loss of precision creates residue ETH stuck in the contract
            uint256 ethAmount = (msg.value * weight) / totalWeight;
            // --->

            // This is slightly less than ethAmount because slippage
            uint256 depositAmount = derivative.deposit{value: ethAmount}();
            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;
            totalStakeValueEth += derivativeReceivedEthValue;
        }
        // mintAmount represents a percentage of the total assets in the system
        uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

Relevant code snippet from POC

contract LostFunds is Test {

  SafEth safEth;
  SfrxEth sfrxEth;
  WstEth wstEth;
  Reth reth;
  address admin = address(0xcafe);
  address user = address(0xdeadbeef);

  function setUp() public {
    vm.createSelectFork("mainnet");

    // deploy contracts
    safEth = SafEth(payable(address(
      new ERC1967Proxy(address(
        new SafEth()),
        abi.encodeWithSelector(SafEth.initialize.selector, "safEth", "seth")))));
    safEth.transferOwnership(admin);

    sfrxEth = SfrxEth(payable(address(
      new ERC1967Proxy(address(
        new SfrxEth()),
        abi.encodeWithSelector(SfrxEth.initialize.selector, address(safEth))))));

    reth = Reth(payable(address(
      new ERC1967Proxy(address(
        new Reth()),
        abi.encodeWithSelector(Reth.initialize.selector, address(safEth))))));

    wstEth = WstEth(payable(address(
      new ERC1967Proxy(address(
        new WstEth()),
        abi.encodeWithSelector(WstEth.initialize.selector, address(safEth))))));

    // equal weights
    uint weight = 1e27;
    vm.startPrank(admin);
      safEth.addDerivative(address(sfrxEth), weight);
      safEth.addDerivative(address(reth), weight);
      safEth.addDerivative(address(wstEth), weight);
    vm.stopPrank();
  }

  function testStuckFunds() public {
    // ensure contract balance is zero
    uint balanceBefore = address(safEth).balance;
    assertEq(balanceBefore, 0);

    // example value leaving residue
    uint value = 111882089711632596292;
    // get some ETH
    vm.deal(user, value);

    // stake
    vm.prank(user);
    safEth.stake{value: value}();

    // get contract balance
    uint balanceAfter = address(safEth).balance;
    // convince ourselves not all ETH has been deployed
    assertTrue(balanceAfter > 0);
  }
}

Full POC here https://gist.github.com/alpeware/7bd7e481efab941934256b34a14c97fb#file-lostfunds-t-sol

Logs https://gist.github.com/alpeware/7bd7e481efab941934256b34a14c97fb#file-logs-txt

Tools Used

Foundry

Recommended Mitigation Steps

• if (msg.value - totalStakeValueEth) is > 0, add to a randomly chosen derivative

• add a new external function stakeBalance allowing anyone to distribute the contract's balance across all derivatives without the contract taking any shares (this would also address the issue with ETH directly sent to the contract for any other reason)