VIP: disable re-entrancy by default

[charles-cooper]

Simple Summary

per title

Motivation

once EIP-1153 is enabled, the cost of disabling re-entrancy into a contract before the selector table is even traversed comes down to two TSTOREs and one TLOAD, so approximately 300 gas, which is a small overhead compared to even the cost of a warm CALL, 200 gas. re-entrancy attacks are one of the biggest sources of vulnerabilities in smart contracts (todo: gather some hard stats on this), and it makes sense from a safety perspective to disable them by default. in the future, i also expect a future EIP will bring the cost of TLOAD/TSTORE down in the general case.

Specification

there are multiple possibilities here:

1. introduce a LOCK keyword, which creates critical section/non-reentrant blocks. ex.

   LOCK:
       some_contract.foo() # re-entrancy back into this contract unavailable here

2. disallow re-entrancy entirely (allow re-entrancy at call site by a new syntax), ex.

   some_contract.foo() # re-entrancy back into this contract unavailable here
   some_contract.foo(..., reeentrant_functions=(func1, func2)) # func1 and func2 can be re-entered into
   some_contract.foo(..., reentrant_functions=*) # all functions can be re-entered into

EDIT: it seems the preferred implementation in terms of language design will be to remove the current @nonreentrant decorator and replace with an @reentrant decorator (with inverted semantics)

Backwards Compatibility

this fundamentally changes the behavior of existing vyper contracts. to discuss.

Dependencies

will be cleaner to implement syntax-wise with the addition of #2856.

References

Copyright

Copyright and related rights waived via CC0

[fubuloubu]

1. Is too magical, not personally for this
2. Looks pretty good! Seems like it would need function pointer types

Also let's add another option
3. Invert @nonreentrant decorator and have it optionally white-list certain functions

@public
@reentrant # can be called again no matter what call out is made
def all_calls_allowed()

@public # no calls to this allowed after call out
def call_A() 

@public
@reentrant(call_A,...) # can only call this if call out from inside `call_A`
def only_A_reentrancy_allowed()

But in general, 2 seems most flexible since it's at the call site

[jtriley-eth]

Agree with @fubuloubu here, the @reentrant decorator with optional functions that may be reentered seems sufficient. In either case, I am on board with non-reentrant by default.

[pcaversaccio]

Fully support disabling reentrancy by default. @charles-cooper if you need hard stats, you can simply link to my reentrancy attacks list: https://github.com/pcaversaccio/reentrancy-attacks. Also, for the sake of completeness, I cross-reference my similar initiative in Solidity here: ethereum/solidity#12996. I think the reentrant decorator with the customisability of who can reenter feels the most ergonomic to me.

[charles-cooper]

inverting the nonreentrant decorator has the benefit of not incurring a performance hit for every function, but it doesn't carry the same level of safety. two thoughts,

1. it could be potentially an "intermediate" stage where we can see how users respond to the change without committing to the full change of only enabling re-entrancy at call site, and
2. these two options are not necessarily mutually exclusive, we could have both reentrancy at call site and also inverted reentrant decorator.

[pcaversaccio]

One comment regarding view functions. You can't disable reentrancy by default for view functions since it would require a state changing operation as part of the tx. Thus, question is whether the decorator reentrant should be allowed or whether there is a better solution to this.

[charles-cooper]

One comment regarding view functions. You can't disable reentrancy by default for view functions since it would require a state changing operation as part of the tx. Thus, question is whether the decorator reentrant should be allowed or whether there is a better solution to this.

the nonreentrant modifier can be used on view functions, see #2921

[pcaversaccio]

the nonreentrant modifier can be used on view functions, see #2921

So you want to keep this decorator as well?

[charles-cooper]

So you want to keep this decorator as well?

that's not what i meant, i'm just saying in the context of view functions, there are still reasonable semantics for nonreentrancy, they are just slightly modified -- the function cannot be "re-entered" into, i.e. it checks the flag even if it does not set it.

[charles-cooper]

2. Looks pretty good! Seems like it would need function pointer types

Btw, no function pointers needed! It would just be the name of the function, it's not really like it's callable or you could pass it to other functions like an object.

[scherrey]

Disabling re-entrancy by default fits with the 'default safe' policies of Vyper. Definitely lets do this. How to deal with the change in backwards compatibility is a bigger question. Maybe a contract level declaration REENTRANT_DEFAULT=True that you'd add to old contracts to get the original semantics? (I suspect a better symbol can be discovered but that's the idea.)

Otherwise - I like @reentrant decorator as being the most pythonic syntax as it addresses meta or non-functional aspects of the function rather than being a runtime expression.

await as a keyword doesn't fit right with me per my comments in #2856.

[pcaversaccio]

For the sake of documentation, linking a very interesting write-up by @0xalpharush on the discussed matter: https://gist.github.com/0xalpharush/15d903ec43334b081caece21a0bd7a20. Also, @jtriley-eth provided interesting thoughts (in Solidity tho) for reflection.

[ControlCplusControlV]

I support this a lot, but I have a couple ideas/questions, I am not sure how useful this one would be but having @reentrant(5) to allow a specific number of re-entrant calls at compile time could allow adjustable safety, although under the surface it might require a different implementation compared to unlimited re-entrancy

[pcaversaccio]

I support this a lot, but I have a couple ideas/questions, I am not sure how useful this one would be but having @reentrant(5) to allow a specific number of re-entrant calls at compile time could allow adjustable safety, although under the surface it might require a different implementation compared to unlimited re-entrancy

Do you have a specific use case in mind where this would be useful?

[ControlCplusControlV]

Do you have a specific use case in mind where this would be useful?

