fix: write instant price to oracle and don't write liquidity data

[xenide]

Motivation

• when calculating TWAP, we need the instant price to calculate the accumulator that has elapsed since the last oracle write (which is almost all the time)

• as discussed within the team, for a given base and quote token, the price feed (pair) to be used will be controlled by governance instead of which pair has more liquidity, so we're doing away with writing liquidity data in the observations

• Additional math changes: the following problems have always been there but because the way we write tests we don't progress the time, no oracle writing was done so it wasn't caught. Now that we update the instant price at every block, we caught it

  1. StableMath::_computeLiquidityFromAdjustedBalances would overflow sometimes given very large inputs
     • Solution: introduce fullMulDiv for iterative algorithm for 2 arithmetic operations as overflow can happen for large numbers in those specific operations.
  2. StableOracleMath::calcSpotPrice would sometimes fail when normalized reserves are too small (e.g. 1e3) such that when multiplied and wad-ed down, the result is 0
     • Solution: return a default price of 1e18 if either numerator / denominator is 0
       • Risk assessment: if we're using the pair as an oracle probably it will have more liquidity than just 1-2 tokens (in normalized terms). And if there's so few tokens (1e3) we wouldn't be using the pair as a price feed.
     • Other solutions explored:
       • scaling all reserves by 1e18 if any of the reserves are < 1e18. This causes overflows in some test cases
       • checking if only derivativeY is 0, and then setting it to 1. This is insufficient as derivativeX can be zero too and make the function return 0 then cause a problem with the log compression
     • Potential solutions not yet explored:
       • instead of calculating precisely the spot price, we estimate it with simulating a swap with zero fees 0.01% of the pool's liquidity (to simulate a very small input). This should be close enough
       • use another impl to calculate spot price. Though this impl may be susceptible to the same problem we're facing (going to 0 for small inputs)
  • max change per change feat: introduce max change per trade #228
  • reducing oracle buffer size fix: reduce oracle buffer size #229

Solution

[OliverNChalk]

What does this attack achieve? Every time the attacker trades back to normal price they update the accumulator with the correct price. So they're actually just making the oracle more accurate? The instant price only affects for 1 second and if we clamp the rate of change per second that shouldn't matter much at all

[xenide]

Nope, because we only update the 1st trade of the timestamp, when they trade / arb back to the normal price in the same block, nothing happens to that oracle reading in the 2nd trade of the block.

In the same block
1st trade - manipulate oracle, inaccurate instant prices written to oracle
2nd trade - arb it back so no one else does it, oracle sample unchanged

Attacker monitors the pair to make sure no other trades take place: that way the instant prices get time to accumulate. If someone comes in to trade, the instant prices get yanked towards fair price. The attacker can repeat the same 2-trade action to mess with the instant prices again, and continue to monitor.

To the outside world monitoring prices, it didn't change much. But sitting in the oracle sample is the manipulated instant prices waiting to be written and accumulated.

This test case demonstrates that perhaps updating instant prices at every trade is necessary.

[OliverNChalk]

Instant prices don't get written into the oracle but you mean because the oracle price accumulates weight until the next trade it can influence the oracle up the max trade amount?

[OliverNChalk]

Feels like cleanest solution is to just write the oracle every swap?

[xenide]

Yes the cleanest is to write the oracle every swap, and only updating the instant price if it's not the first swap of the block, which is the previous impl with the else branch.

This is the only way to force any manipulators to put their capital at risk (increase the cost) when moving prices, as there is no guarantee they can arb it back in the next block. And if they arb it back in the next block, then the manipulation only lasted for 1 second

[OliverNChalk]

Why the need for an else if you write the update on every swap, because otherwise it will move the index?

[xenide]

yes, then we'll have multiple samples per timestamp which creates problems when reading the oracle.

The alternative is to read if lPrevious.timestamp is the current timestamp we're attempting to write, and if so, only overwrite the instant prices. But that's basically an else as well

[xenide]

just want to document this for posterity:

I attempted to simplify this condition to only lTimeElapsed > 0, which therefore implies that an oracle sample is created in the very first mint. However, I did not adopt this approach in the end for one edge case. In the edge case of a swap happening after the very first mint (pair creation). the previous timestamp would not be zero, and therefore the clamped price would not be correctly calculated (it'd always be 0 cuz the time elapsed is 0).

So it's back to the original approach of writing only the instant prices for the very first block (regardless of how many things happen in that block), and use the previous oracle observation as a "placeholder" to temporarily store the instant prices for accumulation in the future. This approach is more semantically correct as we're not "creating" a sample (as we don't write a timestamp), but instead just using it as a placeholder.