Create a onlyWallet guard to prevent reentrancy with lower guarantees

[kevincheng96]

SEE EDIT

This gas optimized reentrancy guard (nonReentrantOptimized) saves around ~5k+ gas compared to the approach of writing the re-entrancy guard to StateManager. We leave a note suggesting that it is not as safe as the version that writes to StateManager.

Regardless, this guard should be safe for most scripts since the modifier uses a storage slot addressed at a hash with an unknown string preimage. The guard is safe to use as long as scripts don't write directly to this storage slot in the same execution context.

I originally intended to add the guard to Ethcall.sol as well, but decided not to due to the extra 15k gas overhead. I also believe the Ethcall use-case is very different and it should really only be used to call into known and safe contracts. A note of caution is added to Ethcall to call out that it is not reentrancy safe.

EDIT: After some discussion, we decided to use the onlyWallet guard, which is a cheaper reentrancy guard with weaker guarantees. It protects against typical reentrancy paths that involve a call, but cannot protect against "recursive reentrancy" involving delegatecalls. That being said, reentrancy attacks rarely, if ever, involve delegatecalls and there are loads more to worry about when a script uses delegatecall.

[fluffywaffles]

curious -- what do we mean by "obscure the hash preimage"?

to clarify my understanding of preimage attacks: they are attacks where it is possible to either:

• find x given y such that h(x) = y; that is, guess the input given output y for the hash function h
• find x' != x such that h(x') = h(x); that is, construct a hash collision

these attacks usually exploit qualities of the hash function itself, that is, they would be based on inadequacies of this particular implementation of keccak256.

I don't quite follow how or why it would be that subtracting 1 from the hash obscures the preimage, unless we are assuming that the source code (and therefore the literal input, which is written in the source) is inaccessible.

In practice, in our case, the source is accessible -- quark will be open source.

While thinking about this problem, it occurs to me that since re-entrancy flags are ephemeral / transactional, we could actually use something like block.prevrandao to (at least in theory) nondeterministically obscure the hash preimage vary the hash, in such a way that even by looking at the source it would be very difficult to predict the hash even knowing the input. This non-constancy would protect the re-entrancy guard from a previously-run script setting (and never clearing) the storage slot.

An interesting added protection of adding in a "salt" from prevrandao is that a script cannot write to the storage slot a priori, and thereby grief a later script that depends on the slot for non-re-entrancy. With a constant slot, a script A that runs before a non-re-entrant script B could brick re-entrancy protections by doing sstore(slot, 1) -- script B will revert until the flag is manually cleared.

I guess the point, though, is that by using an internal constant the hash will get inlined at its use-sites and this may make it more difficult to find the hashed value, and even if you do, the input will be slightly obscured by the subtraction? I guess we're protecting the bytecode?

of course, if you just wanted to attack the storage slot, you could interpret the bytecode to the point where you can find the hash -- you don't need the input to write to the slot.

hmm. well, sorry for all the musings, but curious to hear what your thoughts are -- I am probably missing something?

[kevincheng96]

Discussed with 🧇 offline about this. I'm coming around to the idea that onlyWallet (e.g. require(msg.sender == address(this)) is the best reentrancy guard for Quark scripts. I'll type some of our discussions below:

Here's my original case for nonReentrantOptimized: It's basically a version of the nonReentrant modifier, but 5k gas cheaper and griefable only if the user signs a bad txn. The user has to sign a txn that purposely writes some value to the REENTRANCY_FLAG_SLOT, which btw is guaranteed not to clash by accident because it's slot is determined by the hash of a very a specific string. Quark cannot prevent users from signing bad txns, so we've always said we're not going to design Quark to protect against malicious txns signed by the user. So yes, this flag is griefable, but only if the user signs a bad txn; far worse things can happen if a bad txn is signed.

That being said, onlyWallet is an even simpler and cheaper check, with the main downside being that it doesn't protect against self-reentrancy attacks. This can happen in two ways:

1. Reentrancy via callback. This can done by a delegatecall from an exploitable script to a malicious contract that triggers a callback in the Quark wallet. This requires callbacks to be enabled by the exploitable script. Also, the delegatecall is unsafe in the first place, so a reentrancy guard would be rather pointless here
2. Reentrancy via circular functions in the script. If an exploitable script has multiple functions that can all be internally called from one another, it is possible to construct a reentrancy transaction in this case (e.g. function A triggers function B that triggers function A again). However, this is just seems like a dangerous (potentially malicious?) script. I guess it's worth noting that a normal reentrancy guard will prevent an exploit here.

The plan of action from here is to write more tests to fully understandable the attack vectors of the onlyWallet guard and make a decision after that.

[cwang25]

Agree with onlyWallet approach, since we can just limit the source to be from wallet directly while other defi contracts don't have control over the sender's address.

[kevincheng96]

We can consider adding the onlyWallet guard since it should be much cheaper.

[kevincheng96]

Actually, looks like adding the onlyWallet guard prevents the callback flow from using the guarded function again because that flow uses delegatecall instead of callcode. Ethcall is likely to be used in the callback flow, so we'll avoid adding the guard for now.

See commit 5fe6a2d for more in-depth documentation of this behavior.

[fluffywaffles]

some minor comments, but overall lgtm