If the same Uniswap V3 pool is used for swapping within open/close/liquidate operations, traders are charged more fees than they should be

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L208-L215 https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L399-L406

Vulnerability details

Impact

When traders open or close a position using LPs' Uniswap V3 position within the active range, they can utilize the same Uniswap V3 pool where LPs provide liquidity. This means while the swap operation already charge traders fee, the fee growth caused by the open or close position swap also increase fee growth that will be charged again to the trader.

The more severe scenario occurs when the LP inside Particle acts as a liquidator, intentionally providing the same Uniswap V3 pool for the swap operation. This results in higher fee growth and tokens owed to them.

Proof of Concept

It can be observed that inside openPosition and _closePosition, users can provide the pool they want to use for swapping.

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

    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
        ...

        // swap to meet the collateral requirement
>>>     (cache.amountSpent, cache.amountReceived) = Base.swap(
            cache.tokenFrom,
            cache.tokenTo,
            params.amountSwap,
            collateralTo - cache.amountToBorrowed - params.marginTo, // amount needed to meet requirement
            DEX_AGGREGATOR,
            params.data
        );

        ...
    }

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

    function _closePosition(
        DataStruct.ClosePositionParams calldata params,
        DataCache.ClosePositionCache memory cache,
        Lien.Info memory lien,
        address borrower
    ) internal {
        // optimistically use the input numbers to swap for repay
        /// @dev amountSwap overspend will be caught by refundWithCheck step in below
>>>     (cache.amountSpent, cache.amountReceived) = Base.swap(
            cache.tokenFrom,
            cache.tokenTo,
            params.amountSwap,
            0, /// @dev we check cache.amountReceived is sufficient to repay LP in below
            DEX_AGGREGATOR,
            params.data
        );

        // based on borrowed liquidity, compute the required return amount
        /// @dev the from-to swapping direction is reverted compared to openPosition
        (cache.amountToAdd, cache.amountFromAdd) = Base.getRequiredRepay(lien.liquidity, lien.tokenId);
        if (!lien.zeroForOne) (cache.amountToAdd, cache.amountFromAdd) = (cache.amountFromAdd, cache.amountToAdd);

        // the liquidity to add must be no less than the available amount
        /// @dev the max available amount contains the tokensOwed, will have another check in below at refundWithCheck
        if (
            cache.amountFromAdd > cache.collateralFrom + cache.tokenFromPremium - cache.amountSpent ||
            cache.amountToAdd > cache.amountReceived + cache.tokenToPremium
        ) {
            revert Errors.InsufficientRepay();
        }

        // add liquidity back to borrower
        if (lien.zeroForOne) {
            (cache.liquidityAdded, cache.amountToAdd, cache.amountFromAdd) = LiquidityPosition.increaseLiquidity(
                cache.tokenTo,
                cache.tokenFrom,
                lien.tokenId,
                cache.amountToAdd,
                cache.amountFromAdd
            );
        } else {
            (cache.liquidityAdded, cache.amountFromAdd, cache.amountToAdd) = LiquidityPosition.increaseLiquidity(
                cache.tokenFrom,
                cache.tokenTo,
                lien.tokenId,
                cache.amountFromAdd,
                cache.amountToAdd
            );
        }

        // obtain the position's latest FeeGrowthInside after increaseLiquidity
>>>     (, , , , , , , , cache.feeGrowthInside0LastX128, cache.feeGrowthInside1LastX128, , ) = Base
            .UNI_POSITION_MANAGER
            .positions(lien.tokenId);

        // caculate the amounts owed since last fee collection during the borrowing period
>>>        (cache.token0Owed, cache.token1Owed) = Base.getOwedFee(
            cache.feeGrowthInside0LastX128,
            cache.feeGrowthInside1LastX128,
            lien.feeGrowthInside0LastX128,
            lien.feeGrowthInside1LastX128,
            lien.liquidity
        );

        ...

        // pay for interest
>>>     lps.addTokensOwed(lien.tokenId, cache.token0Owed, cache.token1Owed);
    }

As previously mentioned, If the LP Uni V3 position is withing active range, the LP provider can intentionally use the same pool to increase the fee growth and token owed for them when liquidating a position. And traders will pay fee more than it should.

Tools Used

Manual review

Recommended Mitigation Steps

Seems there is no obvious way to solve the issue inside openPosition. But for _closePosition, move query for cache.feeGrowthInside0LastX128 and cache.feeGrowthInside1LastX128 before the swap is performed.

Assessed type

Context

[c4-judge]

0xleastwood marked the issue as primary issue

[wukong-particle]

I think this would be an unavoidable behavior. Swapping using the liquidity from the same pool indeed should pay the swap fee. Regarding "maliciously" providing swapping liquidity and lending liquidity and be the liquidator at the same time, then users might not want to interact with such pools since it's too centralized. As long as there are other liquidity providers in the play, doing manipulation will pay the price for benefitting other LPs. As long as there's enough LPs, manipulation won't have enough incentive.

[c4-sponsor]

wukong-particle (sponsor) acknowledged

[c4-judge]

0xleastwood marked the issue as selected for report

[romeroadrian]

Not sure this is an issue, whether to include or not the same pool in the path taken by the dex aggregator doesn't cause any real issue, those are fees after all and should be factored in the logic, and in any case these should be low amounts since it's a fee from a single trade.

[0xleastwood]

I guess it is unclear how to fix this as there are already safety checks to ensure a liquidation returns sufficient capital to repay LPs. This seems to be more of an issue of skimming funds by forcing the borrower to pay more fees which is another issue altogether. But the warden has not highlighted this and is more focused on the swaps itself. I think this makes sense to be QA for these reasons.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

0xleastwood marked the issue as grade-a