Good Entry - n1punp's results

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L338

Vulnerability details

Impact

For some pools like ETH-DAI, BTC-DAI, and many others, claimFee functionality will always revert due to math overflow in returnExpectedBalanceWithoutFees calculation --> this will leave all users unable to claim the accrued trading fees at all.

Proof of Concept

In the returnExpectedBalanceWithoutFees calculation, it tries to compute the sqrtPriceX96. However, this calculation may overflow for some token pairs for example:

ETH-DAI pair, say TOKEN0 is ETH, and TOKEN1 is DAI: TOKEN0_PRICE will then be 1800 * 1018 TOKEN1_PRICE will then be 1 * 1018 Now, let's look at the calculation:

(2 ** 192 * ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE)
= 2**192 * (1800 * 10**18 * 10**18 / (1 * 10**18))
= 1.12 * 10**79

which will exceed uint256 maximum value, leading to integer overflow. This can also happen to other token pairs as well (where the relative token price is sufficiently high and token1 decimals is 18).

Tools Used

Manual Review

Recommended Mitigation Steps

• Do a proper overflow check. For example, check if ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE) exceeds 264. If yes, then instead of multiplying the value by 2192 in the front, we can reduce it to 2128 in this case to prevent overflow, and then multiply by another 232 outside sqrt.

So, it'll look sth like:

uint val = ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE);
if (val >= 2**64) {
  ... sqrt(2**128 * val) * 2**32 ... // pull some factors out of sqrt to prevent overflow
} else {
  ... sqrt(2**192 * val) ... // won't overflow in this case
}

Assessed type

Math