c4-bot-5 commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L218-L244

Vulnerability details

Impact

Excess tokens from margins provided by traders when opening position could become stuck inside the ParticlePositionManager due to precision loss when calculating the token premium portion.

Proof of Concept

When traders call openPosition, eventually it will calculate cache.token0PremiumPortion and cache.token1PremiumPortion before put it into lien data.

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L218-L244

    /// @inheritdoc IParticlePositionManager
    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
       ...
        if (params.zeroForOne) {
>>>         cache.token0PremiumPortion = Base.uint256ToUint24(
                ((params.marginFrom + cache.amountFromBorrowed - cache.feeAmount - cache.amountSpent) *
                    Base.BASIS_POINT) / cache.collateralFrom
            );
>>>         cache.token1PremiumPortion = Base.uint256ToUint24(
                ((cache.amountReceived + cache.amountToBorrowed + params.marginTo - collateralTo) * Base.BASIS_POINT) /
                    collateralTo
            );
            if (
                cache.token0PremiumPortion < params.tokenFromPremiumPortionMin ||
                cache.token1PremiumPortion < params.tokenToPremiumPortionMin
            ) revert Errors.InsufficientPremium();
        } else {
>>>         cache.token1PremiumPortion = Base.uint256ToUint24(
                ((params.marginFrom + cache.amountFromBorrowed - cache.feeAmount - cache.amountSpent) *
                    Base.BASIS_POINT) / cache.collateralFrom
            );
>>>         cache.token0PremiumPortion = Base.uint256ToUint24(
                ((cache.amountReceived + cache.amountToBorrowed + params.marginTo - collateralTo) * Base.BASIS_POINT) /
                    collateralTo
            );
            if (
                cache.token0PremiumPortion < params.tokenToPremiumPortionMin ||
                cache.token1PremiumPortion < params.tokenFromPremiumPortionMin
            ) revert Errors.InsufficientPremium();
        }

        // create a new lien
        liens[keccak256(abi.encodePacked(msg.sender, lienId = _nextRecordId++))] = Lien.Info({
            tokenId: uint40(params.tokenId),
            liquidity: params.liquidity,
            token0PremiumPortion: cache.token0PremiumPortion,
            token1PremiumPortion: cache.token1PremiumPortion,
            startTime: uint32(block.timestamp),
            feeGrowthInside0LastX128: cache.feeGrowthInside0LastX128,
            feeGrowthInside1LastX128: cache.feeGrowthInside1LastX128,
            zeroForOne: params.zeroForOne
        });

        emit OpenPosition(msg.sender, lienId, collateralTo);
    }

It can be observed that when calculating cache.token0PremiumPortion and cache.token1PremiumPortion, precision loss could occur, especially when the token used has a high decimals (e.g. 18) while BASIS_POINT used only 1_000_000. When this happens, the token could become stuck inside the ParticlePositionManager.

Coded PoC :

The PoC goal is to check the amount of tokens inside the ParticlePositionManager after the position is opened, premium is added, liquidated, the LP withdraws the tokens and collects fees, and the admin collects the treasury.

Add the following test inside test/OpenPosition.t.sol and also add import "forge-std/console.sol"; to the test contract.

   function testLiquidateAndClearLiquidity() public {
        address LIQUIDATOR = payable(address(0x7777));
        uint128 REPAY_LIQUIDITY_PORTION = 1000;
        testBaseOpenLongPosition();
        // add enough premium
        uint128 premium0 = 500 * 1e6;
        uint128 premium1 = 0.5 ether;
        vm.startPrank(WHALE);
        USDC.transfer(SWAPPER, premium0);
        WETH.transfer(SWAPPER, premium1);
        vm.stopPrank();

        vm.startPrank(SWAPPER);
        TransferHelper.safeApprove(address(USDC), address(particlePositionManager), premium0);
        TransferHelper.safeApprove(address(WETH), address(particlePositionManager), premium1);
        particlePositionManager.addPremium(0, premium0, premium1);
        vm.stopPrank();
        // 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 + liquidityInside / REPAY_LIQUIDITY_PORTION
        );
        (, 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);
        particlePositionManager.liquidatePosition(
            DataStruct.ClosePositionParams({lienId: uint96(0), amountSwap: amountSwap, data: data}),
            SWAPPER
        );
        vm.stopPrank();
        vm.startPrank(LP);
        // console.log("liquidity before : ");
        // console.log(_liquidity);
        (, , , , , , , _liquidity, , , , ) = nonfungiblePositionManager.positions(_tokenId);
        (uint256 amount0Decreased, uint256 amount1Decreased) = particlePositionManager.decreaseLiquidity(
            _tokenId,
            _liquidity
        );
        (uint256 amount0Returned, uint256 amount1Returned) = particlePositionManager.collectLiquidity(_tokenId);
        vm.stopPrank();
        vm.startPrank(ADMIN);
        particlePositionManager.withdrawTreasury(address(USDC), ADMIN);
        particlePositionManager.withdrawTreasury(address(WETH), ADMIN);
        vm.stopPrank();
        uint256 usdcBalanceAfter = USDC.balanceOf(LP);
        uint256 wethBalanceAfter = WETH.balanceOf(LP);
        console.log("balance LP after - USDC:");
        console.log(usdcBalanceAfter);
        console.log("balance LP after -  WETH:");
        console.log(wethBalanceAfter);
        console.log("balance inside manager -  USDC:");
        console.log(USDC.balanceOf(address(particlePositionManager)));
        console.log("balance inside manager - WETH:");
        console.log(WETH.balanceOf(address(particlePositionManager)));

    }

Run the test :

forge test -vv --fork-url $MAINNET_RPC_URL --fork-block-number 18750931 --match-contract OpenPositionTest --match-test testLiquidateAndClearLiquidity -vvv

Output log :

  balance LP after - USDC:
  15000228083

  balance LP after -  WETH:
  9999997057795940602

  balance inside manager -  USDC:
  1123

  balance inside manager - WETH:
  516472404479

It can be observed that some tokens stuck inside the ParticlePositionManager contract.

Tools Used

Manual review + foundry

Recommended Mitigation Steps

Use a higher scaling for BASIS_POINT, considering the fact that tokens commonly have a high number of decimals

Assessed type

Math

c4-judge commented 9 months ago

0xleastwood marked the issue as primary issue

wukong-particle commented 9 months ago

Good point. Appreciated the detailed PoC. On our end, I think we should have documented the use of tokenPremiumPortion more clearly. It's a compromise we took to ensure the lien structure stored on chain is less than 3 uint256. We compressed a lot and were only left with uint24.

We think a dust of ~1000 USDC when a user is spending 15B USDC is acceptable. We could add a function to collect dust if needed (might be complicated though).

If there can be serious attack around this dust, please do raise it. Thanks!

c4-judge commented 9 months ago

0xleastwood marked the issue as selected for report

c4-sponsor commented 9 months ago

wukong-particle (sponsor) acknowledged

romeroadrian commented 8 months ago

These are leftover amounts that are caused by the rounding of the premium as a portion (premium -> portion -> premium). It looks more on the QA side to me.

0xleastwood commented 8 months ago