Split operation may unexpectedly revert or take excessive user’s funds for every split after the first one

[hats-bug-reporter[bot]]

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

Description: Description\ The current implementation allows users to use split functionality to split their one position into two separate positions. However, if the split happens after we’ve already split the first parent position, the funds will still be taken from the user resulting in an unexpected revert (if the user has no more funds to deposit) making the split inaccessible for this particular user or in an excessive taking of funds as every split will transfer funds from the user.

Attack Scenario\

Let’s consider the following scenario:

1. The user decides to make a split from the parent position and deposits collateral into either sDAI (in the case of the MainnetRouter) or in a savingsXDaiAdapter (in the case of the GnosisRouter) contracts.
2. Everything goes normal for this split but then the user makes a decision to narrow down his position even more and calls one of the split functions again.
3. At this time, he will face either revert (if he has no funds at the moment) or the contract will take more funds which is an unexpected behavior as well. Moreover, in the case of the splitFromBase() function in the GnosisRouter, the user is not able to make a split at all if he hasn’t deposited any more funds.

Attachments To initiate a split operation, the user has to either call splitFromBase() in the GnosisRouter or splitFromDai() in the MainnetRouter:

MainnetRouter.sol::31-37

function splitFromDai(Market market, uint256 amount) external {
        DAI.transferFrom(msg.sender, address(this), amount);
        DAI.approve(address(sDAI), amount);
        uint256 shares = sDAI.deposit(amount, address(this));

        _splitPosition(IERC20(address(sDAI)), market, shares);
    }

GnosisRouter.sol::28-35

/// @notice Splits a position using xDAI and sends the ERC20 outcome tokens back to the user.
    /// @dev The ERC20 associated to each outcome must be previously created on the wrapped1155Factory.
    /// @param market The Market to split.
    function splitFromBase(Market market) external payable {
        uint256 shares = savingsXDaiAdapter.depositXDAI{value: msg.value}(address(this));

        _splitPosition(sDAI, market, shares);
    }

The problem is that the contract currently take funds from the users every time not making sure that the parentCollectionId for this split is equal to 0 (meaning it’s the first slip). Therefore, this situation (when `parentCollectionId != 0) is incorrectly handled:

Router.sol::56-64

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

As a result, the users will not be able to make more splits other than one (the first split) without depositing additional funds facing either tx revert or transferring more funds than expected into the protocol (effectively losing them).

Recommendation Take the funds from the user for splitting only in the case when parentCollectionId == 0.

[xyzseer]

splitFromDai / splitFromBase are expected to be called for a market with parentCollectionId == 0 (otherwise you are spliting a child market, and child markets are not splitted using sDAI but using an outcome token of the parent market)