search

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

0 stars 0 forks source link

Mantissa calculations assumes 18 decimals #106

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

tensors

Vulnerability details

Impact

The functions in FixedMath assume the tokens you are working with have 18 decimals. It looks like all yield sources currently use 18 decimals, new tokens added need to be checked.

Proof of Concept

https://github.com/pooltogether/fixed-point/blob/c513c2da18aa5f8787cb84befdb7da9e4a52ae2b/contracts/FixedPoint.sol#L33

https://github.com/pooltogether/fixed-point/blob/c513c2da18aa5f8787cb84befdb7da9e4a52ae2b/contracts/FixedPoint.sol#L59

https://github.com/pooltogether/aave-yield-source/blob/bc65c875f62235b7af55ede92231a495ba091a47/contracts/yield-source/ATokenYieldSource.sol#L145-L149

Dividing by the wrong scale can lose tokens if the number of decimals is <18. If the number of decimals is >18, then the decimals past the 18th digit will be truncated to 0, but this typically doesn't matter.

Recommended Mitigation Steps

Generalize the variables used (1e18) in FixedMath, or double check decimals with new team members before adding yield sources.

asselstine commented 3 years ago

This is a misunderstanding of our fixed point math; we could have used 10, 12, 20 or any other number of decimals. It doesn't care what the denomination of the number is; it simply shifts it 18 decimal places to improve the accuracy of fractions.

We used 18 because it's a familiar number, plenty accurate, and is easy to format.

dmvt commented 3 years ago

Per sponsor's explanation, closing.