Lack of input validation market when in router contract

[hats-bug-reporter[bot]]

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

Description: Description\ Describe the context and the effect of the vulnerability.

Attack Scenario\

https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/Router.sol#L143

   function redeemPositions(IERC20 collateralToken, Market market, uint256[] calldata outcomeIndexes) public {
@        bytes32 parentCollectionId = market.parentCollectionId();
        uint256 initialBalance;

        if (parentCollectionId == bytes32(0)) {
            initialBalance = collateralToken.balanceOf(address(this));
        }

        _redeemPositions(collateralToken, market, outcomeIndexes);

        if (parentCollectionId == bytes32(0)) {
            uint256 finalBalance = collateralToken.balanceOf(address(this));

            if (finalBalance > initialBalance) {
                collateralToken.transfer(msg.sender, finalBalance - initialBalance);
            }
        }
    }

the input market is not validated.

the user can pass in a malicious market address to redeem position and cause loss of fund.

https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/Router.sol#L167

 function _redeemPositions(IERC20 collateralToken, Market market, uint256[] calldata outcomeIndexes) internal {
 @       bytes32 parentCollectionId = market.parentCollectionId();
 @       bytes32 conditionId = market.conditionId();
        uint256 tokenId = 0;

        uint256[] memory indexSets = new uint256[](outcomeIndexes.length);

        for (uint256 j = 0; j < outcomeIndexes.length; j++) {
            indexSets[j] = 1 << outcomeIndexes[j];
            tokenId = getTokenId(collateralToken, parentCollectionId, conditionId, indexSets[j]);

            // first we need to unwrap the outcome tokens that will be redeemed.
 @           (IERC20 wrapped1155, bytes memory data) = market.wrappedOutcome(outcomeIndexes[j]);
            uint256 amount = wrapped1155.balanceOf(msg.sender);

            wrapped1155.transferFrom(msg.sender, address(this), amount);

            wrapped1155Factory.unwrap(address(conditionalTokens), tokenId, amount, address(this), data);
        }

        uint256 initialBalance = 0;

        if (parentCollectionId != bytes32(0)) {
            // if we are redeeming from a child market, the user may already have parent tokens so we need to track the balance change.
            tokenId = conditionalTokens.getPositionId(address(collateralToken), parentCollectionId);
            initialBalance = conditionalTokens.balanceOf(address(this), tokenId);
        }

        conditionalTokens.redeemPositions(address(collateralToken), parentCollectionId, conditionId, indexSets);

        if (parentCollectionId != bytes32(0)) {
            // if we are redeeming from a child market, redeemPositions() returned outcome tokens of the parent market. We need to wrap and send them to the user.
            uint256 finalBalance = conditionalTokens.balanceOf(address(this), tokenId);

            if (finalBalance > initialBalance) {
                // wrap to erc20.
@               (IERC20 parentWrapped1155, bytes memory parentData) = market.parentWrappedOutcome();

                conditionalTokens.safeTransferFrom(
                    address(this), address(wrapped1155Factory), tokenId, finalBalance - initialBalance, parentData
                );

                // transfer the ERC20 back to the user.
                parentWrapped1155.transfer(msg.sender, finalBalance - initialBalance);
            }
        }
    }

all external query to market contract is labeled with @

a malicious market contract can be

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract MaliciousMarket {

    // Unique identifier for the parent collection
    function parentCollectionId() public view returns (bytes32) {
        // Implementation needed
        return bytes32(0); // Placeholder
    }

    // Unique identifier for the condition
    function conditionId() public view returns (bytes32) {
        // Implementation needed
        return bytes32(0); // Placeholder
    }

    // Returns the wrapped outcome
    function wrappedOutcome() public view returns (address token, bytes memory data) {
        // Implementation needed
        return (address(0), ""); // Placeholder
    }

    // Returns the parent wrapped outcome
    function parentWrappedOutcome() public view returns (address token, bytes memory data) {
        // Implementation needed
        return (address(0), ""); // Placeholder
    }

    // Additional logic can be added here
}

for example, the user can make a market with no parentCollectionId return a parentCollectionId that comes from another market, etc..

basically the parameter market needs to be validated in every function in router contract (split, merge, and redeem)

the fix is in the MarketFactory as well.

  function createMarket(
        CreateMarketParams memory params,
        string memory marketName,
        InternalMarketConfig memory config
    ) internal returns (address) {
        (Market.ConditionalTokensParams memory conditionalTokensParams, Market.RealityParams memory realityParams) =
            createNewMarketParams(params, config);

        Market instance = Market(market.clone());

        instance.initialize(
            marketName,
            params.outcomes,
            params.lowerBound,
            params.upperBound,
            conditionalTokensParams,
            realityParams,
            realityProxy
        );

        emit NewMarket(
            address(instance),
            marketName,
            params.parentMarket,
            conditionalTokensParams.conditionId,
            conditionalTokensParams.questionId,
            realityParams.questionsIds
        );

        markets.push(address(instance));

        // added code here +++++
        createdMarket[address(instance)] = true; 

        return address(instance);
    }

and then in the router contract, check if the market contract comes from the market factory

if(marketFactory.createdMarket[market] == false) {
 revert ("Invalid market");
}

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[greenlucid]

the user can pass in a malicious market address to redeem position and cause loss of fund.

That's safety, not security. That can be fixed at the UI level to prevent users from being exposed to malicious markets. Also:

Out of Scope:

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

[ctf-sec]

For example in Split position

@greenlucid

 // wrap & transfer the minted outcome tokens.
        for (uint256 j = 0; j < partition.length; j++) {
            uint256 tokenId = getTokenId(collateralToken, parentCollectionId, conditionId, partition[j]);

            (IERC20 wrapped1155, bytes memory data) = market.wrappedOutcome(j);

            // wrap to erc20.
            conditionalTokens.safeTransferFrom(address(this), address(wrapped1155Factory), tokenId, amount, data);

            // transfer the ERC20 back to the user.
            wrapped1155.transfer(msg.sender, amount);
        }

this is the code in split Position,

the problem is that in this case,

it is clear that

It is expect that user should wrap conditional token A to get wrapped1155 A.

But the user can wrap conditional token A and get wrapped1155 B

because user control the returned wrapped1155 and return data.

 (IERC20 wrapped1155, bytes memory data) = market.wrappedOutcome(j);

then user can unwrap wrapped1155 B and redeem the position from token B, it is not about create malicious market to display on UI, it is about mint wrapped1155 B maliciously and redeem them later.

[clesaege]

The router is just there to do a lot of stuff at once instead of asking the user to sign a ton of TX. As pointed out by Green, it's possible to create malicious markets (by creating those contracts directly), but those won't be picked up by the frontend.

As per contest rules, are excluded:

• Issues where the user is harming itself by interacting improperly with the contracts. It's possible for the users to call functions with improper parameters, incompatible tokens, etc but we assume that functions are called properly similarly to how they are called in the frontend. It's the job of smart contracts to protect against malicious behaviours, but it's the job of the frontend to protect against stupid ones!

[clesaege]

If you think your issue is correct, please explain how someone could call the router to harm others.