Underflow could happened when calculating Uniswap V3 position's fee growth and can cause operations to revert

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L318-L342

Vulnerability details

Impact

When operations need to calculate Uniswap V3 position's fee growth, it used similar function implemented by uniswap v3. However, according to this known issue : Uniswap/v3-core#573. The contract is implicitly relies on underflow/overflow when calculating the fee growth, if underflow is prevented, some operations that rely on fee growth will revert.

Proof of Concept

It can be observed that current implementation of getFeeGrowthInside not allow underflow/overflow to happen when calculating feeGrowthInside0X128 and feeGrowthInside1X128, because the contract used solidity 0.8.23.

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L318-L342

    function getFeeGrowthInside(
        address token0,
        address token1,
        uint24 fee,
        int24 tickLower,
        int24 tickUpper
    ) internal view returns (uint256 feeGrowthInside0X128, uint256 feeGrowthInside1X128) {
        IUniswapV3Pool pool = IUniswapV3Pool(UNI_FACTORY.getPool(token0, token1, fee));
        (, int24 tickCurrent, , , , , ) = pool.slot0();
        (, , uint256 lowerFeeGrowthOutside0X128, uint256 lowerFeeGrowthOutside1X128, , , , ) = pool.ticks(tickLower);
        (, , uint256 upperFeeGrowthOutside0X128, uint256 upperFeeGrowthOutside1X128, , , , ) = pool.ticks(tickUpper);

        if (tickCurrent < tickLower) {
            feeGrowthInside0X128 = lowerFeeGrowthOutside0X128 - upperFeeGrowthOutside0X128;
            feeGrowthInside1X128 = lowerFeeGrowthOutside1X128 - upperFeeGrowthOutside1X128;
        } else if (tickCurrent < tickUpper) {
            uint256 feeGrowthGlobal0X128 = pool.feeGrowthGlobal0X128();
            uint256 feeGrowthGlobal1X128 = pool.feeGrowthGlobal1X128();
            feeGrowthInside0X128 = feeGrowthGlobal0X128 - lowerFeeGrowthOutside0X128 - upperFeeGrowthOutside0X128;
            feeGrowthInside1X128 = feeGrowthGlobal1X128 - lowerFeeGrowthOutside1X128 - upperFeeGrowthOutside1X128;
        } else {
            feeGrowthInside0X128 = upperFeeGrowthOutside0X128 - lowerFeeGrowthOutside0X128;
            feeGrowthInside1X128 = upperFeeGrowthOutside1X128 - lowerFeeGrowthOutside1X128;
        }
    }

This could impact crucial operation that rely on this call, such as liquidation, could revert unexpectedly. This behavior is quite often especially for pools that use lower fee.

Coded PoC :

Add the following test to /test/OpenPosition.t.sol :

  function testLiquidationRevert() public {
        address LIQUIDATOR = payable(address(0x7777));
        uint128 REPAY_LIQUIDITY_PORTION = 1000;
        _setupLowerOutOfRange();
        testBaseOpenLongPosition();
        // get lien info
        (, uint128 liquidityInside, , , , , , ) = particlePositionManager.liens(
            keccak256(abi.encodePacked(SWAPPER, uint96(0)))
        );
        // start reclaim
        vm.startPrank(LP);
        vm.warp(block.timestamp + 1);
        particlePositionManager.reclaimLiquidity(_tokenId);
        vm.stopPrank();
        // add back liquidity requirement
        vm.warp(block.timestamp + 7 days);
        IUniswapV3Pool _pool = IUniswapV3Pool(uniswapV3Factory.getPool(address(USDC), address(WETH), FEE));
        (uint160 currSqrtRatioX96, , , , , , ) = _pool.slot0();
        (uint256 amount0ToReturn, uint256 amount1ToReturn) = LiquidityAmounts.getAmountsForLiquidity(
            currSqrtRatioX96,
            _sqrtRatioAX96,
            _sqrtRatioBX96,
            liquidityInside
        );
        (uint256 usdCollateral, uint256 ethCollateral) = particleInfoReader.getRequiredCollateral(liquidityInside, _tickLower, _tickUpper);

        // get swap data
        uint160 currentPrice = particleInfoReader.getCurrentPrice(address(USDC), address(WETH), FEE);
        uint256 amountSwap = ethCollateral - amount1ToReturn;

        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
            tokenIn: address(WETH),
            tokenOut: address(USDC),
            fee: FEE,
            recipient: address(particlePositionManager),
            deadline: block.timestamp,
            amountIn: amountSwap,
            amountOutMinimum: 0,
            sqrtPriceLimitX96: currentPrice + currentPrice / SLIPPAGE_FACTOR
        });
        bytes memory data = abi.encodeWithSelector(ISwapRouter.exactInputSingle.selector, params);
        // liquidate position
        vm.startPrank(LIQUIDATOR);
        vm.expectRevert(abi.encodeWithSelector(Errors.InsufficientRepay.selector));
        particlePositionManager.liquidatePosition(
            DataStruct.ClosePositionParams({lienId: uint96(0), amountSwap: amountSwap, data: data}),
            SWAPPER
        );
        vm.stopPrank();
    }

Also modify FEE inside /test/Base.t.sol to 500 :

contract ParticlePositionManagerTestBase is Test {
    using Lien for mapping(bytes32 => Lien.Info);

    IERC20 public constant WETH = IERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
    IERC20 public constant USDC = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
    IERC20 public constant DAI = IERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F);
    uint256 public constant USDC_AMOUNT = 50000000 * 1e6;
    uint256 public constant DAI_AMOUNT = 50000000 * 1e18;
    uint256 public constant WETH_AMOUNT = 50000 * 1e18;
    address payable public constant ADMIN = payable(address(0x4269));
    address payable public constant LP = payable(address(0x1001));
    address payable public constant SWAPPER = payable(address(0x1002));
    address payable public constant WHALE = payable(address(0x6666));
    IQuoter public constant QUOTER = IQuoter(0xb27308f9F90D607463bb33eA1BeBb41C27CE5AB6);
    int24 public constant TICK_SPACING = 60;
    uint256 public constant BASIS_POINT = 1_000_000;
-    uint24 public constant FEE = 3000; // uniswap swap fee
+    uint24 public constant FEE = 500; // uniswap swap fee
   ..
}

Run the test :

forge test --fork-url $MAINNET_RPC_URL --fork-block-number 18750931 --match-contract OpenPositionTest --match-test testRevertUnderflow -vvvv

Log output :

It can be observed that the liquidation revert due to the underflow.

Tools Used

Manual review.

Recommended Mitigation Steps

Use unchecked when calculating feeGrowthInside0X128 and feeGrowthInside1X128.

Assessed type

Error

[c4-judge]

0xleastwood marked the issue as primary issue

[romeroadrian]

Really interesting side effect of upgrading code from solidity < 0.8 👍

[wukong-particle]

Oh this one is great. Will update the code to uncheck feeGrowthInside0X128 and feeGrowthInside1X128 calculations.

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

wukong-particle (sponsor) confirmed