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

feat[lang]: protect external calls with keyword #2938

Merged
merged 44 commits into from Mar 6, 2024

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Jun 24, 2022

What I did

implement #2856

settled on extcall+staticcall to distinguish between calls which can have side effects and not.

(note: wait until after v0.3.4)

How I did it

How to verify it

see new tests

Commit message

this commit adds `extcall` and `staticcall` keywords to the vyper
language. these are now a requirement for the user to add to distinguish
internal calls from:
1) external calls which can have side effects (`extcall`), and
2) external calls which are guaranteed by the EVM to not have side
   effects (`staticcall`).

`extcall` is used for `nonpayable` or `payable` functions (which emit
the `CALL` opcode), while `staticcall` is used for `view` and `pure`
functions (which emit the `STATICCALL` opcode).

the motivation for this is laid out more in the linked GH issue, but it
is primarily to make it easier to read, audit and analyze vyper
contracts, since you can find the locations of external calls in source
code using text-only techniques, and do not need to analyze (or have
access to the results of an analysis) in order to find where external
calls are. (note that this has become a larger concern with with the
introduction of modules in vyper, since you can no longer distinguish
between internal and external calls just by looking for the `self.`
prefix).

an analysis of some production contracts indicates that the frequency
of external calls has somewhat high variability, but is in the range of
one `extcall` (or `staticcall`) per 10-25 (logical) sloc, with
`staticcalls` being about twice as common. therefore, based on the
semantic vs write load of the keyword, the keyword should be somewhat
easy to type, but it also needs to be long enough and unusual enough to
stand out in a text editor.

the differentiation between `extcall` and `staticcall` was added
because, during testing of the feature, it was found that being able to
additionally infer at the call site whether the external call can have
side effects or not (without needing to reference the function
definition) substantially enhanced readability.

