Hash collision in the `PanopticMath.updatePositionsHash` function could prompt wrong positions in the `positionIdList` to be validated

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L100-L109 https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1382-L1394

Vulnerability details

Impact

The PanopticPool._validatePositionList is a critical function in the Panoptic protocol. This function is used to ensure that the positions in the incoming user's list match the existing active option positions.

The _validatePositionList function is used by multiple critical functions in the protocol such as _mintOptions, _validateSolvency, liquidate, forceExercise and settleLongPremium. These functions ensure the solvency of the protocol and hence can be considered as highly critical functions for the health and sustainability of the protocol.

The PanopticPool._validatePositionList function calls the PanopticMath.updatePositionsHash in its execution flow. The PanopticMath.updatePositionsHash function is used to update an existing account's "positions hash" with a new single position tokenId. This updated position hash is calculated as shown below:

            uint248 updatedHash = uint248(existingHash) ^ 
                (uint248(uint256(keccak256(abi.encode(tokenId))))); 

But there is an issue with the above calculation since the hashed value of the encoded tokenId, by the keccak256 is truncated to a uint248 (from uint256) before performing XOR with the existingHash. This truncation is performed to keep the most significant 8 bytes to store the count of the positions.

Due to the above truncation a hash collision could occur where two different tokenIds with two different option positions could result in the same truncated uint248 hash value. As a result a malicious user can provide a manipulated tokenId inplace of a valid tokenId in the positionIdList, as an input to the PanopticPool._validatePositionList function such that when truncated (hash value) to uint248, the resulting updatedHash would equal to the existing active option positions stored in the s_positionsHash[account] mapping (stored in the storage).

As it was explained earlier since the _validatePositionList is used by critical functions such as _mintOptions, _validateSolvency, liquidate, forceExercise and settleLongPremium, the above vulnerability could create many undesired behaviors in the protocol and could break the protocol.

For example during the liquidate function call a malicious liquidator can use the above vulnerability to provide malicious positionIdListLiquidator and positionIdList as input TokenId[] arrays to the PanopticPool.liquidate function. If the malcious tokenId or tokenIds result in the same truncated hash as of the valid tokenIds in the storage option position, the transaction will proceed without revert.

As a result the malicious liquidator can liquidate a position with different position parameters for a given liquidatee which is an undesirable state to the protocol. This could lead to more liquidity being removed or added to the UniswapV3 pool than the valid position truly hold. This could further lead to broken state in the fee calculation as well. Further to the above a malicious liquidator can bypass the _checkSolvencyAtTick check in the liquidate function by providing malicoius tokenIds which would give him enough balance to stay solvent. This happens because the preceeding _validatePositionList check will not be able to identify the malicious tokenId in the positionIdListLiquidator array since the truncated hash (to uint248) matches the valid tokenId hash, stored in the storage (s_positionsHash[account]) for the given tokenId position.

The above vulnerability could create such undesired behavior and broken state in the other critical functions I explained above as well. This could lead the panoptic protocol to be insolvent as well.

Proof of Concept

        unchecked {
            // update hash by taking the XOR of the new tokenId
            uint248 updatedHash = uint248(existingHash) ^
                (uint248(uint256(keccak256(abi.encode(tokenId)))));
            // increment the top 8 bit if addflag=true, decrement otherwise
            return
                addFlag
                    ? uint256(updatedHash) + (((existingHash >> 248) + 1) << 248)
                    : uint256(updatedHash) + (((existingHash >> 248) - 1) << 248);
        }

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L100-L109

        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();

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1382-L1394

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to use the complete hash value of the tokenId (uint256) when updatedHash is calculated in the PanopticMath.updatePositionsHash function. This will ensure no hash collision will occur for two different tokenIds. To save the count of the positions a separate state variable can be used in the PanopticPool contract.

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #498

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)