code-423n4 / 2023-12-particle-findings

2 stars 1 forks source link

increaseLiquidity/decreaseLiquidity Lack of slippage protection #29

Closed c4-bot-1 closed 9 months ago

c4-bot-1 commented 9 months ago

Lines of code

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

Vulnerability details

Vulnerability details

In ParticlePositionManager.mint(), there is slippage protection by params.amount0Min / params.amount1Min

But in increaseLiquidity(), pool.mint() will also be executed There is no slippage protection

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

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

It is recommended to add slippage protection to avoid LP losses

Note: decreaseLiquidity() has a similar problem

Impact

Lack of slippage protection in increaseLiquidity/decreaseLiquidity

Recommended Mitigation

    function increaseLiquidity(
        uint256 tokenId,
        uint256 amount0,
        uint256 amount1,
+       uint256 amount0Min,
+       uint256 amount1Min
    ) external override nonReentrant returns (uint128 liquidity, uint256 amount0Added, uint256 amount1Added) {
        (liquidity, amount0Added, amount1Added) = lps.increaseLiquidity(tokenId, amount0, amount1);
+      require(amount0Added>= amount0Min,"invalid amount0Added");
+      require(amount1Added>= amount1Min,"invalid amount1Added");
    }

Assessed type

Uniswap

c4-judge commented 9 months ago

0xleastwood marked the issue as duplicate of #2

c4-judge commented 8 months ago

0xleastwood marked the issue as satisfactory