To be honest I doubt this actually sees much if any use, but think it would be good to have in edge cases. Like for example in the code for a DAO, it may be fine if an execute_proposal function calls itself (maybe executing a bunch of sub-proposals), but to prevent spam a proposal can only execute so many sub proposals. I'm just think building in optionality now could be useful in the future

[pcaversaccio]

To be honest I doubt this actually sees much if any use, but think it would be good to have in edge cases. Like for example in the code for a DAO, it may be fine if an execute_proposal function calls itself (maybe executing a bunch of sub-proposals), but to prevent spam a proposal can only execute so many sub proposals. I'm just think building in optionality now could be useful in the future

If I had to code such a solution I would probably use Solidity with inline assembly to optimise it. I don't think we need to cover the 0.0001%, but rather the 99.999% with the above proposal. But, one way to incorporate your idea could be by using an additional kwarg max_calls in the reentrant decorator, such as:

@external
def call_A():
    """
    @dev No calls to this function are allowed after call-out.
    """
   ... 

@external
def call_B():
    """
    @dev No calls to this function are allowed after call-out.
    """
   ... 

@external
@reentrant(call_A, call_B, max_calls=5)
def only_A_and_B_reentrancy_allowed():
    """
    @dev Can only call this function if call-out from inside `call_A` or `call_B`.
         Overall maximum of 5 reentering calls allowed.
    """
   ...

But again, complexity is the enemy of security. So I don't know whether this additional feature can be justified.

[ControlCplusControlV]

I really like the syntax for that, would support implementing it like that

[scherrey]

@external
@reentrant(call_A, call_B, max_calls=5)
def only_A_and_B_reentrancy_allowed():
"""
@dev Can only call this function if call-out from inside call_A or call_B.
Overall maximum of 5 reentering calls allowed.
"""
...

But again, complexity is the enemy of security. So I don't know whether this additional feature can be justified.

I like the idea of declarative syntax, especially for meta-data concepts like reentrancy. However they must have the highest semantic expressiveness possible because of the consequences of getting such things wrong. You clearly have already considered this with your warning about complexity and security. Very true.

Looking at this from a contract auditor's or exploiter's perspective, specifying some arbitrary counts of the number of times a reentrant call may come through this path without clear semantics of the relevance of that number scares/excites me respectively. But your idea of specifying reentrancy options via certain code paths is actually quite interesting and, if taken correctly, eliminates any need to specify an arbitrary number.

@reentrant(call_A, call_B, any_one_once) expresses that either call_A or call_B may come through this code path once but not both.

@reentrant(call_A, call_B, in_order) says that this code path may be traversed via call_A once initially, and only ever again if call_B is called afterwards but may not come via call_B unless first via call_A.

You can imagine several policy enumerations that might be found practical. But note that if you ever need a new call path to be allowed through, all that's necessary is to create another function inside the contract to route it through. This eliminates the need for any numeric declaration as you'll instead have an explicit named code path that can source the reentrancy under limited conditions. This makes reentrancy attacks incredibly difficult to create and auditing of the contract far easier than arbitrary numbers that don't express the semantics of how and why that number was arrived at nor how it could possibly be enforced.

It's the code paths that are critical - not the number of times it's passed though.

Ultimately, this is some pretty esoteric stuff that may be useful for composition of advanced DAO contracts but I think we should first do something simple and make sure we have optimum code efficiency for that model before extending the language this far. However, doing it as decorators does make it easier to do "take backs" if we want to consider some options as "experimental".

[charles-cooper]

For the sake of keeping this issue as clean as possible, I'm going to nip discussion of the enumerated reentrancy protection feature in the bud. It is a neat idea, but @ControlCplusControlV as you suspected it would require a different implementation that what I am envisioning, and there just doesn't seem to be a use case (besides theoretical ones) for it. It is such an odd use case that it can and should be implemented in user code. If that changes (i.e. it starts appearing as a common pattern in the wild), we can revisit, but at this time we should not be considering it further. @ControlCplusControlV if you want to continue brainstorming, please use https://github.com/vyperlang/vyper/discussions or the discord until the idea matures further.

[fubuloubu]

If reentrancy protection provided by default (escaped with decorators), basically once you've made a call into a contract, and it dispatches control flow back to a caller, that caller cannot do any call back including view calls. It would be good to assess what sorts of limits that might place, especially relating to implementing something like flashloans on a token, where the caller might want to see the total supply of the token to make a decision.

Im not sure if there are other more critical cases, but it's worth understanding this limitation

[pcaversaccio]

FWIW, dex aggregators like 1inch also use reentrancy as a feaure. Also there, we should conduct an impact analysis.

[charles-cooper]

it seems the preferred implementation in terms of language design will be to remove the current @nonreentrant decorator and replace with an @reentrant decorator (with inverted semantics)

[pcaversaccio]

it seems the preferred implementation in terms of language design will be to remove the current @nonreentrant decorator and replace with an @reentrant decorator (with inverted semantics)

how about read-only functions using such semantics?

[charles-cooper]

also, starting from 0.4.0, nonreentrant locks should be global only -- see #3760

[charles-cooper]

how about read-only functions using such semantics?

i don't think any special cases need to be considered for view functions. they just behave as before.

[ControlCplusControlV]

There is such thing as read-only re-entrancy, wonder if there would be any way to help mitigate that with this

[charles-cooper]

There is such thing as read-only re-entrancy, wonder if there would be any way to help mitigate that with this

this was already discussed previously in this issue #3380 (comment)

[pcaversaccio]

Given the current state of the EVM, only STATICCALL-based reentrancy doesn't pose any risk. This might change if the EVM would allow for parallelisation where during a view function callback the state already changed due to another state-changing, but independent transaction. This feature (disable reentrancy by default) is a great win for the ecosystem overall!