Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VIP: Reentrant Lock Decorator #1204

Closed
jacqueswww opened this issue Jan 18, 2019 · 5 comments
Closed

VIP: Reentrant Lock Decorator #1204

jacqueswww opened this issue Jan 18, 2019 · 5 comments
Labels
VIP: Approved VIP Approved VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting

Comments

@jacqueswww
Copy link
Contributor

jacqueswww commented Jan 18, 2019

Simple Summary

Add a decorator called @nonreentrant to be used on all functions that handle funds and at the discretion of the developer.

Abstract

Using a lock around a function call one can prevent a function doing send / external call with value to do a re-entrant call.

Motivation

Re-entrancy is super difficult to detect as a programmer, Vyper should make it simple not to introduce or hard to get wrong.

As an extra security measure it is encouraged to force the usage of @nonreentrant on functions that are sending funds, either with the value= parameter or the send() function. This is just an extra layer of protection, however attacks are still possible if a function uses state-setting and fund moving in separate transactions.

Specification

Using a unique identifier per function (derivitive of method_id), we can set a lock per function. See https://github.com/protofire/zeppelin-solidity/blob/master/contracts/ReentrancyGuard.sol.

In pseudo vyper code this would be the equivelant of something like:

locks: map(bytes32, bool)

def withdraw():
    assert self.locks[method_id('withdraw()', bytes32)] == False
    self.locks[method_id('withdraw()', bytes32)] = True
     # Body of code.
    self.locks[method_id('withdraw()', bytes32)] = False

Backwards Compatibility

This will effect backwards compatibility if this becomes a forced decorator, if only an optional decorator it will not effect backwards compatibility.

Dependencies

Optional Implementation of @nonreentrant: None (5200 gas per application)
Forced implementation of @nonreentrant: Net-Gas Metering (400 gas per application, well worth it!)

Copyright

Copyright and related rights waived via CC0

@jacqueswww jacqueswww added the VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting label Jan 18, 2019
@jacqueswww jacqueswww changed the title No-reentrant Decorator No-reentrant Lock Decorator Jan 18, 2019
@fubuloubu fubuloubu changed the title No-reentrant Lock Decorator VIP: No-reentrant Lock Decorator Jan 18, 2019
@jacqueswww jacqueswww changed the title VIP: No-reentrant Lock Decorator VIP: Reentrant Lock Decorator Jan 18, 2019
@charles-cooper
Copy link
Member

Should the first line of the withdraw example read like follows?

    assert self.locks[method_id('withdraw()', bytes32)] == False

@fubuloubu
Copy link
Member

Added the edit!

@fubuloubu
Copy link
Member

I don't think we should "enforce" this as a default, rather this is a good tool for developers to use in situtations where it is called for

We should instead raise warnings of when a function should have this decorator, to reinforce the pattern that not every function will need it

@fubuloubu
Copy link
Member

@jacqueswww
Copy link
Contributor Author

Marked as accepted, @nonreentrant(key)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VIP: Approved VIP Approved VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting
Projects
None yet
Development

No branches or pull requests

3 participants