Add default re-entry protection with opt-out keyword.

[MicahZoltu]

As most are now aware, the EVM is susceptible to reentry attacks if the developer is not very careful to avoid them. The fundamental problem is that people don't realize that when they call an external contract any amount of code may run before that returns, even when the call is something simple like a send.

To protect against this, Solidity should automatically put re-entry guards around any functions that have an external contract call in them. The simplest (most naive) re-entry guard would look something like this:

contract Foo {
    bool inBar = false;
    function bar(someAddress) {
        if (inBar) throw;
        inBar = true;
        someAddress.send();
        inBar = false;
    }
}

This code could be automatically added to a contract that looked like this:

contract Foo {
    function bar(someAddress) {
        someAddress.send();
    }
}

The side effect of this of course would be that the function will cost an additional ~10,000 gas (20,000 for the store true and -10,000 for the false). For savvy developers who need to allow for reentry or who want to save the 5000 gas and believe they have audited their code a keyword could be added to allow bypassing of this auto-generated code like: function reenterable bar(someAddress).

Also, this re-entry guard should only be applied to functions that actually make an external call, if they don't then there is no reason for the extra gas cost.

The primary advantage of something like this is to make code safe from reentry attacks by default, and require users to go read some documentation to figure out how to reduce their gas costs, which would at least ensure they are aware of the issue. It wall also make auditing code easier since any function that doesn't have reenterable on it can be known to be safe against reentry attacks.

[pruby]

A group of us discussed this in the solidity gitter account - can read about it there. I would strongly support this notion of "safe by default", even if it costs a bit by default as well. Those who are confident they have applied operations in a safe order could avoid this cost.

An example of how to enforce this would be:

PUSH (unique code)
SLOAD
PUSH (error address)
JUMPI
PUSH (1)
PUSH (unique code)
SSTORE
 ... function body ...
PUSH (0)
PUSH (unique code)
SSTORE.

This would give a total overhead of 20,072 on entry a cost of 6 (EDIT: wrong) when the postamble runs, and a refund of 15,000 when the transaction returns, for a total cost to the sender of 5,078 gas. Switching a storage register between two non-zero values would have lower peak cost, but much higher net cost (10,084 by my count).

EDIT: duplicating entries rather than repeatedly pushing that unique code might reduce bytecode length at the cost of a few extra instructions.

EDIT 2: looks like I misread the docs on this one. Cost of a store looks like it's paid even if you're clearing, an extra 5,000 gas. Better option would be:

PUSH (unique code)
SLOAD
PUSH (-1)
EQ
PUSH (error address)
JUMPI
PUSH (1)
PUSH (unique code)
SSTORE
 ... function body ...
PUSH (-1)
PUSH (unique code)
SSTORE.

With a net cost for most calls as above of 10,084. A slight trap is that this would cost 25,084 on first invocation or require a 20,006 cost fix in the constructor, making gas measurements somewhat unreliable, which may be undesirable.

[holiman]

I think this safeguard is not needed for someAddress.send(1), since it will only use the default gas. It should only be used for when non-default gas is used in external call. So for example someAddress.bazonk(). On bytecode-level, the information to make this distinction is lost, but at Solidity-level, external calls are already treated specially[1] and this could be a sensible addition.

[1] someAddress.bazonk() will check return value, and cause invalid jump on VM failure (could be caused by OOG, stack-depth, illegal jump etc)

[MicahZoltu]

Talking with @joeykrug about this and he mentioned that there is another class of re-entrant bugs that can occur across functions. For example, if there are two functions that both check-act-zero some piece of data then you could re-enter into another one.

function foo() {
    if (balance <= 0) return;
    withdrawBalance(balance);
    balance = 0;
}

function bar(otherAccount) {
    if (balance <= 0) return;
    transferBalance(balance, otherAccount);
    balance = 0;
}

Unfortunately, this is harder to protect against without increasing the gas cost of every function in the class because the reentered function may not actually call out, it could be a simple function that only mutates internal state and then returns.

[chriseth]

I think reentrancy protection provides the false impression of safety. This is comparable to mutexes that make you think that it solves concurrency problems until you notice the concept of a deadlock, dependencies between different objects and other things.

Having said that, I think it should be possible to add this feature using modifiers, modifier areas and inheritance. If those are not enough, I'm fine with making modifications.

[pruby]

