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

[fkirc]

As a newcomer to Solidity, I am curious whether such reentrancy-guards are still a recommended pattern.
According to https://ethereum.stackexchange.com/questions/19333/reentrancy-flag-without-sstore, reentrancy-guards might consume quite a lot of gas.
I would be grateful if somebody could point me to a solution that works without any additional storage.

[frangio]

@fkirc Check out our article Reentrancy After Istanbul to learn about other techniques to protect against reentrancy.

I should note that ReentrancyGuard uses storage in a way that makes it cheaper than most other storage uses (e.g. balance counters), because the guard slot is reset to its original value at the end of a transaction, and it is always at a non-zero value. Both of those conditions imply significantly cheaper costs to writing to storage.

The resulting overhead may not be significant relative to the inherent runtime cost of your contract, so I'd encourage you to measure the costs and make a decision based on that. (However, there are other downsides to reentrancy guards which I mention in the article shared above.)

[fkirc]

Thank you for the summary.
Based on your article, it seems to me that reentrancy-guards are still the most general solution.
The "checks-effect-pattern" is a good pattern, but it requires more care than just one additional modifier per function.
And "pull-payments" is rather an app-specifc pattern in my view.

[MicahZoltu]

I'm personally a fan of untrusted external calls always being tail called. This is similar to the check-effect pattern, but a bit more extreme and (IMO) a bit easier to enforce codebase wide without too much thought. Just come up with a naming convention for any function that tail calls into an external contract, and then also name any function that calls that with the same naming convention.

[fkirc]

I agree that untrusted tailcalls are a good pattern.
But even then, untrusted tailcalls might still be exploitable via reentrancy-attacks.
Let's take a look at the following deposit-function that mints pool-tokens when depositing ETH into a contract (think of a simplified liquidity pool):

function deposit() external payable {
        uint256 depositValue = msg.value;
        require(depositValue >= 1, "EMPTY_DEPOSIT");

        uint256 newPoolValue = address(this).balance;
        require(newPoolValue >= depositValue, "POOL_TOO_LOW");
        uint256 oldPoolValue = newPoolValue - depositValue;

        uint256 oldSupply = totalSupplyOfPoolToken();
        uint256 newSupply;
        if (oldPoolValue == 0) {
            newSupply = depositValue;
        } else {
            newSupply = (depositValue * oldSupply) / oldPoolValue;
        }

        require(newSupply >= 1, "DEPOSIT_TOO_LOW");
        mintPoolToken(msg.sender, newSupply); // This might be an untrusted tailcall
    }

If an attacker recurses into this function via the untrusted tailcall in the last line, then the attacker might obtain an excessive amount of pool-tokens for a small amount of ETH.
This could allow to steal almost all the ETH in the pool via a separate withdraw-function (not listed here).

[MicahZoltu]

@fkirc Can you help me understand why that contract is problematic?

[MicahZoltu]

I'm still not seeing it. Perhaps to phrase things differently, how would the caller re-entering on the last line be any different from the caller just calling deposit() twice in a row either as part of a contract or as two transactions?

[MicahZoltu]

You would need to send more ETH on the reentry in order to deposit again, which means the deposit contract will work as expected.

[fkirc]

Thanks for your hints, let's forget about the reentrancy-vulnerability. As you said, the deposit contract will work as expected.