vyperlang / vyper

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

fix[codegen]: recursive dynarray oob check #4091

Closed charles-cooper closed 3 weeks ago

charles-cooper commented 1 month ago

What I did

fix another bug in dynarray recursion

reported by @trocher

How I did it

How to verify it

Commit message

this commit fixes more edge cases in `abi_decode` dynarray
validation. these are bugs which were missed (or regressions) in
1f6b9433fbd524, which itself was a continuation of eb011367cc769.

there are multiple fixes contained in this commit.

- similar conceptual error as in 1f6b9433fbd524. when the
length word is out-of-bounds and its runtime is value is zero,
`make_setter` does not enter recursion and therefore there is
no oob check. an example payload which demonstrates this is in
`test_nested_invalid_dynarray_head()`. the fix is to check the
size of the static section ("embedded static size") before entering
the recursion, rather than child_type.static_size (which could be
zero). essentially, this checks that the end of the static section is
in bounds, rather than the beginning.

- the fallback case in `complex_make_setter` could be referring to a
tuple of dynamic types, which makes the tuple itself dynamic, so there
needs to be an oob check there as well.

- `static_size()` is more appropriate than `min_size()` for abi payload
validation, because you can have "valid" ABI payloads where the runtime
length of the dynamic section is zero, because the heads in the static
section all point back into the static section. this commit replaces
the `static_size()` check with `min_size()` check, everywhere.

- remove `returndatasize` check in external calls, because it gets
checked anyways during `make_setter` oob checks.

- add a comment clarifying that payloads larger than `size_bound()` get
rejected by `abi_decode` but not calldata decoding.

tests for each case, contributed by @trocher

---------

Co-authored-by: trocher <trooocher@proton.me>

Description for the changelog

Cute Animal Picture

image

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.85%. Comparing base (7c8862a) to head (9027c30).

:exclamation: Current head 9027c30 differs from pull request most recent head 913e748

Please upload reports for the commit 913e748 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4091 +/- ## ========================================== - Coverage 91.26% 88.85% -2.41% ========================================== Files 109 109 Lines 15565 15552 -13 Branches 3417 3416 -1 ========================================== - Hits 14205 13819 -386 - Misses 929 1227 +298 - Partials 431 506 +75 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.