vyperlang / vyper

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

feat[test]: implement `abi_decode` spec test #4095

Closed charles-cooper closed 3 weeks ago

charles-cooper commented 1 month ago

What I did

add a spec-based differential fuzzer test for abi_decode

How I did it

How to verify it

Commit message

this commit implements a spec-based differential fuzzer for
`abi_decode`.

it introduces several components:

- a "spec" implementation of `abi_decode`, which is how vyper's
  abi_decode should behave on a given payload, implemented in python

- a hypothesis strategy to draw vyper types

- hypothesis strategy to create valid data for a given vyper type

- a hypothesis strategy to _mutate_ a given payload which is designed
  to introduce faults in the decoder. testing indicated splicing
  pointers into the payload - either valid pointers or "nearly" valid
  pointers - had the highest success rate for finding bugs in the
  decoder. the intuition here is that the most difficult part of the
  decoder is validating out-of-bound pointers in the payload, so
  pointers represent "semantically high-value" data to the fuzzer.

- some hypothesis tuning to ensure a good distribution of types

over several days of testing+tuning, this fuzzer independently found
the bugs fixed in 44bb281ccaa and 21f7172274e (which were originally
found by manual review).

Description for the changelog

Cute Animal Picture

![Put a link to a cute animal picture inside the parenthesis-->]()

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 49.20%. Comparing base (44bb281) to head (f74024c).

:exclamation: Current head f74024c differs from pull request most recent head 0976fb5

Please upload reports for the commit 0976fb5 to get more accurate results.

Files Patch % Lines
vyper/builtins/functions.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4095 +/- ## =========================================== - Coverage 91.26% 49.20% -42.07% =========================================== Files 109 109 Lines 15554 15552 -2 Branches 3415 3417 +2 =========================================== - Hits 14196 7652 -6544 - Misses 928 7302 +6374 - Partials 430 598 +168 ```

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

cyberthirst commented 4 weeks ago

i think that for the sake of completeness, we should also test with dirty memory

let's consider that a ptr points outside the buffer to a) dirty memory b) clean memory

we currently only test for b) and require that the spec matches the implementation

the spec should always raise when ptr points outside the buffer. but what if the implementation doesn't revert when pointing outside the buffer to dirty memory?

cyberthirst commented 4 weeks ago

i think it's important to include structs - one of the original errors was incorrect validation of structs within dynarrays

charles-cooper commented 4 weeks ago

i think it's important to include structs - one of the original errors was incorrect validation of structs within dynarrays

I mean structs share the same code path as tuples, but with named fields in the front-end. I don't think we are missing much by not including them

cyberthirst commented 3 weeks ago

i think it's important to include structs - one of the original errors was incorrect validation of structs within dynarrays

I mean structs share the same code path as tuples, but with named fields in the front-end. I don't think we are missing much by not including them

we skip them for dynarrays, right?

charles-cooper commented 3 weeks ago

i think it's important to include structs - one of the original errors was incorrect validation of structs within dynarrays

I mean structs share the same code path as tuples, but with named fields in the front-end. I don't think we are missing much by not including them

we skip them for dynarrays, right?

Ah right, those are disallowed by the language semantics. Ok let's add them