Lines of code

Vulnerability details

C4 issue

M-19: V3Oracle susceptible to price manipulation

Comments

The original V3Oracle contract calculates amount0 and amount1 with prices directly taken from slot0 function :

function _getAmounts(PositionState memory state)
        internal
        view
        returns (uint256 amount0, uint256 amount1, uint128 fees0, uint128 fees1)
    {
        ...
            (amount0, amount1) = LiquidityAmounts.getAmountsForLiquidity(
                state.sqrtPriceX96, state.sqrtPriceX96Lower, state.sqrtPriceX96Upper, state.liquidity
            );

       ...
    }

   function _initializeState(uint256 tokenId) internal view returns (PositionState memory state) {
        (
           ....
        ) = nonfungiblePositionManager.positions(tokenId);
        state.tokenId = tokenId;
        state.token0 = token0;
        state.token1 = token1;
        state.fee = fee;
        state.tickLower = tickLower;
        state.tickUpper = tickUpper;
        state.liquidity = liquidity;
        state.feeGrowthInside0LastX128 = feeGrowthInside0LastX128;
        state.feeGrowthInside1LastX128 = feeGrowthInside1LastX128;
        state.tokensOwed0 = tokensOwed0;
        state.tokensOwed1 = tokensOwed1;
        state.pool = _getPool(token0, token1, fee);
        (state.sqrtPriceX96, state.tick,,,,,) = state.pool.slot0();
    }

This makes the protocol open to price manipulation attack since an attacker can change sqrtPriceX96 in a Uniswap v3 pool.

Mitigation

PR #26

In PR #26, V3Oracle introduce a new variable derivedSqrtPriceX96 and uses this to calculate amount0 and amount1:

function _getAmounts(PositionState memory state) internal pure returns (uint256 amount0, uint256 amount1) {
        if (state.liquidity != 0) {
            state.sqrtPriceX96Lower = TickMath.getSqrtRatioAtTick(state.tickLower);
            state.sqrtPriceX96Upper = TickMath.getSqrtRatioAtTick(state.tickUpper);
            (amount0, amount1) = LiquidityAmounts.getAmountsForLiquidity(
                state.derivedSqrtPriceX96, state.sqrtPriceX96Lower, state.sqrtPriceX96Upper, state.liquidity
            );
        }
    }

derivedSqrtPriceX96 is calculated as:

function _populatePrices(PositionState memory state) internal view {
        (state.price0X96, state.cachedChainlinkReferencePriceX96) =
            _getReferenceTokenPriceX96(state.token0, state.cachedChainlinkReferencePriceX96);
        (state.price1X96, state.cachedChainlinkReferencePriceX96) =
            _getReferenceTokenPriceX96(state.token1, state.cachedChainlinkReferencePriceX96);

        // checks derived pool price for price manipulation attacks
        // this prevents manipulations of pool to get distorted proportions of collateral tokens - for borrowing
        // when a pool is in this state, liquidations will be disabled - but arbitrageurs (or liquidator himself)
        // will move price back to reasonable range and enable liquidation
        uint256 derivedPoolPriceX96 = state.price0X96 * Q96 / state.price1X96;

        // current pool price
        uint256 priceX96 = FullMath.mulDiv(state.sqrtPriceX96, state.sqrtPriceX96, Q96);
        _requireMaxDifference(priceX96, derivedPoolPriceX96, maxPoolPriceDifference);

        // calculate derived sqrt price
        state.derivedSqrtPriceX96 = SafeCast.toUint160(Math.sqrt(derivedPoolPriceX96) * (2 ** 48));
    }

derivedSqrtPriceX96 is derivedPoolPriceX96 cast to uint160
derivedPoolPriceX96 is calculated based on price0X96 and price1X96
price0X96 and price1X96 are obtained from either chainlink feed or UniswapV3 pool twap values. Both these 2 sources cannot be manipulated by attackers.

The mitigation resolved the original issue.

Conclusion

LGTM

code-423n4 / 2024-04-revert-mitigation-findings