Add asmERC20 implemetation to fix interaction with bad ERC20 after solc 0.4.22

[k06a]

Fixes no issues in this repo

🚀 Description

Allows smart contracts to interact with bad ERC20 implementations, ie returning void from transfer, transferFrom or approve. Instead of revert introduced methods returns bool.

New methods:

• asmTransfer
• asmTransferFrom
• asmApprove

Info:

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:fix).

I'll do tests/docs/linter after the discussion.

[k06a]

Also we think asmName and asmSymbol could be added to the library to allow handle string and bytes32 return types: https://github.com/multitoken/MultiToken/blob/master/contracts/ext/CheckedERC20.sol#L92

[come-maiz]

Copying our discussion from gitter:
Anton Bukov
@k06a
12:16
Hi, what do you think of this PR? #1344
Every contract interacting with arbitrary ERC20 tokens, should use this reliable api.
Leo Arias
@ElOpio
12:24
After a super quick look, I don't like that it uses assembly and call.
Anton Bukov
@k06a
12:24
There are no another way to avoid revert in solidity after calling transfer which returns void
Leo Arias
@ElOpio
12:25
A change like this shouldn't be discussed at the ERC level first? If it's important for everybody, everybody should push for a better way to solve it.
Anton Bukov
@k06a
12:26
Since solidity compiler 0.4.22 return size mismatch in function definition and result leads to revert
Leo Arias
@ElOpio
12:28
Yes, I understand why you have to do it this way. it's just that I always prefer to discuss upstream before making a patch downstream.
Anton Bukov
@k06a
12:28
@ElOpio agree, will create ERC

[frangio]

Hi @k06a! Thanks for this suggestion.

The purpose of SafeERC20 is to provide helpers that revert when the underlying ERC20 transaction fails. The safety comes from there not being a boolean return value that can be easily ignored. So asmTransfer returning a bool is not a good fit for the library.

That said, I don't think the implementation of asmTransfer is correct. Before the returndatasize opcode was introduced, not returning anything was equivalent to returning false. Some ERC20 implementations could have used this fact to signal failure, yet asmTransfer would be returning true as if the transfer had succeeded. I don't think it's possible to implement such a thing correctly, unfortunately.

[nventuro]

There's been some recent activity related to this: https://decentraland.org/blog/technology/safe-erc20-transfers

I think we should extend our current safeTransfer, safeApprove, etc. functions to also support tokens that don't return a value. The way we'd do this is by checking the size of the returned data: if a boolean is returned, revert unless true, if nothing is returned, assume success (and if something else than a boolean is returned, revert as well).

So SafeERC20 would:

