Asymmetry contest - bytes032's results

Lines of code

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

Vulnerability details

Impact

Direct theft of funds from other stakers

The "stake" function allows external users to stake funds into a derivatives system in exchange in safEth. The function performs a few checks on the staking request to ensure it meets specific criteria, such as minimum and maximum values.

The function then calculates the underlying value of each derivative object in terms of ETH, using a for loop to iterate over each object in an array. The total underlying value is then used to calculate the pre-deposit price of the safETH token.

The underlying value is calculated using the formula (ethPerDerivative / the balance of the derivative) / 1e18

The balance of the derivative is fetched in each function by calling the underlying token of the derivative, e.g., IERC20(underlying).balanceOf(address(this)); 1, 2, 3 where the implementation of ethPerDerivative can vary. 1, 2, 3

After that, if the supply is zero it initializes it with a price of 1 otherwise, it calculates it with the following formula.

(10 ** 18 * underlyingValue) / totalSupply.

However, because underlyingValue can be influenced externally and depositors can influence the totalSupply, this can lead to a complex attack vector which would result into theft of funds for other depositors.

Consider the following scenario:

Bob deposits 0.5e18 (because there's a min deposit requirement) and receives 0,5001992725 safEth in exchange.

Now, if Alice is to stake 200e18, the price would be 1000000006116652746 and she would receive 200079708657045533684 safEth in exchange.

However, Bob has another plan. Immediately after his staking, he unstakes 0.5e18 - 1 wei, which leaves him with just 1 safEth.

Now if Alice is to stake, underlyingValue equal 2 totalSupply would equal 1

so if she stakes 200e18 now, the price equals ( 1e18 * 2 ) / 2 , hence she would get 100039855415051776431 safEth in exchange, which is not a problem yet, because she can withdraw them with a very minimal difference (~0.25e18), so Alice decides to proceed.

When Alice submits her transaction, Bob immediately notices it in the mempool and generously donates 100e18 to the SfrxEth derivative and the WstETH derivative, which directly would manipulate this computation.

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

Now that underlyingValue is manipulated, the price equals the astonishing 100079713278686337891000000000000000000, so as a result Alice gets only 1 safEth in exchange.

Knowing what happened, Bob is very quick to unstake his 1 safEth, yielding him ~ 49,80 ETH profit.

To prove this, I've built a PoC using the test suite of the protocol, with two very small modifications so it's easier to use.

I've added this function to WstEth and SfrxEth to simulate Bob's donation easier.

  function feedMe () external payable {
    (bool sent, ) = WST_ETH.call{ value: msg.value }("");
  }

And additionally, I have set Reth's weight to 0 just to simplify the example.

it("Direct theft of funds", async function () {
      const startingBalance = await adminAccount.getBalance();
      console.log("Bob balance before attack:",ethers.utils.formatEther(startingBalance));
      await safEthProxy.connect(adminAccount).stake({ value: ethers.utils.parseEther("0.5") });
      let balance = await safEthProxy.balanceOf(adminAccount.address)

      await safEthProxy.connect(adminAccount).unstake(
        balance.sub(1)
      );      

      let der1 = await safEthProxy.derivatives(1);
      let der2 = await safEthProxy.derivatives(2);

      const iface = new ethers.utils.Interface(['function feedMe()']);
      const data = iface.encodeFunctionData('feedMe');

      // SfrxEth
      await adminAccount.sendTransaction({
        data: data,
        to: der1,
        value: ethers.utils.parseEther("100")
      })

      // WstETH
      await adminAccount.sendTransaction({
        data: data,
        to: der2,
        value: ethers.utils.parseEther("100")
      })


      console.log("Alice balance before staking:",ethers.utils.formatEther(await alice.getBalance()));
      await safEthProxy.connect(alice).stake({ value: ethers.utils.parseEther("200") });
      let aliceSafEthBalance = await safEthProxy.balanceOf(alice.address);
      

      await safEthProxy.connect(adminAccount).unstake(1);

      console.log("Bob balance after attack:",ethers.utils.formatEther(await adminAccount.getBalance()));

      await safEthProxy.connect(alice).unstake(aliceSafEthBalance);

      console.log("Alice balance after unstaking:",ethers.utils.formatEther(await alice.getBalance()));
    
    });

Running yarn hardhat test ./test/SafEth.test.ts --grep "Direct theft of funds" in the console, yields the following result.

Tools Used

Manual review, Hardhat

Recommended Mitigation Steps

This type of attack always has different nuances and variations, so in general, relying on external calls that can be unexpectedly manipulated is not recommended.

1. Implement a lock mechanism: Introduce a lock mechanism that prevents users from unstaking their funds immediately after staking. This can be done by setting a minimum lock duration, during which users cannot unstake their funds. This will prevent attackers from manipulating the underlying value and total supply in the short term.
2. Average the price over a certain period: Instead of using the current underlying value and total supply to determine the price, use a moving average over a certain period. This would reduce the effect of sudden changes in the underlying value and total supply on the price calculation.

Lines of code

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

Vulnerability details

Impact

The addition of invalid derivative contracts can severely hinder the functionality of the SafEth contract, rendering it useless until a new contract is deployed.

Proof of Concept

  function addDerivative(
    address _contractAddress,
    uint256 _weight
  ) external onlyOwner {
    // @audit contractAddress can be 0?
    derivatives[derivativeCount] = IDerivative(_contractAddress);
    // @audit weight can be 0?
    weights[derivativeCount] = _weight;
    derivativeCount++;

    uint256 localTotalWeight = 0;
    for (uint256 i = 0; i < derivativeCount; i++)
      localTotalWeight += weights[i];
    totalWeight = localTotalWeight;
    emit DerivativeAdded(_contractAddress, _weight, derivativeCount);
  }

The "addDerivative" function adds a new derivative contract to the list of existing derivatives stored in the contract. The contract's owner can only execute this function. The function requires two inputs, the address of the derivative contract to be added and its associated weight.

The function starts by adding the derivative contract to the "derivatives" mapping, associating it with the current count of derivatives stored. Next, the weight of the derivative is also added to the "weights" mapping and associated with the current count of derivatives held. The "derivativeCount" variable is then incremented to keep track of the number of derivatives stored.

Even though the function can be called only by the owner, that doesn't remove the fact that if the owner adds a derivative with not supported/zero address, it will brick pretty much all the functionality of the contract with no way to fix it except for deploying a new one.

The issue here is that if there's a corrupt/not supported/zero address derivative, the full functionality of the protocol is bricked because of two reasons:

1. All the calls to stake/unstake will revert because the functions always go through derivativeCount and reference them through derivatives[i] to call withdraw/deposit/ethPerDerivative, etc.
2. There's no way to change the corrupt/not supported/zero address derivative

The vulnerability itself is in the stake/unstake. The contract always goes through derivativeCount and references them by derivatives[i], which will revert if the address it is trying to call is not supported or is zero. Meaning if there's a corrupt derivative, the whole functionality is bricked, because there's no way to change the contractAddress of already existing derivatives and every

Lastly, adding a duplicate derivative can also be problematic for the protocol in various ways.

To prove this point, I've edited one of the tests.

describe("Large Amounts", function () {
    it("Should deposit and withdraw a large amount with minimal loss from slippage", async function () {
      const startingBalance = await adminAccount.getBalance();
      const depositAmount = ethers.utils.parseEther("200");

      await safEthProxy.addDerivative(
        "0xc429dd84c9a9a7c786764c7dcaF31e30bd35BcdF",
        0
      );

      const tx1 = await safEthProxy.stake({ value: depositAmount });

      const mined1 = await tx1.wait();
      const networkFee1 = mined1.gasUsed.mul(mined1.effectiveGasPrice);

      const tx2 = await safEthProxy.unstake(
        await safEthProxy.balanceOf(adminAccount.address)
      );
      const mined2 = await tx2.wait();
      const networkFee2 = mined2.gasUsed.mul(mined2.effectiveGasPrice);
      const finalBalance = await adminAccount.getBalance();

      expect(
        within1Percent(
          finalBalance.add(networkFee1).add(networkFee2),
          startingBalance
        )
      ).eq(true);
    });

Running yarn hardhat test ./test/SafEth.test.ts --grep "Should deposit and withdraw a large amount with minimal loss from slippage"

Yields this result

After discussing this with the protocol team, I further confirmed that this is a legit issue. Toshi agreed it makes sense to add some kind of protection towards that.

Taking into consideration the potential impact of the mentioned issues on the protocol's functionality and availability and referring to the docs, I would rate this vulnerability as a medium risk

Tools Used

Manual Review, Hardhat

Recommended Mitigation Steps

To mitigate the issues associated with adding corrupt, not supported, zero address, or duplicate derivatives, the following approaches can be taken:

1. Allow removal of derivatives: Modify the existing implementation by adding a function that enables the contract owner to remove derivatives from the protocol. This will allow for removing problematic derivatives that may have been accidentally added or are causing issues.
2. Implement positive validation and allow listing: Adopt a positive validation approach, where only whitelisted derivatives can be added to the protocol. Additionally, incorporate ERC165 standard for contract validation to ensure that only valid derivatives are added.

import "@openzeppelin/contracts/introspection/ERC165.sol";

mapping(address => bool) public allowedDerivatives;

function allowDerivative(address derivative) external onlyOwner {
    allowedDerivatives[derivative] = true;
}

function disallowDerivative(address derivative) external onlyOwner {
    allowedDerivatives[derivative] = false;
}

function addDerivative(address derivative, uint256 weight) external onlyOwner {
    require(allowedDerivatives[derivative], "Derivative not allowed");
    require(ERC165(derivative).supportsInterface(/*derivative specific interface*/), "Invalid derivative");
    ...
}

3. Prevent duplicate derivatives: Maintain a mapping of the currently added derivatives by their address key, ensuring that no duplicates are added to the protocol.

mapping(address => bool) public isDerivativeAdded;

function addDerivative(address derivative, uint256 weight) external onlyOwner {
    require(!isDerivativeAdded[derivative], "Duplicate derivative");
    ...
    isDerivativeAdded[derivative] = true;
}

function removeDerivative(uint256 index) external onlyOwner {
    ...
    isDerivativeAdded[derivatives[index]] = false;
}

Lines of code

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

Vulnerability details

Impact

The impact of this issue is that users can potentially lose a portion of their funds when withdrawing due to fluctuations in the SfrxEth derivative price caused by the use of Curve's Oracle. Although this is a niche scenario, it can still result in significant user losses under certain circumstances.

Proof of Concept

The ethPerDerivative function is used by SafEth when staking to define the deposit price and calculate the underlying value.

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

...

      uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
        depositAmount
      ) * depositAmount) / 10 ** 18;

...

But in SfrxEth it is also used when withdrawing.

    function withdraw(uint256 _amount) external onlyOwner {

        IsFrxEth(SFRX_ETH_ADDRESS).redeem(
            _amount,
            address(this),
            address(this)
        );
        uint256 frxEthBalance = IERC20(FRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        IsFrxEth(FRX_ETH_ADDRESS).approve(
            FRX_ETH_CRV_POOL_ADDRESS,
            frxEthBalance
        );

        uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
            (10 ** 18 - maxSlippage)) / 10 ** 18;
            
        IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
            1,
            0,
            frxEthBalance,
            minOut
        );
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

Every derivative has its own ethPerDerivative implementation, Reth, WstEth, SfrxEth However, the issue lies in the calculation of the SfrxEth price.

    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
            10 ** 18
        );
        return ((10 ** 18 * frxAmount) /
            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());
    }

