Individual TWAP feeds should not be multiplied or divided to calculate final value in getTwapWBTC()

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreUniswapFeed.sol#L112

Vulnerability details

Impact

getTwapWBTC() calculates TWAP value of WBTC/USD by first fetching WBTC/WETH and WETH/USDC TWAPS, and then dividing them to return the result. This is incorrect maths and unlike spot prices, two TWAPS can not be multiplied or divided to calculate the third TWAP. Just generally in mathematics, two averages when multiplied or divided are not supposed to necessarily yield the value of a third average:

        return ( uniswapWETH_USDC * 10**18) / uniswapWBTC_WETH;

This particular report will highlight the fact that 2 different feeds can not be divided or multiplied to obtain the TWAP price of another i.e.:

• Multiplication style:

  • If $\text{TWAP of tokenX/tokenA = P}$
  • And $\text{TWAP of tokenA/tokenY = Q}$
  • Then $\text{TWAP of }$ $tokenX/tokenY \not = P * Q$ (may be equal in some cases, but not necessarily)

• Division style:

  • If $\text{TWAP of tokenX/tokenA = P}$
  • And $\text{TWAP of tokenY/tokenA = Q}$
  • Then $\text{TWAP of }$ $tokenX/tokenY \not = P/Q$ (may be equal in some cases, but not necessarily)

Attempting to do so results in an incorrect value. The more volatile the price feeds are, the more extreme this result is. Example presented in the PoC section.

Some additional input

Note that this particular issue is distinct from some of the other ones, like the issue titled "Incorrect maths calculates inverse TWAP" which highlighted the improper inverting of token0/token1 TWAP feed while calculating token1/token0. That's because in the Multiplication style mentioned above, there is no "virtual inverse" happening (otherwise one could have made an argument that in the Division style example, we effectively calculated 1/TWAP of tokenY/tokenA TWAP and then multiplied, choosing to represent it as a division, which would make this issue quite similar to the other issue "Incorrect maths calculates inverse TWAP").
The current issue persists even in the absence of an inverse TWAP and is hence distinct. Consider this:

• If wbtc_wethFlipped = true on L106 then TWAP uniswapWBTC_WETH is flipped. Let's say that ORIGINAL_uniswapWBTC_WETH feed was flipped to get uniswapWBTC_WETH.
• Then if weth_usdcFlipped = true on L109 then TWAP uniswapWETH_USDC is NOT flipped. So we are okay here.
• Then L112 attempts to divide by uniswapWBTC_WETH which is the same as multiplying with 1/uniswapWBTC_WETH which is the same as multiplying with ORIGINAL_uniswapWBTC_WETH.

So basically, no contribution was made by the "inverse issue" to any error which may happen here.
Now we move on to verify the issue at hand - that of incorrectly using 2 TWAPS to calculate a third one.

Proof of Concept

We'll formulate a simplified example to show the issue. The actual Uniswap TWAP has cumulative prices stored which are then used for further calculations, but the following example is good enough to highlight the fundamental issue. Let's assume that the following prices exist at different timestamps:	t	tokenX spot price	tokenY spot price	tokenA spot price	tokenX/tokenA	tokenA/tokenY
1	10000	50	250	40	5
2	11000	54	270	40.74074	5
3	10800	55	260	41.53846	4.72727
-	TWAP: (10000+11000+10800)/3 = 10600	TWAP: (50+54+55)/3 = 53	TWAP: (250+270+260)/3 = 260	TWAP: (40+40.74074+41.53846)/3 = 40.75973	TWAP: (5+5+4.72727)/3 = 4.90909

As per the protocol's current logic:

TWAP of tokenX/tokenY = TWAP_tokenX/tokenA * TWAP_tokenA/tokenY = 40.75973 * 4.90909 = 200.09318

The actual TWAP however is:

TWAP of tokenX/tokenY = average of spot price ratios at each timestamp = (10000/50 + 11000/54 + 10800/55) / 3 = (200 + 203.70370 + 196.36363) / 3 = 200.02244 

Protocol's calculation has a deviation of 0.035% from the actual TWAP price.

