New secondary function visibility modifier 'critical'

[samlavery]

I'm proposing adding an additional optional visibility modifier to the current set of 'private, internal, public, external' called 'critical'. This visibility modifier is enforced at the EVM level, as the others are. The control logic implied by making a function 'critical' in metadata is simply that more than one instance of the function selector cannot be appended to the callstack at runtime. Any form of CALL applied to a function selector marked critical that already exists on the stack will result in an instant error throw back to the caller and treated the same as an out of gas exception(CALL returns false). No additional contract functionality on the callee will be executed, most importantly, not the the fallback function.

I don't know the EVM internal data structures well enough to recommend keeping a separate stack for critical function selectors or to simply iterate over the normal callstack prior to function entrance. I suspect it's probably more performant to keep a separate callstack, at the expense of a little memory. No 'busy wait' type blocking algorithm should be considered as a near term feature, due to the sheer amount of complexity it would introduce. This proposal does not need to make any modification to the existing exception handling, but of course any additional exception type exacerbates the limitations of the current system. That discussion should be deferred in the interest of getting runtime safety into the vm as soon as possible.

The whole purpose of this proposal is simply to allow strong guarantees that a function isn't reentrant which can then be the basis for higher level atomic functions without increasing gas costs as an interpreted function modifier based solution would do. A nice side benefit is that contract authors may begin to check and handle false return values, as they should be doing anyway. No existing contract code will be impacted and will work as it does today.

[Smithgift]

I think this is basically possible with normal solidity. Example off the top of my head.

bool lock;

modifier critical() {
    if(lock) throw;
    lock = true;
    _
    lock = false;
}

Admittedly, this would lock all functions with critical if even one is running, but that's simple enough to fix.

[vbuterin]

@Smithgift that might not deal with re-entrancy attacks where A.f() calls B.g() calls A.h() where h != f; so locking all non-constant functions when a critical function is called may in fact be the right thing to do.

[Smithgift]

I could see that. However, if a critical function normally calls inside the contract, and those functions are non-constant, things could get painful. There's probably some assembly-level hack to do real reentry detection for the whole contract, by setting some flag before the dispatcher executes.

It seems that much of the current security issues have to do with the lack of standard (verified?) implementations for common features. (In those days there was no standard library in Ethereum: every coder wrote what was right in his own eyes...) Most of the time, there's no reason to do any specific alterations to a ERC20 token, so why reimplement it again and again?

In a way, this is happening because for the first time, random people off the Series of Tubes can easily write money-handling code. And most programming is not that dangerous. Just yesterday I implemented A*/Djikstra pathfinding for what may be the third time in my life. It's complex code, and I could probably have found something off npm, but there's no harm in doing it myself. If it finds a slightly suboptimal path, whatevs, But a simple money handling contract that has a recursive send somewhere in it? Ouch.

[axic]

I'm proposing adding an additional optional visibility modifier to the current set of 'private, internal, public, external' called 'critical'. This visibility modifier is enforced at the EVM level, as the others are.

@samlavery private, internal, public, external are modifiers internal to Solidity. They do not even exist at the JSON ABI level. And the EVM doesn't enforce any of these.

(Read more about the modifiers here: http://solidity.readthedocs.io/en/latest/control-structures.html#function-calls)

[github-actions]

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

[github-actions]

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.