Platform: Code4rena
Start Date: 16/09/2021
Pot Size: $200,000 SUSHI
Total HM: 26
Participants: 16
Period: 14 days
Judge: alcueca
Total Solo HM: 13
Id: 29
League: ETH
Rank: 8/16
Findings: 2
Award: $6,618.60
🌟 Selected for report: 4
🚀 Solo Findings: 0
317.693 SUSHI - $3,971.16
hack3r-0m
https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/libraries/MathUtils.sol#L22 the difference is computed incorrectly when a > b.
As it only used in within1 function, scope narrows down to where difference(a, b) <= 1; is exploitable.
cases where difference(a, b) <= 1 should be true but is reported false:
cases where difference(a, b) <= 1 should be false but is reported true:
within1 is used at the following locations:
It is possible to decrease the denominator and increase the value of the numerator (when calculating y) using constants and input to make within1 fail
Mitigation:
Add else condition to mitigate it.
unchecked { if (a > b) { diff = a - b; } else { diff = b - a; } }
(re-submitting this issue after withdrawing past one since I forgot to add more details and POC)
🌟 Selected for report: hack3r-0m
70.5984 SUSHI - $882.48
hack3r-0m
https://eips.ethereum.org/EIPS/eip-20
According to ERC20 standards, decimals() function is optional i.e erc20 may or may not implement it.
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.
Manual Review
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.
#0 - maxsam4
2021-09-22T09:47:30Z
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.
#1 - alcueca
2021-10-24T05:17:56Z
The warden is right that the code functioning correctly depends on external assumptions. There being no risk to funds, a severity of 1 is justified.
🌟 Selected for report: hack3r-0m
70.5984 SUSHI - $882.48
hack3r-0m
Since the returned bool value (first parameter) of static call is ignored, there should be a check to make sure masterDeployer is non-zero and is a contract.
#0 - maxsam4
2021-09-22T08:57:57Z
#1 - alcueca
2021-10-24T05:33:19Z
It is safe to assume it is not zero in the current implementation, but not in any future implementations that reuse the ConstantProductPool, but that implement a new factory, and the sponsor should be aware. To dispute, sponsor should show that there won't be future implementations, or that some process is in place to check inputs in governance actions or deployments.
🌟 Selected for report: hack3r-0m
70.5984 SUSHI - $882.48
hack3r-0m
Since the returned bool value (first parameter) of static call is ignored, there should be a check to make sure masterDeployer is non-zero and is a contract.
#0 - maxsam4
2021-09-22T08:56:49Z
masterDeployer is passed to the contract by the factory. It's not user input. Therefore, it is safe to assume that its not zero.
#1 - alcueca
2021-10-24T05:32:27Z
It is safe to assume it is not zero in the current implementation, but not in any future implementations that reuse the HybridPool, but that implement a new factory, and the sponsor should be aware. To dispute, sponsor should show that there won't be future implementations, or that some process is in place to check inputs in governance actions or deployments.
#2 - alcueca
2021-10-25T05:50:07Z
Or just document it in the smart contract