One can imagine much more volatile prices or difference in degree of fluctuations between tokens would result in a much higher price deviation from the actual figure.

Mathematical PoC

One can also provide a proof mathematically. Assuming -	t	tokenX spot price	tokenY spot price	tokenA spot price	tokenX/tokenA	tokenA/tokenY
1	x1	y1	a1	xa1 = x1/a1	ay1 = a1/y1
2	x2	y2	a2	xa2 = x2/a2	ay2 = a2/y2
3	x3	y3	a3	xa3 = x3/a3	ay3 = a3/y3
-	TWAP: (x1+x2+x3)/3	TWAP: (y1+y2+y3)/3	TWAP: (a1+a2+a3)/3	TWAP: (xa1+xa2+xa3)/3	TWAP: (ay1+ay2+ay3)/3

As per the protocol's current logic:

TWAP of tokenX/tokenY = TWAP_tokenX/tokenA * TWAP_tokenA/tokenY = (xa1+xa2+xa3)/3 * (ay1+ay2+ay3)/3 = (xa1+xa2+xa3) * (ay1+ay2+ay3) / 9 = (x1/a1 + x2/a2 + x3/a3) * (a1/y1 + a2/y2 + a3/y3) / 9

The actual TWAP however is:

TWAP of tokenX/tokenY = average of spot price ratios at each timestamp = (x1/y1 + x2/y2 + x3/y3) / 3 

It's trivial to see from here on that the two expressions are not equal.

Tools used

Manual inspection

Recommended Mitigation Steps

• Either consider updating the code to fetch the price from the exact token pair TWAP feed instead of calculating through other token pairs
• OR Fetch the spot prices for token0 & token1 at each time stamp for a certain duration of time and then calculate the average within a new function.

Assessed type

Oracle

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) acknowledged

[Picodes]

The 2 expressions are not equal but the goal of the protocol is achieved: what they use for the twap price is more resilient to a sudden drop in price than the spot price. I consider that this is an instance of "function not working as in specs" so of Low severity and not an instance of Medium severity in the absence of convincing evidence that this could lead to real harm being done.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[t0x1cC0de]

Hi @Picodes,
Thanks for your judgement & the comments provided. I would like to highlight a few important facts and dispute the QA tag awarded to this finding as well as Issue #283, also raised by me. Since both are related to TWAP (different problems though), I am clubbing my points together here to make it easier to read -

• I cannot fully understand the rationale behind downgrading this to QA inspite being sponsor acknowledged. I would like to understand at what threshold does C4 make a call that "the goal of increasing the robustness", even via incorrect TWAP calculations is good enough and hence whether to mark it as Med or QA? Specially when the innovators of the system, Uniswap themselves recommend against it in Section 5.2 which I had mentioned previously too in my report (I had mentioned 5.1 earlier by mistake, but it's in the same vicinity).
  

• Even in the case of the present report, the judgement seems to be okay with the fact that the prices could be evaluated as 0.5% to maybe 2 or 3% different from the actual uniswap-provided weighted average prices. In my mind, it defeats the purpose of using the "mean" price of a TWAP if we are going to go ahead and implement it incorrectly with a X% deviation. Since you have mentioned about exploring 'real harm', let's look at this for a moment:

  • We can consider 2 operating price feeds with the third one either way-off or returning zero for simplicity. One of the operational feeds of course is Uniswap TWAP. Currently, we have effectively a coding style where the TWAP is always giving us a price with some error deviation. Even if the other price feed say Chainlink is accurate, the protocol averages out the prices by calculating averagePrice = ( priceA + priceB ) / 2 inside _aggregatePrices() which still preserves the erroneous price to an extent.

We are forcing incorrect prices throughout the protocol, despite the fix being simple enough - use the correct TWAP feeds instead of doing internal calculations. The impact seems quite obvious and apparent, spread to almost all of the protocol. Is the protocol as well as the end-user aware of this impact, this error which has crept in due to use of inverse TWAP calculations or their multiplication/divisions? Put differently, from a business perspective, would an end-user be okay knowing that the prices being calculated for them are almost always not reflecting the sources they have been promised?

Hence my request to please review.

Thank you