True - this wouldn't have stopped the DAO attack, just made it less efficient (they could have transferred the tokens after only one splitDAO call). To be effective, we would need to set a flag that was checked before every non-constant external function call, so that we could never re-enter in to the same contract twice in the call stack. Any function declaring itself re-entrant would be expected to be valid when called in combination with any other function, but non re-entrant ones would not have to be compatible with each other.

chriseth: While you can certainly add a re-entry guard with modifiers, I think what we're recommending here is that it should be a default as it's the naive coders who most need protection.

[joeykrug]

Another problem is it can reenter to a different contract that interacts with the other contract causing an issue. A good example would be if thedao used externs to deal with sending dao tokens. Then protecting re-entry wouldn't work

[taoeffect]

Please feel free to tell me that I should be posting this question someplace else (sorry for not knowing where it should go).

Regarding the reentrancy related issues, I've been having a convo with @pipermerriam on reddit who seems to suggest that they can all be fixed by removing recursion from the language.

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?

Thanks so much for any help figuring this out!

[taoeffect]

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.

[pipermerriam]

I do not recall saying what you are reporting me to have said. Can you please provide a link?

[taoeffect]

@pipermerriam Yeah I'd linked to this, but I think I just misunderstood what you were saying about recursion, my bad.

Regarding the message-passing/Actor model, note that there is also Clojure Agent's approach, a more efficient, possibly better variant of the Actor model.

[lightuponlight]

I believe that making recursion optional for contracts and/or methods would be a good approach.

Many (most?) contracts should not be called recursively, and an exception thrown. But there are times when a contract is designed to be be called recursively. The EVM and languages should allow developers to distinguish which methods can acceptably be called recursively and which cannot.

The definition of recursion would be, multiple instances of the same contract method on the call stack.

[pipermerriam]

I haven't tested this code but in theory this provides re-entrance protection.

contract EnterOnce {
        mapping (bytes4 => bool) is_occupied;

        modifier no_recursion {
            if (is_occupied[msg.sig]) throw;
            is_occupied[msg.sig] = true;
            _
            is_occupied[msg.sig] = false;
        }
}

I'm curious if anyone has thoughts or opinions on this pattern.

[holiman]

Well, that one doesn't work well with returns in function body. Actually, incompatible with any kind of returnvalue

[chriseth]

Yes, I am actually inclined towards changing the behaviour there, i.e. a return in the function body or in a modifier further "invards" will just be like a "break" towards the next modifier: https://github.com/ethereum/solidity/issues/686

I think there should be no contract relying on the previous behaviour because it was useless anyway.

[phiferd]

Hey, I was just about to file an issue for this, when I found one was already open.

@chriseth : I understand your concern about a false sense of security, and I believe that's a real effect, but I think the benefit outweighs the concern. After all, you could make a similar argument for almost any security feature (and people have done so for helmets, seat belts, airbags, anti-lock breaks, ...).

Also, I'm not sure how exactly the gas calculation works, but I wonder if certain operations that are designed to ensure safety should be gas free (might not be possible). Everyone who is involved in Solidity/Ethereum has an interest in making is as secure as possible. Hacks like the DAO are existential threats to Ethereum and crypto currency in general. Charging gas for writing secure code provides an incentive (albeit small) to avoid safe code when you convince yourself it's not needed.

[redsquirrel]

I wonder if certain operations that are designed to ensure safety should be gas free

That's a great idea.

[phiferd]

@chriseth: is the right place to bring up gas-free operations here: https://github.com/ethereum/evm2.0-design/issues ?

[VoR0220]

After having multiple discussions about this, I think this would be better handled at the EVM level, and that introducing this has the ability to break numerous contracts whenever multiple people interact with it.

[phiferd]

Since backward compatibility seems likely to be a problem, can we re-purpose this issue to an "opt-in" feature? Even if the EVM handles this, there will still need to be some new opt-in keyword, right?

@VoR0220: Can you explain what you mean by "whenever multiple people interact with it"? I thought the issue was not related to concurrent transactions, but instead when a call to an external contract calls back into the calling contract. Therefore, this could be an issue with a single user executing a single transaction, I think.

[chriseth]

I think default reentrancy protection is a weird programming model. Everything else can be achieved by the modifiers in version 0.4.0.

[AFDudley]

@chriseth Sorry, I think I'm misunderstanding you. What's weird about the default NOT favoring adversaries? Or put another way, we should expect programing in the EVM to be weird because functions can't trust callers or callees?

Again, apologies, I am simply trying to understand the design philosophy that underlies this decision, if you've written about this elsewhere, i'll happily accept a link.