hats-finance / Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d

0 stars 1 forks source link

Unsafe use of `safeApprove` function in `contracts/nest/SingelTokenBuybackUpgradeable.sol`. #5

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

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

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x780273f04aca1197d7cf3a7d242a41e0664084d3a9352c17a44368d68b4bf710 Severity: medium

Description: Description

In contracts/nest/SingelTokenBuybackUpgradeable.sol::buybackTokenByV2:

 function buybackTokenByV2(
        address inputToken_,
        IRouterV2.route[] calldata inputRouters_,
        uint256 slippage_,
        uint256 deadline_
    ) external virtual override onlyCorrectInputToken(inputToken_) onlyCorrectSlippage(slippage_) returns (uint256 outputAmount) {
        //REDACTED

        IRouterV2 router = IRouterV2(routerV2PathProviderCache.router());
        inputTokenCache.safeApprove(address(router), amountIn);

        //REDACTED

Version 4.9.5 of safeApprove (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.5/contracts/token/ERC20/utils/SafeERC20.sol#L45-L54) is currently in use in the contract.

USDT approve method in ethereum mainnet (it checks that allowance is zero):

function approve(
    address _spender,
    uint _value
    ) public onlyPayloadSize(2 * 32) {
//REDACTED
    require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));
    allowed[msg.sender][_spender] = _value;
//REDACTED
}

Attack Scenario

In the current implementation, if the SingelTokenBuybackUpgradeable contract is left with an extra allowance for router, then users will not be able to call SingelTokenBuybackUpgradeable.sol::buybackTokenByV2 function due to revert of non zero allowance.

Attachments

NA

  1. Proof of Concept (PoC) File

Manual Analysis

  1. Revised Code File (Optional)
0xmahdirostami commented 3 months ago

could you describe a scenario where the allowance will remain?

USDT approve method in ethereum mainnet (it checks that allowance is zero):

is it the same on Blast?

I think the issue is invalid

0xEricTee commented 3 months ago

@0xmahdirostami looks like only USDB stable coin in blast chain. https://blastexplorer.io/tokens