Integer Overflow in Pool ID Storage *unchecked Addition Can Lead to Incorrect Pool ID in SFPM

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/SemiFungiblePositionManager.sol#L385-L390

Vulnerability details

Impact

SemiFungiblePositionManager Contract (@contracts/SemiFungiblePositionManager.sol:385-390)

// store the UniswapV3Pool => poolId information in a mapping
// add a bit on the end to indicate that the pool is initialized
// (this is for the case that poolId == 0, so we can make a distinction between zero and uninitialized)
unchecked {
    s_AddrToPoolIdData[univ3pool] = uint256(poolId) + 2 ** 255;
}

The intention here is to store the poolId in the s_AddrToPoolIdData mapping and set the most significant bit to indicate that the pool is initialized. However, the addition of 2 ** 255 to the poolId can potentially cause an overflow.

If the poolId is already a large value close to the maximum value of uint256, adding 2 ** 255 to it will result in an overflow, and the stored value will be incorrect. This can lead to unexpected behavior and issues when retrieving the poolId from the mapping later on.

Proof of Concept

The initializeAMMPool function stores the pool information in two mappings: s_poolContext and s_AddrToPoolIdData. If the poolId stored in s_AddrToPoolIdData is incorrect due to the overflow, there will be an inconsistency between the two mappings. This inconsistency can lead to further issues and unexpected behavior when accessing pool-related data.

For Example:

// Assume the maximum value of uint256 is 2^256 - 1
uint256 maxUint256 = type(uint256).max;

// Assume the poolId is very close to the maximum value
uint64 poolId = uint64(maxUint256 - 10);

// Attempt to store the poolId with the most significant bit set
unchecked {
    s_AddrToPoolIdData[univ3pool] = uint256(poolId) + 2 ** 255;
}

The poolId is set to a value very close to the maximum value of uint256. When 2 ** 255 is added to it, an overflow occurs. The resulting value stored in s_AddrToPoolIdData will be incorrect and will not have the intended most significant bit set.

When this incorrect value is retrieved later from the mapping, it can lead to unexpected behavior and errors in other parts of the contract that rely on the poolId.

Tools Used

Vs Code

Recommended Mitigation Steps

Use bitwise OR (|) instead of addition.

unchecked {
    s_AddrToPoolIdData[univ3pool] = uint256(poolId) | (1 << 255);
}

Using bitwise OR can set the most significant bit without the risk of overflow. The 1 << 255 expression shifts the bit 1 to the 255th position, effectively setting the most significant bit to 1 without affecting the other bits of the poolId.

Assessed type

Math

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

It can't overflow as it's a uint64