From 618c859ada93fa54db197b565f6e843013e02df5 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Thu, 3 Aug 2023 11:31:53 +0800 Subject: [PATCH 01/13] validate iter calls --- vyper/semantics/analysis/local.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/vyper/semantics/analysis/local.py b/vyper/semantics/analysis/local.py index c0c05325f2..79a1ed7cf5 100644 --- a/vyper/semantics/analysis/local.py +++ b/vyper/semantics/analysis/local.py @@ -424,6 +424,15 @@ def visit_For(self, node): if assign: raise ImmutableViolation("Cannot modify array during iteration", assign) + # Check for state-modifying functions in `iter` + iter_call_nodes = node.iter.get_descendants(vy_ast.Call) + for call_node in iter_call_nodes: + call_type = get_exact_type_from_node(call_node.func) + if isinstance(call_type, ContractFunctionT) and call_type.is_mutable: + raise ImmutableViolation( + "Cannot call state-modifying functions for `range` expression", call_node + ) + # Check if `iter` is a storage variable. get_descendants` is used to check for # nested `self` (e.g. structs) iter_is_storage_var = ( From 62775e35024811b65e07319872d4d4835425a2c9 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Thu, 3 Aug 2023 11:32:00 +0800 Subject: [PATCH 02/13] add tests --- tests/parser/syntax/test_for_range.py | 96 ++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/tests/parser/syntax/test_for_range.py b/tests/parser/syntax/test_for_range.py index b2a9491058..ff0ee888a7 100644 --- a/tests/parser/syntax/test_for_range.py +++ b/tests/parser/syntax/test_for_range.py @@ -1,7 +1,7 @@ import pytest from vyper import compiler -from vyper.exceptions import StructureException +from vyper.exceptions import ImmutableViolation, StructureException fail_list = [ ( @@ -12,7 +12,59 @@ def foo(): pass """, StructureException, - ) + ), + ( + """ +interface A: + def foo()-> uint256: nonpayable + +@external +def bar(x:address): + a: A = A(x) + for i in range(a.foo(), bound = 12): + pass + """, + ImmutableViolation, + ), + ( + """ +interface A: + def foo()-> uint256: nonpayable + +@external +def bar(x:address): + a: A = A(x) + for i in range(max(a.foo(), 123), bound = 12): + pass + """, + ImmutableViolation, + ), + ( + """ +interface A: + def foo()-> uint256: nonpayable + +@external +def bar(x:address): + a: A = A(x) + for i in range(a.foo(), a.foo()+1): + pass + """, + ImmutableViolation, + ), + ( + """ +interface A: + def foo()-> uint256: nonpayable + +@external +def bar(x:address): + a: A = A(x) + for i in range(min(a.foo(), 123), min(a.foo(), 123) + 1): + pass + """, + ImmutableViolation, + ), ] @@ -51,6 +103,46 @@ def kick_foos(): for foo in self.foos: foo.kick() """, + """ +interface A: + def foo()-> uint256: view + +@external +def bar(x:address): + a: A = A(x) + for i in range(a.foo(), bound = 12): + pass + """, + """ +interface A: + def foo()-> uint256: view + +@external +def bar(x:address): + a: A = A(x) + for i in range(max(a.foo(), 123), bound = 12): + pass + """, + """ +interface A: + def foo()-> uint256: view + +@external +def bar(x:address): + a: A = A(x) + for i in range(a.foo(), a.foo()+1): + pass + """, + """ +interface A: + def foo()-> uint256: view + +@external +def bar(x:address): + a: A = A(x) + for i in range(min(a.foo(), 123), min(a.foo(), 123) + 1): + pass + """, ] From 249d35d1443de42650122a8e05e55e72d7cb7b65 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Thu, 3 Aug 2023 11:36:00 +0800 Subject: [PATCH 03/13] minor formatting --- tests/parser/syntax/test_for_range.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/parser/syntax/test_for_range.py b/tests/parser/syntax/test_for_range.py index ff0ee888a7..872d655ec8 100644 --- a/tests/parser/syntax/test_for_range.py +++ b/tests/parser/syntax/test_for_range.py @@ -21,7 +21,7 @@ def foo()-> uint256: nonpayable @external def bar(x:address): a: A = A(x) - for i in range(a.foo(), bound = 12): + for i in range(a.foo(), bound=12): pass """, ImmutableViolation, @@ -34,7 +34,7 @@ def foo()-> uint256: nonpayable @external def bar(x:address): a: A = A(x) - for i in range(max(a.foo(), 123), bound = 12): + for i in range(max(a.foo(), 123), bound=12): pass """, ImmutableViolation, @@ -47,7 +47,7 @@ def foo()-> uint256: nonpayable @external def bar(x:address): a: A = A(x) - for i in range(a.foo(), a.foo()+1): + for i in range(a.foo(), a.foo() + 1): pass """, ImmutableViolation, @@ -110,7 +110,7 @@ def foo()-> uint256: view @external def bar(x:address): a: A = A(x) - for i in range(a.foo(), bound = 12): + for i in range(a.foo(), bound=12): pass """, """ @@ -120,7 +120,7 @@ def foo()-> uint256: view @external def bar(x:address): a: A = A(x) - for i in range(max(a.foo(), 123), bound = 12): + for i in range(max(a.foo(), 123), bound=12): pass """, """ @@ -130,7 +130,7 @@ def foo()-> uint256: view @external def bar(x:address): a: A = A(x) - for i in range(a.foo(), a.foo()+1): + for i in range(a.foo(), a.foo() + 1): pass """, """ From 29953dda6700295c686126233633ce8faf46c9ad Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Thu, 3 Aug 2023 11:36:25 +0800 Subject: [PATCH 04/13] improve comment --- vyper/semantics/analysis/local.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vyper/semantics/analysis/local.py b/vyper/semantics/analysis/local.py index 79a1ed7cf5..4134e83d7d 100644 --- a/vyper/semantics/analysis/local.py +++ b/vyper/semantics/analysis/local.py @@ -424,7 +424,7 @@ def visit_For(self, node): if assign: raise ImmutableViolation("Cannot modify array during iteration", assign) - # Check for state-modifying functions in `iter` + # Check for state-modifying function calls in `iter` iter_call_nodes = node.iter.get_descendants(vy_ast.Call) for call_node in iter_call_nodes: call_type = get_exact_type_from_node(call_node.func) From c5b9e977d0d8c6cabc8f6a5070181a36880fa36f Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Thu, 3 Aug 2023 11:51:39 +0800 Subject: [PATCH 05/13] reorder --- vyper/semantics/analysis/local.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/vyper/semantics/analysis/local.py b/vyper/semantics/analysis/local.py index 4134e83d7d..5243b8111b 100644 --- a/vyper/semantics/analysis/local.py +++ b/vyper/semantics/analysis/local.py @@ -404,6 +404,15 @@ def visit_For(self, node): if not type_list: raise TypeMismatch("Iterator values are of different types", node.iter) + # Check for state-modifying function calls in `range` expression + range_call_nodes = node.iter.get_descendants(vy_ast.Call) + for call_node in range_call_nodes: + call_type = get_exact_type_from_node(call_node.func) + if isinstance(call_type, ContractFunctionT) and call_type.is_mutable: + raise ImmutableViolation( + "Cannot call state-modifying functions for `range` expression", call_node + ) + else: # iteration over a variable or literal list if isinstance(node.iter, vy_ast.List) and len(node.iter.elements) == 0: @@ -424,15 +433,6 @@ def visit_For(self, node): if assign: raise ImmutableViolation("Cannot modify array during iteration", assign) - # Check for state-modifying function calls in `iter` - iter_call_nodes = node.iter.get_descendants(vy_ast.Call) - for call_node in iter_call_nodes: - call_type = get_exact_type_from_node(call_node.func) - if isinstance(call_type, ContractFunctionT) and call_type.is_mutable: - raise ImmutableViolation( - "Cannot call state-modifying functions for `range` expression", call_node - ) - # Check if `iter` is a storage variable. get_descendants` is used to check for # nested `self` (e.g. structs) iter_is_storage_var = ( From bdc195998d89ab3652f629586cb30b68fa770243 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Thu, 3 Aug 2023 15:07:25 +0800 Subject: [PATCH 06/13] fix test --- .../exceptions/test_constancy_exception.py | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/parser/exceptions/test_constancy_exception.py b/tests/parser/exceptions/test_constancy_exception.py index 4bd0b4fcb9..d9d4bd6a3f 100644 --- a/tests/parser/exceptions/test_constancy_exception.py +++ b/tests/parser/exceptions/test_constancy_exception.py @@ -48,17 +48,6 @@ def foo() -> int128: def foo() -> int128: x: address = create_minimal_proxy_to(0x1234567890123456789012345678901234567890, value=9) return 5""", - # test constancy in range expressions - """ -glob: int128 -@internal -def foo() -> int128: - self.glob += 1 - return 5 -@external -def bar(): - for i in range(self.foo(), self.foo() + 1): - pass""", """ glob: int128 @internal @@ -120,6 +109,17 @@ def foo(f: Foo) -> Foo: f.a[1] = [0, 1] return f """, + # test constancy in range expressions + """ +glob: int128 +@internal +def foo() -> int128: + self.glob += 1 + return 5 +@external +def bar(): + for i in range(self.foo(), self.foo() + 1): + pass""", ], ) def test_immutability_violations(bad_code): From fd3307e834d05dfc9cd9a79c2d404e825e79b64a Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Thu, 3 Aug 2023 15:07:41 +0800 Subject: [PATCH 07/13] fix format --- tests/parser/exceptions/test_constancy_exception.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/parser/exceptions/test_constancy_exception.py b/tests/parser/exceptions/test_constancy_exception.py index d9d4bd6a3f..0cecb12ca7 100644 --- a/tests/parser/exceptions/test_constancy_exception.py +++ b/tests/parser/exceptions/test_constancy_exception.py @@ -119,7 +119,8 @@ def foo() -> int128: @external def bar(): for i in range(self.foo(), self.foo() + 1): - pass""", + pass + """, ], ) def test_immutability_violations(bad_code): From eed6755372aaf9a94b4d2fa8ea41a93ed210cd66 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Sat, 5 Aug 2023 11:00:59 +0800 Subject: [PATCH 08/13] add builtins and pop --- vyper/semantics/analysis/local.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/vyper/semantics/analysis/local.py b/vyper/semantics/analysis/local.py index 5243b8111b..cf1714799e 100644 --- a/vyper/semantics/analysis/local.py +++ b/vyper/semantics/analysis/local.py @@ -404,11 +404,24 @@ def visit_For(self, node): if not type_list: raise TypeMismatch("Iterator values are of different types", node.iter) - # Check for state-modifying function calls in `range` expression + # Check for state-modifying expressions in `range` expression range_call_nodes = node.iter.get_descendants(vy_ast.Call) for call_node in range_call_nodes: call_type = get_exact_type_from_node(call_node.func) - if isinstance(call_type, ContractFunctionT) and call_type.is_mutable: + disallowed_builtins = ( + "raw_call", + "create_minimal_proxy_to", + "create_copy_of", + "create_from_blueprint", + ) + if ( + # state-modifying internal and external calls + (isinstance(call_type, ContractFunctionT) and call_type.is_mutable) + # `pop` on dynamic arrays + or (isinstance(call_type, MemberFunctionT) and call_type.is_modifying) + # state-modifying builtin functions + or call_node.get("func.id") in disallowed_builtins + ): raise ImmutableViolation( "Cannot call state-modifying functions for `range` expression", call_node ) From e28a9e9688ec96bb288e43a9595f0f3a1903e5c2 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Sat, 5 Aug 2023 11:01:11 +0800 Subject: [PATCH 09/13] add test for pop --- tests/parser/syntax/test_for_range.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/parser/syntax/test_for_range.py b/tests/parser/syntax/test_for_range.py index 872d655ec8..6ec2a47593 100644 --- a/tests/parser/syntax/test_for_range.py +++ b/tests/parser/syntax/test_for_range.py @@ -65,6 +65,20 @@ def bar(x:address): """, ImmutableViolation, ), + # Cannot call `pop()` in for range because it modifies state + ( + """ +arr: DynArray[uint256, 10] +@external +def test()-> (DynArray[uint256, 6], DynArray[uint256, 10]): + b: DynArray[uint256, 6] = [] + self.arr = [1,0] + for i in range(self.arr.pop(), self.arr.pop() + 2): + b.append(i) + return b, self.arr + """, + ImmutableViolation, + ) ] From 5f2050d6bbd364e726810566392ecf8a0325ae0f Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Sat, 5 Aug 2023 11:10:05 +0800 Subject: [PATCH 10/13] add builtin tests --- tests/parser/syntax/test_for_range.py | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/parser/syntax/test_for_range.py b/tests/parser/syntax/test_for_range.py index 6ec2a47593..496d68dc3a 100644 --- a/tests/parser/syntax/test_for_range.py +++ b/tests/parser/syntax/test_for_range.py @@ -78,6 +78,42 @@ def test()-> (DynArray[uint256, 6], DynArray[uint256, 10]): return b, self.arr """, ImmutableViolation, + ), + ( + """ +@external +def bar(x:address): + for i in range(1 if raw_call(x, b'', revert_on_failure=False) else 2 , bound = 12): + pass + """, + ImmutableViolation, + ), + ( + """ +@external +def foo(a: address): + for i in range(1 if convert(create_minimal_proxy_to(a), uint256) > 2 else 2, bound=12): + pass + """, + ImmutableViolation, + ), + ( + """ +@external +def foo(a: address): + for i in range(1 if convert(create_copy_of(a), uint256) > 2 else 2, bound=12): + pass + """, + ImmutableViolation, + ), + ( + """ +@external +def foo(a: address): + for i in range(1 if convert(create_from_blueprint(a), uint256) > 2 else 2, bound=12): + pass + """, + ImmutableViolation, ) ] From 2f140c149698f78346c072d70661ccaff74c5e21 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Sat, 5 Aug 2023 11:12:09 +0800 Subject: [PATCH 11/13] fix lint --- tests/parser/syntax/test_for_range.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/parser/syntax/test_for_range.py b/tests/parser/syntax/test_for_range.py index 496d68dc3a..60e5599b10 100644 --- a/tests/parser/syntax/test_for_range.py +++ b/tests/parser/syntax/test_for_range.py @@ -114,7 +114,7 @@ def foo(a: address): pass """, ImmutableViolation, - ) + ), ] From 6d78357b6bb5885c701233a8cb85694cedcda6a3 Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Mon, 7 Aug 2023 17:32:26 +0800 Subject: [PATCH 12/13] handle raw call static call --- vyper/semantics/analysis/local.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/vyper/semantics/analysis/local.py b/vyper/semantics/analysis/local.py index cf1714799e..765fe79608 100644 --- a/vyper/semantics/analysis/local.py +++ b/vyper/semantics/analysis/local.py @@ -408,8 +408,8 @@ def visit_For(self, node): range_call_nodes = node.iter.get_descendants(vy_ast.Call) for call_node in range_call_nodes: call_type = get_exact_type_from_node(call_node.func) + func_name = call_node.get("func.id") disallowed_builtins = ( - "raw_call", "create_minimal_proxy_to", "create_copy_of", "create_from_blueprint", @@ -420,7 +420,14 @@ def visit_For(self, node): # `pop` on dynamic arrays or (isinstance(call_type, MemberFunctionT) and call_type.is_modifying) # state-modifying builtin functions - or call_node.get("func.id") in disallowed_builtins + or func_name in disallowed_builtins + # `raw_call` is handled specially due to the `is_static_call` kwarg + or ( + func_name == "raw_call" + and not {i.arg: i.value for i in call_node.keywords}.get( + "is_static_call", False + ) + ) ): raise ImmutableViolation( "Cannot call state-modifying functions for `range` expression", call_node From 1d214ee9b5725967373f506e16582ea3c8c2f9eb Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Mon, 7 Aug 2023 17:32:31 +0800 Subject: [PATCH 13/13] fix tests --- tests/parser/syntax/test_for_range.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/parser/syntax/test_for_range.py b/tests/parser/syntax/test_for_range.py index 60e5599b10..81f4de82b3 100644 --- a/tests/parser/syntax/test_for_range.py +++ b/tests/parser/syntax/test_for_range.py @@ -83,7 +83,13 @@ def test()-> (DynArray[uint256, 6], DynArray[uint256, 10]): """ @external def bar(x:address): - for i in range(1 if raw_call(x, b'', revert_on_failure=False) else 2 , bound = 12): + for i in range(1 if raw_call( + x, + b'', + max_outsize=32, + ) == b"vyper" else 2, + bound=12 + ): pass """, ImmutableViolation, @@ -193,6 +199,19 @@ def bar(x:address): for i in range(min(a.foo(), 123), min(a.foo(), 123) + 1): pass """, + """ +@external +def bar(x:address): + for i in range(1 if raw_call( + x, + b'', + max_outsize=32, + is_static_call=True + ) == b"vyper" else 2, + bound=12 + ): + pass + """, ]