New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ERC827: abuse of CUSTOM_CALL will cause unexpected result #1044

Closed
p0n1 opened this Issue Jun 24, 2018 · 6 comments

Comments

Projects
None yet
5 participants
@p0n1

p0n1 commented Jun 24, 2018

馃帀 Description

Dangerous ERC827 implementation about anythingAndCall.

require(_spender.call.value(msg.value)(_data));

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)

@mg6maciej mg6maciej referenced this issue Jun 24, 2018

Merged

Remove ERC827 token. #1045

4 of 4 tasks complete
@frangio

This comment has been minimized.

Show comment
Hide comment
@frangio

frangio Jun 24, 2018

Member

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.

Member

frangio commented Jun 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@p0n1

p0n1 Jun 25, 2018

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!

p0n1 commented Jun 25, 2018

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

This comment has been minimized.

Show comment
Hide comment
@AugustoL

AugustoL Jun 25, 2018

Contributor

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

Contributor

AugustoL commented Jun 25, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@AugustoL

AugustoL Jun 27, 2018

Contributor

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

Contributor

AugustoL commented Jun 27, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jpitts

jpitts Jul 6, 2018

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

jpitts commented Jul 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@vittominacori

vittominacori Jul 21, 2018

Contributor

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? 馃う鈥嶁檪锔

Contributor

vittominacori commented Jul 21, 2018

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? 馃う鈥嶁檪锔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment