Potential Question ID Collisions in Market Creation

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x24a915ee6bc6a35843f696d8d38ca7803e67d7499a774f800e020bff060ea789 Severity: medium

Description:

Details

The Seer Protocol's MarketFactory contract creates prediction markets linked to Reality.eth questions. The askRealityQuestion function is responsible for generating unique question IDs and submitting these questions to Reality.eth.

The current implementation of askRealityQuestion generates a question_id based on a set of parameters that don't include any user-specific or time-specific data. This could potentially lead to question ID collisions in some cases, where two different markets end up referencing the same Reality.eth question.

Code Snippet

function askRealityQuestion(
    string memory encodedQuestion,
    uint256 templateId,
    uint32 openingTime,
    uint256 minBond
) internal returns (bytes32) {
    bytes32 content_hash = keccak256(abi.encodePacked(templateId, openingTime, encodedQuestion));
    bytes32 question_id = keccak256(
        abi.encodePacked(
            content_hash, arbitrator, questionTimeout, minBond, address(realitio), address(this), uint256(0)
        )
    );
    // Rest of the function...
}

Impact

1. In the event of a collision, two different markets could end up referencing the same Reality.eth question.
2. This could lead to market resolutions based on unintended questions, potentially causing confusion and financial losses for participants.

Scenario

1. Two users create markets with identical parameters (same question, templateId, openingTime, and minBond).
2. Due to the deterministic nature of the question_id generation, both markets generate the same question_id.
3. The second market creation reuses the existing Reality.eth question instead of creating a new one.
4. Both markets end up referencing the same Reality.eth question, which is correct behavior but could potentially lead to unexpected interactions between the markets.

Fix

Modify the question_id calculation to include the sender's address and the current block timestamp. This ensures uniqueness even if all other parameters are identical:

function askRealityQuestion(
    string memory encodedQuestion,
    uint256 templateId,
    uint32 openingTime,
    uint256 minBond
) internal returns (bytes32) {
    bytes32 content_hash = keccak256(abi.encodePacked(templateId, openingTime, encodedQuestion));
    bytes32 question_id = keccak256(
        abi.encodePacked(
            content_hash, arbitrator, questionTimeout, minBond, address(realitio), address(this), msg.sender, block.timestamp
        )
    );
    // Rest of the function remains the same
    ...
}

Poc

describe("Question ID Collision", function () {
  it("should generate the same question ID for identical market parameters", async function () {
    // Create the first market
    const tx1 = await marketFactory.createCategoricalMarket(categoricalMarketParams);
    const receipt1 = await tx1.wait();
    const event1 = receipt1.events?.find(e => e.event === "NewMarket");
    const questionId1 = event1?.args?.[4];

    // Create a second market with identical parameters
    const tx2 = await marketFactory.createCategoricalMarket(categoricalMarketParams);
    const receipt2 = await tx2.wait();
    const event2 = receipt2.events?.find(e => e.event === "NewMarket");
    const questionId2 = event2?.args?.[4];

    // Check that both markets have the same question ID
    expect(questionId1).to.equal(questionId2);

    // Verify that both markets point to the same Reality.eth question
    const marketAddress1 = (await marketFactory.allMarkets())[0];
    const marketAddress2 = (await marketFactory.allMarkets())[1];
    const market1 = await ethers.getContractAt("Market", marketAddress1);
    const market2 = await ethers.getContractAt("Market", marketAddress2);

    const questionsIds1 = await market1.questionsIds();
    const questionsIds2 = await market2.questionsIds();

    expect(questionsIds1[0]).to.equal(questionsIds2[0]);
  });
});

This test does the following:

1. Creates two categorical markets with identical parameters.
2. Extracts the question IDs from the emitted events.
3. Compares the question IDs to show they are identical.
4. Retrieves the actual question IDs stored in the Market contracts to verify they indeed point to the same Reality.eth question.

This test demonstrates that creating two markets with identical parameters results in the same question ID being used

[Tobi0x18]

To create second question with the same question_id of the first question is impossible in RealityETH-3.0.

File: contracts\src\interaction\reality\RealityETH-3.0.sol
335:     function _askQuestion(bytes32 question_id, bytes32 content_hash, address arbitrator, uint32 timeout, uint32 opening_ts, uint256 min_bond)  
336:         stateNotCreated(question_id) 

176:     modifier stateNotCreated(bytes32 question_id) {
177:         require(questions[question_id].timeout == 0, "question must not exist");
178:         _;
179:     }
180: 

[clesaege]

@Auditor0x18 yes, but we make sure not to ask the question if it was asked already.

It is possible for 2 markets to reference the same question. This is good as it prevents having multiple bonds and disputes on the same question. We expect this to be used quite often to create conditional markets (like "Who will win the US election?" TRUMP / HARRIS and then two condition markets on "What will be the inflation in the US in 2025?" one conditional on TRUMP the other on HARRIS, but sharing the same question for inflation).