Asymmetry contest - Emmanuel's results

Lines of code

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

Vulnerability details

Impact

Each derivative has a function ethPerDerivative, which returns the eth price of the derivative.

In the WstEth derivative, ethPerDerivative is meant to be calculated as eth/wsteth, but instead was calculated as steth/wsteth.

This could affect calculations in the stake function that uses ethPerDerivative in L73 and L92 of SafEth contract

Proof of Concept

/**
        @notice - Get price of derivative in terms of ETH
     */
    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
    }// @audit returns steth/wsteth instead of eth/wsteth

Tools Used

Manual review

Recommended Mitigation Steps

Just like the SfrxEth derivative, you may use this:

function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        uint256 stEthAmount = IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
        return ((10 ** 18 * stethAmount) /
            IStEthEthPool(0xc5424b857f758e906013f3555dad202e4bdb4567).get_virtual_price()); // @audit gets steth/eth price
    }/

Lines of code

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

Vulnerability details

Impact

If a user tries to stake when weights of derivatives are set to 0, his eth will be locked in the contract, and no safeth would be minted to him.

There is no guarantee that weights of all derivatives are not 0 because

• Safeth contract is initialized with no derivatives added
• Admin could set derivatives weights to 0 for some reasons.

When a user stakes during this period, his eth is collected, but no safEth would be minted because the for loop, which would increase the value of totalStakedEth, would either - not be entered (in the case admin has not added any derivatives) or

• when all the weights are set to 0, if (weight==0) continue skips the addition.

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

What makes this loss more painful is that there is no way to withdraw eth from the SafEth contract, making all eth sent locked (Unless Contracts are upgraded).

Proof of Concept

You may include and run this test in SafEth.test.ts file.

it("Should not get minted safeth when derivatives weights are 0", async () => {
  const accounts = await ethers.getSigners();
  adminAccount = accounts[0];
  alice = accounts[1];

  // Normal Behaviour
  let aliceSafEthInitialBalance = await safEthProxy.balanceOf(alice.address);
  let aliceEthInitialBalance = await ethers.provider.getBalance(alice.address);
  const amountOfEtherToStake = ethers.utils.parseEther("5");

  await safEthProxy.connect(alice).stake({
    value: amountOfEtherToStake,
  });

  let aliceSafEthFinalBalance = await safEthProxy.balanceOf(alice.address);
  let aliceEthFinalBalance = await ethers.provider.getBalance(alice.address);
  expect(
    aliceSafEthFinalBalance
      .sub(aliceSafEthInitialBalance)
      .gte(amountOfEtherToStake)
  );
  expect(
    aliceEthInitialBalance.sub(aliceEthFinalBalance).lte(amountOfEtherToStake)
  );

  // Abnormal Behaviour

  // adjust weights of derivatives to 0
  await safEthProxy.connect(adminAccount).adjustWeight(0, 0);
  await safEthProxy.connect(adminAccount).adjustWeight(1, 0);
  await safEthProxy.connect(adminAccount).adjustWeight(2, 0);

  aliceSafEthInitialBalance = await safEthProxy.balanceOf(alice.address);
  aliceEthInitialBalance = await ethers.provider.getBalance(alice.address);

  await safEthProxy.connect(alice).stake({
    value: amountOfEtherToStake,
  });

  aliceSafEthFinalBalance = await safEthProxy.balanceOf(alice.address);
  aliceEthFinalBalance = await ethers.provider.getBalance(alice.address);
  // SafEth minted to alice is 0 but eth balance reduced
  expect(aliceSafEthFinalBalance.sub(aliceSafEthInitialBalance)).eq(0);
  expect(
    aliceEthInitialBalance.sub(aliceEthFinalBalance).lte(amountOfEtherToStake)
  );
});

Tools Used

Manual Review

Recommended Mitigation Steps

Consider including this check in the stake function, just before the _mint(msg.sender, mintAmount) call: require(totalStakeValueEth!=0,"Cannot stake eth")