False negatives in property-based tests

[pcaversaccio]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (b454567 2023-02-06T00:29:35.2383332Z)

What command(s) is the bug in?

forge test

Operating System

Linux

Describe the bug

I realised that within a forge test run, sometimes addresses are fuzzed which are very unfortunate choices and it's getting complicated to always assume the right params. In order to reproduce what I exactly mean, let's have a look at the following run: https://github.com/pcaversaccio/snekmate/actions/runs/4134068222/jobs/7144742870

This one fails with the following error:

[FAIL. Reason: Call did not revert as expected Counterexample: calldata=0xa32994070000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b00000000000000000000000000000000000000000000000000000000000041600000000000000000000000000000000000000000000000000000000000001e190000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000009c9, args=[0x2e234DAe75C793f67A35089C9d99245E1C58470b, 0x0000000000000000000000000000000000004160, 0x0000000000000000000000000000000000001e19, 0x00000000000000000000000000000000000000000000000000000000000009c9]] testFuzzSafeTransferFromWithData(address,address,address,bytes) (runs: 58, μ: 1456762, ~: 1457262)

So the problem here is the input address 0x2e234DAe75C793f67A35089C9d99245E1C58470b is unfortunately the same as the erc721ReceiverMock I create in the test:

ERC721ReceiverMock erc721ReceiverMock = new ERC721ReceiverMock(
    receiverMagicValue,
    ERC721ReceiverMock.Error.None
);

After re-running the test everything passes as intended: https://github.com/pcaversaccio/snekmate/actions/runs/4134068222.

Also, I started using things like the following since sometimes it fuzzes also the test contracts etc.:

vm.assume(account.code.length == 0);

My question I would like to discuss is, whether other face similar problems & what can be done to fine-tune it. For instance, having a sort of modifier that ensures that any fuzzed addresses within a test function should not equal any created contract addresses within the test function would already help a lot.

[wirew0lf]

Came across this issue, by deafault I've seen foundry tends to use addresses of contracts defined in the setUp() function as msg.sender in invariant testing. As a workaround I've been excluding them with the excludeSender() function.

[alexroan]

+1 on this issue.

[pcaversaccio]

For the sake of history, our recent Twitter discussion about this issue (check the different threads).

[grandizzy]

@pcaversaccio there is a fix in that could help #7595 basically if you use targetContract and targetSender to mark the contract to be fuzzed / sender's, there should be no exception from. Would that help in your scenario above? Thank you!

[pcaversaccio]

@pcaversaccio there is a fix in that could help #7595 basically if you use targetContract and targetSender to mark the contract to be fuzzed / sender's, there should be no exception from. Would that help in your scenario above? Thank you!

Yup, that helps indeed. I recently get a lot of CreateCollision collisions in my tests (example CI: https://github.com/pcaversaccio/createx/actions/runs/8869059876/job/24349330795#step:16:257)

Any Idea how to prevent this in a smart way (usually rerunning fixes it, but it's annoying tbh)

[grandizzy]

yeah, is same issue with failures described in #7497 (comment) I managed to reproduce locally by creating a failures file with the counterexample from GH action log you pointed (that is containing single line cc ce87454cc0ca562369e13d0e7219ae6deb1133b8d919db8f5b058f670f243f93), will make a fix for forge clean

[grandizzy]

@pcaversaccio can this one be closed now?

[pcaversaccio]

Yes.