Use `SafeERC20` .

[gnosis]

Use `SafeERC20` .

[fedgiac]

I compared two different approaches to safe ERC20 transactions, one is OpenZeppelin's SafeERC20 and the other is Uniswap's dedicated function.

I was interested in the gas cost of submitting a transaction for each of the two approaches and for different types of ERC20 tokens.
The functions are only slightly different and the behavior appears to be the same. Uniswap's one is consistently cheaper by about 1000 gas/transfer. Details follow, each test uses a token with a different behavior on transfer:

Gas cost Uniswap:      33740
Gas cost OpenZeppelin: 34710
      ✓ transferReturnsNothing: success when returning nothing (54ms)
Gas cost Uniswap:      33740
Gas cost OpenZeppelin: 34710
      ✓ transferReturnsTrue: success when returning true (40ms)
Gas cost Uniswap:      63812
Gas cost OpenZeppelin: 64780
      ✓ transferReturnsFalse: revert when returning false (53ms)
Gas cost Uniswap:      63555
Gas cost OpenZeppelin: 64641
      ✓ transferReverts: revert when reverting (47ms)

(The large difference in cost when the transaction reverts is because of the absent gas refunds.)

Tokens.sol

pragma solidity ^0.6.12;

import "@openzeppelin/contracts/presets/ERC20PresetMinterPauser.sol";
import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract Executor {
    using SafeERC20 for IERC20;

    bytes4 private constant SELECTOR = bytes4(
        keccak256(bytes("transferFrom(address,address,uint256)"))
    );

    function safeTransferUniswap(
        address token,
        address from,
        address to,
        uint256 value
    ) internal {
        (bool success, bytes memory data) = token.call(
            abi.encodeWithSelector(SELECTOR, from, to, value)
        );
        require(
            success && (data.length == 0 || abi.decode(data, (bool))),
            "UniswapV2: TRANSFER_FAILED" //string(data)
        );
    }

    function testSafeTransferOpenzeppelin(
        address token,
        address from,
        address to,
        uint256 value
    ) public {
        IERC20(token).safeTransferFrom(from, to, value);
    }

    function testSafeTransferUniswap(
        address token,
        address from,
        address to,
        uint256 value
    ) public {
        safeTransferUniswap(token, from, to, value);
    }
}

contract TransferReturnsNothing is ERC20PresetMinterPauser {
    constructor()
        public
        ERC20PresetMinterPauser("TransferReturnsNothing", "TRN")
    {}

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public override(ERC20) returns (bool) {
        super.transferFrom(sender, recipient, amount);
        return true;
    }
}

contract TransferReturnsTrue is ERC20PresetMinterPauser {
    constructor()
        public
        ERC20PresetMinterPauser("TransferReturnsTrue", "TRT")
    {}

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public override(ERC20) returns (bool) {
        super.transferFrom(sender, recipient, amount);
        return true;
    }
}

contract TransferReturnsFalse is ERC20PresetMinterPauser {
    constructor()
        public
        ERC20PresetMinterPauser("TransferReturnsFalse", "TRF")
    {}

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public override(ERC20) returns (bool) {
        super.transferFrom(sender, recipient, amount);
        return false;
    }
}

contract TransferReverts is ERC20PresetMinterPauser {
    constructor() public ERC20PresetMinterPauser("TransferReverts", "TR") {}

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public override(ERC20) returns (bool) {
        super.transferFrom(sender, recipient, amount);
        require(false);
    }
}

Tests.ts

import type {
  TransactionResponse,
  TransactionReceipt,
} from "@ethersproject/abstract-provider";
import { ethers, waffle } from "@nomiclabs/buidler";
import { expect } from "chai";
import { Contract, utils, Signer } from "ethers";

