Enforcing ABI length checks for return data from calls can be breaking

[axic]

This was identified independently by @yaronvel (https://gitter.im/ethereum/solidity-dev?at=5af1490f97f07c7e13787516) and @mattdf / @Ivshti / @decanus (https://gist.github.com/decanus/bdc08df5bf73af349333d1a8cae07d23).

To show the problem as a simple example:

interface Token {
  function transfer() returns (bool);
}

contract Good {
  function transfer() returns (bool) { return true; }
}

contract Bad {
  function transfer() {}
}

contract Wallet {
  function transfer(address token) {
    require(Token(token).transfer());
  }
}

The interface expects the transfer function to return a value and as a result Wallet compiles. In the current version of Solidity this will trigger:

• a length check when ABI decoding the returned data (fails if the length is < 32)
• using the returndatasize opcode to retrieve the length

This fails when passing in an instance of Bad, because it returns 0 bytes.

Apparently, this wasn't a problem with previous versions, because it didn't had the length check. Also because the return types are not part of the ABI selector hash, this wasn't enforced in the past at all.

As a result there are token contracts out there which do not comply with the above ERC20 interface and this change deprives decentralised exchanges from dealing with them if compiled with a recent Solidity version.

[axic]

I wonder though about the expectation.

If it doesn’t return a bool, the old compiler should have taken it as a 0, which means false. I think the real solution is to stop using require if you know some tokens are incompatible. The call itself will bubble up exceptions so if it fails, the caller will fail too.

[Ivshti]

Is there a notable case where an ERC20 might return false but not throw?

[decanus]

@Ivshti As far as I know, the older ERC20 contracts returned false. Open Zeppelin I think only changed it recently

[Ivshti]

OpenZeppelin behaviour in the adex contract is to not return anything, but only throw. Perhaps it returned false before that

[Arachnid]

There's an important distinction between what an interface specifies and what a contract does.

If the interface says it returns bool (which ERC20 does), then the implementation must return an ABI-encoded boolean if it returns, even if it's always the same value.

The current convention is for ERC20 tokens to revert if there's an error and return true if there isn't, because this is safest should work for everyone. An ERC20 token that returns 0 bytes is violating the ERC20 interface.

But, suddenly changing Solidity behaviour for these nonstandard return values in a point release isn't a great thing.

[axic]

Apparently the reason this worked in the old compiler is that the output memory area overlapping with the input memory area and since the input contained the function selector, it was never all zeroes, and as a result, never false.

[mattdf]

On @axic's comment, this behaviour is triggered when the child contract ends on a STOP instruction, or when it ends on a RETURN instruction with returnsize of 0, because then the memarea of input is not written to at all, and:

MLOAD (inoffset/outoffset)
ISZERO
ISZERO

Ensures that this always returns true, since the method signature makes the first 32 bytes always nonzero.

This means that contracts that call children that specify return values but end in STOP or RETURN(0) can be tricked into thinking that the input calldata is what is returned by the child.

[DaveAppleton]

Is there a notable case where an ERC20 might return false but not throw?

Anything that copied BAT contracts would do so.
At the time they were taken as pretty safe to copy.

    function transfer(address _to, uint256 _value) returns (bool success) {
      if (balances[msg.sender] >= _value && _value > 0) {
        balances[msg.sender] -= _value;
        balances[_to] += _value;
        Transfer(msg.sender, _to, _value);
        return true;
      } else {
        return false;
      }
    }

[yaronvel]

Just to give some background. This behavior was detected on OMG token. And afaik most existing DEX contract will break (will not be able to trade OMG) once compiled with new version.

I agree it is not a solidity issue, and I am also not sure the compiler should fix anything.
However, this should be brought to the attention of the public, or at least relevant projects.

[chriseth]

Calling a function on a contract that does not return the expected type is a runtime error and should be treated as such. The caller has to decode the data into something. If the returned size is zero, there is no way to create a value out of it and the correct way is to revert.

As a workaround, you can use --evm-version homestead to make it still work for faulty contracts, but you will observe undefined behaviour with regards to the return value, which might be dangerous.

Another way to make an ERC20-non-compliant contract work is by creating an intermediate contract that translates between the two interfaces.

[decanus]

So I did a bit of digging and found that any token using Open Zeppelin between OpenZeppelin/openzeppelin-contracts@52120a8#diff-da41308b41a2748120cc2cfa52b1ea00 and OpenZeppelin/openzeppelin-contracts@6331dd1#diff-da41308b41a2748120cc2cfa52b1ea00 does not conform to ERC20. So the landscape of tokens which would be affected by this feature is likely to be huge.

[Arachnid]

@chriseth Agreed this is a bug in the contracts concerned - but this change in a point-release of Solidity is a breaking change for existing code.

[chriseth]

@Arachnid I agree, we should have been more careful there, so sorry about that.

To clarify the --evm-version homestead option: This removes that feature, reading of dynamic return data (where the size check is required) and the use of staticcall which can be breaking in a similar way.

Also not that the versioning scheme does not distinguish between bugfix releases and feature releases. We only distinguish breaking (change in the second component of the version number as long as the first is zero) and non-breaking (change only in the last copmonent of the version number, as long as the first is zero) changes, where a change is defined as non-breaking if and only if it does not change the compilability and semantics of previously compilable contracts unless they rely on undocumented or undefined features or bugs in the compiler.

[3esmit]

What about this case of Bad Token GavCoin?
https://etherscan.io/address/0x0b8d56c26d8cf16fe1bddf4967753503d974de06#code

It defines returns (bool) at signature but never returns true:

    function transfer(address _to, uint256 _value) when_owns(msg.sender, _value) returns (bool success) {
        Transfer(msg.sender, _to, _value);
        accounts[msg.sender].balance -= _value;
        accounts[_to].balance += _value;
    }

I think is not the case, but also an example of bad implemented token that fail even before the return lenght check (in case you require token transfer to return true, as in spec)..

[ngutman]

Another "high-profile" non ERC20 compatible token is OMG for example.

function transferFrom(address _from, address _to, uint _value) onlyPayloadSize(3 * 32);
function transfer(address _to, uint _value) onlyPayloadSize(2 * 32);

This is highly problematic for DEXes, as soon as they redeploy their contracts with solidity > 0.4.24 such tokens won't be usable on the platform.

[axic]

It defines returns (bool) at signature but never returns true:

@3esmit it implicitly returns false, which I would categorise as a programmer's error. One could argue this implicit behaviour isn't the best. For that there are proposals to change it and I would rather discuss that separately.

This is highly problematic for DEXes, as soon as they redeploy their contracts with solidity > 0.4.24 such tokens won't be usable on the platform.

As mentioned by @chriseth, it is still possible to compile contracts with non-byzantium mode to remove the length check. I've raised issues at truffle (trufflesuite/truffle-compile#72) and remix (https://github.com/ethereum/remix-ide/issues/1282) to implement support for this option. I hope that will help. We also plan to document this a bit further and raise awareness.

I have also created this proof of concept (do not use!) to show it should be possible creating wrappers even for recent Solidity versions: https://gist.github.com/axic/85f12e78885a5336f2806357b1ba6ea1

[holiman]

@axic regarding that poc snippet. If someone embeds that into a contract, and calls it internally. So that method foo calls safeTransfer.

Won't that become a JUMP instead of a CALL internally? And if so, is there a possible corruption happening since you're writing to memory location 0 instead of what solidity designates the 'free memory' area ('0x60` onwards or whatever it is?)

[axic]

Yes it would be a JUMP. Memory location 0 is designated as a "scratch area" used by hashing functions, so it is safe to use. It will be overwritten by the next use of KECCAK256.

Also the free memory area starts now at 0x80, but better to rely on the actual pointer in 0x40.

[axic]

For long term compatibility it may be better to use the free memory pointer, just not update it in the function.

[BrendanChou]

Created my own version of safe transfer / transferFrom / approve. @axic is there more information about the memory locations somewhere? 0x0 and 0x40 and other important ones?

My implementation:
https://gist.github.com/BrendanChou/88a2eeb80947ff00bcf58ffdafeaeb61

[chriseth]

https://solidity.readthedocs.io/en/v0.4.24/miscellaneous.html#layout-in-memory

[lukas-berlin]

This issue is now 22 days old and I see no response in the community. This is a critical issue for the affected contracts. When users send their BadToken to any contract (DEX, DutchEx, ... ) compiled after solc 0.4.22 the token will most likely be stuck on such contracts
I did write an email to OMG. I think there should be a bigger outcry. These tokens may need to be forked, or the need to implement some proxy as @chriseth suggested

[holiman]

This issue is now 22 days old and I see no response in the community.

Well, Chris posted this a couple of days ago: https://medium.com/@chris_77367/explaining-unexpected-reverts-starting-with-solidity-0-4-22-3ada6e82308c

[lukas-berlin]

I compiled a list of affected contracts from the tokens listed on etherdelta. 130/913 tokens are affected.
https://gist.github.com/lukas-berlin/f587086f139df93d22987049f3d8ebd2

[lukas-berlin]

I published an article about this issue, https://medium.com/@lukas.berlin/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca with a safeTransfer proposal similar to the proposal of @BrendanChou

[BrendanChou]

Has anyone looked into transferFrom and approve? Are there tokens out there that do not return values for these functions?

[lukas-berlin]

@BrendanChou Yes the faulty interface from OpenZeppelin also includes approve() and transferFrom(), so transferFrom() has the same problem as transfer(). It would require the same solution. I have never seen a contract that calls approve()

[BrendanChou]

Most DEXs will probably not have to use approve, but any smart contract that interacts with DEXs will. Example from dYdX

[p0n1]

Most of the incompatible ERC20 tokens (above 1703) have used token-advanced.sol code in solidity-org repo and I created PRs to fix.

Check this blog post for detailed report.
https://medium.com/loopring-protocol/an-incompatibility-in-smart-contract-threatening-dapp-ecosystem-72b8ca5db4da

And also, we are trying to make a working solution for the dApps and dexs, which is similar to the work of @lukas-berlin and @BrendanChou. But using call with function signature instead of still using incompatible no return function interface in code. Community should care more about common seen template code.

Hoping not to break anything. Help review please.
https://github.com/sec-bit/badERC20Fix/blob/master/badERC20Fix.sol

[Ivshti]

After a brief discussion with @chriseth, it seems that this should be mitigatable by replicating the behavior of the old require in assembly, but we will have to construct the bytecode of the transfer() call ourselves

@mattdf @decanus thoughts?

[Ethearnal]

Hello everyone,
I know this is an old issue but our token (ERT - address: 0x8c78A83DE6FAa64B100B6055BDF3a1f0b445eFD2) for example is still affected by this. Any new solutions or work-arounds available?

[BlinkyStitt]

Is there a script-able way to test if a contract has this issue?

[p0n1]

Is there a script-able way to test if a contract has this issue?

Adelaide is a static analysis tool which could detect issues like erc20-return-false or erc20-no-return.

Try it if you are interested.

https://github.com/sec-bit/adelaide/blob/secbit-ssae/README.secbit.md

[BlinkyStitt]

Great, thanks!

Has there been any progress with https://github.com/sec-bit/badERC20Fix/blob/master/badERC20Fix.sol ? It looks good to me, but I am very new to solidity and so that doesn't mean anything.

[p0n1]

Great, thanks!

Has there been any progress with https://github.com/sec-bit/badERC20Fix/blob/master/badERC20Fix.sol ? It looks good to me, but I am very new to solidity and so that doesn't mean anything.

@wysenynja badERC20Fix is a demo solution for DApp and DEX by adding a safe transfer/trasferFrom wrapper to solve the issue.

Different DEXs take diff ways.

This is 0x trick and this is Loopring one.

[BlinkyStitt]

Looks like uniswap didn’t know about this.

https://mobile.twitter.com/UniswapExchange/status/1072286775115161611

[BlinkyStitt]

I realize this is the solidity repo, but does anyone have a way to handle this in Vyper?