code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L38 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L72 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L90

Vulnerability details

Impact

setDefaultFeePercentage, setCustomFeePercentageForCollateral, setCustomFeePercentageForSafe functions assume that the underlying ERC20 token has 18 decimal digits. Whilst this is true most of the time, an ERC20 token can have a different decimals value. These functions would not return the intended values if the token decimals is different than 18. In such a case slurp mechanism would not function properly.

Proof of Concept

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L38 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L72 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L90

Tools Used

Manual analysis

Recommended Mitigation Steps

setCustomFeePercentageForCollateral and setCustomFeePercentageForSafe functions should check against the underlying token's decimals value.

Joeysantoro commented 2 years ago

FEI is immutable and has 18 decimals. If another project wishes to fork Turbo they should consider this but I don't see the need to change the code here and am disputing the issue.

transmissions11 commented 2 years ago

Seems to be a misunderstanding of how fixed point multiplication works, even if Fei had something other than 18 decimals, multiplying a non wad by a wad is fine.

ie: (1e16 * 0.5e18) / 1e18 still equals 0.5e16

GalloDaSballo commented 2 years ago

Agree with the sponsor here, the percentage variables are expressed with 18 decimals and then divided by same order of magnitude, as such the decimals of the specific token is not relevant

In lack of any POC, am marking invalid

code-423n4 / 2022-02-tribe-turbo-findings