Language feature: disallow state-changing effects after an external call by default

[pcaversaccio]

Abstract

Generally disallowing state-changing effects after an external function call and enabling the possibility to mark functions that specifically do this.

Motivation

I started this discussion on Twitter after another reentrancy attack (Cc: @chriseth). Reentrancy attacks are ubiquitous and even though there are toolings available (e.g. Slither) that conduct a static analysis it requires an initial setup as well as a proper understanding of how to interpret the results. In order to make the Solidity development more secure and sustainable I feel it's time to finally introduce such a language feature that disallows state-changing effects after an external function call and mark functions that specifically do this.

Specification

All state-changing effects after an external function call are disallowed. If you want to allow however such a possibility, we introduce a new modifier unprotected whose default value is false.

Backwards Compatibility

The code that previously compiled fine will not compile anymore if there is a state-change effect after an external call and the new modifier unprotected is unknown, so it's a breaking change.

[chriseth]

Just a comment on the name of the modifier: I don't like opinionated words like "safe" or "protected" because they give a false impression of safety or protection, we should find a more neutral one.

Also more generally: Also wrt. free functions, it might be a good idea to explicitly pass/provide something like a "context" as a parameter to functions that allows external calls (or maybe even state changes without external calls). In this case, we could have a special property on this context that allows state changes after an external call. By default, the context would transform into a "constant state" context after an external call - there are the linear types again.

Also something to consider more specifically to this issue: Is it OK to do another external call after the first external call? What about a delegatecall? Even if we do not use delegatecall, how do we distinguish a "data contract" that is associated to the current contract from a potentially malicious external contract?

After all these questions, I'm more and more inclined that this should rather be "off-loaded" to user code and the compiler should instead provide the required features for the type system.

[pcaversaccio]

You are right about the name of the modifier - after thinking through it today maybe we even do not need a modifier but can use the unchecked block syntax directly. I think in a broader sense this language feature is consistent with the checked arithmetic introduced in solc version 0.8.0 since it also wants to prevent unwanted behaviour that could be exploited (one of the major reasons behind introducing libraries such as SafeMath were smart contract exploits based on overflow/underflow vulnerabilities).

There are three types of reentrancy:

• Single Function Reentrancy
• Cross-Function Reentrancy
• Cross-Contract Reentrancy

For the cross-contract reentrancy, this one can happen when a state from one contract is used in another contract, but that state is not fully updated before getting called.

The conditions required for the cross-contract reentrancy to be possible are as follows:

• The execution flow can be controlled by the attacker to manipulate the contract state.
• The value of the state in the contract is shared or used in another contract.

The best outcome for this language feature would be that one could write the contracts without bothering about these three types by default. Of course, we can always argue to "offload" all the responsibility to the developers but past experience and events showcase blatantly that this does not work as intended.

Coming back to your context idea - I really like it but couldn't we simply achieve that via the unchecked block syntax. I.e. an unchecked block does not only not control for an unrestricted integer that falls outside the range of the result type but also allows for a "non-constant state". By default, however, the context is a "constant state".

Also something to consider more specifically to this issue: Is it OK to do another external call after the first external call? What about a delegatecall? Even if we do not use delegatecall, how do we distinguish a "data contract" that is associated to the current contract from a potentially malicious external contract?

I'm not yet sure what would be the best design here tbh.

[pcaversaccio]

I would like to quickly add some further thoughts behind this language feature: The main question is whether we should build the language in a via negativa or via positiva way. I strongly tend based on the experience and events over the last 6 years to the first one. If you look at all the exploits that happened over the last 6 years (including DAO) we haven't got really smarter w.r.t. to reentrancy attacks as it seems (I also blame a little bit the auditors who push for the low-level calls instead e.g. transfer). Too many devs are still not paranoid enough! And Solidity already started implementing such safety features via unchecked for over/underflows. It's definitely a tradeoff here and my reasoning is practice-driven.

[pcaversaccio]

@cameel @hrkrshnn what are your opinions on my thoughts here?

[leonardoalt]

I think this shouldn't go into the language with safe/protected keywords and such, and should rather have similar behavior as checked arithmetic.

IMO, reentrancy guards could be added by default for every external call, and bypassed if the call is in an unchecked block. This also follows Rust's unsafe style which seems popular.

[leonardoalt]

#13124 is a dup of this issue in general but offers two different approaches:

• Aggressive: basically what I said above
• Conservative: try to detect whether state changes are performed after external calls, and if yes add the reentrancy guards.

Copying what I said in the other issue:

You could combine both approaches, eg have the aggressive approach by default which can be bypassed by the user if the external call is in an unchecked block, and the compiler can also choose to optimize it away if it detects the conditions in the conservative approach.

[lukehutch]

Standard reachability analysis should be able to determine quite easily whether any given line can modify state or call a function in another contract. Then Solidity just needs to ensure that every function has a partition between state modification first, and external calls second.

If you want the user to have to explicitly signify whether a function updates state, you already have that: non-pure/view. If you want the user to have to explicitly signify which functions can call other contracts, you could add a caller modifier or something. Then you could not call a state modifier function or modify state after a caller function or an address.call within any function -- and any function that calls a caller function or calls address.call also itself becomes a caller function.

[leonardoalt]

I personally really dislike the idea of function modifiers for reentrancy. It's too binary and does not represent the amount of different use cases. Moreover:

• the strategy you mention first can yield false positives for safe cases that do not conform exactly to this pattern
• I understand your second paragraph but it sounds really confusing. Imagine documenting/implementing this, it's gonna be full of edge cases and potentially make the analysis more dangerous/confusing.

Edit:

• I'm not against the reachability analysis. I think it is useful as an optimization for cases that are safe for sure.
• I just think the simplest would be to add a guard to every call by default with the possibility of removing it. No extra keyword, no complicated analysis besides the optimization above, no edge cases. Plus it conforms to a standard that the language already has.

[lukehutch]

What I proposed is already basically what Slither does (minus the modifiers), but Solidity should give these warnings rather than Slither, because this is such a serious source of security vulnerabilities.

Yes, in the general case precise reachability analysis is uncomputable, unless you use conservative logic that looks to see whether something might be reachable given some specific runtime control flow (then reachability analysis becomes simple and computable).

Re. the modifiers, there's a reason why Solidity made visibility modifiers and mutation modifiers required a few versions ago. Being explicit establishes a contract between the programmer and the compiler. And what I explained in my 2nd paragraph is exactly what Solidity already does (albeit with inverted logic) with mutation modifiers for pure/view functions. So basically the machinery is all already there in the compiler to implement this simply.

[lukehutch]

It will be much easier to implement robust automatic runtime protections against reentrancy attacks at runtime once EIP-1153 is merged, because it will allow for flags to be set whose lifecycle is bound to a transaction.

However, I believe the vast majority of reentrancy attack vulnerabilities can be detected and prevented through static analysis.

[leonardoalt]

It will be much easier to implement robust automatic runtime protections against reentrancy attacks at runtime once EIP-1153 is merged, because it will allow for flags to be set whose lifecycle is bound to a transaction.

Are you confident that's gonna go in? I personally am not.

However, I believe the vast majority of reentrancy attack vulnerabilities can be detected and prevented through static analysis.

Agree that potential reentrancy can be detected with syntactic static analysis. But what about false positives?

I think there are multiple ideas here that are complementary and provide different levels of safety and user experience. We need to get more people into this discussion, not only from the team. We also need to gather actual data on how many / what type of contracts would benefit from each proposed solution, and how many would be made worse / more annoying.