Same values for poolId == 0 and pool which is uninitialized may cause wrong minting/burning of positions

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L368
https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1468-L1470

Vulnerability details

In initializeAMMPool() there may be a combination for univ3pool which will give a poolId value of 0. Although the s_AddrToPoolIdData[univ3pool] will store a non-zero value to make a distinction between zero and uninitialized univ3pool, there is still a confusion while fetching the poolId via getPoolId(address univ3pool).
This function getPoolId() will return 0 to both an initialized(with zero) poolId and for an uninitialized univ3pool.

    function getPoolId(address univ3pool) external view returns (uint64 poolId) {
        poolId = uint64(s_AddrToPoolIdData[univ3pool]);
    }

Impact

Assume two pools in UniswapV3

  IUniswapV3Pool constant USDC_WETH_5 = IUniswapV3Pool(0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640);
  IUniswapV3Pool constant USDC_WETH_30 = IUniswapV3Pool(0x8ad599c3A0ff1De082011EFDDc58f1908eb6e6D8);

Both are having same token0 and token1 but different fees.
Now there is a possibility that for the first univ3pool USDC_WETH_5 during initializeAMMPool() in panoptic SemiFungiblePositionManager, the poolId value returned by getPoolId() is zero. The second univ3pool USDC_WETH_30 is still not initialized in SemiFungiblePositionManager.
For both the initialized and uninitialized pools, the tokenId value will be same i.e., the 64 bits of Univ3 Pool Address, as well as token0 and token1.
So if a user wishes to create a new position for the uninitialized pool (USDC_WETH_30) using mintTokenizedPosition, the position gets actually created against the initialized pool(USDC_WETH_5), since all the relevant parameters are same.
This is because both getPoolId(USDC_WETH_5) and getPoolId(USDC_WETH_30) returns same value of 0, which can cause confusion in tokenId structure.
The same holds while using burnTokenizedPosition()

Any upstream protocol integerating with panoptic will not be able to find any such errors.

Proof of Concept

The below test code shows how both initialized(to zero) and uninitialized pool gets value of 0 from getPoolId()
Assume two pools in UniswapV3

  IUniswapV3Pool constant USDC_WETH_5 = IUniswapV3Pool(0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640);
  IUniswapV3Pool constant USDC_WETH_30 = IUniswapV3Pool(0x8ad599c3A0ff1De082011EFDDc58f1908eb6e6D8);

The first pool USDC_WETH_5 is initialized and USDC_WETH_30 is uninitialzed in SemiFungiblePositionManager.
Also assume that this pool USDC_WETH_5 returns a poolId of 0.
For this the PanopticMath.sol needs to be Mocked as below

    function getPoolId(address univ3pool) internal pure returns (uint64) {
        if (univ3pool == address(0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640)) {
           return uint64(0);
        }
        return uint64(uint160(univ3pool) >> 96);
    }

Add the below sample foundry test into SemiFungiblePositionManager.t.sol and run

function test_initializeAMMPool_zero_poolId() public {
    _cacheWorldState(USDC_WETH_5);
    sfpm.initializeAMMPool(token0, token1, fee);
    address univ3pool = V3FACTORY.getPool(token0, token1, fee);
    console.log("Initialized   univ3pool, poolId =", univ3pool, sfpm.getPoolId(univ3pool));

    _cacheWorldState(USDC_WETH_30);
    univ3pool = V3FACTORY.getPool(token0, token1, fee);
    console.log("Uninitialized univ3pool, poolId =", univ3pool, sfpm.getPoolId(univ3pool));

}

The run logs output is as follows which shows that both have getPoolId() = 0

Logs:
  Initialized   univ3pool, poolId = 0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640 0
  Uninitialized univ3pool, poolId = 0x8ad599c3A0ff1De082011EFDDc58f1908eb6e6D8 0

Recommended Mitigation Steps

Donot allow the value of poolId equal to 0; Add a check as below in function initializeAMMPool()

Line 368        uint64 poolId = PanopticMath.getPoolId(univ3pool);
Line 369
Line 370        while ((address(s_poolContext[poolId].pool) != address(0)) || (poolId == 0)) {     // modified line
Line 371            poolId = PanopticMath.getFinalPoolId(poolId, token0, token1, fee);
Line 372        }

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

dyedm1 marked the issue as disagree with severity

[c4-sponsor]

dyedm1 (sponsor) confirmed

[dyedm1]

Any upstream protocol integerating with panoptic will not be able to find any such errors.

agreed this might be a little confusing, but Panoptic at least always calls initialize before using a pool. Also, it is extremely difficult to find a pool with such a poolId (no birthday attack possible) and could only be achieved with a large amount of computing power and ample time. Even if someone were to create such a pool, it wouldn't be a relevant one because at least one of the tokens would have to have a randomized address.

Id say low/QA or nothing for this just because it isn't really feasible in the wild/has low/no impact even if achieved.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

In the described scenario there is no loss of funds but a broken functionality which is very unlikely to happen, so Low severity seems appropriate.

[c4-judge]

Picodes marked the issue as grade-b

[ksk2345]

The issue #128 has details of generating address collisions in the section "Feasibility of the Attack"
#128

The issue @260 has details of generating an token address with collision using rust/go program
#260

@dyedm1 has commented that "could only be achieved with a large amount of computing power and ample time", but with details given in both the above issues, the comment wrt feasibility is wrong. Also, in the issue#128 @dyedm1 seems to agree with the feasibility, but disagrees about the feasibility in this issue#594

For extra safety and logical correctness, it makes sense to never have a situration when the pool-id = 0

Regarding the impact, as i have mentioned in the submission, the users will be minting/burning positions in wrong set of pool, which may result in loss against their investment strategy. So user funds are affected.

[dyedm1]

The key point is not that a pool with 8 trailing zero bytes is impossible to compute, but that it is very expensive and there are no significant incentives/impacts. To date, no one has ever mined an address with 8 leading or trailing zero bytes (even though there is a gas/vanity incentive to do so). To create the situation described, you would have to spend a lot of time and money on compute to create an impact that might, at worst, confuse users about the poolId to use, which they should be verifying by getting the address from the returned poolId to make sure it's the pool they want to deposit in. Issue 128 is actually harder (compute-wise) than this one to create, so much so that it's also present in official Uniswap router contracts considered "safe", but there are large-scale incentives since all the approved funds could be drained.