c4-bot-10 commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/LiquidityPosition.sol#L253-L263 https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/LiquidityPosition.sol#L178-L204

Vulnerability details

Impact

According to uniswap V3 docs for increase and decrease liquidity:

In production, amount0Min and amount1Min should be adjusted to create slippage protections.

However, current implementation inside Particle doesn't allow users to provide the amount minimum.

Proof of Concept

This are the current implementation for increasing and decreasing liquidity, it can be observed that amount0Min and amount1Min provided are 0 :

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/LiquidityPosition.sol#L178-L204

    function increaseLiquidity(
        address token0,
        address token1,
        uint256 tokenId,
        uint256 amount0,
        uint256 amount1
    ) internal returns (uint128 liquidity, uint256 amount0Added, uint256 amount1Added) {
        // approve spending for uniswap's position manager
        TransferHelper.safeApprove(token0, Base.UNI_POSITION_MANAGER_ADDR, amount0);
        TransferHelper.safeApprove(token1, Base.UNI_POSITION_MANAGER_ADDR, amount1);

        // increase liquidity via position manager
        (liquidity, amount0Added, amount1Added) = Base.UNI_POSITION_MANAGER.increaseLiquidity(
            INonfungiblePositionManager.IncreaseLiquidityParams({
                tokenId: tokenId,
                amount0Desired: amount0,
                amount1Desired: amount1,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            })
        );

        // reset approval
        TransferHelper.safeApprove(token0, Base.UNI_POSITION_MANAGER_ADDR, 0);
        TransferHelper.safeApprove(token1, Base.UNI_POSITION_MANAGER_ADDR, 0);
    }

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/LiquidityPosition.sol#L253-L263

    function decreaseLiquidity(uint256 tokenId, uint128 liquidity) internal returns (uint256 amount0, uint256 amount1) {
        (amount0, amount1) = Base.UNI_POSITION_MANAGER.decreaseLiquidity(
            INonfungiblePositionManager.DecreaseLiquidityParams({
                tokenId: tokenId,
                liquidity: liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            })
        );
    }

There are several instances where increasing and decreasing liquidity are called :

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

    function increaseLiquidity(
        uint256 tokenId,
        uint256 amount0,
        uint256 amount1
    ) external override nonReentrant returns (uint128 liquidity, uint256 amount0Added, uint256 amount1Added) {
        (liquidity, amount0Added, amount1Added) = lps.increaseLiquidity(tokenId, amount0, amount1);
    }

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

    function decreaseLiquidity(
        uint256 tokenId,
        uint128 liquidity
    ) external override nonReentrant returns (uint256 amount0Decreased, uint256 amount1Decreased) {
        (amount0Decreased, amount1Decreased) = lps.decreaseLiquidity(tokenId, liquidity);
    }

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

    function _closePosition(
        DataStruct.ClosePositionParams calldata params,
        DataCache.ClosePositionCache memory cache,
        Lien.Info memory lien,
        address borrower
    ) internal {
       ...

        // 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
            );
        }
   ...
}

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

    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
        if (params.liquidity == 0) revert Errors.InsufficientBorrow();

        // local cache to avoid stack too deep
        DataCache.OpenPositionCache memory cache;

        // prepare data for swap
        (
            cache.tokenFrom,
            cache.tokenTo,
            cache.feeGrowthInside0LastX128,
            cache.feeGrowthInside1LastX128,
            cache.collateralFrom,
            collateralTo
        ) = Base.prepareLeverage(params.tokenId, params.liquidity, params.zeroForOne);

        // decrease liquidity from LP position, pull the amount to this contract
>>>     (cache.amountFromBorrowed, cache.amountToBorrowed) = LiquidityPosition.decreaseLiquidity(
            params.tokenId,
            params.liquidity
        );
      ...
   }

This causes the operations to have minimal slippage protection when involving decreasing or increasing liquidity

Tools Used

Manual review

Recommended Mitigation Steps

When it is possible, allow user to provide amount0Min and amount1Min

Assessed type

Uniswap

c4-judge commented 9 months ago

0xleastwood marked the issue as duplicate of #2