trauki - approve() call with incorrect function signature will make any SoftVault deployed with USDT as the underlying token unusable

[sherlock-admin2]

trauki

medium

approve() call with incorrect function signature will make any SoftVault deployed with USDT as the underlying token unusable

RohanNero

medium

approve() call with incorrect function signature will make any SoftVault deployed with USDT as the underlying token unusable

Summary

Open Zeppelin's safeTransfer() is used throughout the blueberry contracts, this ensures that calls to contracts without return values don't fail, however, the EnsureApprove contract uses a normal IERC20 interface and a normal approve() function call. Since USDT doesn't return a boolean as expected by the interface, this would leave the contract unusable.

Vulnerability Detail

If a SoftVault is deployed using USDT as the uToken, the contract's main functions won't work as intended since the ensureApprove function call will fail.

USDT approve signature: function approve(address spender, uint value) public;

OpenZeppelin ERC20 approve signature: function approve(address spender, uint256 value) external returns (bool);

Impact

This will cause loss of funds because the cost to deploy the contract will essentially have been wasted. The developers explicitly stated that they intend to use USDT as an underlying token in the SoftVault's NatSpec:

Code Snippet

https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/utils/EnsureApprove.sol#L27

Tool used

Manual Review

Recommendation

Add support for USDT by importing another interface with ERC20 functions that don't return values.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

0xyPhilic commented:

invalid because the IERC20 interface would fit also USDT as it has an approve function - no function signature is used anywhere

[Gornutz]

Item discussed in the previous competition full context is given here - https://github.com/sherlock-audit/2023-04-blueberry-judging/issues/15#issuecomment-1542727941

[Shogoki]

Item discussed in the previous competition full context is given here - sherlock-audit/2023-04-blueberry-judging#15 (comment)

Actually i do not understand why the issue was closed in the last contest with the comment:

USDT only can call approve function when the allowance is zero or to set the allowance to zero first.

This is not related to the stated issue, that the approval will revert because the generic IERC20 interface is used, which is expecting a bool return data.

[nevillehuang]

Escalate:

Referring to @Gornutz comment, this was marked as No-Fix and Non-Reward in previous contest, and should be invalid according to sherlock's rules here:

In an update contest, issues from the previous contest with wont fix labels are not considered valid.

In addition, EnsureApprove.sol uses _ensureApprove() directly to approve tokens (for e.g. USDT) for contracts inheriting it, so the issue above of needing an interface is not a problem.

[sherlock-admin2]

Escalate:

Referring to @Gornutz comment, this was marked as No-Fix and Non-Reward in previous contest, and should be invalid according to sherlock's rules here:

In an update contest, issues from the previous contest with wont fix labels are not considered valid.

In addition, EnsureApprove.sol uses _ensureApprove() directly to approve tokens (for e.g. USDT) for contracts inheriting it, so the issue above of needing an interface is not a problem.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Shogoki]

Escalate:

Referring to @Gornutz comment, this was marked as No-Fix and Non-Reward in previous contest, and should be invalid according to sherlock's rules here:

In an update contest, issues from the previous contest with wont fix labels are not considered valid.

Hmm, yep that is kind of an interesting case. I agree, that it is probably fair in terms of rewards to consider this invalid. However as stated in my comment above, i do not understand the reasoning for closing this in the initial contest.

[securitygrid]

The function signature only needs the function name and parameters, and does not need a return value.

[Shogoki]

The function signature only needs the function name and parameters, and does not need a return value.

While this is true for the call, it is still a problem as the transaction will revert because Solidity checks for the expected return size and compares it to the actual returned data size.

[nevillehuang]

The function signature only needs the function name and parameters, and does not need a return value.

While this is true for the call, it is still a problem as the transaction will revert because Solidity checks for the expected return size and compares it to the actual returned data size.

As @securitygrid said, there will be no reverts, given allowance is approved to zero first. The interface only requires the function selector to match and call the function in the USDT contract check this link here

When a contract is called, the EVM (Ethereum Virtual Machine) reads the first four bytes of the provided data to determine the function selector. The EVM uses this selector to match it with the correct function within the contract. If a match is found, the function is executed. If no match is found, the function call fails.

For example, function signature of approve() here would be bytes4(keccak256("approve(address,uint)"));

