c4-bot-2 commented 6 months ago

Lines of code

Vulnerability details

C4 issue

M-19: V3Oracle susceptible to price manipulation

Comments

V3Oracle relies on calculating the price of a NFT position tokens from the Uniswap V3 Pool. This calculation comes from the following logic in V3Oracle:

// _initializeState()
// AUDIT: pulls sqrtPriceX96 from slot0 which is manipulatible
(state.sqrtPriceX96, state.tick,,,,,) = state.pool.slot0();

// _getAmounts()
(amount0, amount1) = LiquidityAmounts.getAmountsForLiquidity(
    state.sqrtPriceX96, state.sqrtPriceX96Lower, state.sqrtPriceX96Upper, state.liquidity
);

// getValue()
value = (price0X96 * (amount0 + fees0) / Q96 + price1X96 * (amount1 + fees1) / Q96) * Q96 / priceTokenX96;
feeValue = (price0X96 * fees0 / Q96 + price1X96 * fees1 / Q96) * Q96 / priceTokenX96;
price0X96 = price0X96 * Q96 / priceTokenX96;
price1X96 = price1X96 * Q96 / priceTokenX96;

The core of the issue is that V3Oracle relies on the pool's sqrtPriceX96 to retrieve the value of the NFT's position. The sqrtPriceX96 is manipulatable and therefore the position value is manipulatable.

Mitigation

PR #26

Major changes include:

V3Vault._populatePrices() is created to retrieve token prices, retrieve their derived square root pricing, and checks that the max difference between the price and derived price are not far apart. In addition, the most important change within this function is that the derived pool price is square rooted. This will curb potential price manipulation. The return value of the square root will be used as the LiquidityAmounts.getAmountsForLiquidity sqrtRatioX96 function argument. This updated square rooted price prevents a price manipulation of the NFT position.

Minor changes include:

V3Vault._initializeState() is renamed to _loadPositionState().
V3Oracle._getChainlinkPriceX96() now checks to see if the chainlink price from the latest chainlink round data is 0. If the price is 0, _getChainlinkPriceX96() will revert. This is an acceptable change as a price of 0 can indicate an issue with Chainlink and the price should not be trusted.
V3Oracle.getPositionBreakdown() has refactored some of it's code into separate functions:
- V3Oracle._getFees() is created to handle calculating the uncollected fees owed for a given position.
- V3Oracle.getLiquidityAndFees() is created to handle retrieving a position's liquidity and fees.

Based on these changes, specifically square rooting the pool price, the Oracle is safe from slot0 price manipulation. All other changes are minor refactorings to improve organization and code quality of the codebase.

Anything Else We Should Know

AutoRange.execute() and AutoExit.execute() also calls slot0 and utilizes the sqrtPriceX96 value but does not square root the price. Although the report discussion suggests that the slot0 bug only impacts the Oracle, the raw slot0() price in both AutoRange.execute() and AutoExit.execute() may be impacted by fluctuations in market prices. This in turn creates potential inaccurate slippage amounts at any given time during turbulent markets. There is little to no harm in square rooting this change.

Conclusion

LGTM

c4-judge commented 6 months ago

jhsagd76 marked the issue as satisfactory