function sendContractTransaction(
  contract: Contract,
  signer: Signer,
  functionNameOrSignature: string,
  ...args: unknown[]
): [Promise<TransactionResponse>, Promise<TransactionReceipt>] {
  contract = contract.connect(signer);
  const provider = waffle.provider;
  //const signer = contract.signer;
  const contractTransaction = contract.populateTransaction[
    functionNameOrSignature
  ](...args);
  const populatedTransaction = contractTransaction.then((transaction) =>
    signer.populateTransaction(transaction),
  );
  const signedTransaction = populatedTransaction.then((transaction) =>
    signer.signTransaction(transaction),
  );
  const transactionHash = signedTransaction.then((transaction) =>
    utils.keccak256(transaction),
  );
  const result = signedTransaction.then((transaction) =>
    provider.sendTransaction(transaction),
  );
  const receipt = transactionHash
    .then((hash) => provider.getTransaction(hash))
    .then((response) => response.wait())
    .catch((error) => error.receipt);
  return [result, receipt];
}

describe("Safe ERC20", () => {
  // US: Uniswap, OZ: Openzeppelin
  const [
    transactionSender,
    fromUS,
    fromOZ,
    toUS,
    toOZ,
  ] = waffle.provider.getWallets();

  const amount = "1".padEnd(19, "0");

  let transferReturnsNothing: Contract;
  let transferReturnsTrue: Contract;
  let transferReturnsFalse: Contract;
  let transferReverts: Contract;

  let executor: Contract;

  beforeEach(async () => {
    const TransferReturnsNothing = await ethers.getContractFactory(
      "TransferReturnsNothing",
    );
    const TransferReturnsTrue = await ethers.getContractFactory(
      "TransferReturnsTrue",
    );
    const TransferReturnsFalse = await ethers.getContractFactory(
      "TransferReturnsFalse",
    );
    const TransferReverts = await ethers.getContractFactory("TransferReverts");
    const Executor = await ethers.getContractFactory("Executor");

    transferReturnsNothing = await TransferReturnsNothing.deploy();
    transferReturnsTrue = await TransferReturnsTrue.deploy();
    transferReturnsFalse = await TransferReturnsFalse.deploy();
    transferReverts = await TransferReverts.deploy();
    executor = await Executor.deploy();

    for (const token of [
      transferReturnsNothing,
      transferReturnsTrue,
      transferReturnsFalse,
      transferReverts,
    ]) {
      for (const from of [fromUS, fromOZ]) {
        await token.mint(from.address, amount);
        await token
          .connect(from)
          .approve(executor.address, amount, { from: from.address });
      }
    }
  });

  describe("behavior", async () => {
    it("transferReturnsNothing: success when returning nothing", async () => {
      const token = transferReturnsNothing;
      const transferUS = executor.testSafeTransferUniswap(
        token.address,
        fromUS.address,
        toUS.address,
        amount,
      );
      const transferOZ = executor.testSafeTransferOpenzeppelin(
        token.address,
        fromOZ.address,
        toOZ.address,
        amount,
      );
      const receiptUS = await (await transferUS).wait();
      const receiptOZ = await (await transferOZ).wait();
      console.log(`Gas cost Uniswap:      ${receiptUS.gasUsed.toString()}`);
      console.log(`Gas cost OpenZeppelin: ${receiptOZ.gasUsed.toString()}`);
    });

    it("transferReturnsTrue: success when returning true", async () => {
      const token = transferReturnsTrue;
      const transferUS = executor.testSafeTransferUniswap(
        token.address,
        fromUS.address,
        toUS.address,
        amount,
      );
      const transferOZ = executor.testSafeTransferOpenzeppelin(
        token.address,
        fromOZ.address,
        toOZ.address,
        amount,
      );
      const receiptUS = await (await transferUS).wait();
      const receiptOZ = await (await transferOZ).wait();
      console.log(`Gas cost Uniswap:      ${receiptUS.gasUsed.toString()}`);
      console.log(`Gas cost OpenZeppelin: ${receiptOZ.gasUsed.toString()}`);
    });

    it("transferReturnsFalse: revert when returning false", async () => {
      const token = transferReturnsFalse;
      const [transferUS, receiptUS] = sendContractTransaction(
        executor,
        transactionSender,
        "testSafeTransferUniswap",
        token.address,
        fromUS.address,
        toUS.address,
        amount,
      );
      await expect(transferUS).to.be.revertedWith("UniswapV2: TRANSFER_FAILED");
      const [transferOZ, receiptOZ] = sendContractTransaction(
        executor,
        transactionSender,
        "testSafeTransferOpenzeppelin",
        token.address,
        fromOZ.address,
        toOZ.address,
        amount,
      );
      await expect(transferOZ).to.be.revertedWith(
        "SafeERC20: ERC20 operation did not succeed",
      );
      console.log(
        `Gas cost Uniswap:      ${(await receiptUS).gasUsed.toString()}`,
      );
      console.log(
        `Gas cost OpenZeppelin: ${(await receiptOZ).gasUsed.toString()}`,
      );
    });

    it("transferReverts: revert when reverting", async () => {
      const token = transferReverts;
      const [transferUS, receiptUS] = sendContractTransaction(
        executor,
        transactionSender,
        "testSafeTransferUniswap",
        token.address,
        fromUS.address,
        toUS.address,
        amount,
      );
      //await transferUS;
      await expect(transferUS).to.be.revertedWith("UniswapV2: TRANSFER_FAILED");
      const [transferOZ, receiptOZ] = sendContractTransaction(
        executor,
        transactionSender,
        "testSafeTransferOpenzeppelin",
        token.address,
        fromOZ.address,
        toOZ.address,
        amount,
      );
      //await transferOZ;
      await expect(transferOZ).to.be.revertedWith(
        "SafeERC20: low-level call failed",
      );
      console.log(
        `Gas cost Uniswap:      ${(await receiptUS).gasUsed.toString()}`,
      );
      console.log(
        `Gas cost OpenZeppelin: ${(await receiptOZ).gasUsed.toString()}`,
      );
    });
  });
});

