The attacker can mint legit outcome tokens for free

[hats-bug-reporter[bot]]

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

Description: The attacker can mint legitimate outcome tokens by creating a custom market contract. This attack is directly exploitable.

Attack Scenario

Market A: (which created by factory and its legit)

• parentCollectionId = bytes32(0)
• conditionId = X

The attacker calls the splitPosition() function with custom market contract.

function splitPosition(IERC20 collateralToken, Market market, uint256 amount) public {
        if (market.parentCollectionId() == bytes32(0)) {
            // transfer the collateral tokens to the Router.
            collateralToken.transferFrom(msg.sender, address(this), amount);
        }
        _splitPosition(collateralToken, market, amount);
    }

First, the attacker's custom market contract returns a non-zero parentCollectionId to bypass the collateral transfer. Then, the splitPosition() function calls _splitPosition(). Inside this function, as shown below, market.parentCollectionId() is called again:

function _splitPosition(IERC20 collateralToken, Market market, uint256 amount) internal {
        bytes32 parentCollectionId = market.parentCollectionId();
        bytes32 conditionId = market.conditionId();

This time, the attacker’s custom market contract returns bytes32(0), then in this line:

bytes32 conditionId = market.conditionId();

the custom market contract returns the conditionId of Market A.

Now, the _splitPosition() function assumes that the collateral was paid by the caller, when it was not, and proceeds to mint the outcome tokens of Market A, which are legitimate.

Recommendation

this could be one of the ways to fix this issue:

function splitPosition(IERC20 collateralToken, Market market, uint256 amount) public {
        bytes32 id = market.parentCollectionId();
        if (id == bytes32(0)) {
            // transfer the collateral tokens to the Router.
            collateralToken.transferFrom(msg.sender, address(this), amount);
        }
        _splitPosition(collateralToken, market, amount, id);
    }

function _splitPosition(IERC20 collateralToken, Market market, uint256 amount, bytes32 id) internal {
        bytes32 parentCollectionId = id);
        bytes32 conditionId = market.conditionId();

[xyzseer]

the actual minting happens on ConditionalTokens.sol, and you need to provide the tokens otherwise it will revert here https://github.com/gnosis/conditional-tokens-contracts/blob/master/contracts/ConditionalTokens.sol#L135

[0xdanial]

the actual minting happens on ConditionalTokens.sol, and you need to provide the tokens otherwise it will revert here https://github.com/gnosis/conditional-tokens-contracts/blob/master/contracts/ConditionalTokens.sol#L135

@xyzseer Thanks for your comment. this reverts when there is no collateral token in Router contract, otherwise attacker can drain Router contract funds.

[dontonka]

My 2 cents.

This looks like a legit finding in case the router holds collateral token (is it the case or after every transaction it should be empty?) as those would be used in the line mention by @xyzseer and that would be all good, no reverts.

Nevertheless, a coded PoC should be provided to confirm the validity of this finding. The attacker would also need to bypass fully ConditionalTokens::splitPosition and also here conditionalTokens.safeTransferFrom, so not out of the wood id say, but a good start.

        // 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);
        }

[xyzseer]

This looks like a legit finding in case the router holds collateral token (is it the case or after every transaction it should be empty?) as those would be used in the line mention by @xyzseer and that would be all good, no reverts.

it should be empty after every transaction, the router is not expected to hold any funds

[clesaege]

The router is just there to do a bunch of calls at once to have a better UX (otherwise users would need to sign a bunch of txs). It doesn't hold funds.