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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Language-based fix for "Callstack attack" bug #656

Closed
taoeffect opened this Issue Jun 16, 2016 · 14 comments

Comments

Projects
None yet
7 participants
@taoeffect

taoeffect commented Jun 16, 2016

See this work by Andrew Miller and Zikai Wen (don't know their GitHub usernames):

http://hackingdistributed.com/2016/06/16/scanning-live-ethereum-contracts-for-bugs/

The work-around they recommend is highly unintuitive and seems poorly documented, with the bug affecting a "majority of smart contracts":

We start by trying out our heuristics on the Etherscrape repository of Solidity source code. As of March 20, 2016, the Etherscrape repo contained 361 Solidity contract programs, 56 of which contained a send instruction. Of these contract programs, we’d infer that the majority (at least 36 of 56) do not use either of the defensive programming techniques.

And:

Upon inspection, not one of the Solidity programs that passed our heuristic check actually applied the recommended best-practice of testing the callstack directly.

Apparently this bug has caused the loss of thousands of $ worth of ETH already, and therefore seems like something that should receive special attention from the language, perhaps deprecating "send" and creating a new language construct for doing it.

@amiller

This comment has been minimized.

Show comment
Hide comment
@amiller

amiller Jun 16, 2016

This issue is a good idea. (The post authors are myself and @alexwzk)

Note that the underlying problem is also carried over from Serpent, and it's in part a consequence of peculiarities of the EVM itself. https://github.com/LeastAuthority/ethereum-analyses/blob/master/GasEcon.md#hazards

The Solidity compiler is a great place to put warnings. One option might be to make it an error to use send() but ignore the return code. Or to automatically bracket functions with a "callstack check".

Making send behave exactly like all the other method calls would at least remove the "education surface," and make it easier for programmers to have correct intuitions about its behavior.

It would be a also be a good idea to try to address the "reentrancy" hazard in the same way, perhaps by providing Solidity support for macros, but I guess that should be a different issue.

amiller commented Jun 16, 2016

This issue is a good idea. (The post authors are myself and @alexwzk)

Note that the underlying problem is also carried over from Serpent, and it's in part a consequence of peculiarities of the EVM itself. https://github.com/LeastAuthority/ethereum-analyses/blob/master/GasEcon.md#hazards

The Solidity compiler is a great place to put warnings. One option might be to make it an error to use send() but ignore the return code. Or to automatically bracket functions with a "callstack check".

Making send behave exactly like all the other method calls would at least remove the "education surface," and make it easier for programmers to have correct intuitions about its behavior.

It would be a also be a good idea to try to address the "reentrancy" hazard in the same way, perhaps by providing Solidity support for macros, but I guess that should be a different issue.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jun 16, 2016

Contributor

We are thinking about deprecating ether and replacing it by a token contract (and then issuing a warning for the use of send). The "reentrancy" hazard is present in every single interaction with other contracts. Programmers need to be aware that any unknown contract could be malicious - interacting with an unknown contract is like reading data from the network.

Contributor

chriseth commented Jun 16, 2016

We are thinking about deprecating ether and replacing it by a token contract (and then issuing a warning for the use of send). The "reentrancy" hazard is present in every single interaction with other contracts. Programmers need to be aware that any unknown contract could be malicious - interacting with an unknown contract is like reading data from the network.

@amiller

This comment has been minimized.

Show comment
Hide comment
@amiller

amiller Jun 16, 2016

+1, that's actually a really comprehensive way to prevent this.

Whenever possible, you should only interact with "unknown contracts" indirectly and asynchronously. Sending to a trustworthy external contract is a way to do that.

amiller commented Jun 16, 2016

+1, that's actually a really comprehensive way to prevent this.

Whenever possible, you should only interact with "unknown contracts" indirectly and asynchronously. Sending to a trustworthy external contract is a way to do that.

@taoeffect

This comment has been minimized.

Show comment
Hide comment
@taoeffect

taoeffect Jun 16, 2016

We are thinking about deprecating ether and replacing it by a token contract

Sorry I don't quite understand, could you elaborate on this?

taoeffect commented Jun 16, 2016

We are thinking about deprecating ether and replacing it by a token contract

Sorry I don't quite understand, could you elaborate on this?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jun 17, 2016

Contributor

We could create a token contract (that complies with the standard token API) where you can send ether and receive exactly 1 token per wei. In the same way you can always convert those tokens back into wei.

Transferring tokens and paying with those tokens will be done using the standard token API.

Recommending programmers to use that token contract instead of ether would also automatically open up your contracts to all other tokens that might exist, because they share the same API.

This is also in line with making Ether less special among all currencies/tokens in Ethereum.

Contributor

chriseth commented Jun 17, 2016

We could create a token contract (that complies with the standard token API) where you can send ether and receive exactly 1 token per wei. In the same way you can always convert those tokens back into wei.

Transferring tokens and paying with those tokens will be done using the standard token API.

Recommending programmers to use that token contract instead of ether would also automatically open up your contracts to all other tokens that might exist, because they share the same API.

This is also in line with making Ether less special among all currencies/tokens in Ethereum.

@taoeffect

This comment has been minimized.

Show comment
Hide comment
@taoeffect

taoeffect Jun 17, 2016

Cool, thanks for that, @chriseth! That clears things up!

taoeffect commented Jun 17, 2016

Cool, thanks for that, @chriseth! That clears things up!

@AFDudley

This comment has been minimized.

Show comment
Hide comment
@AFDudley

AFDudley Jun 21, 2016

The "reentrancy" hazard is present in every single interaction with other contracts. Programmers need to be aware that any unknown contract could be malicious - interacting with an unknown contract is like reading data from the network.

In a world of perfectly rational people this position is correct and reasonable.

But developers on the ethereum blockchain are not perfectly rational people.

You don't only need to empower competent developers you need discourage malicious and incompetent developers

Even in your above quote you're implying that merely "knowing" the contact will provide any smart contract developer with the knowledge of if the contract is malicious or not!

If it's trivial to detect malicious contracts, than it should be addressed in the compiler and run-time. if it's not trivial to detect malicious contracts, maybe the platform should be modified to make such detections trivial. If you believe such detection is impossible or intractable then you are conceding that your platform will never meet reasonable security assumptions and you should publicly advocate your beliefs.

AFDudley commented Jun 21, 2016

The "reentrancy" hazard is present in every single interaction with other contracts. Programmers need to be aware that any unknown contract could be malicious - interacting with an unknown contract is like reading data from the network.

In a world of perfectly rational people this position is correct and reasonable.

But developers on the ethereum blockchain are not perfectly rational people.

You don't only need to empower competent developers you need discourage malicious and incompetent developers

Even in your above quote you're implying that merely "knowing" the contact will provide any smart contract developer with the knowledge of if the contract is malicious or not!

If it's trivial to detect malicious contracts, than it should be addressed in the compiler and run-time. if it's not trivial to detect malicious contracts, maybe the platform should be modified to make such detections trivial. If you believe such detection is impossible or intractable then you are conceding that your platform will never meet reasonable security assumptions and you should publicly advocate your beliefs.

@gcolvin

This comment has been minimized.

Show comment
Hide comment
@gcolvin

gcolvin Jun 21, 2016

Contributor

It's non-trivial to detect malicious contracts, unless you can apply a specification for what is "malicious" to their bytecode, and the bytecode of all the contracts they call, and so on ...

Contributor

gcolvin commented Jun 21, 2016

It's non-trivial to detect malicious contracts, unless you can apply a specification for what is "malicious" to their bytecode, and the bytecode of all the contracts they call, and so on ...

@robmyers

This comment has been minimized.

Show comment
Hide comment
@robmyers

robmyers Jun 22, 2016

Wouldn't using tokens just introduce a layer of indirection, where re-entrancy attacks still apply just with tokens rather than Ether?

Would the language allowing send/call only at the end a public function and the compiler using static analysis to ensure that any path through the code from that function doesn't include any other sends/calls "good enough"?

Or is that naive. :-/

robmyers commented Jun 22, 2016

Wouldn't using tokens just introduce a layer of indirection, where re-entrancy attacks still apply just with tokens rather than Ether?

Would the language allowing send/call only at the end a public function and the compiler using static analysis to ensure that any path through the code from that function doesn't include any other sends/calls "good enough"?

Or is that naive. :-/

@taoeffect

This comment has been minimized.

Show comment
Hide comment
@taoeffect

taoeffect Jun 22, 2016

Wondering if either of these questions from #662 have relevance to this issue as well?

Q1: Removing recursion seems to me like a simple fix and a small price to pay for addressing this concern.

Q2: I'm no expert on the EVM, but I have this feeling that if we had a language that was based on message-passing (as opposed to function calling), that all of the concerns here might disappear. I.e. like in Erlang or (maybe) Smalltalk. Am I delusional or is there something there to that idea?

EDIT: copy/pasting addendum from #662:

I.e. in a message-passing model, these types of reentrancy bugs should be impossible, I believe.

There is just a queue of messages. That's it. No "function calls".

This is what makes Erlang so stable and is also the model that is used by "latest hotness" elegant & safe languages like Elm.

EDIT2: Also, see Clojure's Agents, a variant of the Actor approach.

taoeffect commented Jun 22, 2016

Wondering if either of these questions from #662 have relevance to this issue as well?

Q1: Removing recursion seems to me like a simple fix and a small price to pay for addressing this concern.

Q2: I'm no expert on the EVM, but I have this feeling that if we had a language that was based on message-passing (as opposed to function calling), that all of the concerns here might disappear. I.e. like in Erlang or (maybe) Smalltalk. Am I delusional or is there something there to that idea?

EDIT: copy/pasting addendum from #662:

I.e. in a message-passing model, these types of reentrancy bugs should be impossible, I believe.

There is just a queue of messages. That's it. No "function calls".

This is what makes Erlang so stable and is also the model that is used by "latest hotness" elegant & safe languages like Elm.

EDIT2: Also, see Clojure's Agents, a variant of the Actor approach.

@VoR0220

This comment has been minimized.

Show comment
Hide comment
@VoR0220

VoR0220 Jun 22, 2016

Contributor

@robmyers if the token is designed correctly, then no.

Contributor

VoR0220 commented Jun 22, 2016

@robmyers if the token is designed correctly, then no.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 4, 2016

@chriseth Is there a clear guide on how to send to other contracts while avoiding the re-entrancy problem?

ghost commented Jul 4, 2016

@chriseth Is there a clear guide on how to send to other contracts while avoiding the re-entrancy problem?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jul 4, 2016

Contributor

@Physes yes, the documentation recommends the withdraw pattern. If that is not feasible, first perform all checks, then all state changes and as a last step, send the ether.

Contributor

chriseth commented Jul 4, 2016

@Physes yes, the documentation recommends the withdraw pattern. If that is not feasible, first perform all checks, then all state changes and as a last step, send the ether.

@chriseth chriseth added this to the 99-nice-but-how milestone Aug 11, 2016

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 3, 2017

Contributor

This has been properly fixed inside the EVM and by a warning for unchecked send in the meantime.

Contributor

chriseth commented Mar 3, 2017

This has been properly fixed inside the EVM and by a warning for unchecked send in the meantime.

@chriseth chriseth closed this Mar 3, 2017

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