Skip to content

Conversation

@charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Feb 21, 2025

What I did

mitigating PR for GHSA-4w26-8p97-f4jp (tracking issue: #4489). block the behavior

How I did it

How to verify it

Commit message

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:
- https://github.com/vyperlang/vyper/security/advisories/GHSA-4w26-8p97-f4jp

Description for the changelog

Cute Animal Picture

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

@charles-cooper charles-cooper added the release - must release blocker label Feb 21, 2025
@charles-cooper charles-cooper added this to the v0.4.1 milestone Feb 21, 2025
@codecov
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.06%. Comparing base (2ffc8b3) to head (d17bc13).
Report is 93 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4487   +/-   ##
=======================================
  Coverage   92.06%   92.06%           
=======================================
  Files         120      120           
  Lines       17330    17335    +5     
  Branches     2932     2935    +3     
=======================================
+ Hits        15955    15960    +5     
  Misses        957      957           
  Partials      418      418           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

if var.typ._is_prim_word:
continue
# oob - GHSA-4w26-8p97-f4jp
if var in right.variable_writes or right.contains_risky_call:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

staticall probably isn't risky for this case. but i don't think it matters that much

@charles-cooper charles-cooper enabled auto-merge (squash) February 23, 2025 19:35
@charles-cooper charles-cooper merged commit dd5a3d9 into vyperlang:master Feb 23, 2025
159 checks passed
lemenkov pushed a commit to fedora-ethereum/vyper that referenced this pull request Feb 27, 2025
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release - must release blocker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants