code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Minter.sol#L101-L108

Vulnerability details

Impact

calculate_growth might end up calculating the wrong rate for update_period._growth due to precision loss.

The formula implementation might also differ from the one that has been shown in the docs, causing an even bigger difference in the result.

Proof of Concept

Code doesnt match with expected output according to scientific calculator and formula presented in the docs.

Formula as prescribed in docs (calculator): (1/2) * 15000000e18 * (1/100000)**3 = 3.66968047 * 10**−7

Formula as implemented (calculator): (((((15000000e18 * 1) / 100000) * 1) / 100000) *1) /100000 /2 = 3669.680468 Code output from same formula above taking preicsion loss into account (REMIX): 7500000000

Obs: values for _veTotal and _veloTotal were arbitrarily chosen.

Tools Used

Online Calculator and REMIX IDE.

Recommended Mitigation Steps

Verify if code implemented reflects expected calculation result and verify if occuring precision loss does not affect functionality.

GalloDaSballo commented 2 years ago

From my own understanding the multiple divisions will just cut out some rounding errors, forcing numbers to be step-wise modulo _veTotal and _veloTotal

Running my own experiments I haven't found realistic examples (minted will always be less than totalSupply) where the rounding causes any meaningful loss

I think the finding is of QA severity