Remaining dust from Ether deposits is not returned to users

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L88

Vulnerability details

When users stake Ether, some dust is left from the calculations corresponding to the Ether that has to be deposited for each derivative. This is due to some precision loss when calculating the weighted amounts.

Impact

Users lose the Ether dust that is not being used for staking. The dust is locked on the SafEth contract.

Proof of Concept

There is some precision loss in line 88: uint256 ethAmount = (msg.value * weight) / totalWeight;

Each weighted ethAmount is deposited for each derivative, but the sum of all those amounts is less than the msg.value. That dust is left on the contract and not returned to the user.

File: File: contracts/SafEth.sol
63:     function stake() external payable {

82: 
83:         uint256 totalStakeValueEth = 0; // total amount of derivatives worth of ETH in system
84:         for (uint i = 0; i < derivativeCount; i++) {
85:             uint256 weight = weights[i];
86:             IDerivative derivative = derivatives[i];
87:             if (weight == 0) continue;
88:             uint256 ethAmount = (msg.value * weight) / totalWeight; // @audit precision loss
89: 
90:             // This is slightly less than ethAmount because slippage
91:             uint256 depositAmount = derivative.deposit{value: ethAmount}();
92:             uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
93:                 depositAmount
94:             ) * depositAmount) / 10 ** 18;
95:             totalStakeValueEth += derivativeReceivedEthValue;
96:         }
97:         // mintAmount represents a percentage of the total assets in the system
98:         uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
99:         _mint(msg.sender, mintAmount);
100:         emit Staked(msg.sender, msg.value, mintAmount);
101:     }

Link to code

Test

Add the following test to the describe("Af Strategy") in test/SafEth.test.ts to prove that Ether dust is locked in the contract:

  describe("Dust", function () {
    it.only("keeps ether dust in contract", async function () {
      const accounts = await ethers.getSigners();
      const user = accounts[0];

      // Verify that the contract has no Ether before the stake
      let safEthBalance = await ethers.provider.getBalance(safEthProxy.address);
      expect(safEthBalance).eq(0);

      // Perform stake
      const depositAmount = ethers.utils.parseEther("1");
      await safEthProxy.connect(user).stake({ value: depositAmount });

      // Verify that the contract now has some extra dust locked
      safEthBalance = await ethers.provider.getBalance(safEthProxy.address);
      expect(safEthBalance).gt(0);
    });
  });

Tools Used

Manual review

Recommended Mitigation Steps

Return the Ether dust not used for staking to the user:

// File: contracts/SafEth.sol

     function stake() external payable {
        // ...

         uint256 totalStakeValueEth = 0; // total amount of derivatives worth of ETH in system
+        uint256 totalValueEth = 0;
         for (uint i = 0; i < derivativeCount; i++) {
             uint256 weight = weights[i];
             IDerivative derivative = derivatives[i];
             if (weight == 0) continue;
             uint256 ethAmount = (msg.value * weight) / totalWeight;
+            totalValueEth += ethAmount;
 
             // This is slightly less than ethAmount because slippage
             uint256 depositAmount = derivative.deposit{value: ethAmount}();
             uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                 depositAmount
             ) * depositAmount) / 10 ** 18;
             totalStakeValueEth += derivativeReceivedEthValue;
         }
         // mintAmount represents a percentage of the total assets in the system
         uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
-        _mint(msg.sender, mintAmount);
-        emit Staked(msg.sender, msg.value, mintAmount);
+        _mint(totalValueEth, mintAmount);
+        uint256 dust = msg.value - totalValueEth;
+        (bool sent, ) = address(msg.sender).call{value: dust}("");
+        require(sent, "Failed to send Ether");
+        emit Staked(msg.sender, totalValueEth, mintAmount);
     }

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[toshiSat]

This will result in more gas spent by user to recover dust therefore resulting in a lower ETH balance overall.

[c4-sponsor]

toshiSat marked the issue as sponsor disputed

[c4-judge]

Picodes marked issue #152 as primary and marked this issue as a duplicate of 152

[c4-judge]

Picodes marked the issue as satisfactory