Inability to claim winnings due to malicious setting of ConditionalToken's outcome slot length

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xaab2011cc37a3989b965518d2a174c1198d2d80ce41c8925140b3bbff5cd941d Severity: high

Description: Description\ Markets can be maliciously misconfigured through conditionalToken.prepareCondition().

Users can create a market through various functions such as createCategoricalMarket(), createMultiCategoricalMarket(), createScalarMarket(), and createMultiScalarMarket(). These functions call the internal createMarket() function which calls which eventually calls prepareCondition().

    function prepareCondition(bytes32 questionId, uint256 outcomeSlotCount) internal returns (bytes32) {
        bytes32 conditionId = conditionalTokens.getConditionId(address(realityProxy), questionId, outcomeSlotCount);

        if (conditionalTokens.getOutcomeSlotCount(conditionId) == 0) {
            conditionalTokens.prepareCondition(address(realityProxy), questionId, outcomeSlotCount);
        }

        return conditionId;
    }

This function checks if the condition has already been prepared by checking if the outcomeSlotCount == 0. If it is 0, it must be prepared by calling conditionalTokens.prepareCondition() with the correct parameters, notably outcomeSlotCount.

However, conditionalTokens.prepareCondition() is external without access control. Anyone can call this function and supply address(realityProxy) and the same questionId with a false value for outcomeSlotCount.

This will result in some market choices not being able to be redeemed. If the attacker sets the outcomeSlotCount to 2 while the market is initialized with many more choices, the higher index choices will not be able to receive payouts.

    function reportPayouts(bytes32 questionId, uint[] calldata payouts) external {
        uint outcomeSlotCount = payouts.length;
        require(outcomeSlotCount > 1, "there should be more than one outcome slot");
        // IMPORTANT, the oracle is enforced to be the sender because it's part of the hash.
        bytes32 conditionId = CTHelpers.getConditionId(msg.sender, questionId, outcomeSlotCount);
        require(payoutNumerators[conditionId].length == outcomeSlotCount, "condition not prepared or found");

The above reportPayouts() function will revert if the caller attempts to report a payout for an array length longer than the length set by the malicious user.

Attachments

1. Proof of Concept (PoC) File

   • User wants to create a prediction market with 8 choices.
   • Malicious user frontruns and sets the outcomeSlotCount = 2
   • The market time ends, choice 7 wins.
   • Market attempts to report payout. This will revert because conditionalTokens.reportPayouts() expects a payouts array of length 2.

2. Revised Code File (Optional)

   • Add a require statement to ensure that if the outcomeSlotCount is already set for the questionId, it is the correct amount. If not, revert. Though this does open up to a griefing bug where a malicious user could cause all market creatiions to revert though it mitigates the fund loss issue.

function prepareCondition(bytes32 questionId, uint256 outcomeSlotCount) internal returns (bytes32) {
        bytes32 conditionId = conditionalTokens.getConditionId(address(realityProxy), questionId, outcomeSlotCount);

        if (conditionalTokens.getOutcomeSlotCount(conditionId) == 0) {
            conditionalTokens.prepareCondition(address(realityProxy), questionId, outcomeSlotCount);
        }
++    else {
            require(outcomeSlotCount == conditionalTokens.getOutcomeSlotCount(conditionId), "outcomeSlotMismatch")
        }

        return conditionId;
    }

[greenlucid]

prepareCondition is called immediately when the market is created, there's no chance for anyone to frontrun anything

[fatherGoose1]

I'm unsure what you mean. This function is external and anyone can call, effectively setting the payoutNumerators[conditionId] to an incorrect length. prepareCondition is called when the market is created, yes, but an attacker supplying the incorrect value beforehand (by frontrunning) will taint the entire market from functioning correctly.

    function prepareCondition(address oracle, bytes32 questionId, uint outcomeSlotCount) external {
        // Limit of 256 because we use a partition array that is a number of 256 bits.
        require(outcomeSlotCount <= 256, "too many outcome slots");
        require(outcomeSlotCount > 1, "there should be more than one outcome slot");
        bytes32 conditionId = CTHelpers.getConditionId(oracle, questionId, outcomeSlotCount);
        require(payoutNumerators[conditionId].length == 0, "condition already prepared");
        payoutNumerators[conditionId] = new uint[](outcomeSlotCount);
        emit ConditionPreparation(conditionId, oracle, questionId, outcomeSlotCount);
    }

[xyzseer]

User wants to create a prediction market with 8 choices. Malicious user frontruns and sets the outcomeSlotCount = 2

here we will have two different conditionIds => two different markets because the outcomeSlotCount is used to generate the conditionId, see https://github.com/gnosis/conditional-tokens-contracts/blob/master/contracts/CTHelpers.sol#L11

[clesaege]

Yeah, as @xyzseer pointed out, the outcomeSlotCount is used in the hash determining the conditionId, so a different outcomeSlotCount will lead to a different conditionId.