USDT approvals does not check return value violating EIP20 in `LibDirectGovernanceFarmer.depositSingle()`

[0xRizwan]

Title

USDT approvals does not check return value violating EIP20 in LibDirectGovernanceFarmer.depositSingle()

Severity

Medium

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 protcol uses USDT as one of the token in LibDirectGovernanceFarmer.depositSingle(). 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 approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {      @audit // does not return success boolean
        require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

        allowed[msg.sender][_spender] = _value;
        Approval(msg.sender, _spender, _value);
    }

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.

In LibDirectGovernanceFarmer.depositSingle(),

    function depositSingle(
        address token,
        uint256 amount,
        uint256 durationWeeks
    ) internal returns (uint256 stakingShareId) {

       . . . some comment

        //TODO approve token to be transferred to Staking contract
        IERC20(token).approve(data.ubiquity3PoolLP, amount);

       . . . some comment

Here, one of the deposited token is USDT and USDT does not check return value for approve() as discussed above. Therefore, return value must be checked for USDT approvals.

Recommendation

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

    function depositSingle(
        address token,
        uint256 amount,
        uint256 durationWeeks
    ) internal returns (uint256 stakingShareId) {
        DirectGovernanceData storage data = directGovernanceStorage();
        // DAI / USDC / USDT / Ubiquity Dollar
        require(
            isMetaPoolCoin(token),
            "Invalid token: must be DAI, USD Coin, Tether, or Ubiquity Dollar"
        );

      . . . some code .

        //[Ubiquity Dollar, DAI, USDC, USDT]
        uint256[4] memory tokenAmounts = [
            token == address(data.ubiquityDollar) ? amount : 0,
            token == data.token0 ? amount : 0,
            token == data.token1 ? amount : 0,
            token == data.token2 ? amount : 0
        ];

      . . . some code 

        //TODO approve token to be transferred to Staking contract
-        IERC20(token).approve(data.ubiquity3PoolLP, amount);
+        IERC20(token).safeApprove(data.ubiquity3PoolLP, amount);
        IERC20(data.ubiquity3PoolLP).safeIncreaseAllowance(staking, lpAmount);
        stakingShareId = IStaking(staking).deposit(lpAmount, durationWeeks);

        IStakingShare(stakingShare).safeTransferFrom(
            address(this),
            msg.sender,
            stakingShareId,
            1,
            bytes("")
        );

      . . . some code 

cc- @pavlovcik @molecula451

[molecula451]

same, all transfers are safe by globally enabling the imported interface https://github.com/ubiquity/ubiquity-dollar/blob/38c3656539ae19fe9be162f566b36ec62a3e6e41/packages/contracts/src/dollar/libraries/LibDirectGovernanceFarmer.sol#L17

[ubiquibot[bot]]

# Issue was not closed as completed. Skipping.