hats-finance / SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7

1 stars 0 forks source link

`CollateralToken` as Rebasing tokens is not supported in `splitPosition()` and `mergePositions()` functions #86

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

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

Github username: @saidqayoumsadat Twitter username: S2AQ143 Submission hash (on-chain): 0xe2f5462516bcfb5a93fbba8bd78073356186e096f26abc9829919bb2d2d52232 Severity: medium

Description:

Description

Please note, sponsor has confirmed that collateralToken can be ANY ERC20 token, their confirmation can be checked here and here so this issue about Rebasing tokens like stETH is being reported considering these above issue comments by sponsors and based on that issue. This is not a duplicate of #64, as fees on transfer are different from rebasing tokens.

Certain ERC20 tokens like Rebasing tokens adjust the user's balance automatically either positively or negatively over time. The problem stems from the fact that Router.sol does not account for this dynamic change in balance. Consequently, these changes can result in token mismanagement, leading to potential understatements or overstatements of user balances. This can either lock funds within the system or improperly distribute them.

Affected Functions:

splitPosition() splits the position and sends the collateral token to router contract.

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

and mergePositions() merges the position and sends back the collateral token to user.

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

The current implementation does not account for rebasing tokens like stETH, which dynamically change in balance. When these tokens are used:

Examples of Rebasing Tokens:

Impact

Recommendation

clesaege commented 2 months ago

Please note, sponsor has confirmed that collateralToken can be ANY ERC20 token, their confirmation can be checked https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/issues/16#issuecomment-2374745654 and https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/issues/37#issuecomment-2374905252

This is very disingenuous. In the first link, I state:

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.

And second link is linking to someone who is not working on Seer.

As per contest rules, are excluded: