hats-finance / SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7

1 stars 0 forks source link

`collateralToken` will not work if it's using USDT stablecoin #116

Open hats-bug-reporter[bot] opened 1 month ago

hats-bug-reporter[bot] commented 1 month ago

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

Description: Description\

In Mainnet, a non-standard ERC20 stable tokens, such as USDT, require approve 0 first, otherwise they will revert.

In Router, the splitPosition function exist to transfers collateral to the Router, splits the position and sends the ERC20 outcome tokens back to the user. But here, when the collateralToken is USDT stablecoin, there will be issue.

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

Even though it's said it will be using DAI, in a document it's said currently it's support sDAI, but can add more underlying in the future, and USDT is a possible stablecoin one

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

Recommendation

Consider to set the allowance to zero before increasing the allowance and use safeApprove/safeIncreaseAllowance.

0xbitsurfer commented 1 month ago

I noticed that someone submit the same issue #4, to join into discussion about USDT approval issue, I initially read on SeeR docs that Currently, the front only supports sDAI but we can add more underlying in the future or even change sDAI to something else. and natspec description about Collateral Token, which made me submit this USDT issue as a LOW issue. For other audit contest, this USDT approval issue is deemed as medium, but looking how the docs explain, I'd consider this as Low.

The #16 issue is related with this Collateral Token, which lays on assumption the collateral token might be a different asset. If the #16 is validated as LOW, then this USDT approval issue should also be considered as valid with the same initial assumption collateral token can be other token than DAI.


SeeR-PM-1727276214346.json

This is a resubmit issue from 0xdff7a584b263adafa1fde811f35dc87564f5cbe55a5e78d3ea8f98df2a8766ac. After I submit on Hats UI, the issue is not showing on github and I contact Hats team about this situation. Initially I was asked to wait this issue will be resolved and showed on Github, but I just got a reply from Hats team (Ofir) that I should resubmit again. (I expect Hats team can able to decrypt the hash/submission encryption to validate if the submission indeed the same as this resubmit issue, to validate the authenticity of it)

greenlucid commented 1 month ago

This is an issue on USDT deviating from ERC20. Don't invoke even change sDAI to someone else to suggest a non compliant token.

The #16 issue is related to an assumption of a different token, but still ERC20 compliant. If #16 is validated, is just because it's referring to an ERC20 compliant token, and it drives the contract to respect the standard better. USDT not being ERC20 compliant breaks validity.

clesaege commented 1 month ago

First this is out of scope, as are excluded:

Second that's a bug in the USDT token which is not following the standard.

Third, I'm the one who removed the link to the fault implementation in the ERC20 standard requiring an approve of 0.