Skip to content

Commit

Permalink
fix[codegen]: disable augassign with overlap (vyperlang#4487)
Browse files Browse the repository at this point in the history
in vyper, the behavior for AugAssign is to perform the bounds checks
only before evaluation of the rhs, rather than before-and-after. in
other words, the following code:

```vyper
def poc():
    a: DynArray[uint256, 2] = [1, 2]
    a[1] += a.pop()
```

is equivalent to:

```vyper
def poc():
    a: DynArray[uint256, 2] = [1, 2]
    a[1] += a[len(a) - 1]
    a.pop()
```

rather than:

```vyper
def poc():
    a: DynArray[uint256, 2] = [1, 2]
    s: uint256 = a[1]
    t: uint256 = a.pop()
    a[1] = s + t  # reverts due to oob access
```

this commit blocks the potentially missing bounds check by panicking
when there is a potential write on the rhs of an AugAssign which could
change the length on the lhs.

references:
- GHSA-4w26-8p97-f4jp

---------

Co-authored-by: cyberthirst <[email protected]>
  • Loading branch information
2 people authored and lemenkov committed Feb 27, 2025
1 parent ae446e9 commit 6fb182e
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 1 deletion.
131 changes: 130 additions & 1 deletion tests/functional/codegen/features/test_assignment.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import pytest

from vyper.exceptions import ImmutableViolation, InvalidType, TypeMismatch
from vyper.exceptions import CodegenPanic, ImmutableViolation, InvalidType, TypeMismatch


def test_augassign(get_contract):
Expand Down Expand Up @@ -39,6 +39,135 @@ def augmod(x: int128, y: int128) -> int128:
print("Passed aug-assignment test")


@pytest.mark.parametrize(
"source",
[
"""
@external
def poc():
a: DynArray[uint256, 2] = [1, 2]
a[1] += a.pop()
""",
"""
a: DynArray[uint256, 2]
def side_effect() -> uint256:
return self.a.pop()
@external
def poc():
self.a = [1, 2]
self.a[1] += self.side_effect()
""",
"""
a: DynArray[uint256, 2]
def side_effect() -> uint256:
self.a = [1]
return 1
@external
def poc():
self.a = [1, 2]
self.a[1] += self.side_effect()
""",
"""
a: DynArray[uint256, 2]
interface Foo:
def foo() -> uint256: nonpayable
@external
def foo() -> uint256:
return self.a.pop()
@external
def poc():
self.a = [1, 2]
# panics due to extcall
self.a[1] += extcall Foo(self).foo()
""",
],
)
@pytest.mark.xfail(strict=True, raises=CodegenPanic)
def test_augassign_oob(get_contract, tx_failed, source):
# xfail here (with panic):
c = get_contract(source)

# not reached until the panic is fixed
with tx_failed(c):
c.poc()


@pytest.mark.parametrize(
"source",
[
"""
a: public(DynArray[uint256, 2])
interface Foo:
def foo() -> uint256: view
@external
def foo() -> uint256:
return self.a[1]
@external
def entry() -> DynArray[uint256, 2]:
self.a = [1, 1]
# panics due to staticcall
self.a[1] += staticcall Foo(self).foo()
return self.a
"""
],
)
@pytest.mark.xfail(strict=True, raises=CodegenPanic)
def test_augassign_rhs_references_lhs(get_contract, tx_failed, source):
# xfail here (with panic):
c = get_contract(source)

assert c.entry() == [1, 2]


@pytest.mark.parametrize(
"source",
[
"""
@external
def entry() -> DynArray[uint256, 2]:
a: DynArray[uint256, 2] = [1, 1]
a[1] += a[1]
return a
""",
"""
@external
def entry() -> DynArray[uint256, 2]:
a: uint256 = 1
a += a
b: DynArray[uint256, 2] = [a, a]
b[0] -= b[0]
b[0] += b[1] // 2
return b
""",
"""
a: DynArray[uint256, 2]
def read() -> uint256:
return self.a[1]
@external
def entry() -> DynArray[uint256, 2]:
self.a = [1, 1]
self.a[1] += self.read()
return self.a
""",
],
)
def test_augassign_rhs_references_lhs2(get_contract, source):
c = get_contract(source)
assert c.entry() == [1, 2]


@pytest.mark.parametrize(
"typ,in_val,out_val",
[
Expand Down
7 changes: 7 additions & 0 deletions vyper/codegen/stmt.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,13 @@ def parse_AugAssign(self):
# single word load/stores are atomic.
raise TypeCheckFailure("unreachable")

for var in target.referenced_variables:
if var.typ._is_prim_word:
continue
# oob - GHSA-4w26-8p97-f4jp
if var in right.variable_writes or right.contains_risky_call:
raise CodegenPanic("unreachable")

with target.cache_when_complex("_loc") as (b, target):
left = IRnode.from_list(LOAD(target), typ=target.typ)
new_val = Expr.handle_binop(self.stmt.op, left, right, self.context)
Expand Down

0 comments on commit 6fb182e

Please sign in to comment.