The function uses a curve pool as a price oracle, specifically the price_oracle function.

The price_oracle() function works as follows:

def price_oracle() -> uint256:
    amp: uint256 = self._A()
    xp: uint256[N_COINS] = self.balances
    D: uint256 = self._get_D(xp, amp)
    return self._ma_price(xp, amp, D)

Get the current value of amp by calling the _A() function. amp represents the amplification factor of the pool, which is a measure of the pool's liquidity. Fetch the current pool balances of all coins, stored in the balances array. Calculate the invariant D by calling the _get_D(xp, amp) function with the pool balances xp and amplification factor amp. D is the invariant value of the pool, and it is used to maintain the pool's liquidity. Calculate the price using the _ma_price(xp, amp, D) function, which returns the moving average price of the asset. This function internally calls the_get_p(xp, amp, D)function to calculate the raw price, and then applies an exponential moving average (EMA) to smooth out the price over time. The key functions used within price_oracle() are:

_A(): This function returns the current amplification factor, considering any ongoing ramping up or down. _get_D(xp, amp): This function calculates the invariant D of the pool, which is a measure of the pool's liquidity. It uses an iterative approach to converge to the value of D. _get_p(xp, amp, D): This function calculates the raw price of one asset in terms of another. It utilizes the amplification factor amp, pool balances xp, and invariant D to compute the price. _ma_price(xp, amp, D): This function calculates the moving average price of an asset by applying an exponential moving average (EMA) to the raw price obtained from the _get_p(xp, amp, D) function.

