hats-finance / SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7

1 stars 0 forks source link

WETH9 Compatibility Issue in Router Contract #71

Open hats-bug-reporter[bot] opened 2 weeks ago

hats-bug-reporter[bot] commented 2 weeks ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x1a282676a47f90d1f8c903055566bc151f8ce3eb4ccd4845c70b43e14d4ab2b8 Severity: low

Description: Description

The Router contract uses a standard transferFrom method to transfer collateral tokens, including WETH9, without considering the varying implementations of WETH9 across different blockchain networks. Specifically, on chains such as Blast the WETH9 contract does not handle the case where src == msg.sender in the same way as other chains. This discrepancy can lead to failed transactions when users attempt to split positions using WETH9 as collateral on these networks.

If this issue is not addressed, users on affected chains like Blast may be unable to split positions using WETH9 as collateral. This could significantly impair the functionality of the protocol on these networks, potentially leading to:

  1. Failed transactions and wasted gas fees for users
  2. Inconsistent behavior of the protocol across different chains
  3. Reduced adoption and usage of the protocol on affected networks
  4. Potential loss of funds if users are unable to split or manage their positions as intended

Attachments

Proof of Concept (PoC) File

https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/main/contracts/src/Router.sol#L37-L40

function splitPosition(IERC20 collateralToken, Market market, uint256 amount) public {
    if (market.parentCollectionId() == bytes32(0)) {
        // This transferFrom call may fail on chains like Blast, Arbitrum, and Fantom when collateralToken is WETH9
        collateralToken.transferFrom(msg.sender, address(this), amount);
    }
    _splitPosition(collateralToken, market, amount);
}

Recommended Mitigations

  1. Add WETH9-specific handling:

Introduce a check to identify if the collateral token is WETH9 and handle it separately. This can be done by storing the WETH9 address for each supported chain.

address public immutable WETH9_ADDRESS;

constructor(address _weth9Address) {
    WETH9_ADDRESS = _weth9Address;
}

function splitPosition(IERC20 collateralToken, Market market, uint256 amount) public {
    if (market.parentCollectionId() == bytes32(0)) {
        if (address(collateralToken) == WETH9_ADDRESS) {
            // WETH9-specific handling
            IERC20(WETH9_ADDRESS).transferFrom(msg.sender, address(this), amount);
        } else {
            // Standard ERC20 handling
            collateralToken.transferFrom(msg.sender, address(this), amount);
        }
    }
    _splitPosition(collateralToken, market, amount);
}
  1. Implement a safeTransferFrom function:

Create a utility function that handles transfers safely for both WETH9 and standard ERC20 tokens.

function safeTransferFrom(IERC20 token, address from, address to, uint256 amount) internal {
    bool success;
    if (address(token) == WETH9_ADDRESS) {
        // WETH9-specific handling
        (success, ) = address(token).call(abi.encodeWithSelector(token.transferFrom.selector, from, to, amount));
    } else {
        // Standard ERC20 handling
        success = token.transferFrom(from, to, amount);
    }
    require(success, "Token transfer failed");
}

function splitPosition(IERC20 collateralToken, Market market, uint256 amount) public {
    if (market.parentCollectionId() == bytes32(0)) {
        safeTransferFrom(collateralToken, msg.sender, address(this), amount);
    }
    _splitPosition(collateralToken, market, amount);
}

This mitigation provides an approach to handling the WETH9 compatibility issue. Implementing this solution will improve cross-chain compatibility and reduce the risk of failed transactions on networks with non-standard WETH9 implementations.

xyzseer commented 2 weeks ago

As per contest rules, are excluded: - Issues about the collateral token being non standard or malicious. We assume sDAI will be used.

clesaege commented 2 weeks ago

As per contest rules, are excluded: