sherlock-audit / 2023-04-jojo-judging

7 stars 4 forks source link

ArbitraryExecution - `quoteAllAvailablePoolsWithTimePeriod` can be manipulated with low liquidity pools #438

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

ArbitraryExecution

high

quoteAllAvailablePoolsWithTimePeriod can be manipulated with low liquidity pools

Summary

quoteAllAvailablePoolsWithTimePeriod can be manipulated with low liquidity pools, and there exist Uniswap V3 pools on Arbitrum that JOJO may quote the price from that are low liquidity and therefore manipulatable.

Vulnerability Detail

The quoteAllAvailablePoolsWithTimePeriod function from the StaticOracle contract is used in the getMarkPrice function of uniswapPriceAdaptor.sol to retrieve the weighted arithmetic mean of the tick prices of all applicable Uniswap V3 pools for the given period. However, the returned price can potentially be manipulated if the liquidity of a queried pool is low enough. This is because the arithmetic mean is susceptible to outliers. The potential for Uniswap V3 pools to be manipulated is usually considered a theoretical vulnerability for high-liquidity pools. However, there are specific instances of low liquidity Uniswap V3 pools on Arbitrum that JOJO will attempt to quote a price from, therefore making manipulation a real attack vector.

In one such instance, the deployed StaticOracle contract that JOJO intends to use on Arbitrum returns the following three Uniswap V3 pools for the WBTC/USDC pair: 0xac70bD92F89e6739B3a08Db9B6081a923912f73D, 0xA62aD78825E3a55A77823F00Fe0050F567c1e4EE, and 0x83450968eC7606F98Df1C170f8C922d55A13f236. Two of the three pools have low liquidity, which makes the average arithmetic mean of the three pools manipulatable.

Impact

Manipulating the price of a token used in a perpetual opens up the opportunity for arbitrage on the JOJO protocol which in turn could increase counterparty risk. Additionally, if the price exceeds the allowed difference set by JOJO, this could cause a permanent DOS of the uniswapPriceAdaptor and emergencyOracle fallback oracle mechanism. Despite this oracle mechanism being the fallback to Chainlink, a permanent DOS of the backup price oracle system should be considered a critical failure.

Code Snippet

https://github.com/Mean-Finance/uniswap-v3-oracle/blob/9935263665c5a16f9c385e909bcc6edcc8d56970/solidity/contracts/StaticOracle.sol#L158-L174

function _quote(
    uint128 _baseAmount,
    address _baseToken,
    address _quoteToken,
    address[] memory _pools,
    uint32 _period
  ) internal view returns (uint256 _quoteAmount) {
    require(_pools.length > 0, 'No defined pools');
    OracleLibrary.WeightedTickData[] memory _tickData = new OracleLibrary.WeightedTickData[](_pools.length);
    for (uint256 i; i < _pools.length; i++) {
      (_tickData[i].tick, _tickData[i].weight) = _period > 0
        ? OracleLibrary.consult(_pools[i], _period)
        : OracleLibrary.getBlockStartingTickAndLiquidity(_pools[i]);
    }
    int24 _weightedTick = _tickData.length == 1 ? _tickData[0].tick : OracleLibrary.getWeightedArithmeticMeanTick(_tickData);
    return OracleLibrary.getQuoteAtTick(_weightedTick, _baseAmount, _baseToken, _quoteToken);
}

Tool used

Manual review.

Recommendation

JOJO should consider replacing quoteAllAvailablePoolsWithTimePeriod with quoteSpecificPoolsWithTimePeriod and selecting a subset of Uniswap V3 pools with sufficient liquidity to avoid price manipulation.

JoscelynFarr commented 1 year ago

fix link: https://github.com/JOJOexchange/smart-contract-EVM/commit/1dbc9001be667af42952c110e9fdf04fd7826669 https://github.com/JOJOexchange/JUSDV1/commit/eed86242c2be0cd70e6b412124eb05ed5e3c92dc

IAm0x52 commented 1 year ago

Fixes look good. Pools are now specified instead of being pulled dynamically