vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.87k stars 799 forks source link

VIP [DRAFT]: Safely expose `DELEGATECALL` #3699

Open makemake-kbo opened 10 months ago

makemake-kbo commented 10 months ago

Simple Summary

Allow Vyper contracts to safely DELEGATECALL into other Vyper contracts by enforcing strict storage slot layouts between contracts.

Motivation

DELEGATECALL allows a contract to delegate its execution context to another contract. This is a very powerful tool, enabling patterns like minimal proxies, upgradeable contracts, and more. As it delegates execution, DELEGATECALLing into a malicious or buggy contract can be fatal.

We assume that storage and execution are always linear and predictable. With DELEGATECALL this is no longer the case. While working with DELEGATECALL, we have to keep a mental model of storage slots between multiple contracts in mind.

To mitigate this, Vyper should enforce compiler level storage slot layouts. Forcing proxies to implement the same storage layout the contract they're calling has.

Specification

Add a new keyword storage. Every single contract that uses storage slots needs to define it's storage layout:

# Parent

storage number:
    thing: uint256

@external
def add(_value: uint256):
    self.thing += _value

# Proxy

from parent import number

proxies: number

target: address

@external
def __init__(_target: address):
    self.target = _target

@external
def add (_value: uint256):
    # hypothetical delegatecall function that takes in:
    # address of the contract we're delegatecalling into,
    # its storage layout,
    # the function we're calling,
    # and data,
    # as arguments.
    delegatecall(target, number, "add", _value)

Contracts that do not implement the same layout with the same types are invalid and fail to compile:

# Invalid proxy

storage not_valid:
    not_a_number: address
[...]
   # Error because we're not implementing `number`
   delegatecall(target, number, "add", _value)
[...]

This proposal would also be compatible with the https://github.com/vyperlang/vyper/issues/3556 syntax:

[...]
storage overriden:
   implementation: public(address) @ 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc
   admin: public(address) @ 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103
[...]

Backwards Compatibility

The proposed specification is not backwards compatible with any Vyper version.

Dependencies

Probably libraries?

References

Storage slot override syntax: https://github.com/vyperlang/vyper/issues/3556 Initial DELEGATECALL discussion: https://github.com/vyperlang/vyper/issues/1015

Copyright

Copyright and related rights waived via CC0

charles-cooper commented 10 months ago

maybe the name proxies: makes sense instead of implements storage:

charles-cooper commented 10 months ago

Every single contract that uses storage slots needs to define it's storage layout:

all contracts implicitly define a storage layout just by declaring variables. so we could even skip the storage keyword and instead of having the storage layout be a named member on the module, just have:

proxies: parent
charles-cooper commented 10 months ago

@makemake-kbo what are you thinking as syntax for safely calling a contract via delegatecall?

a couple ideas:

delegatecall SomeContract(*args)
SomeContract.__delegate_call__(*args)

(both of these would presumably have a compile time check that SomeContract proxies self)

makemake-kbo commented 10 months ago

@charles-cooper i think both look fine and would be ok with either. i have a slight preference for the first option

Every single contract that uses storage slots needs to define it's storage layout:

all contracts implicitly define a storage layout just by declaring variables. so we could even skip the storage keyword and instead of having the storage layout be a named member on the module, just have:

proxies: parent

the idea with the more verbose syntax was that using storage is something extra. i wanted to highlight the importance of storage, if that makes sense. another thing is that you could technically reuse the storage patterns but thats probably not that useful and might be harmful if missused.

id be fine with changing it if it adds confusion or if it looks too complicated.

charles-cooper commented 10 months ago

by the way, i want to point out that this proposal is useful even without the presence of delegatecall. for instance, off-chain tools might want to have the "storage layout interface" to figure out how to interpret the storage space of a contract.

cyberthirst commented 10 months ago

I like the idea.

I think that adding the storage kw is unnecessary. I would argue that the same holds even for the proxies kw - couldn't it be implicit in the use of delegatecall?

Also the word proxies seems to be a bit too narrow and not capturing the whole semantics of delegatecall.

How would the storage compatibility be defined? Slot addrs and type compatibility? I think that even syntactic compatibility is interesting (eg swapped two addresses in the logic - eg operator vs owner).

Lastly, I would rather opt for just a compiler warning rather than a compile error, seems too restrictive.

charles-cooper commented 10 months ago

How would the storage compatibility be defined? Slot addrs and type compatibility? I think that even syntactic compatibility is interesting (eg swapped two addresses in the logic - eg operator vs owner).

i think slot, type, and name should all be equal