read only version of nonReentrant modifier

[thedavidmeister]

🧐 Motivation

Help slither understand read only reentrancy guards by implementing a modifier like nonReentrant that does NOT modify the state, and just checks _reentrancyGuardEntered.

Slither has a special case in it for analysing nonReentrant but gives false positives on cross function reentrancies where view functions read state that is modified after a reentrant call.

E.g. https://github.com/crytic/slither/issues/735#issuecomment-1620704314

If there was a modifier that was compatible with view functions then slither could understand this too.

📝 Details

Add a nonReentrantView modifier that is a thin wrapper around _reentrancyGuardEntered()

e.g.

modifier nonReentrantView() {
  if(_reentrancyGuardEntered()) {
    revert ReentrancyGuardReentrantCall();
  }
  _;
}

[Amxx]

Hello @thedavidmeister

Just to confirm I understand correctly, the motivation for adding this modifier is only to prevent slither raising false positives?

If that is the case, I would be strongly against adding this. It could create confusion for devs over which one should be used. This would contribute negativelly to security.

If its just about Slither raising false positives, then slither should be fixes and our code should not be modified.

[thedavidmeister]

@Amxx no, that's not the only reason, it's an example

read only reentrancy is a real issue, which OZ doesn't provide a modifier to solve, and slither is correct in complaining about it

if devs are confused and don't know which one to use, they're likely writing insecure code already because right now it's much more difficult to do correctly, you have to manually implement the snippet i posted basically

to be clear, the slither false positive comes into play only if you fix the issue manually and then slither still complains (because it doesn't understand the ad-hoc hand rolled solution). In the base case that a dev simply leaves a view function unguarded, and slither reports an issue, that's a real issue for the dev to fix somehow.

i.e. the baseline that OZ provides with modifiers alone is often insecure atm

here's more info

https://officercia.mirror.xyz/DBzFiDuxmDOTQEbfXhvLdK0DXVpKu1Nkurk0Cqk3QKc

the main issue with the current nonReentrant modifier is that it always modifies the state, there's no variant where it just checks the state, which make it incompatible with view functions

i'm suggesting a second modifier that checks the reentrancy state without attempting to modify it, so that getters don't report inconsistent values as part of a cross function reentrancy

then, in a hypothetical world where there's a standard modifier for people to fix this problem with, slither could check it to ensure that the issues it is reporting have been solved in a standard way (slither is already OZ aware for the existing nonReentrant modifier)

[ernestognw]

Given read-only reentrancy already has some successful exploits (totaling 220k in the Quickswap case alone) I'd credit the idea of having a nonReentrant modifier for view functions and if developers are already using a similar solution to this one, I think it's reasonable to add the change.

Nonetheless, I also agree with @Amxx concerns of people getting confused. It's true that a user might be already in trouble if they don't fully understand how reentrancy works, but I also don't want to give people reasons to throw the nonReentrant everywhere without a reason because in such case, it would also increase the runtime costs of external functions accessing view getters.

We would need some data to evaluate better if there's a significative impact of users using unnecessary nonReentrant modifiers

[thedavidmeister]

@ernestognw the snippet i posted is roughly the cost of one storage read and and a couple of jumps due to the internal function call and if

total cost would be ~200 gas.

slither already tells you if there's a cross function problem where you write to storage somewhere and it can be read reentrently elsewhere

e.g.

Foo.bar (src/Foo.sol#123) can be used in cross function reentrancies:

The existence of readily available FOSS tools that tell a dev if and where issues are seems out of scope for OZ though.

What we can say re: gas costs:

• A view only reentrancy modifier will always be cheaper than one that has to perform two writes in addition to the read
• A dev that puts unnecessary modifiers on their functions is already doing so in the write case, so it doesn't change much if they're also adding a few hundred gas to some read functions as well
• In the case where a dev doesn't put a guard on their view functions, we can expect the gas costs downstream to be far greater than what the modifier would have cost

E.g. from the linked article

Downstream consumers have to defensively try/catch some other reentrancy-guarded function (assuming an appropriate method even exists) before they know if the read is safe. This would incur the gas costs of an external call, the cost of a write reentrancy guard, whatever else that method does, and then error handling, on top of then reading the view function.

Somewhat tangentially, also note that OZ makes the reentrant status of the contract private which means it is unlikely that downstream consumers can directly check whether it is safe to read, other than the hacky "try to write and catch" approach.

[ernestognw]

You're right on the costs analysis, the order of magnitude of the cost is not exaggerated and these costs are already assumed by protocols protecting against read-only reentrancy. My concern is less about the total gas cost added and more about unintentional gas costs added.

I'd be in favor of adding the modifier but the reason I'm wary is because adding it doesn't fully solve problems such as defensive try/catch on guarded functions, which I would consider even more problematic in terms of gas costs as you suggest. I'm worried users won't understand these aspects and began throwing the protection everywhere.

Somewhat tangentially, also note that OZ makes the reentrant status of the contract private which means it is unlikely that downstream consumers can directly check whether it is safe to read, other than the hacky "try to write and catch" approach

At this point, I'll be more interested in solving the private initialize state issue, which also enables users to guard their own view functions. Note that there's a workaround for this, which is reading the value directly from storage and expose in a getter.

The way I see it is that it's very easy for experimented users to build the view modifier on their own (they're already doing it afaik) than expect nonadvanced users to know when to use the modifier, so I'm leaning towards just making the reentrant status public. If the read-only reentrancy landscape evolves in a way in which it's easy to make a mistake with your own modifier then we should add the modifier and maintain it accordingly.

[thedavidmeister]

@ernestognw

My concern is less about the total gas cost added and more about unintentional gas costs added. I'm worried users won't understand these aspects and began throwing the protection everywhere.

My initial reaction is... "so what if they do?"

People like being conservative by default, and then only removing protections if/when they can prove/show

• that it is safe to do so (often at great expense through proofs/tests/tools/time).
• the performance benefits justify the risk of being wrong about it being safe to do so

Throwing protection everywhere seems to be the basic philosophy of choosing to work with Solidity+OZ rather than yul/huff in the first place.

200 gas is comparable to or cheaper than the gas cost of the following "always on by default" safety features baked into Solidity and OZ already

• checking the index of an array exists
• checking a math operation doesn't overflow
• checking that tokens transfer/approve safely
• looping over newly allocated memory to zero it out
• padding the alignment of memory due to use of bytes
• converting between numeric types safety
• safely calling any external contract
• checking if a function is payable
• allocating and copying new memory always (e.g. when abi encoding) instead of mutating/sharing data in place or working directly with pointers

Deciding whether or not any given potential reentrancy is perfectly safe or unsafe is very difficult to do just by looking at the code. If it wasn't then I'm sure mature tooling like slither would have fixed all their reentrancy false positive/negatives long ago.

adding it doesn't fully solve problems such as defensive try/catch on guarded functions

ok, can we talk about this, because as i understand, the proposed modifier completely solves the need to do this

in what scenario does the view modifier NOT remove the need for an external consumer to defensively ping unrelated functions in the hope of spotting an error without unintended side effects?

The way I see it is that it's very easy for experimented users to build the view modifier on their own (they're already doing it afaik) than expect nonadvanced users to know when to use the modifier, so I'm leaning towards just making the reentrant status public.

