Photo by Adli Wahid on Unsplash

Constantinople hard fork enabled new kind of reentrancy attack. However, this attack does not even violate Checks-Effects-Interactions pattern, which is considered to protect from reentrancy. Neither it does fit the traditional definition of reentrancy (some of them, there are more than one). In this article, I rethink what reentrancy actually is and what measures should be taken to protect code against it.

What happened

Ethereum was preparing for Constantinople hard fork when ChainSecurity team found a possibly critical issue in EIP-1283. With this optimization enabled transfer function provides just enough gas to change storage. Until now it was often considered effect-free, though that was never explicitly guaranteed. So, there can be live contracts that will become vulnerable to reentrancy attack after hardfork, even if they were perfectly safe at the time of deployment. Actually, at least one such contract has been found. Fortunately, it doesn’t hold any valuable assets.

What was considered reentrancy

What was interesting in this story and mostly went unnoticed is that the actual example used by ChainSecurity is not a “traditional” reentrancy issue. It has sly differences from the (in)famous DAO attack in the way how security assumptions of the contract are violated.

But what is actually considered reentrancy today?

We’ve checked multiple resources about Solidity smart contracts security, including ConsenSys best practices, DASP Top 10, SWC Registry, and the list of known attacks by Sigma Prime.

On the whole, one is recommended to check every external call in the contract’s function and see if it can somehow change the state of the contract and thus break the function’s logic. As a precaution one can finish all “internal work” before making external calls or use reentrancy guard pattern.

We’ve also checked different security tools to see what they detect as possible reentrancy (Mythil, Slither, Securify, Manticore). They mostly look for external calls followed by storage changes and recommend following CEI pattern. Some tools only consider external calls with a non-zero value (DAO-style attacks).

So effectively most developers have little help other than “don’t write to storage after external calls”.

Improving code quality and why this definition is obsolete

So, reentrancy is either defined too wide to be practical or is considered to be a subset of CEI pattern violations. In many cases, developers just follow CEI pattern and assume that the code is secure against reentrancy attack.

However, ChainSecurity’s example does not violate CEI pattern, nonetheless, the contract is vulnerable.

I believe that reentrancy attack is a part of a wider class of issues that arise from the incorrect assumption

“this external call does not introduce any side effect on my contract, so my function’s execution will continue as expected”.

Check this example:

Splitter contract is vulnerable to reentrancy attack. It allows anyone to deposit some ether and later send it to two addresses according to custom share . Unfortunately, Attacker can modify this share setting on-the-fly, thus actually withdrawing his deposit twice.

The simplest fix would be to precalculate both sent values before first transfer so Attacker cannot influence “interaction” part of function’s body. Surprisingly, each of the two transfers is both “interaction” (execution goes to another contract) and “effect” (ether is transferred) in terms of CEI pattern. That makes me ask what else can be considered an “effect”.

Let me slightly modify the first example. So now EtherOrDollars contract allows a user to withdraw funds in form of either ether or US dollars based on their preferences. To withdraw dollars the contract emits special event( WithdrawDollars ) so service can send fiat to the user:

In this case, the emitted event was the “effect“ in terms of CEI. On the whole, almost anything can be an “effect”, e.g. token transfer (which is actually another external call). So, check everything that goes after an external call carefully.

Also, in this example, we read eth[msg.sender] twice assuming it won’t change between the calls. Obviously, this is a bad code style, which lead to vulnerability. As a rule of thumb I recommend reading state variables before the first external call.

Conclusion

We should stop thinking about reentrancy as “sstore after external call”. Try to use the wide definition of reentrancy (until we have better one).

For now, some recommendations:

Remember that a wide variety of effects might be critical for the contract: writing to storage, sending ether, emitting events, consuming the wrong amount of gas.

Don’t assume that transfer or send are effect-free, treat them as “interaction” as well.

or are effect-free, treat them as “interaction” as well. Use reentrancy guard. It is simple, but do not mess up.

Follow CEI pattern, but read all the required data (storage variables, address balance, data from other contracts) to local variables before calling external functions (and make sure any called function controlled by you get required data via arguments rather than getters).

This article was prepared with the help of Alexander Drygin, Igor Sobolev, Ivan Ivanitskiy, Kirill Gorshkov, and Pavel Kondr.