Position owner may not receive accumulated LP fees when they claim liquidity

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/src/ILOPool.sol#L242-L260

Vulnerability details

Impact

Position owner may not receive accumulated LP fees when they claims liquidity, if they are not the first one to claim since the last time the LP fees are collected.

Proof of Concept

When user calls claim() to claim liquidity, the unlocked liquidity combined with the accumulated LP fees are sent to the position owner. The LP fees user will receive is calculated based on the position liquidity and deducted by the performance fee.

            (, uint256 feeGrowthInside0LastX128, uint256 feeGrowthInside1LastX128, , ) = pool.positions(positionKey);
            uint256 fees0 = FullMath.mulDiv(
                                feeGrowthInside0LastX128 - position.feeGrowthInside0LastX128,
                                positionLiquidity,
                                FixedPoint128.Q128
                            );

            uint256 fees1 = FullMath.mulDiv(
                                feeGrowthInside1LastX128 - position.feeGrowthInside1LastX128,
                                positionLiquidity,
                                FixedPoint128.Q128
                            );

            // amount of fees after deduct performance fee
            (fees0, fees1) = _deductFees(fees0, fees1, _project.performanceFee);

After calculation, protocol then collect LP fees from Uniswap pool.

        (uint128 amountCollected0, uint128 amountCollected1) = pool.collect(
            address(this),
            TICK_LOWER,
            TICK_UPPER,
            type(uint128).max,
            type(uint128).max
        );

Then part of the fees combined with unlocked liquidity are sent to the claimer.

        // transfer token for user
        TransferHelper.safeTransfer(_cachedPoolKey.token0, ownerOf(tokenId), amount0);
        TransferHelper.safeTransfer(_cachedPoolKey.token1, ownerOf(tokenId), amount1);

The rest are sent to __FEE_TAKER__.

        // transfer fee to fee taker
        TransferHelper.safeTransfer(_cachedPoolKey.token0, feeTaker, amountCollected0-amount0);
        TransferHelper.safeTransfer(_cachedPoolKey.token1, feeTaker, amountCollected1-amount1);

The problem is that all the uncollected LP fees are collected by the time, if some other users call claim(), they won't receive LP fees because there are no uncollected fees left. Consider the following case:

1. Both Alice and Bob own a ILO position;
2. After pool launched and some swaps, there are LP fees accumulated in the Uniswap pool;
3. Alice calls to claim, the LP fees are collected, part of the fees along with the unlocked liquidity are sent to Alice;
4. Later when Bob calls to claim, he only receive unlocked liquidity because the collected LP fees are 0.

Tools Used

Manual Review

Recommended Mitigation Steps

Collect only the fees owned by the claimer (i.e. fee0 and fee0 before deducted by performance fee).

        (uint128 amountCollected0, uint128 amountCollected1) = pool.collect(
            address(this),
            TICK_LOWER,
            TICK_UPPER,
-           type(uint128).max,
+           fee0WithPerformanceFee,
-           type(uint128).max
+           fee1WithPerformanceFee
        );

Assessed type

Uniswap

[c4-judge]

alex-ppg marked the issue as duplicate of #43

[c4-judge]

alex-ppg marked the issue as partial-75

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)