stake() and rebalanceToWeight() might lose dust (funds) to the contract.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.
stake() and rebalanceToWeight() might lose dust (funds) to the contract. The former might lead some loss of funds to the staker.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The stake() function allows a user to stake ETH into SafEth in exchange for safETH. It deposits ETH in proportion to weights to a list of underlying derivative contracts.

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

The problem is that when calculating the proportion of ETH for each underlying derivative contract, there is some rounding down error due to the use of division by totalWeight. Since the protocol does not have a limit for the weight for each derivative contract, totalWeight could large, let's say when totalWeight = 100e18 (the dev uses 1e18 in their tests), then the round down error might be large as well.

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;
            totalStakeValueEth += derivativeReceivedEthValue;
        }

Similar problem occurs for the rebalanceToWeights() function, which also calculates the portion of ETH that needs to be allocated to each underlying derivative contract, with an implied rounding down error subject to the quantity of totalWeight.

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;
            totalStakeValueEth += derivativeReceivedEthValue;
        }

In summary, in both cases, some ETH dust will be left in the contract and for the case of stake(), it means some loss for the user.

Tools Used

VSCode

Recommended Mitigation Steps

Two mitigations: 1) first, we can restrict the totalWeight <= 10000, which inmplies each weight <= 10000;
2) we will make sure all dust goes to the last derivative contract so that no dust of ETH will be left in SafETH.

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;

        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
+       uint256 remainingEthAmount = msg.value;
        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
+           if(i == derivativeCount - 1) ethAmount = remainingEthAmount; // @audit: last derivative 
            uint256 depositAmount = derivative.deposit{value: ethAmount}();
take all remaining
            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;
            totalStakeValueEth += derivativeReceivedEthValue;
+           remainingEthAmount -= ethAmount;
        }
        // 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);
    }

function rebalanceToWeights() external onlyOwner {
        uint256 ethAmountBefore = address(this).balance;
        for (uint i = 0; i < derivativeCount; i++) {
            if (derivatives[i].balance() > 0)
                derivatives[i].withdraw(derivatives[i].balance());
        }
        uint256 ethAmountAfter = address(this).balance;
        uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;

+       uint256 remainingEth = ethAmountToRebalance;
        for (uint i = 0; i < derivativeCount; i++) {
            if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
            uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
                totalWeight;
+           if(i == derivativeCount-1) ethAmount = remainingEth;
            // Price will change due to slippage
            derivatives[i].deposit{value: ethAmount}();
+           remainingEth -= ethAmount;
        }
        emit Rebalanced();
    }

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #455

[c4-judge]

Picodes marked the issue as satisfactory