In summary, the price_oracle() function calculates the moving average price of one asset in terms of another, considering the current state of the pool, amplification factor, and invariant.

In most cases, the price would be reasonably stable, but there are scenarios when depositing and removing liquidity combined by changing EMA can result in fluctuations that will be enough for the users to lose funds.

To prove that, I've built a PoC in foundry, which shows a price difference of 0,0019105773 between just two blocks.

// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.17;
import "forge-std/Test.sol";
import "forge-std/console.sol";
import { ERC20 } from "solmate/tokens/ERC20.sol";

contract CurveOraclePriceTest is Test {
    ICurvePool curvePool = ICurvePool(0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577);


    function setUp() public {
        vm.createSelectFork(
            "https://eth.llamarpc.com",
            15889435
        );
    }

    function testPrice() public {
        console.log(curvePool.price_oracle());
    }

}

Running the test with block 15889435 yields the following result:

Re-running it again with block 15889436 shows a significant change:

This means that if a user withdraws 3000 eth and 1000 of them go through the Sfrx derivative, this could potentially result to around 1,9105773 loss for the user when withdrawing if the transaction doesn't get executed in the same block.

Finally, this fluctuation happens right after this transaction, which calls add_liquidity for 328 ETH.

Even though that's quite a niche and a long shot, I'm rating this as a high because there's a real chance that assets can be partially lost