The problem with this stance is that in defi it's the end users that need to convince themselves that something is safe to put their money into, not just the code author.

Having and adopting clear standard approaches to mitigating known risks lowers the bar for readers to understand whether something is safe or not.

There is value in being able to say "you don't even have to care about whether reeentrancy is an issue here" vs. "you have to read every other line of the contract to figure out whether this line is vulnerable".

[thedavidmeister]

i was thinking a bit more about why reentrancy is such a pain, and what is good about a modifier, other than just "it would be a standard"

basically if we think about what makes things hard to reason about in the first place, how much code you have to read and keep in your brain to understand any given line of code is a major concern

a rough spectrum would look like, given a line of code L, can i understand it:

• just by looking at L in isolation?
• by looking at the things in the same lexical scope?
• across logically related or even dynamic scopes?
• across unrelated scopes or even globally?

the thing that makes reentrancy issues nasty is that it falls at the bottom of the spectrum.

the thing that is nice about a reentrancy guard modifier is that it is at the top of the spectrum.

from the perspective of understanding if some code is secure, regardless of whether it is or not, the main benefit of the guard is that it converts proving a problem doesn't exist globally (difficult) to proving a solution does exist locally (easy).

[thedavidmeister]

https://www.coindesk.com/tech/2023/07/21/defi-protocol-conic-finance-hacked-for-1700-ether/

1700 ETH lost this week on conic to read only reentrancy

[eMarchenko]

For all teams that don't understand reentrancy deep enough I'd recommend slapping nonReentrant everywhere. I'd love these teams to utilize nonReentrantView as well, since otherwise they can't guarantee the correct behavior of their view functions. I.e., under reentrant condition some getters might return inadequate results. As @thedavidmeister has shown above, reentrancy is difficult to reason about, so we should provide a simple policy for devs that guarantees code security, even with some gas cost overhead. Besides, experienced devs can analyze their own code and decide if they need these modifiers.