• For tokens that adhere to the ERC20 interface (returning boolean), revert when the returned value is false. Tokens such as OpenZeppelin's, which always returns true but reverts on errors, are also safely handled by this treatment.
• For tokens that do not return any value, assume the call was successful (that it would've reverted in case of an error).

This leaves out tokens that return true on success, but don't return anything on failure (the problematic scenario @frangio mentioned sometimes happened pre returndatasize). I think the right trade-off is to not support such outdated behavior, however.

Would love to hear anyone's thoughts on this. Particularly about that last paragraph.

[nachomazzara]

My two cents for this:

• We should take care of the tokens which throw if you call transfer or approve with 0 as BNB. E.g: We could take transfer(from, to, 0) as a success because pre and post-conditions have passed.

function transfer(address _to, uint256 _value) {
 if (_to == 0x0) throw; // Prevent transfer to 0x0 address. Use burn() instead
 if (_value <= 0) throw;
 if (balanceOf[msg.sender] < _value) throw; // Check if the sender has enough
 if (balanceOf[_to] + _value < balanceOf[_to]) throw; // Check for overflows
 balanceOf[msg.sender] = SafeMath.safeSub(balanceOf[msg.sender], _value); // Subtract from the sender
 balanceOf[_to] = SafeMath.safeAdd(balanceOf[_to], _value); // Add the same to the recipient
 Transfer(msg.sender, _to, _value); // Notify anyone listening that this transfer took place
}

• To avoid the inconsistencies about returnDataSize, we could check pre and post-conditions. I know it will consume more gas than doing it directly with asm.

E.g transfer :

function safeTransfer(IERC20 _token, address _to, uint256 _value) internal returns (bool) {
        uint256 prevBalance = _token.balanceOf(address(this));

        if (prevBalance < _value) {
            // Insufficient funds
            return false;
        }

        address(_token).call(
            abi.encodeWithSignature("transfer(address,uint256)", _to, _value)
        );

        if (prevBalance - _value != _token.balanceOf(address(this))) {
            // Transfer failed
            return false;
        }

        return true;
    }

• I would return always true or false, not revert. Just to give more flexibility on how to use it.

The full implementation of what we did at Decentraland is here

[nventuro]

We should take care of the tokens which throw if you call transfer or approve with 0 as BNB. E.g: We could take transfer(from, to, 0) as a success because pre and post-conditions have passed.

How do you propose we handle such a situation? The only reasonable (for some definition of 'reasonable') way that I can think of is having some sort of registry of non-compliant tokens, and not performing the call for those when the value is 0.

To avoid the inconsistencies about returnDataSize, we could check pre and post-conditions. I know it will consume more gas than doing it directly with asm.

Which inconsistencies do you worry about? The pre and post check approach is very interesting though.

I would return always true or false, not revert. Just to give more flexibility on how to use it.

That'd make it inconsistent, since some of the tokens do revert on failure. I'd rather all errors be handled the same way, and not give the false illusion that a contract may react to a failed call.

[k06a]

Throw handling will only works with gas limiting to some constant value (maybe 250k gas?)

[nachomazzara]

How do you propose we handle such a situation? The only reasonable (for some definition of 'reasonable') way that I can think of is having some sort of registry of non-compliant tokens, and not performing the call for those when the value is 0.

a) Check pre and post-conditions as the transfer example here.
b) We could just return true if the value to transfer is 0.
P.S: Here you can see a list of non-compliant tokens

Which inconsistencies do you worry about? The pre and post check approach is very interesting though.

I mean, the inconsistencies of:

Before the returndatasize opcode was introduced, not returning anything was equivalent to returning false,

Checking pre and post-conditions are expensive but efficient

That'd make it inconsistent since some of the tokens do revert on failure. I'd rather all errors be handled the same way, and not give the false illusion that a contract may react to a failed call.

This library will be an abstract layer above ERC20 standard methods. So, will be used for new contracts, right? in that case, we could have two ways of using it like:

require(token.asmTransfer(from, to, value), "Transfer failed")

or

if (approve(owner, 0)) {
} else {
}

Tokens as BNB doesn't accept 0 on approve preventing you to clear it. So, we set the minimum possible.
As reverting on transfers, but not in approve would be weird, we opt to change the reverts to return false. But It will also depend on the scope of this library.

[k06a]

Pre and post checks should also respect that some tokens can have fees, for example EURS and USDT.

[k06a]

Pre and post checks should also respect that some tokens can have fees, for example EURS and USDT.

[alcuadrado]

I've been discussing this with @frangio and @nachomazzara and my opinion pretty much aligns with @nventuro's.

I think we should extend our current safeTransfer, safeApprove, etc. functions to also support tokens that don't return a value.

I think this makes sense. I'd assume that using SafeERC20 should be enough when dealing with third-party ERC20s, and that's not true right now.

This leaves out tokens that return true on success, but don't return anything on failure (the problematic scenario @frangio mentioned sometimes happened pre returndatasize). I think the right trade-off is to not support such outdated behavior, however.

I really doubt such implementation exists IRL. Had solidity been generating that kind of code as an optimization, everything would be broken now that they always check the returndatasize.

There's still the possibility of someone doing that in asm, but I wouldn't support that.

• I would return always true or false, not revert. Just to give more flexibility on how to use it.

That's fine if you want to normalize the different implementations to make them as compliant with ERC20 as possible. But I think the purpose of SafeERC20 is different.

[nventuro]

Closing in favor of #1655.