refactoring/misc updates:
- update and clean up the grammar, especially the `variable_access` rule
  (cf. https://github.com/lark-parser/lark/blob/706190849ee/lark/grammars/python.lark#L192)
- add a proper .parent property to VyperNodes
- add tokenizer changes to make source locations are correct
- ban standalone staticcalls
- update tests -- in some cases, because standalone staticcalls are now
  banned, either an enclosing assignment was added or the mutability of
  the interface was changed.
- rewrite some assert_compile_failed to pytest.raises() along the way
- remove some dead functions

cf. GH issue / VIP 2856 

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@charles-cooper charles-cooper added this to the v0.4.0 milestone Jun 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2022

Codecov Report

Attention: Patch coverage is 97.16312% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 85.36%. Comparing base (45c6b6a) to head (b625c04).

Files Patch % Lines
vyper/codegen/expr.py 91.89% 1 Missing and 2 partials ⚠️
vyper/ast/parse.py 92.30% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2938      +/-   ##
==========================================
+ Coverage   85.23%   85.36%   +0.12%     
==========================================
  Files          92       92              
  Lines       13949    14031      +82     
  Branches     3130     3141      +11     
==========================================
+ Hits        11889    11977      +88     
+ Misses       1565     1560       -5     
+ Partials      495      494       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fubuloubu
Copy link
Member

breaking change, wait until 0.4.0

@charles-cooper
Copy link
Member Author

for now i'm putting in the behavior as warning instead of an exception


if isinstance(self.expr.func, vy_ast.Name):
function_name = self.expr.func.id
from vyper.builtins._signatures import BuiltinFunctionT

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.builtins._signatures
begins an import cycle.
vyper/codegen/expr.py Fixed Show fixed Hide fixed
@charles-cooper charles-cooper changed the title feat: protect external calls with await keyword feat: protect external calls with keyword Feb 29, 2024
@classmethod
def handle_external_call(cls, expr, context):
# TODO fix cyclic import
from vyper.builtins._signatures import BuiltinFunctionT

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.builtins._signatures
begins an import cycle.
@charles-cooper charles-cooper marked this pull request as ready for review March 4, 2024 02:08
vyper/ast/nodes.pyi Outdated Show resolved Hide resolved
vyper/codegen/stmt.py Outdated Show resolved Hide resolved
vyper/codegen/expr.py Outdated Show resolved Hide resolved
vyper/ast/grammar.lark Show resolved Hide resolved
vyper/ast/grammar.lark Show resolved Hide resolved
vyper/ast/grammar.lark Show resolved Hide resolved
vyper/ast/grammar.lark Show resolved Hide resolved
@cyberthirst
Copy link
Collaborator

compiling

@external
def foo():
    s: String[2] = staticcall staticcall concat('a', 'b')
    return

yields:

  File "<unknown>", line 4
    s: String[2] = await      await      concat('a', 'b')
                              ^^^^^
SyntaxError: invalid syntax

The above exception was the direct cause of the following exception:

vyper.exceptions.SyntaxException: invalid syntax (<unknown>, line 4)

  line 4:31 
       3 def foo():
  ---> 4     s: String[2] = staticcall staticcall concat('a', 'b')
  --------------------------------------^
       5     return

see: s: String[2] = await await concat('a', 'b')
also: File "<unknown>", line 4

@cyberthirst
Copy link
Collaborator

I assume this is the type of err that the description is alluding to:

interface I:
    def foo() -> uint256: view
    def bar(a: uint256): payable

event E:
    a: uint256

@external
@payable
def foo(_target: address) -> uint256:
    log E(staticcall I(_target).foo())
    return 1

yields:

    IndexError: list index out of rang

@cyberthirst
Copy link
Collaborator

compiling:

d: DynArray[uint256, 10]

interface I:
    def foo() -> DynArray[uint256, 10]: view


@external
def bar(t: address) -> uint256:
    for i: uint256 in staticcall I(t).foo():
        self.d.append(i)
    return 1

yields:

Error compiling: tests/custom/test5.vy
AssertionError: non-unique symbols {'I(t).foo()2'}

@cyberthirst
Copy link
Collaborator

compiling:

d: DynArray[uint256, 10]

interface I:
    def foo() -> DynArray[uint256, 10]: view
    def bar() -> uint256: payable


@external
def bar(t: address) -> uint256:
    for i: uint256 in range(extcall I(t).bar(), bound=10):
        self.d.append(i)
    return 1

yields:

Error compiling: tests/custom/test5.vy
vyper.exceptions.CodegenPanic: unhandled exception typechecker missed this, parse_ExtCall

  contract "tests/custom/test5.vy:10", function "bar", line 10:28 
        9 def bar(t: address) -> uint256:
  ---> 10     for i: uint256 in range(extcall I(t).bar(), bound=10):
  ------------------------------------^
       11         self.d.append(i)


This is an unhandled internal compiler error. Please create an issue on Github to notify the developers!
https://github.com/vyperlang/vyper/issues/new?template=bug.md

@cyberthirst
Copy link
Collaborator

compiling:

d: DynArray[uint256, 10]

interface I:
    def foo() -> DynArray[uint256, 10]: view


@external
def bar(t: address) -> uint256:
    for i: uint256 in staticcall I(t).foo():
        self.d.append(i)
    return 1

yields:

Error compiling: tests/custom/test5.vy
AssertionError: non-unique symbols {'I(t).foo()2'}

related:

interface I:
        def ohfak() -> decimal: view

@external
def bar(t: address):
    k: decimal = sqrt(staticcall I(t).ohfak())
    return

yields:

Error compiling: tests/custom/test4.vy
AssertionError: non-unique symbols {'I(t).ohfak()18'}

@charles-cooper
Copy link
Member Author

see: s: String[2] = await await concat('a', 'b') also: File "<unknown>", line 4

seems unrelated -- there's a raise SyntaxException(...) from e in vyper/ast/parse.py; we could remove the from e

@charles-cooper
Copy link
Member Author

compiling:

d: DynArray[uint256, 10]

interface I:
    def foo() -> DynArray[uint256, 10]: view
    def bar() -> uint256: payable


@external
def bar(t: address) -> uint256:
    for i: uint256 in range(extcall I(t).bar(), bound=10):
        self.d.append(i)
    return 1

yields:

Error compiling: tests/custom/test5.vy
vyper.exceptions.CodegenPanic: unhandled exception typechecker missed this, parse_ExtCall

  contract "tests/custom/test5.vy:10", function "bar", line 10:28 
        9 def bar(t: address) -> uint256:
  ---> 10     for i: uint256 in range(extcall I(t).bar(), bound=10):
  ------------------------------------^
       11         self.d.append(i)


This is an unhandled internal compiler error. Please create an issue on Github to notify the developers!
https://github.com/vyperlang/vyper/issues/new?template=bug.md

this is a bug on master too, not a regression in this PR

@charles-cooper
Copy link
Member Author

the non-unique symbol panics are related to known bugs i think, not regressions

@charles-cooper charles-cooper changed the title feat: protect external calls with keyword feat[lang]: protect external calls with keyword Mar 5, 2024
@charles-cooper charles-cooper merged commit 2d232eb into vyperlang:master Mar 6, 2024
84 checks passed
@charles-cooper charles-cooper deleted the feat/await branch March 6, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants