Imprecise calculations

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L150-L153

Vulnerability details

Impact

Even if the contract holds as few tokens as a million (with 18 decimals), and the pool holds one millionth of the totalAllocPoint, the division in line #152 may cause too large rounding errors (more than 1% of the value).

Even more severe imprecisions can be observed.

Tools Used

Manual analysis

Recommended Mitigation Steps

At first multiply all values, then preceed to division. This will provide exact results, rounded down. This will also allow for eliminating _concurShareMultiplier variable. It is better to store commulative rewards than an integer with arbitrary precision, which may not be enough.

multiplier is the number of blocks, we can assume it fits within 24 bits - this number of blocks hasn't been yet produced on mainnet.

concurPerBlock is proposed to be equal to 1e14, which fits within 48 bits

pool.allocPoint can be set within 32 bits to provide enough opportunity to weight tokens.

So that the result fits within 104 bits out of 256 available - there is no risk of overflow, even on adding to cummulative rewards.

Of course, the same applies to the calculations within pendingConcur.

[r2moon]

no issues here.

[GalloDaSballo]

I actually agree with the Warden here as there's rounding errors caused by the fact that a division is done on line 120.
Delaying the division would net higher precision.

While the rounding error is there and it can lead to a loss of value, I believe Low Severity is more appropriate as ultimately we're talking about dust amounts and the POC proposed doesn't seem to make an argument for massive values

[JeeberC4]

Adding to warden's QA Report #241