ERC-827 callbacks can lead to reentrancy attack vectors

jpitts · July 6, 2018, 12:56pm

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

ERC827: abuse of CUSTOM_CALL will cause unexpected result

opened 06:18PM - 24 Jun 18 UTC

closed 04:32PM - 28 Jun 18 UTC

p0n1

## 🎉 Description Dangerous ERC827 implementation about `anythingAndCall`. …https://github.com/OpenZeppelin/openzeppelin-solidity/blob/f18c3bc438b366f9cb3a8613f5be160c2cbced5e/contracts/token/ERC827/ERC827Token.sol#L46 Users are allowed to pass arbitrary data, leading to call any function with any data on any contract address. - [x] 🐛 This is a bug report. - [ ] 📈 This is a feature request. ## 💻 Environment Any contract using this ERC827 implementation or with similar CUSTOM_CALL feature will be affected. ## 📝 Details It is a really bad practice to allow the abuse of CUSTOM_CALL in token standard. ```js <address>.call.value(msg.value)(_data) ``` Attackers could call any contract **in the name of vulnerable contract** with CUSTOM_CALL. This vulnerability will make these attacking scenarios possible: - Attackers could steal almost each kind of tokens belong to the vulnerable contract [[1]] [[2]] - Attackers could steal almost each kind of tokens `approved` to the vulnerable contract - Attackers could bypass the auth check in vulnerable contract by proxy of contract itself in special situation [[3]] (**edit**: current openzeppelin implementation is not affected with the help of `require(_to != address(this));`) - Attackers could pass fake values as parameter to cheat with receiver contract [[4]] We ([SECBIT](https://secbit.io)) think that the ERC827 proposal should be discussed further in community **before OpenZeppelin putting the implementation in the repo.** Many developers could use this code without knowledge of hidden danger. [1]: https://etherscan.io/tx/0xb72dcc4d04381ccad416b960e95183e94ee13e942743da913cf139c8abe212e7 [2]: https://etherscan.io/tx/0x40a292d74bddaac2690385aee0c366edf31904ef681b547b1baa3190ba568888 [3]: https://medium.com/@atnio/erc223-smart-contract-breach-and-resolution-vulnerability-relating-to-the-concurrent-9a402495f382 [4]: https://github.com/ethereum/EIPs/issues/827#issuecomment-397857455 \[1\] attack 1, https://etherscan.io/tx/0xb72dcc4d04381ccad416b960e95183e94ee13e942743da913cf139c8abe212e7 \[2\] attack 2, https://etherscan.io/tx/0x40a292d74bddaac2690385aee0c366edf31904ef681b547b1baa3190ba568888 \[3\] custom_call related bug, https://medium.com/@atnio/erc223-smart-contract-breach-and-resolution-vulnerability-relating-to-the-concurrent-9a402495f382 \[4\] pass fake values to receiver contract, https://github.com/ethereum/EIPs/issues/827#issuecomment-397857455

AugustoL · July 9, 2018, 11:44am

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.