Oracle tick rounding the wrong direction can lead to Swapper overpaying for swap

Summary

The UniV3Oracle is intended to be used in situations where asset values can be understated, but must not be overstated. As a result, all results are rounded down to the nearest tick.

However, in the case of the Swapper, we need the opposite. The Swapper's owner has specified the specific discount which they are willing to provide, and the caller is the user calling flash() to perform the swap. In this case, rounding must happen against the caller, not against the owner, to ensure the owner's criteria are being met.

Because the Oracle rounds down to the nearest tick (not the nearest decimal price), the difference can be quite dramatic, and can lead to substantial loss of funds for the Swapper owner.

Vulnerability Detail

When the UniV3Oracle calls OracleLibrary.consult(), it returns the arithmeticMeanTick.

This value is rounded DOWN to the nearest tick. This is because, in most use cases, the price being returned by an oracle is used to determine the value of an asset to be used for something like valuing collateral, where the caller is the one whose collateral is on the line, and it is crucial to ensure that user assets are not overvalued so as to give them an edge.

However, in this case, the oracle is being used to determine the amount owed from the bot performing the swap (from here on "the bot") to the Swap owner (from here on "the owner"). The owner has already included a firm parameter (the scaled offer value) for the discount they are willing to provide. The bot is the caller, who is taking the action within the confines of this predetermined parameter. In this case, the value should be rounded UP to ensure this scaled offer value is protected.

(As an extra data point in reasoning about this, Uniswap rounds values UP when users are buying assets through the platform. In other words, the oracle returns a slightly different price than the actual marginal price that would be received if the bot were buying their assets through the same pool.)

Unfortunately, Uniswap oracle rounding can be quite severe. According to Uniswap's pricing formula, ticks represent 1 basis point (0.01%) of the price. This is true regardless of the price of the asset, the quantity being swapped, or (most importantly) the decimals in the asset.

While in most cases, this may not seem like much, it is likely that Swappers receiving large amounts of funds will only need to offer very slim discounts to incentivize bots. As explained to me by the 0xSplits team in a DM, the slimness of this margin could get so extreme that users may even set a PREMIUM for swaps, because of the risk that the trailing 30 minutes could provide a slight discount.

Proof of Concept

Let's look at an example of an LLC using a Swapper to move all their income into USDC:

They company earns $10mm per year on chain, which is all sent to the Swapper
They aren't concerned about frequency of swaps, so they do the math: setting their $defaultScaledOfferFactor to 99_99_90 should work out to paying approximately $100 for the year, which seems fair.
When the assets are swapped, rounding comes into play. Instead of FAIR_VALUE, their assets are valued at FAIR_VALUE - 0.5 basis points, which equals FAIR_VALUE * 0.995. For their $10mm, that's 9,950,000.
Finally, their scaled offer factor discounts the price further, down to 9_950_000 * 99_99_90 / 1e6 = 9,949,900.50.

The result is that they end up paying $50,099.50 for their swaps, instead of $100.

(Note that this same math holds whether the swaps were done all at once or in multiple separate transactions.)

Impact

Because rounding is performed in the wrong direction, Swap owners who set their $defaultScaledOfferFactor with a small margin may end up paying substantially more than expected for their swaps.

Code Snippet

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-oracle/src/UniV3OracleImpl.sol#L274-L282

Specifically, see L35-36 of this Uniswap contract:

https://github.com/Uniswap/v3-periphery/blob/6cce88e63e176af1ddb6cc56e029110289622317/contracts/libraries/OracleLibrary.sol#L16-L41

Tool used

Manual Review

Recommendation

Implement your own version of the OracleLibrary's consult() function, which doesn't round the arithmeticMeanTick down.

zobront commented 1 year ago

Fixed in https://github.com/0xSplits/splits-oracle/pull/1/ by adding 1 to the tick returned from OracleLibrary.consult()

jacksanford1 commented 1 year ago

Confirming that Splits meant for this fix (1) to be linked to this issue (12): https://github.com/0xSplits/splits-oracle/pull/1#issue-1693202318

sherlock-audit / 2023-04-splits-judging

obront - Oracle tick rounding the wrong direction can lead to Swapper overpaying for swap #12