search

code-423n4 / 2021-06-gro-findings

0 stars 1 forks source link

burnAll should check that factor > 0 and amount > 0 #87

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

pauliax

Vulnerability details

Impact

burnAll should check that factor > 0 and amount > 0. Whenever applyFactor is supposed to be invoked with a base parameter set to false it is first checked that the factor > 0 and only then calls this function, otherwise returning 0. However, the function burnAll in contract RebasingGToken does not check that the factor > 0 so in this case, the SafeMath could fail when trying to divide by 0. Also, burnAll does not check that burnAmount > 0, which I kinda expected as seen in function burn.

Recommended Mitigation Steps

Call applyFactor only if factor() > 0, use 0 amount otherwise. Require amount > 0.

kitty-the-kat commented 3 years ago

No possibility for factor == 0 when assets are in the system. Min amount is specified by user.

ghoul-sol commented 3 years ago

Indeed, it seems this is not an issue when assets are in the system but technically good practice to check divisor for 0 value. I'll keep it as non-critical.