Lack of access control for `collateralToken` in `splitPosition`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x8943e675d42c9026a1d7cf189ec14545b4cd62157368fb9aab1282a7cec984ac Severity: medium

Description: Description\ The Router.sol contract is inherited and used by other router contracts. However, when Router.sol is inherited by the Gnosis and Mainnet routers, the public splitPosition function is still exposed, and there is no access control in place for the use of sDAI in this function. As a result, an attacker can perform the splitPosition operation using any token of their choice that is valued lower than sDAI.

Proof of Concept (PoC)

Public splitPosition function in Router.sol;

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

And the operations in the internal function goes like this;

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

        uint256[] memory partition = getPartition(conditionalTokens.getOutcomeSlotCount(conditionId));

        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();

            uint256 tokenId = conditionalTokens.getPositionId(address(collateralToken), parentCollectionId);

            wrapped1155.transferFrom(msg.sender, address(this), amount);
            wrapped1155Factory.unwrap(address(conditionalTokens), tokenId, amount, address(this), data);
        } else {
            collateralToken.approve(address(conditionalTokens), amount);
        }

        conditionalTokens.splitPosition(address(collateralToken), parentCollectionId, conditionId, partition, amount);

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

There is no place to revert; even if there were specific functions for sDAI, the attacker could deploy the same contract themselves and trick it.

Recommendation

The Mainnet and Gnosis routers already have customized implementations, but if you want to continue supporting the use of a common public function, you can enforce the sDAI addresses for both networks by implementing a chainid check in the public splitPosition function.

[greenlucid]

Those shares split using a lower valued sDAI won't be redeemable for sDAI later, check conditional tokens implementation

[clesaege]

I couldn't see any issue explained in the report. We could in the future support other ERC20 tokens.