maxsam4
commented
3 years ago check the return value of staticcall when calling decimals(), if it is false, do not allow contract creation. if it is true, check it is non-zero.
If decimals function is not present in the token, the pool deployment will fail anyway. We only want to support tokens with decimals.
alcueca
Handle
hack3r-0m
Vulnerability details
Impact
https://eips.ethereum.org/EIPS/eip-20
According to ERC20 standards,
decimals()function is optional i.e erc20 may or may not implement it.https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/pool/HybridPool.sol#L72-L73
above referenced lines call erc20 to obtain the value of decimals, in case staticcall is failed (because erc20 contract might decide not to implement
decimals) values returned are (false, "0x") and translates to "0" decimals when ABI decoded.Marking it low-severity (instead of non-critical) is because sushiswap admins (master deployer) might check decimals manually before creating pool but since code for that specific contract is distributed under GPL-3.0-or-later, one might be able to re-use as it is without making modifications and might not be aware of checking it manually and also cannot edit to make recommended mitigation for this since it is not allowed by the license.
Tools Used
Manual Review
Recommended Mitigation Steps
check the return value of staticcall when calling
decimals(), if it is false, do not allow contract creation. if it is true, check it is non-zero.