USDT approvals does not check return value violating EIP20

[hats-bug-reporter[bot]]

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

Description: Vulnerability Detail

The ERC20.approve() function return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

Per EIP20, approve() function should be as below,

function approve(address _spender, uint256 _value) public returns (bool success)

It means, for ERC20 approvals the return value must be checked to be incompliance with EIP20 tokens.

The issue here is that tokens like USDT don't correctly implement the EIP20 standard and their approve() function return void instead of a success boolean. This can be checked from below USDT approve() function.

    function _claimFees() internal returns (uint256 claimed0, uint256 claimed1) {
        address _token = address(TOKEN);
        (claimed0, claimed1) = IFeesVault(feeVault).claimFees();

        if (claimed0 > 0 || claimed1 > 0) {
            uint256 _fees0 = claimed0;
            uint256 _fees1 = claimed1;

            address _token0 = IPairIntegrationInfo(_token).token0();
            address _token1 = IPairIntegrationInfo(_token).token1();
            if (_fees0 > 0) {
@>                IERC20(_token0).approve(internal_bribe, 0);
                IERC20(_token0).approve(internal_bribe, _fees0);
                IBribe(internal_bribe).notifyRewardAmount(_token0, _fees0);
            }

            if (_fees1 > 0) {
@>                IERC20(_token1).approve(internal_bribe, 0);
                IERC20(_token1).approve(internal_bribe, _fees1);
                IBribe(internal_bribe).notifyRewardAmount(_token1, _fees1);
            }
            emit ClaimFees(msg.sender, claimed0, claimed1);
        }
    }

It is confirmed that, USDT token approval does not check return value for approvals and this is violating the EIP20. Therefore, calling USDT approve function with the correct EIP20 function signatures will always revert.

Tokens that don't actually perform the approval and return false are still counted as a correct approval 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.

Therefore, In the protocol, all functions using approve() must be check the return value.

Recommendation

Use safeApprove OR forceApprove() from openZeppelin’s SafeERC20. This checks the return value and makes it compliant with EIP20.

[0xRizwan]

In GaugeUpgradeable.sol

[BohdanHrytsak]

Thank you for the submission.

The submission mentions two problems:

1. Some tokens incorrectly implement the ERC-20 standard, which leads to unexpected behaviour in the returned parameters of approve methods
2. There is no validation of the result of the approve call

This problem is not valid for the following reasons:

1. The finding is inherited from Thena & Chronos, which leads to the fact that it is OOS, since this problem is not caused by our changes or new functionality, but comes from inherited code
2. Although it is mentioned that certain token implementations do not correctly implement the ERC20 standard and these are popular tokens that should most likely be supported by the protocol, this is true for implementations on the Ethereum network, in fact the declared network on which the protocol will be deployed is Blast