_validatePositionList() positionIdList can forgery

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1367

Vulnerability details

Vulnerability details

_validatePositionList(positionIdList) 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].

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

        unchecked {
            pLength = positionIdList.length - offset;
        }
        // note that if pLength == 0 even if a user has existing position(s) the below will fail b/c the fingerprints will mismatch
        // Check that position hash (the fingerprint of option positions) matches the one stored for the '_account'
        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();
    }

    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

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

The main issue in the code is with (((existingHash >> 248) + 1) << 248). The first uint8 represents the number of tokenIds, and overflow is ignored.

Additionally,when XOR calculation, the known formula is X^Y^Y=X.

This means that calculating the hash for positionIdList[1,2,3] and positionIdList[1,2,3,4,4,4,4......] (256 4 ) will result in the same hash.

Proof of Concept

The following code demonstrates that inserting any 256 tokenIds will yield the same hash.

contract CounterTest is Test {

    function updatePositionsHash(
      uint256 existingHash,
      uint256 tokenId,
      bool addFlag
  ) internal pure returns (uint256) {

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

  function test() external {
    uint256 existingHash = 0;
    uint256 u1 = updatePositionsHash(0,1,true);
    uint256 u2 = updatePositionsHash(u1,2,true);
    uint256 u3 = updatePositionsHash(u2,3,true);
    uint256 newU3=u3;
    for(uint256 i;i< 256;i++){
      newU3 = updatePositionsHash(newU3,789,true);
    }
    console.log(newU3 == u3);
  }
}

$ forge test -vvv

Running 1 test for test/Counter.t.sol:CounterTest
[PASS] test() (gas: 105578)
Logs:
  true

Impact

If a seller has a tokenId premium > required collateral, they can maliciously increase positionIdList by adding any 256*N number of tokenIds, passing the validation in _validateSolvency(), and thus illegally opening a position.

Recommended Mitigation

    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

        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
+           require((existingHash >> 248)<255,"invald");
            return
                addFlag
                    ? uint256(updatedHash) + (((existingHash >> 248) + 1) << 248)
                    : uint256(updatedHash) + (((existingHash >> 248) - 1) << 248);
        }
    }

Assessed type

Context

[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)