ERC20 transfer and transferFrom return values not handled

[hats-bug-reporter[bot]]

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

Description: Description\ Some ERC20 tokens might return false instead of reverting in case of transfer failures. EIP20 states that:

Callers MUST handlefalsefromreturns (bool success). Callers MUST NOT assume thatfalseis never returned!

https://eips.ethereum.org/EIPS/eip-20

Router.sol does not check the return value of transfer and transferFrom when transferring collateralToken. This might lead to inconsistent state for the Seer protocol. This will lead to the fallowing scenarios

• The Router will send ERC20 outcome tokens to msg.sender, even though the deposit of collateral failed

    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); <@ in case of failure the execution flow continues and outcome tokens are minted in _splitPosition()

        }

        _splitPosition(collateralToken, market, amount);

    }

• redeemPositions() will succeed even when the actual transfer of tokens has failed

  
      function redeemPositions(IERC20 collateralToken, Market market, uint256[] calldata outcomeIndexes) public {
  ...
              if (finalBalance > initialBalance) {

                collateralToken.transfer(msg.sender, finalBalance - initialBalance);//@audit  return value not checked in collateral token:

            }         }     }

- `mergePositions()` execution will succeed, even though the transfer of the collateral token has failed

**Attachments**

1. **Proof of Concept (PoC) File**
```solidity
    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); // <@ return value not handled

        }

        _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);// <@ return value not handled

        }

    }
...
    function redeemPositions(IERC20 collateralToken, Market market, uint256[] calldata outcomeIndexes) public {
...
            if (finalBalance > initialBalance) {

                collateralToken.transfer(msg.sender, finalBalance - initialBalance); // <@ return value not handled

            }

        }

    }

2. Revised Code File (Optional) Use OpenZeppelin's SafeERC20 lib when interacting with ERC20 tokens https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

[clesaege]

As per competition rules, are excluded:

• Issues about the collateral token being non standard or malicious. We assume sDAI will be used.

[clesaege]

For what we are concerned now, those contracts were made for sDAI as a collateral, but I do understand that the parameter name and comments make it look like it could be any ERC20.

Even if it's technically excluded, it may still be good to follow the standard recommendations such that we can reuse the base router if we were to make some "arbitrary ERC20 token" router in the future.

[clesaege]

Note that if we do give a reward for this one, this would be from in addition of the reward pool not to penalize other hunters.

[clesaege]

Hey, sorry for the initial reporter but this https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/issues/103 was submitted first and was displayed late due to a bug on hat side. I've talked with the team and they confirmed that the #103 was the first submitted.