[Amxx]

For all teams that don't understand reentrancy deep enough I'd recommend slapping nonReentrant everywhere.

Honestly, they should make the effort of learning and not just put a modifier everywhere.

[thedavidmeister]

@Amxx why? should everyone also learn how to write assembly to avoid spending gas on bounds checking arrays and looping over them to zero out values when they allocate them?

that's a much more gas intensive operation and easier to prove correctness for.

[thedavidmeister]

sorry, i just don't understand the resistance here for a simple solution to a complex and dangerous problem that frequently causes millions of dollars to go missing or put at risk, including experienced teams

the idea that this is just a problem for noobs doesn't square with observed reality

neither does the idea that OZ or even solidity itself, favours gas micro optimisations over "safe by default" tools

i mean... just look at what OZ does to allow for phantom overflow (in world where most use cases sit somewhere around 1e18 in a 256 bit number space) and dust rounding direction (which afaik wasn't even considered in OZ til 4626 vaults specified it, but now justifies several additional jumps):

there's so much in OZ and Solidity that is more gas, more niche, and less dangerous and easier to reason about than reentrancy guards, so I don't understand the idea that teams "should" do anything here other than use a cheap, standard modifier and be done with the problem

@Amxx i think in context "why?" is a reasonable response to "they should", could you give me a little more than a thumbs down so i can understand you better?

[Amxx]

@Amxx i think in context "why?" is a reasonable response to "they should", could you give me a little more than a thumbs down so i can understand you better?

That not really related to the question at end (adding a modifier specificly for view functions), so I did not want to go to far in that direction. But basically the discussion went:

• (you) less-knowledgable devs should use this modifier everywhere
• (me) they should start by learning what reentrancy is, and how to deal with it
  • my underlying point is that understanding the common issues is an essential part of being a smart contract developper, and just blindly using a modifier everywhere is not a good option IMO
• (you) "why? should everyone also learn how to write assembly to avoid spending gas"

IMO, comparing "learning what the common security issues are, and how to properly address them" with "gas goofing in assembly" makes no sens at all. I've been in stil space since 2017, and I can tell you that one is way more critical than the other. I'm honestly wondered that people seem to care about gas usage more than they care about learning and applying security good practices.

---

So going back to the initial discussion, my view is still:

• Reentrancy is a real issue that leads to many hacks
  • I still personnaly believe that this is the result of improper design, and invariant violations. In most cases using non-reentrant is the easy fix, not the right fix.
• While I don't like the non-reentrant construction, I understand it can be usefull in some cases
  • I still think in a majority of the case where its used its not needed, and it limits composability of contracts
• Adding a variant of the modifier that is read-only will serve a very small number of usecases
• Adding a variant of the modifier that is read-only will create confusion as to what the difference is, and which one should be used:
  • lets say someone uses the "non-view" for two functions, one of which is view, he will get an error. Instead of using two different modifier, he may be tempted to use the view version everywhere, because that doesn't give an error. This would bring zero protection to a user that thinks he is protected
• I VERY strongly think that recommanding users to use non-reentrant modifier everywhere is a terrible thing to do. I would strongly oppose any documentation change that goes in that direction.

---

Said otherwize, my issue with a read-only nonReentrant modifier is that I'm worried about the confusion is will create in users mind. I care about the discoverability and clarity of "what should be used when". If the recommandation is "slapping nonReentrant everywhere", then that is no good to me.

[Amxx]

I don't understand the idea that teams "should" do anything here other than use a cheap, standard modifier and be done with the problem

When you use nonReentrant, you block a lot of composability. You lose a lot of features. IMO you are not really fixing the problem. Maybe in some cases that is the only thing you can do, but in many cases you can do so much better. Its not just about cost, its about composable design.

If I was to make an analogy, I'd say its like learning you are allergic to peanuts in your childhood, and instead of trying to learn what is safe and what is unsafe to eat, and only avoid the things that are dangerous (or that you have a doubt about), you just decide to only eat rice and tomatos for the rest of your life... (its cheap and it works, right?)

[frangio]

I don't have the same opinion. I think reentrancy guards are useful and I don't mind when people use them as an additional measure. Sure a protocol is better without them, but a non-reentrant (less composable?) protocol is better than one that gets hacked. Learning about reentrancy doesn't make you immune to it, mistakes happen, and sometimes you want the extra assurance that the reentrancy guard gives you.

My only objection to reentrancy guards is that they can give false assurance unless you literally use the modifier on every function.

And my only concern about the modifier proposed here is the possibility that someone might use nonReentrantView on a non-view function. We don't have a way to prevent that from happening, we can only mitigate by giving the modifier a clear name.

Personally I would be fine with adding this modifier, with a very explicit name, and with documentation about the dangers.

[thedavidmeister]

@Amxx you misunderstand what i'm saying and so I think we're talking past each other.

> * less-knowledgable devs should use this modifier everywhere

I never brought up individual dev experience, and think it's a massive red herring and a waste of time to consider. Of course everyone should educate themselves on every issue to the best of their ability.

What I meant was that even if someone did use it everywhere, who cares? I didn't say "should" i said "we're not here to judge". The only harm they caused is potentially some vague "loss of composability" which may not even apply to their particular protocol, there's no way of knowing without looking at what they're actually building in context.

What I'm really talking about here is assurance vs. correctness.

Taking the peanut allergy...

Chefs in a commercial kitchen are making food for other people and yeah, in that case, most countries do have clear and conservative labelling laws, such as "may contain traces of nuts". Does that mean there are actually nuts in the food? no. It means the food production process is complex and despite best efforts, people make mistakes. If the kitchen as an org can't prove that everything coming out of the facility is nut free, they can't say it is, because somebody might die.

The best in class tools available to devs (as far as I know) cannot prove a contract is using the optimal combination of code patterns and guards, while still being safe. They throw up a lot of warnings like "go look at this" and can't actually understand guards, other than to do hacky things like hardcode a modifier name. So devs are in a position where even if they're convinced their code is fine, that code is handling large sums of other people's money in a very hostile and very permanent environment, and that's a valid reason to want to be extra careful, even if it might seem ridiculous at times.

I don't think it's productive to be passing judgement on devs who want to put guards on their code or not put guards on their code. Objectively there's cost/risk/benefit tradeoffs either way, and a guard is a useful tool in the toolkit. You lose something with guards and you gain something. The art of development is making enough of the right tradeoffs to provide something useful to people.

2 weeks ago i flagged the 1700 eth conic hack as an example of read only reentrancy causing severe damages, this week I'll flag the issues in vyper reentrancy guards that led to ~$50 million drained on curve alone last week.

Surely we consider vyper and curve devs experienced? Still, a faulty guard implementation coupled with insufficient test coverage wiped a lot of funds out. Having a rock solid reentrancy guard implementation is something useful. I'm sure a lot of devs would now feel less comfortable rolling their own modifiers, after what happened to vyper/curve.

> "why? should everyone also learn how to write assembly to avoid spending gas"

I'm saying that OZ is full of examples where comprehensive safety is prioritised over gas (which is an approach I agree with for a library like OZ).

In this thread for this issue, the gas cost was raised as a potential reason to avoid guards in case people overuse guards and cause contracts to be too expensive somehow.

The gas cost of a guard is comparable to the gas cost of checking that an array index exists in foo[i] syntax, it's negligible. Or at least would not be the first place I would go looking to optimise gas, out of all the places we pay an overhead for redundant safety rails it's low on the list.

The point I was actually making is that I don't see gas as a major reason to either adopt or not adopt reentrancy guards.

When you use nonReentrant, you block a lot of composability. You lose a lot of features. IMO you are not really fixing the problem. Maybe in some cases that is the only thing you can do, but in many cases you can do so much better. Its not just about cost, its about composable design.

Ok but...

OZ already provides a reentrancy guard. That decision has been made, to provide that tool in the toolkit. Given that it does exist, we should either make that tool the best possible version of itself, or decide to remove it.

The fact is that offering write-only reentrancy guards is an incomplete solution. Read only reentrancy is dangerous too, and there's no reason that a guard is any more or less valid for reads than writes.

Turning the question around, if OZ already had both read and write guards, would there be a reasonable case for removing read only guards and leaving write guards in the codebase?

The point about composability can be largely resolved by offering keyed guards like vyper does. That way you only block the specific related functions that are incompatible with each other, rather than having a global block on the whole contract. I feel like that's an out of scope topic for this issue though...

[thedavidmeister]

@frangio

My only objection to reentrancy guards is that they can give false assurance unless you literally use the modifier on every function.

And my only concern about the modifier proposed here is the possibility that someone might use nonReentrantView on a non-view function. We don't have a way to prevent that from happening, we can only mitigate by giving the modifier a clear name.

I feel like addressing both of these concerns is exactly the gap that tools like slither try to fill, and probably will never have a clean solution in OZ alone.

To me it makes sense that OZ is designed with slither-like tools in mind as much as it makes sense for slither to be OZ-aware.

Clear names and docs make sense to me, the more explicit it is the easier it should be for things downstream to detect it.