Transfer returned value is not checked

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0xdfd9fffdafad75f6908e98a6fff668823e47f44341113d86ff2e00f07debfdf9 Severity: medium

Description: Vulnerability details

In V3Migrator.migrate(),


    function migrate(MigrateParams calldata params) external override {
        require(params.percentageToMigrate > 0, 'Percentage too small');
        require(params.percentageToMigrate <= 100, 'Percentage too large');

        // burn v2 liquidity to this address
@>        IUniswapV2Pair(params.pair).transferFrom(msg.sender, params.pair, params.liquidityToMigrate);

    . . . some code

The issue here is with the use of unsafe transferFrom() function and this issue is about non-standard behavior of USDT and such weird tokens.

The ERC20.transferFrom() function return a boolean value indicating success. This parameter needs to be checked for success. Per EIP20. transfer() function is given below,

function transferFrom(address _from, address _to, uint256 _value) public returns (bool success)

Therefore, tokens (like USDT) don't correctly implement the EIP20 standard and their transferFrom() function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. as USDT transfer do not revert if the transfer failed.

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Impact

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommendation

Recommend using safeTransferFrom() function from OpenZeppelin's SafeERC20.sol that handle the return value check as well as non-standard-compliant tokens.

hats-finance / Fenix-Finance-0x83dbe5aa378f3ce160ed084daf85f621289fb92f

Transfer returned value is not checked #12