Stable Pear Shrimp - Using wrong format of `questionId` for `NegRiskCtfAdapter` leads to loan operations on resolved multi-outcome markets

[sherlock-admin2]

Stable Pear Shrimp

High

Using wrong format of questionId for NegRiskCtfAdapter leads to loan operations on resolved multi-outcome markets

Summary

PredictDotLoan contract aims at preventing operations with loans as soon as underlying binary questions or multi-outcome markets become resolved. Unfortunately the determination of whether the multi-outcome markets are resolved is implemented incorrectly.

The problem is that though the format of questionIds employed in UmaCtfAdapter and NegRiskAdapter are different, they are treated as the same in PredictDotLoan; as a result of this misinterpretation, the request _isNegRiskMarketDetermined(bytes32 questionId) will always return false. This will lead to treating multi-outcome markets as unresolved, and thus to a guaranteed loss of funds: e.g. giving a loan to the borrow proposal for a position which is guaranteed to resolve to 0.

Root Cause

As outlined in the documentation for Polymarket Multi-Outcome Markets:

The NegRiskOperator and NegRiskAdapter are designed to be used with the UmaCtfAdapter, or any oracle with the same interface. A dedicated UmaCtfAdapter will need to be deployed with the UmaCtfAdapter's ctf set to the address of the NegRiskAdapter, and the NegRiskOperator's oracle set to the address of the UmaCtfAdapter.

In order to prepare a question for a market using the NegRiskOperator, the question must be initialized on the UmaCtfAdapter first. Then, the question may be prepared on the NegRiskOperator where the _requestId parameter is the questionID returned by the UmaCtfAdapter.

As can be seen, questionId as employed in UmaCtfAdapter becomes _requestId in NegRiskOperator, which generates its own questionId, in another format. Concretely:

• UmaCtfAdapter's questionId is generated in UmaCtfAdapter::initialize as follows:

  bytes memory data = AncillaryDataLib._appendAncillaryData(msg.sender, ancillaryData);
  if (ancillaryData.length == 0 || data.length > MAX_ANCILLARY_DATA) revert InvalidAncillaryData();
  
  questionID = keccak256(data);

  Thus, this questionId is obtained by keccak256 of initialization data.

• NegRiskAdapter's questionId is generated via NegRiskOperator::prepareQuestion:

  function prepareQuestion(bytes32 _marketId, bytes calldata _data, bytes32 _requestId)

  which then routes to MarketDataManager::_prepareQuestion:

  function _prepareQuestion(bytes32 _marketId) internal returns (bytes32 questionId, uint256 index) {
      MarketData md = marketData[_marketId];
      address oracle = marketData[_marketId].oracle();
  
      if (oracle == address(0)) revert MarketNotPrepared();
      if (oracle != msg.sender) revert OnlyOracle();
  
      index = md.questionCount();
      questionId = NegRiskIdLib.getQuestionId(_marketId, uint8(index));
      marketData[_marketId] = md.incrementQuestionCount();
  }

  As can be seen, the latter questionId is obtained by merging marketId (248 bits) and index (8 bits).

Despite this discrepancy in formats, the questionId from UmaCtfAdapter is employed in PredictDotLoan for requesting the state of the market from NegRiskAdapter in _assertQuestionPriceUnavailable:

function _assertQuestionPriceUnavailable(QuestionType questionType, bytes32 questionId) private view {
    if (questionType == QuestionType.Binary) {
        _assertBinaryOutcomeQuestionPriceUnavailable(UMA_CTF_ADAPTER, questionId);
    } else {
        if (_isNegRiskMarketDetermined(questionId)) {
            revert MarketResolved();
        }
        _assertBinaryOutcomeQuestionPriceUnavailable(NEG_RISK_UMA_CTF_ADAPTER, questionId);
    }
}

function _isNegRiskMarketDetermined(bytes32 questionId) private view returns (bool isDetermined) {
@>>    isDetermined = NEG_RISK_ADAPTER.getDetermined(NegRiskIdLib.getMarketId(questionId));
}

NegRiskAdapter's getDetermined is implemented as follows:

function getDetermined(bytes32 _marketId) external view returns (bool) {
    return marketData[_marketId].determined();
}

function determined(MarketData _data) internal pure returns (bool) {
    return MarketData.unwrap(_data)[1] == 0x00 ? false : true;
}

As NegRiskIdLib.getMarketId simply masks the last 8 bits away from questionId in the wrong format, and the above code simply reads the data from a mapping, combined it means that getDetermined will always return false as it will read data from an uninitialized mapping entry.

Impact

Guaranteed loss of funds: when a multi-outcome market gets resolved (e.g. we know that candidate A won elections), then all other positions (for candidates B, C, D) automatically become worthless. But if PredictDotLoan still treats the multi-outcome market as unresolved, this allows a multitude of exploits: e.g. grabbing an open loan proposal, and providing as collateral tokens for candidate B; or providing a loan for a still open borrow proposal for candidate A, and potentially seizing much more funds than the provided loan amount.

Mitigation

Apply the necessary missing step of indirection:

• Read the public questionIds mapping from NegRiskOperator, using UmaCtfAdapter's questionId as _requestId:

  mapping(bytes32 _requestId => bytes32) public questionIds;

• Apply this value to request the market state in function _isNegRiskMarketDetermined.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/PredictDotFun/predict-dot-loan/pull/43