Automatic reentrancy protection

[lukehutch]

Abstract

The majority of smart contract vulnerabilities that have been exploited, resulting in the loss of millions to hundreds of millions of dollars per vulnerability, have been reentrancy attacks.

The Checks-Effects-Interactions pattern mitigates or prevents these attacks from being successful.

Following this pattern requires conscious choice and vigilance on the part of the smart contract developer.

The Solidity compiler could automatically enforce this pattern to protect all contracts against reentrancy attacks.

Motivation

The DAO attack should be the only motivation needed.

Specification

There are two possible ways for the Solidity compiler to prevent reentrancy attacks automatically, which I will term the "aggressive" and "conservative" approaches.

Aggressive approach

The Solidity compiler adds code around call, delegatecall etc. as follows, incrementing a depth counter before the call and decrementing it again after the call, to signify that a function of another contract is being executed. Functions of the original contract that are not view or pure (i.e. that can modify state) have entry checks that revert unless the depth counter is zero.

Original source (unimportant code elided with [...]):

contract A {
    address internal addr;

    function one() external {
        addr.call(abi.encodeWithSignature( [...] ));
    }

    function two() external {
        // Do something with side effects
    }

    function three() external view returns (uint256) {
        // Read something without causing side effects
        return [...];
    }
}

Compiler-protected code compiled as if written as follows:

contract A {
    address internal addr;

    uint256 private _callDepth;

    function one() external {
        ++callDepth;
        addr.call(abi.encodeWithSignature( [...] ));
        --callDepth;
    }

    function two() external {
        require(callDepth == 0, "Reentrant call");
        // Do something with side effects
    }

    function three() external view returns (uint256) {
        // Read something without causing side effects
        return [...];
    }
}

There is still a small possibility of triggering a vulnerability in other contracts with this pattern, because a view function may still present an out-of-date view of the internal state of contract A to some other contract B, causing B to be tricked into taking an action that it should not take, but contract A itself would be protected. To prevent even contract B from being tricked in this way, an even more aggressive approach could be taken of adding require(callDepth == 0, "Reentrant call") to even view functions (such as three() in this example). This would disallow all reentrancy, except for in the case of pure functions, which by definition can't rely on internal state. This would limit the power of Solidity, but it would also prevent these attacks.

Conservative approach

A more conservative approach to automated reentrancy protection would be to find the point within the code of each function at which the last possible modification of contract state occurs, where modification of state is defined as:

1. writing to storage
2. overwriting a local variable in memory with a new value
3. calling a function within the contract that is neither view nor pure.

The compiler then statically enforces that functions in other contracts are only callable after the last state update in any function.

The compiler must ensure that this is enforced regardless of the call stack, so if a function f updates states and then makes a call to another contract, f can only be called by other functions if the call to f is the last potentially state-modifying operation in any other function in the contract (so that it cannot be followed by another state update).

Backwards Compatibility

The aggressive approach may break some Solidity usage patterns involving the interaction of multiple contracts, but those usage patterns are potentially dangerous anyway. The main downside to this approach is that failures will only be seen at runtime. Also, this approach will add some overhead in gas and code size.

The conservative approach is significantly better, because all potential issues will be able to be caught at compiletime, forcing the programmer to notice and fix problematic effect ordering issues. Also there will be no overhead to the generated code. The main downside to the conservative approach is that the changes to the compiler will be more complex.

[leonardoalt]

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.

[leonardoalt]

This issue is a dup of #12996 though, so let's keep the discussion there.