Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider not passing precompile addresses as fuzz input #1228

Closed
onbjerg opened this issue Apr 7, 2022 · 5 comments
Closed

Consider not passing precompile addresses as fuzz input #1228

onbjerg opened this issue Apr 7, 2022 · 5 comments
Labels
A-testing Area: testing C-forge Command: forge Cmd-forge-test Command: forge test
Milestone

Comments

@onbjerg
Copy link
Member

onbjerg commented Apr 7, 2022

Some developers find their tests failing because the fuzzer gives them precompile addresses as inputs. We should consider not doing this - is there a legitimate reason to ever have a precompile address as input?

Ref #1159

@onbjerg onbjerg added A-testing Area: testing Cmd-forge-test Command: forge test C-forge Command: forge labels Apr 7, 2022
@mds1
Copy link
Collaborator

mds1 commented Apr 7, 2022

Was chatting with @transmissions11 about this, and don't think it's a good idea for a few reasons:

  1. There could be some obscure edge case in your contracts with those addresses, and we don't want to miss that by excluding them.
  2. Precompile addresses are valid inputs an attacker can send to your contract, so they should not be excluded from testing.
  3. A forge-std helper can wrap vm.assume to ensure the address passed from the fuzzer isn't a precompile, without affecting the core fuzzer, so this doesn't really need to be integrated natively anyway.

@onbjerg onbjerg added this to the v1.0.0 milestone Jul 1, 2022
@mds1
Copy link
Collaborator

mds1 commented Jul 19, 2022

@onbjerg Had this come up recently in a repo. Any objections if I PR in a noPrecompiles modifier or function into forge-std and use that to close this issue?

@onbjerg
Copy link
Member Author

onbjerg commented Jul 19, 2022

I suppose that's ok

Edit: We would need some way to support different flavors I'd assume though. E.g. Optimism's precompiles

@mds1
Copy link
Collaborator

mds1 commented Jul 19, 2022

We would need some way to support different flavors I'd assume though. E.g. Optimism's precompiles

Yep, I just created foundry-rs/forge-std#134 which has a suggested approach on how to do that

@onbjerg
Copy link
Member Author

onbjerg commented Jul 19, 2022

Going to close this in favor of that issue then 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Area: testing C-forge Command: forge Cmd-forge-test Command: forge test
Projects
No open projects
Archived in project
Development

No branches or pull requests

2 participants