code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

If SafEth.addDerivative() is called with an invalid derivative contract address, all users' staked ETH will be frozen, and contract SafEth will be completely unusable.

Proof of Concept

When adding a new derivative contract through SafEth.addDerivative(), passing an invalid derivative contract address (such as the 0 address, an EOA account address, or any other non-derivative address) will make the following functions unusable:

The stake() will be unusable because L73 will revert.
The unstake() will be unusable because L115 will revert.
The rebalanceToWeights() will be unusable because L141 will revert.

Therefore, all users will not be able to stake any more and will not be able to unstake/withdraw their previously staked assets.

What is more serious is that even setting the weight of the invalid derivative contract to 0 will not fix this problem.

Test code for PoC:

diff --git a/test/SafEth.test.ts b/test/SafEth.test.ts
index 9fdfc3d..3ebd124 100644
--- a/test/SafEth.test.ts
+++ b/test/SafEth.test.ts
@@ -322,6 +322,37 @@ describe("Af Strategy", function () {
     });
   });

+  describe("Invalid derivative", async () => {
+    beforeEach(async () => {
+      snapshot = await takeSnapshot();
+    });
+    afterEach(async () => {
+      await snapshot.restore();
+    });
+    it.only("All funds will be locked when an invalid derivative is added", async () => {
+      // stake wil succeed normally
+      await safEthProxy.stake({ value: ethers.utils.parseEther("100") });
+      // unstake wil succeed normally
+      await safEthProxy.unstake(ethers.utils.parseEther("1"));
+
+      // an invalid derivative is added
+      await safEthProxy.addDerivative(
+        "0x1111111111111111111122222222222222222222",
+        0
+      );
+
+      // stake will revert
+      await expect(
+        safEthProxy.stake({ value: ethers.utils.parseEther("10") })
+      ).to.be.revertedWith("Transaction reverted: function returned an unexpected amount of data");
+
+      // unstake will revert, all funds are locked
+      await expect(
+        safEthProxy.unstake(ethers.utils.parseEther("1"))
+      ).to.be.revertedWith("Transaction reverted: function returned an unexpected amount of data");
+    });
+  });
+
   describe("Upgrades", async () => {
     beforeEach(async () => {
       snapshot = await takeSnapshot();

Test outputs:


  Af Strategy
    Invalid derivative
      ✓ All funds will be locked when an invalid derivative is added

  1 passing (10s)
✨  Done in 16.15s.

Tools Used

VS Code

Recommended Mitigation Steps

Due to the severity of the consequences caused by this issue, we must perform strict checks and validations on the addDerivative().

We can avoid this problem as much as possible by using EIP-165.

All derivative contracts (Reth, SfrxEth, WstEth) should inherit from ERC165, and supportsInterface() should be called when adding a new derivative to make sure the new derivative contract implements the IDerivative.sol interface.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as high quality report

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #709