storage collision

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L152
https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L375-L378

Vulnerability details

Impact

In SemiFungiblePositionManager::initializeAMMPool we use FACTORY.getPool to get the pool from uniswap with token 0, token 1
and fee, it returns the address of the pool and then we use PanopticMath.getPoolId to get the first 8 bytes of the address.
The problem is that an attacker can create a collision because we only use 8 bytes. It seems that collision is difficult
to happen, but it is not that difficult if we use a fast language such as rust or go, where you can generate address that has
the same first 8 bytes. This is code written in go.

this code could generate billion of addresses in short period.
simple code to brute force first 8 bytes go

the impact here is that an attacker can change the storage here
because it will store changes to the same storage slot when first 8 bytes matches here.
so an attacker can change the storage and this will effect the whole protocol.

• the protocol must not risk with such thing because it won't take long time until an attacker finds a collision.

Proof of Concept

simple code to brute force first 8 bytes go

NOTE: in this example we are going to use address provided in PanopticMath: 0x8ad599c3A0ff1De082011EFDDc58f1908eb6e6D8
as and example

1. attacker uses Golang code i provided to brute force Token address simulating createPool
2. when Golang code finds the targeted first 8 bytes he uses createPool to Create Pool USDC/TokenX:
   • token0: for example USDC.
   • token1: this is attacker's ERC20 token address (TokenX).
   • fee : fee

• createPool will create pool with address 0x8ad599c3A0ff1De0fffffffffffffffffffffff

3. then attacker calls SemiFungiblePositionManager::initializeAMMPool with parms: token0, token1, fee.
4. getPool will return attacker's pool address:
   • 0x8ad599c3A0ff1De0ffffffffffffffffffffffff (just example)
5. PanopticMath.getPoolId this will return first 8 bytes which is:
   • 0x8ad599c3A0ff1De0
6. now if there is something stored in s_poolContext[uint64(0x8ad599c3A0ff1De0)] it will be change because of the collision with attacker's pool address
   and storage slot will be rewritten with attacker's pool address.

       s_poolContext[poolId] = PoolAddressAndLock({
           pool: IUniswapV3Pool(univ3pool), //  <= this will be rewritten
           locked: false
       });

   from 0x8ad599c3A0ff1De082011EFDDc58f1908eb6e6D8 to 0x8ad599c3A0ff1De0ffffffffffffffffffffffff

Tools Used

manually

Recommended Mitigation Steps

    uint64 poolId = PanopticMath.getPoolId(univ3pool);
    
+   if(address(s_poolContext[poolId].pool) != address(0)) revert PoolId_Exits(); 

    while (address(s_poolContext[poolId].pool) != address(0)) {
        poolId = PanopticMath.getFinalPoolId(poolId, token0, token1, fee);
    }
    // store the poolId => UniswapV3Pool information in a mapping
    // `locked` can be initialized to false because the pool address makes the slot nonzero
    s_poolContext[poolId] = PoolAddressAndLock({
        pool: IUniswapV3Pool(univ3pool),
        locked: false
    });

this would be huge problem for the protocol in long term.

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

dyedm1 (sponsor) disputed

[dyedm1]

This line while (address(s_poolContext[poolId].pool) != address(0)) { poolId = PanopticMath.getFinalPoolId(poolId, token0, token1, fee); }

prevents any such storage collision from occuring as described (and has the same functionality of the suggested check, but the added benefit of not precluding multiple pools with the same last 64 bits from being deployed)

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid