howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/main/src/ILOPool.sol#L184 https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Pool.sol#L517 https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Pool.sol#L490

Vulnerability details

Impact

Incorrect fee distribution in claim().

Proof of Concept

After a successful project launch, investors can claim their share of liquidity. The claim() function calculates the liquidity2Claim to burn. It is important to note that pool.burn() recalculates fees owed to a position, crediting amount0, amount1, and fees accumulated to position.tokensOwed0 and position.tokensOwed1.

    /// @notice Burn liquidity from the sender and account tokens owed for the liquidity to the position
    /// @dev Can be used to trigger a recalculation of fees owed to a position by calling with an amount of 0
    /// @dev Fees must be collected separately via a call to #collect
    /// @param tickLower The lower tick of the position for which to burn liquidity
    /// @param tickUpper The upper tick of the position for which to burn liquidity
    /// @param amount How much liquidity to burn
    /// @return amount0 The amount of token0 sent to the recipient
    /// @return amount1 The amount of token1 sent to the recipient
    function burn(
        int24 tickLower,
        int24 tickUpper,
        uint128 amount
    ) external returns (uint256 amount0, uint256 amount1);

Upon calling burn(), the following updates occur:

        if (amount0 > 0 || amount1 > 0) {
            (position.tokensOwed0, position.tokensOwed1) = (
                position.tokensOwed0 + uint128(amount0),
                position.tokensOwed1 + uint128(amount1)
            );
        }

In Uniswap V3, feeGrowthInside0LastX128 and feeGrowthInside1LastX128 represent the cumulative fees earned per unit of liquidity in token0 and token1 within a specific position's range since the last update. When a user claims, they receive their share of fee0 and fee1 by multiplying these values by positionLiquidity.

The amount0 and amount1 variables are incremented by fees0 and fees1, representing the investor's fair share:

            amount0 += fees0;
            amount1 += fees1;

The function then calls pool.collect() to collect all of token0 and token1 in Position.Info, which includes amounts from burn() plus all fees accumulated since the last update:

    function collect(
        address recipient,
        int24 tickLower,
        int24 tickUpper,
        uint128 amount0Requested,
        uint128 amount1Requested
    ) external override lock returns (uint128 amount0, uint128 amount1) {
        // we don't need to checkTicks here, because invalid positions will never have non-zero tokensOwed{0,1}
        Position.Info storage position = positions.get(msg.sender, tickLower, tickUpper);

      ->amount0 = amount0Requested > position.tokensOwed0 ? position.tokensOwed0 : amount0Requested;
      ->amount1 = amount1Requested > position.tokensOwed1 ? position.tokensOwed1 : amount1Requested;

        if (amount0 > 0) {
            position.tokensOwed0 -= amount0;
            TransferHelper.safeTransfer(token0, recipient, amount0);
        }
        if (amount1 > 0) {
            position.tokensOwed1 -= amount1;
            TransferHelper.safeTransfer(token1, recipient, amount1);
        }

        emit Collect(msg.sender, recipient, tickLower, tickUpper, amount0, amount1);
    }

The amountCollected0 and amountCollected1 variables represent amounts owed to the investor from burning liquidity and all fees accumulated by the position since the last update. However, amountCollected0 - amount0 and amountCollected1 - amount1 are sent to the feeTaker, which can include fees accumulated by other investors who have not yet claimed their fair share.

Consider following: amount0 = 10 and amount1 = 10 (10 , 10) = pool.burn() (9 , 9) = _deductFees()

fees0 = 10 and fees1 = 10 (9 , 9) = _deductFees()

amount0 += fees0 == 18 amount1 += fees1 == 18

Assume amountCollected0 == 30 and amountCollected1 == 30.

feeTaker will receive 30 - 18 when they should have only received 2. Other investors lose out on fees.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider sending all fees accumulated to a different contract where the investors can claim based on their tokenId.

Assessed type

Uniswap

c4-judge commented 4 months ago

alex-ppg marked the issue as duplicate of #43