[nlordell]

I think I see where the difference is;

We use {Address.functionCall} to perform this call, which verifies that the target address contains contract code

I imagine the difference could be in checking the address has code.

This is interesting as it succeeds when there nothing is returned, meaning it would work with any address and not just a contract. The question is, is this OK behaviour for us? I think so as it would require a user signing an order against a token that doesn't exist.

[fleupold]

Wouldn't invoking a function on an address without code or an EOA also revert anyways?

[nlordell]

I think it might depend on how it is called, but I'm not entirely sure.

[nlordell]

So I did some digging and indeed this is the issue.

Consider the following contracts.

Caller.sol

// SPDX-License-Identifier: LGPL-3.0-or-newer
pragma solidity ^0.7.0;
pragma abicoder v2;

contract Caller {
    function test(address target) external returns (bool success, bytes memory result) {
        return target.call(abi.encodeWithSignature("answer()"));
    }
}

contract DeepThought {
    function answer() external pure returns (uint256) {
        return (42);
    }
}

contract EventAnswer {
    event Answer(uint256 value);
    
    function answer() external {
        emit Answer(42);
    }
}

If we eth_call the function Caller.test() with different addresses, we observe that:

• Calling it on an EOA (or any random address for that matter, including address(0)) produces{ "0": "bool: success true", "1": "bytes: result 0x" }
• Calling it on a DeepThought instance produces { "0": "bool: success true", "1": "bytes: result 0x000000000000000000000000000000000000000000000000000000000000002a" }
• Calling it on a EventAsnwer instance produces { "0": "bool: success true", "1": "bytes: result 0x" } as well, and emits a log { "event": "Answer", "args": { "0": "42", "value": "42" } }.

So indeed, checking whether the address has code allows the caller to distinguish between a contract that returns nothing (like some ERC20.transfer implementations) and just a random address.

The question now is whether or not we want to protect against this or, take the Uniswap route and just assume that users won't sign orders to tokens that don't exist.

[nlordell]

Personally, I think that we should just do what Uniswap does to save some gas. My main motivation for this is:

• Malicious solvers can already extract fees from the settlement contract with an interaction
• The infrastructure and front-end will most likely guarantee that these kinds of orders aren't signed.
• We save a CODESIZE call worth of gas for each transfer (2 per order).

[nlordell]

All ERC20 operations are handled in the GPv2TradeExecution contract and use SafeERC20 operations.