Division-before-multiplication precision loss issue for update()

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/amplifier/MuteAmplifier.sol#L88-L121

Vulnerability details

Impact

Detailed description of the impact of this finding. There is a division-before-multiplication precision loss issue for update().

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 update() function has a division-before-multiplication precision loss issue.

First, the calculation of perSecondReward uses a division:

uint256 perSecondReward = totalRewards.div(endTime.sub(firstStakeTime));

Then, the calculation of value uses a multiplication.

if (block.timestamp < endTime) {
                value = sinceLastCalc.mul(perSecondReward);
            } else {
                uint256 sinceEndTime = block.timestamp.sub(endTime);
                value = (sinceLastCalc.sub(sinceEndTime)).mul(perSecondReward);
            }

Suppose that the total reward time is one year = 31,536,000 seconds, then the precision loss could be up to 31,536,000, which is significant for perSecondReward.

Tools Used

VScode

Recommended Mitigation Steps

Use the multiplication-after-division pattern

Mitigation: use multiplication-before-division instead

modifier update() {
        if (_mostRecentValueCalcTime == 0) {
            _mostRecentValueCalcTime = firstStakeTime;
        }

        uint256 totalCurrentStake = totalStake();

        if (totalCurrentStake > 0 && _mostRecentValueCalcTime < endTime) {
            uint256 value = 0;
            uint256 sinceLastCalc = block.timestamp.sub(_mostRecentValueCalcTime);
-            uint256 perSecondReward = totalRewards.div(endTime.sub(firstStakeTime));
+            uint256 rewardPeriod = endTime.sub(firstStakeTime);

            if (block.timestamp < endTime) {
-                value = sinceLastCalc.mul(perSecondReward);
+                 value = sinceLastCalc.mul(totalRewards).div(rewardPeriod);
            } else {
                uint256 sinceEndTime = block.timestamp.sub(endTime);
-                value = (sinceLastCalc.sub(sinceEndTime)).mul(perSecondReward);
+                value = (sinceLastCalc.sub(sinceEndTime)).mul(totalRewards).div(rewardPeriod);
            }

            _totalWeight = _totalWeight.add(value.mul(10**18).div(totalCurrentStake));

            _mostRecentValueCalcTime = block.timestamp;

            (uint fee0, uint fee1) = IMuteSwitchPairDynamic(lpToken).claimFees();

            _totalWeightFee0 = _totalWeightFee0.add(fee0.mul(10**18).div(totalCurrentStake));
            _totalWeightFee1 = _totalWeightFee1.add(fee1.mul(10**18).div(totalCurrentStake));

            totalFees0 = totalFees0.add(fee0);
            totalFees1 = totalFees1.add(fee1);
        }

        _;
    }

[0xA5DF]

Nice find, but I think this would lead to a loss of only 31,536,000 wei in the example above, which is only 3.1e-11 wad and not that significant

[0xA5DF]

No, rounding trims only up to one. Meaning loss per second can be maximum 1 wei (the trimmed variable is perSecondReward which holds the dimension of wei), therefore the total loss is maximum 31e6 wei per year.

[c4-sponsor]

mattt21 marked the issue as sponsor confirmed

[Picodes]

As @0xA5DF points out, the loss of precision is of 1 * some timestamp so is < 31e6. However, as the contract uses an ERC20 which may have any decimal and value, Med severity seems appropriate.

[c4-judge]

Picodes marked the issue as satisfactory

[0xA5DF]

However, as the contract uses an ERC20 which may have any decimal and value

I think this issue impacts only the reward token, which is dMute that has 18 decimals.

[HollaDieWaldfee100]

Agree with @0xA5DF. In the case of the 18 decimal token the loss is only a dust amount

[Picodes]

You have a point. So considering the amount cannot be significant, I'll downgrade to Low. Thanks for flagging.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[chaduke3730]

I agree with the decision.