Openzeppelin-contracts: Remove reentrancy guard contract.

[MicahZoltu]

https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/ReentrancyGuard.sol
It is _way_ too easy to incorrectly use the reentrancy guard contract and end up deadlocking your contract. Recommend removing it from your collection of recommended contracts, even with its caveats listed. In general, reentrancy guards via modifiers while a good idea on the surface introduce an easy way for a contract author to shoot themselves in the foot. I think it would be best to instead provide users with education on patterns/practices to avoid reentrancy problems then to encourage them to _depend_ on modifier based reentrancy protection.

[rstormsf]

hmmm.... IMHO, I don't see anything wrong with this protection. Maybe there will be a case when you need to do some work before you update the state. That's where you would use this modifier. Correct me if I'm wrong

[MicahZoltu]

The problem isn't with the protection of a mutex. The problem is that this _mechanism_ of applying a mutex comes with some significant risks that are easy to miss and can lead to a complete deadlock of your contract(s).

contract Foo {
    bool private lock = false;
    modifier noReentry() {
        require(lock == false);
        lock = true;
        _;
        lock = false;
    }
    function apple() public noReentry returns(uint256 output) {
        output = 5;
        ...
        if (veryRareCondition) {
            output = 3;
            return
        }
        ...
        output = 7;
    }
}

The above contract will deadlock (become permanently unusable) if veryRareCondition ever evaluates to true. Unfortunately, rare conditions like this can end up as unexercised code paths that make it to release, especially in a large project.

A perhaps more subtle and hard to miss problem would be:

contract Foo {
    bool private lock = false;
    modifier noReentry() {
        require(lock == false);
        lock = true;
        _;
        lock = false;
    }
    modifier optionalExecution() {
        if (condition) {
            return;
        }
        _;
    }
    function apple() public noReentry optionalExecution returns(uint256 output) {
        output = 5;
    }
}

In this example it is easier to miss that noReentry is incompatible with optionalExecution.

Both of these scenarios _can_ be avoided with diligence, but I think it would be better to not encourage patterns that enable people to shoot themselves in the foot so easily. In general, locks/mutexes in programming are hard and humans are really bad at not writing deadlocks in large applications where locks are used. Even the best engineers who fully grok the best practices for their usage still sometimes mess up and create a deadlock. While the idea of a mutex can be useful, I think that without tooling to give strong guarantees that the user isn't doing the wrong thing (e.g., a static analyzer that ensures no function that returns is modified) this modifier makes it too easy for the unsuspecting engineer to screw up really badly.

[frangio]

Thanks for reporting, @MicahZoltu!

As I see it, there's two different issues here: 1) a bug report, which is that using the nonReentrant modifier can lead to deadlock, and 2) a suggestion to remove the feature altogether.

Maybe the bug can be fixed and there's no reason to remove the feature. I haven't been able to come up with a solution but I'll let others think it through. My first idea was to use the block number instead of a boolean, but that won't work if there's more than one usage in the same block. There's no way to identify that two calls happen in the same transaction that I can think of.

[spalladino]

@MicahZoltu as far as I understand, the "end" of a modifier is always executed (unless the function throws), regardless of whether the function returns or not. Actually there is a very similar example in the Solidity docs:

contract Mutex {
    bool locked;
    modifier noReentrancy() {
        require(!locked);
        locked = true;
        _;
        locked = false;
    }

    /// This function is protected by a mutex, which means that
    /// reentrant calls from within msg.sender.call cannot call f again.
    /// The `return 7` statement assigns 7 to the return value but still
    /// executes the statement `locked = false` in the modifier.
    function f() noReentrancy returns (uint) {
        require(msg.sender.call());
        return 7;
    }
}

I haven't checked this behaviour yet though. But if the end is indeed executed, I think this issue could be closed, unless I'm not seeing something. WDYT?

[frangio]

You're correct @spalladino. From the docs:

Explicit returns from a modifier or function body only leave the current modifier or function body. Return variables are assigned and control flow continues after the “_” in the preceding modifier.

[Ivshti]

We have to keep in mind, before solidity 0.4.x (or so) there was a caveat where if a function returns, the rest of the modifier code would not be executed, because the "_" in the modifier acted as a pre-processor replacement rather than a function call.

I think this is resolved in solidity now and therefore should not be a problem.

Please correct me if I'm wrong

[frangio]

That's right, @Ivshti. From the changelog in version 0.4.0:

Modifiers: return does not skip part in modifier after _.

Since our contracts require Solidity version at least 0.4.11, it's not a problem for us.

[MicahZoltu]

Ah! Great find! I'm happy to hear this because I actually do use a Mutex and would _like_ to move it to a modifier. :)

[rstormsf]

sounds like the issue could be closed