Panoptic - 0xDING99YA's results

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L578-L635

Vulnerability details

Impact

Under the current implementation of transfer of the ERC1155 token, two situations may occur:

1. A user has some amount tokenId but corresponding liquidity (both current and removed) are zero
2. A user has non zero removed liquidity but has no corresponding tokenId

There are two impacts here. First, in the future when some protocols want to integrate with Panoptic by reading a user ERC1155 token balance or account liquidity, it can get an inaccurate value and may make mistake. Second impact is in the second scenario above it block the user from receiving any tokens containing the same token type, lower tick and upper tick forever because of that non zero removed liquidity.

Proof of Concept

In registerTokenTransfer(), when it transfers token from a user, it will transfer all liquidity (both current and removed liquidity) to the recipient, this is the root issue. Let's think about a scenario below:

1. Alice mints 200 ERC1155 ID1 with only one leg, for that leg isLong = 0, lower tick = LT, higher tick = HT. Alice's current liquidity is C.
2. Alice mints 100 ERC1155 ID2 with only one leg, for that leg isLong = 1, lower tick = LT, higher tick = HT. Alice's current liquidity is now C/2 and removed liquidity is also C/2.
3. Alice now transfers 100 ID1 to Bob, this will pass all the check in transfer. After the transfer, Alice will have 0 current liquidity and 0 removed liquidity, but she still has 100 ID1 and 100 ID2; Bob will have C/2 current liquidity and C/2 removed liquidity and 100 ID1.
4. Bob then burn all the 100 ID1. Now Bob has 0 token ID1, 0 current liquidity, but still has C/2 removed liquidity.
5. Since Bob can't eliminate this C/2 removed liquidity, his corresponding s_accountLiquidity will never be 0 and this can block him receiving any token containing the same token type, lower tick and higher tick forever.

A coded POC is below, to run it, simply add it to SemiFungiblePositionManager.t.sol

