GnosisRouter : unsafe market value passing could be used to steal the funds approved by the user to the router contract

[hats-bug-reporter[bot]]

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

Description: Description

Through the router contract such as GnosisRouter users interact with deposit and split shares and other kind of operations.

For example splitFromBase function.

GnosisRouter.sol#L31-L35

    function splitFromBase(Market market) external payable {
        uint256 shares = savingsXDaiAdapter.depositXDAI{value: msg.value}(address(this));

        _splitPosition(sDAI, market, shares);
    }

This takes the user provided market argument as input.

• The function receives some xDAI (specified by msg.value) from the caller.

• It deposits the received xDAI .

• The deposited xDAI is converted into shares, which represent ownership of the deposit within the protocol.

• These shares are then split or allocated to a specified Market using the _splitPosition function.

As we said, the market could be user given contract address wihch has any custom functionality.

_splitPosition called with this market contract address. The Router contract is approved by other users to spend the collateral tokens. Refer the transferfrom call in splitPosition and other places.

Router.sol#L50-L82

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

Some of the external call it makes is market.parentCollectionId(); to get the parentCollectionId . Nothe the caller will be Router contract in this case.

For above sencaiton, when market is user given contract address, usign the some of custme functon, the approved funds could be stolen insidde the parentCollectionId or in any other external calls by the market contract.

Attack Scenario\

Lets see followng case.

• Alice first approves the GnosisRouter to spend her collateral tokens. This is necessary for the router to be able to manage and split her funds into different markets.
• For example, she might call approve(GnosisRouter, amount) on her collateral token contract, giving the router permission to transfer a certain amount of her tokens.
• Bob deploys a malicious market contract that implements custom functionality.
• Bob call the splitFromBase functon with malicious market contract.
• inside the parentCollectionId(), the market contract could steal the funds since it is approved by the Alice to send by the rotuer. Note the caller would be router inside the market contract.
• note, the parentCollectionId could return valid parentCollectionId wihch is sufficinet to continue the function execution.

2. Revised Code File (Optional)

We see in most of the places, the approved fund is not reset. following suggestions we would like to made.

1. use authorised makret wihch is set by the owner.
2. whenever the approval based transaction completed, reset the approval amount.

[xyzseer]

inside the parentCollectionId(), the market contract could steal the funds since it is approved by the Alice

the market contract is just a value object, there isn't a single place on the codebase giving a Market an approve() to do something

[0xpinky]

hey.. sorry for this mistake.. its a to the rotuer.

inside the parentCollectionId(), the market contract could steal the funds since router is approved by the Alice

[clesaege]

inside the parentCollectionId(), the market contract could steal the funds since router is approved by the Alice

The router is approved, not the market contract.

I think the hunter is confusing calls and delegateCall. The router calls the market contract, it doesn't delegate any call to the market.