The parentMarket can be set to an arbitrary address, as it is not verified whether it was deployed by the MarketFactory.

[hats-bug-reporter[bot]]

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

Description:

Summary

Parent Market can be set to arbitrary address, since its not verified that it was deployed by MarketFactory, which can lead to manipulations of data.

Vulnerability Details

We don't have validation if this parentMarket address actually exist in the markets[] in MarketFactory.sol we directly call it to retrieve its parentCollectionId() and conditionId() and then set it as the parentMarket address for the conditional market we are creating.

  function createNewMarketParams(
        CreateMarketParams memory params,
        InternalMarketConfig memory config
    ) internal returns (Market.ConditionalTokensParams memory, Market.RealityParams memory) {
        //@audit-issue arbitrary address call. 
@>>     bytes32 parentCollectionId = params.parentMarket == address(0)
            ? bytes32(0)
            : conditionalTokens.getCollectionId(
                Market(params.parentMarket).parentCollectionId(),
                Market(params.parentMarket).conditionId(),
                1 << params.parentOutcome
            );
.........
        return (
            Market.ConditionalTokensParams({
                conditionId: conditionId,
                questionId: questionId,
                parentCollectionId: parentCollectionId,
                parentOutcome: params.parentOutcome,
@>>             parentMarket: params.parentMarket,
                wrapped1155: wrapped1155,
                data: data
            }),
            Market.RealityParams({
                questionsIds: questionsIds,
                templateId: config.templateId,
                encodedQuestions: config.encodedQuestions
            })
        );
    }

This means that we can basically return arbitrary data for all of the functions of that parent market. We could do a lot of manipulations such as tricking people that the parentCollectionId is actually referring to an actual existing market ( we can do that as we can return arbitrary data that can build legit parentCollectionId)

• In Router.sol we can return any arbitrary data for the wrappedOutcome, as parentWrappedOutcome() calls our bad parentMarket contract.

      if (parentCollectionId != bytes32(0)) {
          // it's splitting from a parent position, so we need to unwrap these tokens first because they will be burnt to mint the child outcome tokens.
  @>>         (IERC20 wrapped1155, bytes memory data) = market.parentWrappedOutcome();

Additionally s a malicious outcomes we have other possible cases. Since parentWrappedOutcome() is called in _splitPosition(), _mergePositions() and _redeemPositions() we could exhaust the gas from the transaction, since an arbitrary call was just made to our parentMarket contract, leading to DoS and excessive gas charges for the user.

This can prevent users from redeeming/splitting/merging their position via the Router.

Recommendation

In createNewMarketParams() if the parentMarket is not address(0) verify that it exists in the address[] public markets; You may have to convert the market array to a mapping, so we avoid looping.

For example if we create a mapping isMarket

    function createNewMarketParams(
        CreateMarketParams memory params,
        InternalMarketConfig memory config
    ) internal returns (Market.ConditionalTokensParams memory, Market.RealityParams memory) {
+        // Require that markets array contains the parent market.
+       if (params.parentMarket != address(0)) {
+            require(isMarket[params.parentMarket], "Parent market not found");
+        }

[xyzseer]

In that case I think the attacker is DoSsing himself by calling a function with fake values, but you need to explain how a third party can be affected by this manipulation

[MrValioBg]

@xyzseer Of course, I will explain. Its not affecting just the attacker.

Attack Scenario:

• Someone has already created a market X ( Will Joe Biden become president? )
• Malicious actor creates a Conditional market Y ( If Joe Biden becomes president who will be the VP? )
  • During the creation the malicious actor can set a parentMarket address which is arbitrary
  • Malicious contract can return the legit conditionId() of the Y market during the creation so the parentCollectionId in createNewMarketParams() is referring to an actual market X, so we can execute the new creation of market Y.
• Users who use the Router.sol for that Conditional Market Y can arbitrary be affected by the call to the malicious parentMarket address, which only happens via market.parentWrappedOutcome();, as it calls a function on the parentMarket contract - code_snippet This can result in data manipulation for the returned wrapped1155 potentially returning one for different possible outcome, or DoS.
• Additionally, as mentioned other consequences of the arbitrary address would be that potentially we could manipulate public data, which can be very dangerous as it can result in tricking users for possible outcomes, if the information for parentMarket from getMarket() in MarketView is displayed publicly.

[clesaege]

Users who use the Router.sol for that Conditional Market Y

Here, as you state yourself, doesn't affect users of legit markets. This malicious Y market would never get verified on curate.

As per contest rules are excluded:

• Issues about being able to create malicious markets (for example, you can create markets without using the MarketFactory with a malicious arbitrator, create child markets to it) as long as this wouldn't result in those being displayed in the interface looking like a normal markets (if a child market of a malicious market is displayed, but points to some market which is not displayed, cannot be interacted with or is labelled as problematic, we consider it fine as it would not get verified on Curate).

[MrValioBg]

@clesaege, glad that you mentioned that. Actually, it is perfectly referring to the case where we have a market that looks perfectly legitimate.

if a child market of a malicious market is displayed, but points to some market which is not displayed, cannot be interacted with or is labelled as problematic, we consider it fine as it would not get verified on Curate

as long as this wouldn't result in those being displayed in the interface looking like a normal markets

This would indeed be displayed as a normal market because the Conditional Market Y would be created via the MarketFactory. Maybe it’s bad wording on my part labeling it as malicious.

The parentMarket, which the attacker would set, would actually refer to a legitimate market X that’s already deployed from the MarketFactory, as the parentMarket can return actual, legitimate data of an existing market that was also created via the MarketFactory.

Someone has already created a market X ( Will Joe Biden become president? )

Only in the case where it’s called during split/redeem/merge we could conditionally manipulate the outcome to have different behavior.

[xyzseer]

Can you provide a practical example of such conditional manipulation?

[MrValioBg]

@xyzseer

Can you provide a practical example of such conditional manipulation?

Yes, of course. I will get back to you with PoC

[clesaege]

@MrValioBg

the Conditional Market Y would be created via the MarketFactory

legitimate market X that’s already deployed from the MarketFactory

Then both markets are created by the factory and there is no issue.

My understanding is that you are reporting exactly what we were expecting people to report when we wrote the exclusion.