SafEth.addDerivative() does not check whether the newly added derivative contract has already been added before

[code423n4]

Lines of code

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

Vulnerability details

Impact

If the same derivative contract is added multiple times through SafEth.addDerivative(), users can steal additional tokens by SafEth.unstaking. Furthermore, this error cannot be fixed by setting the derivative weight to 0.

Proof of Concept

First, by examining the implementation of SafEth.addDerivative(), we can easily find that it does not check whether the newly added derivative contract already exists in derivatives. So the same derivative contract can be added multiple times.

Then, from the SafEth.unstaking below, we can see that if there are duplicate derivative contract addresses in derivatives, it will be withdraw() repeatedly, resulting in the msg.sender obtaining additional ETH from that derivative.

function unstake(uint256 _safEthAmount) external {
    ...
    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);
    }
    ...
    uint256 ethAmountAfter = address(this).balance;
    uint256 ethAmountToWithdraw = ethAmountAfter - ethAmountBefore;
    // solhint-disable-next-line
    (bool sent, ) = address(msg.sender).call{value: ethAmountToWithdraw}(
        ""
    );
    ...
}

The following test code simulates an occurrence of this error:

1. Initial state: SafEth sets up three derivative contracts with the same weights, and other users have staked 100 ETH.
2. Alice stakes 1 ETH.
3. SafEth.addDerivative() is called with an existing derivative contract addresses
4. Alice unstakes all this tokens.
5. Checking the results: it can be found that the assets unstaked by Alice are approximately 1.3 ETH(because the duplicated derivative contract was withdrawn twice).

Test code for PoC:

diff --git a/test/SafEth.test.ts b/test/SafEth.test.ts
index 9fdfc3d..e908215 100644
--- a/test/SafEth.test.ts
+++ b/test/SafEth.test.ts
@@ -566,6 +566,47 @@ describe("Af Strategy", function () {
         within1Percent(balanceBefore, balanceAfter.add(totalNetworkFee))
       ).eq(true);
     });
+
+    it.only("Steal funds when duplicate derivatives are configured", async () => {
+      const derivativeCount = (await safEthProxy.derivativeCount()).toNumber();
+
+      const initialWeight = BigNumber.from("1000000000000000000");
+      // set all derivatives to the same weight and stake
+      for (let i = 0; i < derivativeCount; i++) {
+        await safEthProxy.adjustWeight(i, initialWeight);
+      }
+      // prepare some stakes
+      await safEthProxy.stake({ value: ethers.utils.parseEther("100") });
+
+      const accounts = await ethers.getSigners();
+      const alice = accounts[2];
+      const depositValue = ethers.utils.parseEther("1");
+      const balanceBefore = await alice.getBalance();
+      let totalNetworkFee = BigNumber.from(0);
+
+      // alice stakes 1 ETH
+      const tx1 = await safEthProxy.connect(alice).stake({ value: depositValue });
+      const mined1 = await tx1.wait();
+      totalNetworkFee = totalNetworkFee.add(mined1.gasUsed.mul(mined1.effectiveGasPrice));
+
+      // an existing derivative is added again
+      const existedDerivative = await safEthProxy.derivatives(0);
+      await safEthProxy.addDerivative(existedDerivative, 0);
+
+      // alice unstake
+      const tx2 = await safEthProxy.connect(alice).unstake(
+        await safEthProxy.balanceOf(alice.address)
+      );
+      const mined2 = await tx2.wait();
+      totalNetworkFee = totalNetworkFee.add(mined2.gasUsed.mul(mined2.effectiveGasPrice));
+
+      const balanceAfter = await alice.getBalance();
+      const balanceDiff = balanceAfter.add(totalNetworkFee).sub(balanceBefore);
+
+      // balanceDiff ~= depositValue * 30%
+      // alice receives an additional 30% of the value deposited
+      expect(balanceDiff).to.gt(depositValue.mul(30).div(100));
+    });
   });

   // get estimated total eth value of each derivative

Test outputs:

  Af Strategy
    Weights & Rebalance
      ✓ Steal funds when duplicate derivatives are configured
  1 passing (10s)
✨  Done in 15.91s.

Tools Used

VS Code

Recommended Mitigation Steps

We should check in SafEth.addDerivative() whether the newly added contract already exists, and revert if it does.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #302

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)