function test_TransferLeadToError() public {
        // We choose a random number to initiate pool
        _initPool(3897);

        /// this tokenId has isLong = 0
        uint256 tokenId = uint256(0).addUniv3pool(poolId).addLeg(
            0,
            1,
            isWETH,
            0,
            0,
            0,
            int24(18000),
            int24(980)
        );

        int24 oneSidedRange = (int24(980) * tickSpacing) / 2;

        (int24 legLowerTick, int24 legUpperTick) = (
            int24(18000) - oneSidedRange,
            int24(18000) + oneSidedRange
        );

        // We mint 200 tokenId for Alice
        (int256 totalCollected, int256 totalSwapped, int24 newTick) = sfpm.mintTokenizedPosition(
            tokenId,
            uint128(200),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        uint256 accountLiquidities = sfpm.getAccountLiquidity(
            address(pool),
            Alice,
            0,
            legLowerTick,
            legUpperTick
        );

        // Make sure Alice has 164 liquidity and 0 removed liquidity
        assertEq(accountLiquidities.leftSlot(), 0);
        assertEq(accountLiquidities.rightSlot(), 0xa4);

        vm.startPrank(Alice);

        // Then Alice mint 100 newTokenId which is exactly same as tokenId except isLong = 1
        uint256 newTokenId = uint256(0).addUniv3pool(poolId).addLeg(
            0,
            1,
            isWETH,
            1,
            0,
            0,
            int24(18000),
            int24(980)
        );

        sfpm.mintTokenizedPosition(newTokenId, uint128(100), TickMath.MIN_TICK, TickMath.MAX_TICK);

        accountLiquidities = sfpm.getAccountLiquidity(
            address(pool),
            Alice,
            0,
            legLowerTick,
            legUpperTick
        );

        // Make sure Alice remove some liquidity, she should have 82 liquidity and 82 removed liquidity
        assertEq(accountLiquidities.leftSlot(), 0x52);
        assertEq(accountLiquidities.rightSlot(), 0x52);

        // Alice then transfer 100 tokenId to Bob
        sfpm.safeTransferFrom(Alice, Bob, tokenId, 100, "");

        // After the transfer, Alice should still have 100 tokenId, Bob should have 100 tokenId
        // Alice should also have 100 newTokenId
        assertEq(sfpm.balanceOf(Alice, tokenId), 100);
        assertEq(sfpm.balanceOf(Bob, tokenId), 100);
        assertEq(sfpm.balanceOf(Alice, newTokenId), 100);

        // Even if Alice has 100 tokenId and 100 newTokenId, her entire liquidity and removed liquidity are both 0
        accountLiquidities = sfpm.getAccountLiquidity(
            address(pool),
            Alice,
            0,
            legLowerTick,
            legUpperTick
        );

        assertEq(accountLiquidities.leftSlot(), 0);
        assertEq(accountLiquidities.rightSlot(), 0);

        // Bob now have Alice's original liquidity, so 82 liquidity and 82 removed liquidity
        accountLiquidities = sfpm.getAccountLiquidity(
            address(pool),
            Bob,
            0,
            legLowerTick,
            legUpperTick
        );

        assertEq(accountLiquidities.leftSlot(), 0x52);
        assertEq(accountLiquidities.rightSlot(), 0x52);

        vm.stopPrank();

        vm.startPrank(Bob);

        // Bob decides to burn all the 100 tokenId
        sfpm.burnTokenizedPosition(tokenId, uint128(100), TickMath.MIN_TICK, TickMath.MAX_TICK);

        // Even if Bob has 0 tokenId and 0 newTokenId, his removed liquidity is still 0x52 and this can't be eliminated forever
        // Bob can never receive any token containing the same token type along with same lower/upper tick
        // because his accountLiquidities for that position will never be 0 from now on
        accountLiquidities = sfpm.getAccountLiquidity(
            address(pool),
            Bob,
            0,
            legLowerTick,
            legUpperTick
        );

        assertEq(accountLiquidities.leftSlot(), 0x52);
        assertEq(accountLiquidities.rightSlot(), 0);
        assertEq(sfpm.balanceOf(Bob, tokenId), 0);
        assertEq(sfpm.balanceOf(Bob, newTokenId), 0);

        vm.stopPrank();
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Just don't allow a user to transfer when he has both non-zero currentLiquidity and removedLiquidity.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1049-L1066 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1093-L1122 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1200-L1248

Vulnerability details

Impact

Currently, when computing the fee a user can collect, Uniswap V3 uses the ways as (currFeeGrowth - prevFeeGrowth) * liquidity / Q128, in Panoptic it uses (currFeeGrowth * liquidity / Q128) - (prevFeeGrowth * liquidity / Q128). However, this slightly difference can result in a user to collect more fees than he should have, thus break the main invariant that "Fees paid to a given user should not exceed the amount of fees earned by the liquidity owned by that user".

Proof of Concept

In Panoptic the fee collection mechanism is as follow:

int256 amountToCollect = _getFeesBase(univ3pool, startingLiquidity, liquidityChunk).sub(
            s_accountFeesBase[positionKey]
        );

feesBase = int256(0)
            .toRightSlot(int128(int256(Math.mulDiv128(feeGrowthInside0LastX128, liquidity))))
            .toLeftSlot(int128(int256(Math.mulDiv128(feeGrowthInside1LastX128, liquidity))));

To show why the slightly difference in calculation can result in the consequence, let's consider some values: currFeeGrowth is 3 * 2^124, prevFeeGrowth is (1 * 2^124 + 1), user position liquidity is 2^4. Based on UniswapV3, the collect fee should be (3 * 2^124 - 1 * 2^124 - 1) * 2^4 / 2^128= (2 * 2^124 - 1) * 2^4/2^128 = (2 * 2^128 - 2^4) / 2^128 = 1. However, based on Panoptic, the fee to be collected is (3 * 2^124 * 2^4)/2^128 - (1 * 2^124 + 1) * 2^4/2^128 = 3 - 1 = 2, which is larger than the fee that the user should obtain. If in the same position there is any other users also collecting the fee, that user may lose his fee due to this error.

The difference between the collected fee is 1 seems non-trivial, but it can be a problem in some cases especially for those token with low decimals. In the documentation Panoptic claims that it supports any ERC20 with no-transfer-fee, if there are some tokens with low decimals but worth a lot, this issue can be very serious. Also note this issue will exist at any case, especially seriously for those pools where deltaFeeGrowth * liquidity is around Q128 level and the severity varies case by case.

To show this issue really exists I will take Gemini USD - USDC pool in Uniswap V3 as an example, and below is the coded POC to show the different calculation indeed cause user collect more fees.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {stdMath} from "forge-std/StdMath.sol";
import {Errors} from "@libraries/Errors.sol";
import {Math} from "@libraries/Math.sol";
import {PanopticMath} from "@libraries/PanopticMath.sol";
import {CallbackLib} from "@libraries/CallbackLib.sol";
import {TokenId} from "@types/TokenId.sol";
import {LeftRight} from "@types/LeftRight.sol";
import {IERC20Partial} from "@testUtils/IERC20Partial.sol";
import {TickMath} from "v3-core/libraries/TickMath.sol";
import {FullMath} from "v3-core/libraries/FullMath.sol";
import {FixedPoint128} from "v3-core/libraries/FixedPoint128.sol";
import {IUniswapV3Pool} from "v3-core/interfaces/IUniswapV3Pool.sol";
import {IUniswapV3Factory} from "v3-core/interfaces/IUniswapV3Factory.sol";
import {LiquidityAmounts} from "v3-periphery/libraries/LiquidityAmounts.sol";
import {SqrtPriceMath} from "v3-core/libraries/SqrtPriceMath.sol";
import {PoolAddress} from "v3-periphery/libraries/PoolAddress.sol";
import {PositionKey} from "v3-periphery/libraries/PositionKey.sol";
import {ISwapRouter} from "v3-periphery/interfaces/ISwapRouter.sol";
import {SemiFungiblePositionManager} from "@contracts/SemiFungiblePositionManager.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {PositionUtils} from "../testUtils/PositionUtils.sol";
import {UniPoolPriceMock} from "../testUtils/PriceMocks.sol";
import {ReenterMint, ReenterBurn} from "../testUtils/ReentrancyMocks.sol";
import {IUniswapV3Pool} from "univ3-core/interfaces/IUniswapV3Pool.sol";

contract LiquidityProvider {
    IERC20 constant token0 = IERC20(0x056Fd409E1d7A124BD7017459dFEa2F387b6d5Cd);
    IERC20 constant token1 = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);

    function uniswapV3MintCallback(
        uint256 amount0Owed,
        uint256 amount1Owed,
        bytes calldata data
    ) external {
        if (amount0Owed > 0) token0.transfer(msg.sender, amount0Owed);
        if (amount1Owed > 0) token1.transfer(msg.sender, amount1Owed);
    }

    function uniswapV3SwapCallback(
        int256 amount0Delta,
        int256 amount1Delta,
        bytes calldata data
    ) external {
        IERC20 token = amount0Delta > 0 ? token0 : token1;

        uint256 amountToPay = amount0Delta > 0 ? uint256(amount0Delta) : uint256(amount1Delta);

        token.transfer(msg.sender, amountToPay);
    }

    function arbitraryCall(bytes calldata data, address pool) public {
        (bool success, ) = pool.call(data);
        require(success);
    }
}

contract CollectFee is Test {
    address constant GeminiUSDCPool = 0x5aA1356999821b533EC5d9f79c23B8cB7c295C61;
    address constant GeminiUSD = 0x056Fd409E1d7A124BD7017459dFEa2F387b6d5Cd;
    address constant USDC = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48;
    LiquidityProvider Alice;
    uint160 internal constant MIN_V3POOL_SQRT_RATIO = 4295128739;
    uint160 internal constant MAX_V3POOL_SQRT_RATIO =
        1461446703485210103287273052203988822378723970342;

    uint256 mainnetFork;

    struct Info {
        uint128 liquidity;
        uint256 feeGrowthInside0LastX128;
        uint256 feeGrowthInside1LastX128;
        uint128 tokensOwed0;
        uint128 tokensOwed1;
    }

    function setUp() public {
        // Use your own RPC to fork the mainnet
        mainnetFork = vm.createFork(
            "Your RPC"
        );
        vm.selectFork(mainnetFork);

        Alice = new LiquidityProvider();
        deal(USDC, address(Alice), 1000000 * 1e6);

        vm.startPrank(address(Alice));
        IERC20(USDC).approve(GeminiUSDCPool, type(uint256).max);
        vm.stopPrank();
    }

    function testFeeCollectionBreakInvariant() public {
        // First swap to get some GeminiUSD balance
        bytes memory AliceSwapData = abi.encodeWithSignature(
            "swap(address,bool,int256,uint160,bytes)",
            address(Alice),
            false,
            int256(20000 * 1e6),
            MAX_V3POOL_SQRT_RATIO - 1,
            ""
        );
        Alice.arbitraryCall(AliceSwapData, GeminiUSDCPool);

        // Then mint some position for Alice, the desired liquidity is 10000000000
        bytes memory AliceMintData = abi.encodeWithSignature(
            "mint(address,int24,int24,uint128,bytes)",
            address(Alice),
            92100,
            92200,
            10000000000,
            ""
        );
        Alice.arbitraryCall(AliceMintData, GeminiUSDCPool);

        // Now we retrieve the initial feeGrowth for token0(Gemini USD) after minting the position for Alice
        (
            uint128 liquidity,
            uint256 prevFeeGrowthInside0LastX128,
            uint256 prevFeeGrowthInside1LastX128,
            ,

        ) = IUniswapV3Pool(GeminiUSDCPool).positions(
                keccak256(abi.encodePacked(address(Alice), int24(92100), int24(92200)))
            );

        // Then we perform two swaps (both from Gemini USD to USDC, first amount is 4800 USD then 5000 USD)
        AliceSwapData = abi.encodeWithSignature(
            "swap(address,bool,int256,uint160,bytes)",
            address(Alice),
            true,
            int256(4800 * 1e2),
            MIN_V3POOL_SQRT_RATIO + 1,
            ""
        );

        Alice.arbitraryCall(AliceSwapData, GeminiUSDCPool);

        AliceSwapData = abi.encodeWithSignature(
            "swap(address,bool,int256,uint160,bytes)",
            address(Alice),
            true,
            int256(5000 * 1e2),
            MIN_V3POOL_SQRT_RATIO + 1,
            ""
        );

        Alice.arbitraryCall(AliceSwapData, GeminiUSDCPool);

        // We burn the position of Alice to update feeGrowth for Gemini USD
        bytes memory AliceBurnData = abi.encodeWithSignature(
            "burn(int24,int24,uint128)",
            int24(92100),
            int24(92200),
            uint128(10000000000)
        );
        Alice.arbitraryCall(AliceBurnData, GeminiUSDCPool);

        // Now we retrieve the updated feeGrowth for token0(Gemini USD)
        (
            uint256 newliquidity,
            uint256 currFeeGrowthInside0LastX128,
            uint256 currFeeGrowthInside1LastX128,
            ,

        ) = IUniswapV3Pool(GeminiUSDCPool).positions(
                keccak256(abi.encodePacked(address(Alice), int24(92100), int24(92200)))
            );

        // This is how UniV3 compute collected fee: (currFee - prevFee) * liquidity / Q128
        console.log("Univ3 fee obtained: ");
        uint256 collectFee = ((currFeeGrowthInside0LastX128 - prevFeeGrowthInside0LastX128) *
            10000000000) / (2 ** 128);
        console.log(collectFee);

        console.log("Panoptic fee1 record: ");
        uint256 collectFee1 = (currFeeGrowthInside0LastX128 * 10000000000) / (2 ** 128);

        console.log("Panoptic fee2 record: ");
        uint256 collectFee2 = (prevFeeGrowthInside0LastX128 * 10000000000) / (2 ** 128);

        // This is how Panoptic compute collected fee: currFee * liquidity / Q128 - prevFee * liquidity / Q128
        console.log("Panoptic way to calculate collected fee: ");
        console.log(collectFee1 - collectFee2);

        // Then we ensure the fee calculated by Panoptic is larger than UniV3
        assertGt(collectFee1 - collectFee2, collectFee);
    }
}

In this case that user can collect 1 more Gemini USD which is around 0.01 USD, and also please note that these are just two normal swaps with thousands of dollars and in reality this effect can accumulate due to much more trading volume per day, also imagine there may be some similar token but pegged to ETH or even BTC instead, in that case will be 0.01 ETH or even 0.01 BTC which can be much more value! Also, in Panoptic there can be multiple persons for the same option position, let's say 10 users for the same option position, the first 9 users who collect the fee will get more, and the last user can get much less fee because each of the 9 person will take part of the fee from him!

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

In the comments it seems Panoptic team has the reason to use this way to calculate the collected fee after much consideration, so I think instead of supporting any ERC20 token, may introducing a whitelist to only add those pool which will not affected by this issue significantly.

Assessed type

Other