ERC-827 callbacks can lead to reentrancy attack vectors

#1

This is an interesting case we all can learn from. Smart contract ERC-827 allows / potentially enables a bad contract security practice.

From issue comments on the ERC-827 EIP by the security researcher:

We wrote a blog post, how replacing ERC20 with ERC827, can lead to unexpected reentrancy attacks.
…

  • Given that potential use cases of transferAndCall are unclear, I see the value of our article in clarifying how it should not be used.
  • Similar issues apply to approveAndCall, however, given that this functionality does not exist in ERC20 (as you said), we would have had to construct an example with vulnerable code to use it. If you have concrete cases, where approveAndCall is called from a smart contract we would be happy to check.

Overall, our intention was just to notify people that special care is needed when using these functions inside a smart contract.

– ritzdorf, CTO @ ChainSecurity

Related tweets and stories:

From OpenZeppelin’s GitHub issue on this:

It is a really bad practice to allow the abuse of CUSTOM_CALL in token standard.

#2

Hello @jpitts, thanks for bringing this up on the ethereum-magicians community.

The issue with transferAndCall is that there is no way to verify the actual balance that was transfered, that is why we recommend the use of approveAndCall to work with “verified” balance in the EIP description.

Now we have another issue that brings problem when you for example want to work with ERC223 or ERC667 tokens, you can use the fallback functions of this tokens from the ERC827 contract and drain funds of contracts. Example: https://github.com/windingtree/erc827/blob/master/contracts/examples/VaultAttack.sol

We are working on changes on the standard to prevent this from happening, the main proposal that we have for now is the use of “allowed” callbacks, wich means that the receiver contracts will be able to allow functions to be executed on it.

I think the allowedCallbacks is a good solution, it adds the necessary interface to allow contracts to execute specific and tested arbitrary calls over them. Removing this permission of execute calls on any contract form the token contract, and it adds only a ~50 lines of code.

I would like to get some feedback from here.