The merging function always redeems the funds for the user even if the merging was not for parentCollectionId == 0 situation

[hats-bug-reporter[bot]]

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

Description: Description\ Currently the user can make merging from more deep separate positions into the one position back. But calling mergeToDai() from the MainnetRouter contract or mergeToBase() from the GnosisRouter makes user always redeem his funds even if the parentCollectionId != 0 resulting in a situation where merging functionality does not work as expected.

Attack Scenario\

Currently the user can call mergeToDai() from the MainnetRouter and mergeToBase() from the GnosisRouter smart contract. Merging operation is created for the user to either return separate positions into the one position again or withdraw his collateral if the parentCollectionId == 0 during the merging. However, the code does not work as expected resulting in a following situation:

1. User splits his position into 2 separate positions with different outcomes using currently existing split functionality.

2. Then user decides to merge it back and calls either mergeToDai() or mergeToBase() from one of the routers.

3. However, this time the collateral is fully withdrawn even though it was not merging operation with parentCollectionId == 0.

4. This leads to a situation where collateral is redeemed even if it was not meant to resulting in an unexpected behavior and DoS as collateral is not present in the router contract.

Attachments

At the moment there is mergeToDai() function that redeems sDAI:

MainnetRouter.sol::43-46

   function mergeToDai(Market market, uint256 amount) external {
        _mergePositions(IERC20(address(sDAI)), market, amount);
        sDAI.redeem(amount, msg.sender, address(this));
    }

And mergeToBase() that redeems xDAI:

GnosisRouter.sol::41-46

  function mergeToBase(Market market, uint256 amount) external {
        _mergePositions(sDAI, market, amount);

        sDAI.approve(address(savingsXDaiAdapter), amount);
        savingsXDaiAdapter.redeemXDAI(amount, msg.sender);
    }

Now let’s say the user decides to merge his position back from two separate positions and call one of these functions. Inside of _mergePositions() function, we have the following implementation for this case:

Router.sol::123-134

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

   if (parentCollectionId != bytes32(0)) {
            // it's merging from a parent position, so we need to wrap these tokens and send them back to the user.
            uint256 tokenId = conditionalTokens.getPositionId(address(collateralToken), parentCollectionId);

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

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

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

So if the parentCollectionId != 0, we burn the positions and get our tokenId associated with the parentCollectionId back. However, the funds will be redeemed instead resulting in a transaction revert even though the collateral was not sent from the conditionalTokens.

Recommendation Check if the merging was for parentCollectionId == 0 and redeem the funds only in this case. This can be implemented the same way as it’s done in the Router by checking the parentCollectionId:

Router.sol::90-97

    function mergePositions(IERC20 collateralToken, Market market, uint256 amount) public {
        _mergePositions(collateralToken, market, amount);

        if (market.parentCollectionId() == bytes32(0)) {
            // send collateral tokens back to the user.
            collateralToken.transfer(msg.sender, amount);
        }
    }