ERC827: abuse of CUSTOM_CALL will cause unexpected result

[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.

• 🐛 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.

<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) 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] 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, ethereum/EIPs#827 (comment)

[frangio]

Thank you for the analysis @p0n1. I agree with your assessment. We will remove this token.

Unfortunately at the time this was merged we didn't have a clear policy on how to treat early ERC drafts. Since then, we have blocked a couple of ERC implementations that were way too early in the process to have received meaningful feedback, and created a proposals directory to put draft ERCs that may still be unstable.

[p0n1]

Yes!

Add ERC draft implementation to proposals directory and tell people that it is just a unstable draft with issue discussion link provided is a good choice!

[AugustoL]

@p0n1 @frangio I agree on moving the ERC827 implementation to proposals folder and continue working on this issues there, I would love to find a way to fix this issues here or on the ERC827 eip issue.
Im going to propose a public call for developers that are interested in collaborating on the standard to improve de decision making process.
I think there is a need for tokens be able to execute more calls rather than just transfer value and that we got a draft good enough to work over it as a community.

[AugustoL]

I created a gitter channel to discuss about ERC827 standard, implementations and governance of the standard.

https://gitter.im/ERC827/Lobby

I also created a public calendar on google calendar for ERC827 community calls, maybe to happen once every two weeks, where anyone in the community can join. The first call will be 5 of june at 5pm GMT +2.

Link to the call: https://calendar.google.com/event?action=TEMPLATE&tmeid=NjU5a2VnZmxsaTBycXVxdXQzYmJkcGpzZDkgbjg3bDdvcXVmMTQybmY4MGxlMGtoM3J2cThAZw&tmsrc=n87l7oquf142nf80le0kh3rvq8%40group.calendar.google.com

Link to the ERC827 calendar: https://calendar.google.com/calendar?cid=bjg3bDdvcXVmMTQybmY4MGxlMGtoM3J2cThAZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ

[jpitts]

FYI, I have posted to the Magicians Forum in order to get more visibility on this issue by the contract security working group.

https://ethereum-magicians.org/t/erc-827-callbacks-transferandcall-et-al-can-lead-to-reentrancy-attack-vectors/660

[vittominacori]

And msg.sender of the called function is the contract itself.
So we are calling other contracts as per the ERC827 contract.

Why it was in openzeppelin official release? 🤦‍♂️