search

sherlock-audit / 2023-11-olympus-judging

9 stars 8 forks source link

jasonxiale - `UniswapV2PoolTokenPrice.getTokenPrice` might return inaccurate value if `lookupToken's balance` is small #201

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 11 months ago

jasonxiale

medium

UniswapV2PoolTokenPrice.getTokenPrice might return inaccurate value if lookupToken's balance is small

Summary

UniswapV2PoolTokenPrice.getTokenPrice's implementation is not correct, which might cause the return value is not inaccurate.

Vulnerability Detail

n UniswapV2PoolTokenPrice.getTokenPrice, the token price is calculated as:

376             // Get the lookupToken in terms of the destinationToken
377             lookupTokenUsdPrice = destinationTokenBalance.mulDiv(
378                 destinationTokenPrice,
379                 lookupTokenBalance
380             );

If we ignore destinationTokenPrice for now, we will get a formula that lookupTokenPrice = destinationTokenBalance / lookupTokenBalance. However this is incorrect. Because for UniswapV2, the formula to calculate price should be lookupTokenPrice = destinationTokenBalance * dx / (lookupTokenBalance + dx), since we're calculating lookupToken price, we should use 1 * 10**lookupToken.decimals() as dx, and we'll get: lookupTokenPrice = destinationTokenBalance * 10**lookupToken.decimals() / (lookupTokenBalance + 10**lookupToken.decimals()) The above formula can be simplifed as lookupTokenPrice = destinationTokenBalance / (lookupTokenBalance + 1), we will call this formula as F1

Regarding to current implementation's formula:lookupTokenPrice = destinationTokenBalance / lookupTokenBalance, we will call this formula as F2:

The gap between those two formula will be:

  1. If lookupTokenBalance = 99, the error will be (1/99 - 1/ 100) / (1/100) * 100% = 1.0101010101010166%, which is accceptable
  2. But if lookupTokenBalance =9, the error will be (1/9 - 1/10) / (1/10) * 100% = 11.1111111111111%, which will be inaccurate

Besides the issue I mentioned above, while calculating the price, the function doesn't include UniswapV2 fee

Impact

UniswapV2PoolTokenPrice.getTokenPrice's implementation is not correct, which might cause the return value is not inaccurate.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/UniswapV2PoolTokenPrice.sol#L306-L384

Tool used

Manual Review

Recommendation

nevillehuang commented 10 months ago

Invalid, unclear where the need of the new formula popped up so I see no basis for this comparison.

The following statement seem to be pointing to a similar issue highlighted in another issue #37 albeit in a different price submodule, but given insufficient impact and proof was described, will still maintain as invalid.

Besides the issue I mentioned above, while calculating the price, the function doesn't include UniswapV2 fee

Hash01011122 commented 10 months ago

As highlighted by @nevillehuang, this issue bears a striking resemblance to issue #37, albeit with a variation in the price submodule. However, after reviewing Watson's issue, which covers every pertinent detail, I am convinced that this should be classified as a duplicate of issue #37 rather than being dismissed as invalid. Watson pointed out that UniswapV2 fees will not be included but explains it in different submodule. Duplicate of #37 (Correct me if I am wrong)

nevillehuang commented 10 months ago

@Hash01011122 Please refrain from commenting on non-escalated issues after escalation period ended because dedicated escalation period is there for a reason. If after escalation period head of judging doesn’t explicitly make adjustments, then it is invalid.