Unless there are two approve functions with different number of arguments, this submission seems like only a valid QA/low finding since there is only 1 approve function exposed in USDT contract

[securitygrid]

@nevillehuang This problem should be considered from the compiled code: The compiled code of IERC20(token).approve(spender, 0) is similar to the following:

(bool ret, bytes data) = token.call(abi.encodeWithSignature("approve(address,uint256)", spender, 0);
if (ret) {
     if (data.length != 1) // since usdt.approve has no return value, so data.length = 0
     {
            revert;
     }
     return abi.decode(data, (bool));
} else {
     revert;
}

[Shogoki]

@nevillehuang This problem should be considered from the compiled code: The compiled code of IERC20(token).approve(spender, 0) is similar to the following:

(bool ret, bytes data) = token.call(abi.encodeWithSignature("approve(address,uint256)", spender, 0);
if (ret) {
     if (data.length != 1) // since usdt.approve has no return vault, so data.length = 0
     {
            revert;
     }
     return abi.decode(data, (bool));
} else {
     revert;
}

That is correct.

To demonstrate the issue we can use the following litle PoC.

1. Creating a Contract EnsureApproveTest.sol

// SPDX-License-Identifier: MIT

pragma solidity 0.8.16;

import {EnsureApprove} from "./utils/EnsureApprove.sol" ;

contract EnsureApproveTest is EnsureApprove {

    function approveMe(address token, uint256 amount) external {
        _ensureApprove(token, msg.sender, amount);
    }
}

2. Creating a Test ensureApprove.ts

import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
import { ethers, upgrades } from "hardhat";
import chai, { expect } from "chai";
import { EnsureApproveTest, IERC20 } from "../typechain-types";
import { roughlyNear } from "./assertions/roughlyNear";
import { near } from "./assertions/near";
import { Contract } from "ethers";

chai.use(roughlyNear);
chai.use(near);

const USDT_ADDRESS = "0xdac17f958d2ee523a2206206994597c13d831ec7";
const USDC_ADDRESS = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48";

describe("Ensure Approve", () => {
  let admin: SignerWithAddress;
  let alice: SignerWithAddress;
  let testContract: EnsureApproveTest;
  let usdt: Contract;
  let usdc: Contract;

  before(async () => {
    [admin, alice] = await ethers.getSigners();
  });

  beforeEach(async () => {
    const EnsureApproveTest = await ethers.getContractFactory("EnsureApproveTest");
    testContract = await EnsureApproveTest.deploy();
    usdt = await   ethers.getContractAt("@openzeppelin/contracts/token/ERC20/IERC20.sol:IERC20", USDT_ADDRESS);
    usdc = await   ethers.getContractAt("@openzeppelin/contracts/token/ERC20/IERC20.sol:IERC20", USDC_ADDRESS);
  });
  describe("IERC20 Interface", () => {
    it("TEST USDC APPROVAL", async () => {
        const approvalBefore = await usdc.allowance(testContract.address, alice.address);
        console.log("Allowance Before", approvalBefore);
        await testContract.connect(alice).approveMe(usdc.address, 1000);
        const approvalAfter = await usdc.allowance(testContract.address, alice.address);
        console.log("Allowance After", approvalAfter);
       expect(approvalAfter).to.equal(1000);
    });

    it("TEST USDT APPROVAL", async () => {
        const approvalBefore = await usdt.allowance(testContract.address, alice.address);
        console.log("Allowance Before", approvalBefore);
        await testContract.connect(alice).approveMe(usdt.address, 1000);
        const approvalAfter = await usdt.allowance(testContract.address, alice.address);
        console.log("Allowance After", approvalAfter);
        expect(approvalAfter).to.equal(1000);
    });

});

});

Running this will result in the 2nd test to fail, because of the mismatching return data length

1 failing

  1) Ensure Approve
       IERC20 Interface
         TEST USDT APPROVAL:
     Error: Transaction reverted: function returned an unexpected amount of data

[hrishibhat]

Result: Medium Has duplicates Considering this a value medium issue based on the discussions above, this was a valid issue that was not acknowledged in the previous contests, but now with additional information is considered a valid medium. I agree the Wont fix rule might need to be more clear of the conditions to which they are applied

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted