rvierdiiev - UniswapV3OracleHelper.getTWAPRatio will show incorrect price for negative ticks
Closed this issue · 19 comments
rvierdiiev
high
UniswapV3OracleHelper.getTWAPRatio will show incorrect price for negative ticks
Summary
UniswapV3OracleHelper.getTWAPRatio will show incorrect price for negative ticks, because getTimeWeightedTick function doesn't round up for negative ticks
Vulnerability Detail
UniswapV3OracleHelper.getTWAPRatio function is used by protocol to validate price deviation in the BunniPrice and also by UniswapV3Price.getTokenTWAP function.
Function itself calls getTimeWeightedTick function to get twap price tick using uniswap oracle. getTimeWeightedTick uses pool.observe to get tickCumulatives array which is then used to calculate timeWeightedTick.
The problem is that in case if tickCumulatives[1] - tickCumulatives[0] is negative, then timeWeightedTick should be rounded down as it's done in the uniswap library.
As result, in case if tickCumulatives[1] - tickCumulatives[0] is negative and (tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0, then returned timeWeightedTick will be bigger then it should be, which opens possibility for some price manipulations and arbitrage opportunities.
Impact
In case if tickCumulatives[1] - tickCumulatives[0] is negative and ((tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0, then returned timeWeightedTick will be bigger then it should be.
Code Snippet
Provided above
Tool used
Manual Review
Recommendation
Add this line.
if (tickCumulatives[1] - tickCumulatives[0] < 0 && (tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0) timeWeightedTick --;
Disagree with the severity. Should be low, as it would be a minor difference in the value.
Hi @0xJem, Could you elaborate more on how minor is this difference? Since this can directly impact price, I think this could at least be medium severity.
Hi @0xJem, Could you elaborate more on how minor is this difference? Since this can directly impact price, I think this could at least be medium severity.
1 tick is 1 basis point from the next tick:
Prices from 0 to ∞ can be expressed in sharp enough granularity using a large-enough (int24) signed integer power of 1.0001, called a tick, such that each tick’s price is exactly .01% (1 basis point) away from its neighbor.
Hi @0xJem, Could you elaborate more on how minor is this difference? Since this can directly impact price, I think this could at least be medium severity.
1 tick is 1 basis point from the next tick:
Prices from 0 to ∞ can be expressed in sharp enough granularity using a large-enough (int24) signed integer power of 1.0001, called a tick, such that each tick’s price is exactly .01% (1 basis point) away from its neighbor.
Might maintain medium severity given although you need huge amount of funds to gain small profits, it is still profits so it would fall under this sherlock rule for medium severity
- A material loss of funds, no/minimal profit for the attacker at a considerable cost
Escalate
This is low. I didn't sumbit this issue. 0.01% price deviation cannot be arbitraged since this price is TWAP.
Escalate
This is low. I didn't sumbit this issue. 0.01% price deviation cannot be arbitraged since this price is TWAP.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
A deviation is still a deviation, and can still be arbitraged given a large sum of funds owned by any multiple user. This would depend on if sherlock views this as "material", can agree that it is low-medium severity.
The UniswapV3 pool is OHM-WETH. The latest OHM is $11.6525, let’s assume the price is a bit higher: $20.
The number of OHM in the pool is 172,801e18, and the number of WETH is 735e18. We assume that the giant whale has 200,000e18 OHM for arbitrage (valued at $20*200,000=$4,000,000), and 0.01% of $20 is $0.002, then his total profit is: 200,000 * 0.002 = $400.
If you deduct the gas on the mainnet, it’s less than $400.
Let's consider using $4,000,000 to make less than $400. That's not practical.
Furthermore, token0=OHM, token1=WETH of this POOL. When will current tick be negative? That's when OHM is more expensive than WETH.
@rvierdiyev any comments? If not I will have to agree with @securitygrid.
hey @nevillehuang @securitygrid
i have to say that i even didn't think about that from such point of view
i just saw incorrect implementation and realized that twap will be affected in some conditions
as for me this issue meets these criterias, as it still can happen
There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.
Causes a loss of funds but requires certain external conditions or specific states.
but i should say again, that i didn't think about it as deep as @securitygrid did and didn't try to find out which token will be token0/token1 in the pool
Fix looks good. Now matches the Uni V3 library
What are the consequences of a TWAP manipulation on the system?
Result:
Low
Unique
@Czar102 can you provide reason why you accepted escalation?
as you didn't provide any input
@rvierdiyev you failed to answer my question on the impact of your finding. I believe @securitygrid made an argument that you failed to counter in the scope of the codebase. I couldn't wait indefinitely for your answer, so I assumed you have nothing more to say.
@Czar102 pls, tag me in question next time, so i understand it is addressed to me.
we didn't do full review, but only part, so sponsor should answer such question, not me.
@rvierdiyev I'll tag you next time, sorry about that.
You should have asked the sponsor and made that point in your original submission as this is Watsons' role to understand what is something impacting. I even explicitly mentioned it in my message on Discord pinned in the first days of the contest:
I would like to draw Watsons' attention to the fact that despite any impact on a view function is a low severity finding per Sherlock rules, in this contest some view functions are to be used by the wider system (not necessarily in-scope), so if the system itself (not external codebases) will be impacted, the finding may be considered valid. Such a finding needs to contain an impact description that draws conclusions in the whole system (i.e. there needs to be a loss of funds/core functionality within the system).