Precision loss in stake function affects share calculation

[code423n4]

Lines of code

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

Vulnerability details

The stake function in SafEth performs divisions before multiplications, which incur in unnecesary precission loss errors, along with other divisions that could be avoided.

Impact

Stakers receive less SafEth shares than expected if calculated in a way to minimize precision errors.

Proof of Concept

There are several blocks on the stake function that could be improved to reduce precision loss.

Link to stake function

On the for block, the underlyingValue is divided by 10 ** 18 for each derivative. This number can be divided just once, to minimize precision loss from each division.

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

Later the underlyingValue is used to calculate the preDepositPrice, and with that the mintAmount:

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;

// ...

uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

There's a division before multiplication covered on the mintAmount calculation, that can be avoided as demonstrated here to reduce precision loss:

uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / ((10 ** 18 * underlyingValue) / totalSupply);
uint256 mintAmount = totalStakeValueEth * totalSupply / underlyingValue;

Same thing happens with totalStakeValueEth which covers another division before multiplication. And also dividing multiple times instead of just once:

uint256 totalStakeValueEth = 0; // total amount of derivatives worth of ETH in system
for (uint i = 0; i < derivativeCount; i++) {
    // ...

    uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
        depositAmount
    ) * depositAmount) / 10 ** 18; // @audit
    totalStakeValueEth += derivativeReceivedEthValue;
}

uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

Tools Used

Manual review

Recommended Mitigation Steps

Refactor the stake function to reduce precision loss by avoiding divisions before multiplications, and minimize errors by dividing only once when possible.

Here's a suggested refactoring, moving divisions to the end of the calculation, and simplifying factors that cancel each other like 10 ** 18 / 10 ** 18.

    function stake() external payable {
        require(pauseStaking == false, "staking is paused");
        require(msg.value >= minAmount, "amount too low");
        require(msg.value <= maxAmount, "amount too high");

        uint256 underlyingValue = 0;

        // 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;
+                    derivatives[i].balance());

        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;

        uint256 totalStakeValueEth = 0; // total amount of derivatives worth of ETH in system
        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;

            // This is slightly less than ethAmount because slippage
            uint256 depositAmount = derivative.deposit{value: ethAmount}();
            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
-           ) * depositAmount) / 10 ** 18;
+           ) * depositAmount);
            totalStakeValueEth += derivativeReceivedEthValue;
        }
        // mintAmount represents a percentage of the total assets in the system
-        uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
+        uint256 mintAmount = totalSupply == 0
+            ? totalStakeValueEth / 10 ** 18
+            : totalStakeValueEth * totalSupply / underlyingValue;
        _mint(msg.sender, mintAmount);
        emit Staked(msg.sender, msg.value, mintAmount);
    }

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

toshiSat marked the issue as disagree with severity

[toshiSat]

QA, I'm not seeing the precision errors

[c4-sponsor]

toshiSat marked the issue as sponsor acknowledged

[c4-judge]

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

[c4-judge]

Picodes marked the issue as satisfactory