_validatePositionList() positionIdList can still lead to forgery

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/PanopticPool.sol#L1334-L1361

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/PanopticPool.sol#L1334-L1361

    function _validatePositionList(
        address account,
        TokenId[] calldata positionIdList,
        uint256 offset
    ) internal view {
        uint256 pLength;
        uint256 currentHash = s_positionsHash[account];

        unchecked {
            pLength = positionIdList.length - offset;
        }

        uint256 fingerprintIncomingList;

        for (uint256 i = 0; i < pLength; ) {
            fingerprintIncomingList = PanopticMath.updatePositionsHash(
                fingerprintIncomingList,
                positionIdList[i],
                ADD
            );
            unchecked {
                ++i;
            }
        }

        // revert if fingerprint for provided '_positionIdList' does not match the one stored for the '_account'
        if (fingerprintIncomingList != currentHash) revert Errors.InputListFail();
    }

This function is called from multiple places and is primarily used to validate the legality of positionIdList.

The main logic involves iterating through tokenIds and performing XOR operations, then comparing the result with s_positionsHash[account].

It calls the function https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/libraries/PanopticMath.sol#L125-L145

    function updatePositionsHash(
        uint256 existingHash,
        TokenId tokenId,
        bool addFlag
    ) internal pure returns (uint256) {
        // add the XOR`ed hash of the single option position `tokenId` to the `existingHash`
        // @dev 0 ^ x = x

        // update hash by taking the XOR of the new tokenId
        uint256 updatedHash = uint248(existingHash) ^
            (uint248(uint256(keccak256(abi.encode(tokenId)))));

        // increment the upper 8 bits (position counter) if addflag=true, decrement otherwise
        uint256 newPositionCount = addFlag
            ? uint8(existingHash >> 248) + 1
            : uint8(existingHash >> 248) - 1;

        unchecked {
            return uint256(updatedHash) + (newPositionCount << 248);
        }
    }

Issue as explained by this previous report is that the overflow is ignored, the suggested fix was tol include axcheck that the returned value is less than 255, but thec heck has not been included in the updatePositionsHash() function so the issue has not been fixed, would be key to note that there is a new check that is now present in the PanopticPool's _updatePositionsHash() as seen here https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/PanopticPool.sol#L1371-L1384, however this function only gets called via the _addUserOption() which is only called when minting options, which would mean that the bug case has been sufficiently fixed for when minting options.

However using this search command: https://github.com/search?q=repo%3Acode-423n4%2F2024-06-panoptic+updatePositionsHash&type=code, we can see that there are multiple other core instances where the unprotected PanopticMath.updatePositionsHash() function gets called, which would then mean that this bug case has not been fixed for these other instances.

Impact

_validatePositionList() positionIdList can still lead to forgery.

Would be key to note that with the current implementation only during minting has this bug case been mitigated against due to the call to _addUserOption when minting options , however during burning, liquidating, and force exercising options this bug case still exists.

Recommended Mitigation Steps

Apply the suggested fix from the report, which is to include a check in the updatePositionsHash()

Assessed type

Invalid Validation

[Picodes]

See https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/libraries/PanopticMath.sol#L143

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)