ParentCollectionId is not handled for the markets where CollateralToken is DAI and sDAI

[hats-bug-reporter[bot]]

Github username: @cpp-phoenix Twitter username: 0xrochimaru Submission hash (on-chain): 0x082055d8565d5d684090c1fac3fbb14713e1da0ad4431bc71a7c91ffaa6e9f9c Severity: high

Description: Description\ In case the Collateral token is DAI or sDAI the redemption process will go through GnosisRouter.sol and MainnetRouter.sol contracts. Which doesn't handle the case if the market has a parentCollectionId attached to it.

Attack Scenario\ During the market creation in MarketFactory.sol there is no excemption for DAI or sDAI collateral token. A valid market can have a parentCollectionId. But during redemption process then methods will expect both CollateralToken as well as ParentToken.

[cpp-phoenix]

Both GenosisRouter.sol and MainnetRouter.sol is missing the handling of ParentCollectionId. Which is handled gracefully 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);
}

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

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

[cpp-phoenix]

For example, If a Market is on Ethereum mainnet and it has sDAI has collateralToken. It also has a parentCollectionId. Now, If a user executes MainnetRouter.mergeToDai(). The user will receive Parent Wrapped1155 Token. But the last line of mergeToDai() will execute sDAI.redeem(amount, msg.sender, address(this)); too. Which is the unintended behaviour.

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

[xyzseer]

same as #31 #33

[cpp-phoenix]

Clearly in case of mergeToDai() the protocol is losing the money as sDAI.redeem(amount, msg.sender, address(this)); will be called and the user will receive extra DAI. Because MainnetRouter is a common contract and there can be a Market where some user deposited DAI. So the execution will infact succeed and amount will be sent to this user by mistake.

[cpp-phoenix]

To explain it further if Router.mergePositions() is used collateralToken.transfer(msg.sender, amount); will not execute if there is a parentCollectionId.

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

But, If MainnetRouter.mergeToDai() or GnosisRouter.mergeToBase() Both sDAI.redeem(amount, msg.sender, address(this)); and savingsXDaiAdapter.redeemXDAI(amount, msg.sender); will be executed. Which will send extra Dai tokens to the caller.

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

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

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

Let me know if a working POC is required.

[cpp-phoenix]

same as #31 #33

@xyzseer The above issues seems to miss the main vulnerability

[xyzseer]

Clearly in case of mergeToDai() the protocol is losing the money

the protocol doesn't hold any funds so I don't see how the it is losing money, but if you can provide a POC I will take a look

[cpp-phoenix]

The method redeemToBase() check the sDAI balance of the contract uint256 initialBalance = sDAI.balanceOf(address(this));. Which shows it can have a sDAI balance.

[clesaege]

The method redeemToBase() check the sDAI balance of the contract uint256 initialBalance = sDAI.balanceOf(address(this));. Which shows it can have a sDAI balance.

This is done to prevent someone from transferring funds to the contract in order to make a user get more sDAI than expected (only relevant if someone were to build a smart contract using the router). If you found a way to retrieve sDAI stucked in the contract, that's great, but out of scope.

[cpp-phoenix]

@clesaege This method is not working as Intended. There should be a revert condition in splitFromBase(), mergeToBase() and redeemToBase() if there is a parentCollectionId of the market. Otherwise the flow is an unintended behaviour. So this issue should be valid as user is gaining from it and not losing anything.

[clesaege]

No, actually we already got this report internally and we put this exclusion:

• 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!

To prevent those. For a report to be valid, something bad should happen. Here nothing bad happens.