Tools Used

Manual review, Etherscan, Foundry

Recommended Mitigation Steps

If you insist on oracles, it is recommended to move away from Curve's oracle and use Chainlink. Chainlink's oracles are based on VWAP, which considers the volume of the traded asset and provides a more accurate reflection of the true market value over a specific time period. This approach will help reduce the potential for slippage in scenarios like this.

Additionally, you will have out-of-the-box protection for stale prices, invalid data, etc.

I recommend using the frxAmount directly because the Curve pool calculates the price using its price calculation functionality during the exchange anyway.

Lines of code

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

Vulnerability details

Impact

When the deposit or withdrawal functionality of a derivative is paused, it can cause the entire contract to become inoperable, as the staking and unstaking functions are designed to call each derivative sequentially. This can significantly impact users and potentially disrupt the contract's normal operations.

Proof of Concept

When staking ETH, the stake function goes through all the derivatives and calls their deposit function.

    for (uint i = 0; i < derivativeCount; i++) {
      uint256 weight = weights[i];
      IDerivative derivative = derivatives[i];
      if (weight == 0) continue;


      // @audit check for anomalies with truncation and with min/max msg.value
      uint256 ethAmount = (msg.value * weight) / totalWeight;

      // This is slightly less than ethAmount because slippage
      uint256 depositAmount = derivative.deposit{ value: ethAmount }();

The same applies when unstaking.

function unstake(uint256 _safEthAmount) external {
    require(pauseUnstaking == false, "unstaking is paused");
    uint256 safEthTotalSupply = totalSupply();

    uint256 ethAmountBefore = address(this).balance;
    for (uint256 i = 0; i < derivativeCount; i++) {
      // withdraw a percentage of each asset based on the amount of safETH

      uint256 derivativeAmount = (derivatives[i].balance() * _safEthAmount) /
        safEthTotalSupply;

      if (derivativeAmount == 0) continue; // if derivative empty ignore
      derivatives[i].withdraw(derivativeAmount);

This can be problematic because if the external calls in the deposit/withdraw functions of the derivative are paused, it will revert the whole transaction, even though all the other derivatives are working correctly.

When staking, this can be kind of mitigated to an extent by setting the paused derivative weight to 0

    for (uint i = 0; i < derivativeCount; i++) {
      uint256 weight = weights[i];
      IDerivative derivative = derivatives[i];
      if (weight == 0) continue;

However, the same cannot be applied when withdrawing. Depending on the circumstances, the deposit functionality will be bricked until the protocol team sets the paused derivative weight to 0, OR the withdrawing functionality will be bricked entirely until... the dependency gets unpaused.

Here are some scenarios where that can happen. When withdrawing, the issue could be within WstEth derivative. It has an external call which is approval call to StETH.

function withdraw(uint256 _amount) external onlyOwner {
    IWStETH(WST_ETH).unwrap(_amount);
    uint256 stEthBal = IERC20(STETH_TOKEN).balanceOf(address(this));
    IERC20(STETH_TOKEN).approve(LIDO_CRV_POOL, stEthBal);

The issue is that the approval functionality of StETH might be paused.

On the other hand, when depositing, the issue could be within SfrxEth. It's deposit function makes an external call to the frxETHMinterContract

frxETHMinterContract.submitAndDeposit{value: msg.value}(address(this));

Or withThe problem is that submitAndDeposit makes a call to _submit, which has a check which could be used to revert if its paused.

The same applies for StETH

Taking into consideration the potential impact of the mentioned issues on the protocol's functionality and availability and referring to the docs, I would rate this vulnerability as a medium risk

Tools Used

Manual Review

Recommended Mitigation Steps

The Reth derivative already includes a poolCanDeposit check, it is advisable to incorporate comparable features for all deposit and withdrawal functions. This is particularly important since the contract's staking and unstaking processes are sequenced by iterating through each derivative.