SafeERC20 only reverts with message if token returns False

It looks like the safeERC20 Library only reverts with the message SafeERC20: ERC20 operation did not succeed if the token returns False, otherwise it just throws the error that resulted in the revert. I imagine this to be by design but it seems a bit odd since less then 1% of all ERC20s return false at all.

Libraries like GPv2SafeERC20 from Gnosis and SafeTransferLib from Solmate/Solady return an error message in case of any revert. I would love to understand the reasoning behind this, in my personal opinion it just makes developers life harder since some tokens don't have helpful revert messages (if any!), so a great revert message can be of great help for any developer trying to find where the problem is.

:1234: Code to reproduce

:computer: Environment

  • openzeppelin-contracts v4.8.1
  • Foundry (forge 0.2.0 (e049b0d 2023-02-23T00:14:26.363312Z))

Consider the reason that this library has been created to begin with, being a few token contracts which had been deployed to the network at a very early phase (2017ish), without properly conforming to the ERC20 standard.

More specifically, these token creators simply did not bother to return a boolean value in some of the ERC20 functions (transfer, approve, etc), which would typically be as simple as returning true upon completion, since nothing needs to be returned otherwise (upon reverting).

This, in turn, leads any attempt to call any of these functions using a standard ERC20 interface, to revert on "evm crash" (kinda similar to undefined behavior in "bare metal languages" like C and C++).

The SafeERC20 library simply wraps any given ERC20 contract in a manner which works around this potential problem.

FYI, you can avoid using it for any token which conforms to the ERC20 standard (for example, if your system isn't design to handle any arbitrary token, but only specific token or tokens which implement the standard correctly).

See this post for more details.


Here is a quick coding example which might help illustrating the problem:

Consider the following terminology:

  • GoodToken - a token contract which conforms to the ERC20 standard
  • BadToken - a token contract which doesn't conform to the ERC20 standard

Then given the following contracts:

contract GoodToken {
    event Transfer(address indexed _from, address indexed _to, uint256 _value);
    function transfer(address _to, uint256 _value) public returns (bool) {
        emit Transfer(msg.sender, _to, _value);
        return true;
    }
}

contract BadToken {
    event Transfer(address indexed _from, address indexed _to, uint256 _value);
    function transfer(address _to, uint256 _value) public {
        emit Transfer(msg.sender, _to, _value);
    }
}

contract Caller {
    function transferGoodToken(address _token, address _to, uint256 _value) public {
        GoodToken(_token).transfer(_to, _value);
    }
    function transferBadToken(address _token, address _to, uint256 _value) public {
        BadToken(_token).transfer(_to, _value);
    }
}

We can observe the following results:

  • Calling transferGoodToken with a the address of a GoodToken as input: completes successfully
  • Calling transferGoodToken with a the address of a BadToken as input: reverts the transaction
  • Calling transferBadToken with a the address of a GoodToken as input: completes successfully
  • Calling transferBadToken with a the address of a BadToken as input: completes successfully

The SafeERC20 works around this potential problem.

1 Like

The context provided by @barakman is already enough for explaining why is there, but what I'm understanding from your post @pedrommaiaa is that it's a headache to not have proper relevant information in errors, is that correct?

I'm taking a look at the code you shared but it's unclear to me what is your suggestion, would you mind clarifying?

Also, talking about token errors, would you mind taking a look to EIP-6093? We're searching for feedback on this proposal, since it'll help us migrate the library to custom errors in the next 5.0 release. Since you mention that revert messages should be helpful, any comment regarding what information makes an error useful is appreciated.

1 Like

@ernestognw Yes that's correct. The code was just a quick example to show that the SafeERC20: ERC20 operation did not succeed revert message is only used when the token returns False, otherwise it just reverts with the error that caused the revert.

My suggestion would be to have this error message being emitted whenever there's a revert, regardless of the token returning False or not.

I will take at look at the proposal, thank you!

Thanks for clarifying @pedrommaiaa.

I don't follow why it'd be better to have the same error for any revert, my feeling is the opposite, we'd be overriding more specific and detailed reasons if we do so.

What would be the benefit of emitting the same message for every case?