fix(invariant): do not record newly created contracts as targetContracts

[Evalir]

Motivation

Potential fix to #6166 and #5625 cc @mds1 @PatrickAlphaC

Solution

Right now, targetContracts() is not "static" after setUp()—it gets updated after each depth call with the newly created contracts and their selectors. This has been foundry's behavior since invariants were added.

However, people expect to only see contracts added in targetContracts during setup to be used for creating inputs. This means that if contracts are deployed after setup they should not see them in fuzz runs.

The fix is to stop adding these new contracts to the targeted contracts.

[mds1]

@Evalir Could you get a bit more concrete on describing the new behavior, e.g. if I use any target* does that prohibit new contracts from being added, and if I use exclude* do new contracts still get added?

I also wonder if this should be behind a flag since it's technically a breaking change? Wonder if anyone relies on this, cc @lucas-manuel

[Evalir]

Yep @mds1 so expanding & clarifying:

• Foundry keeps the list of target contracts through targeted_contracts.
• This list is built, either by:
  • the user explicitly calling it during setup, or
  • foundry adding all contracts deployed during setup if no targeted contracts are specified by the user.
• Right now (which is the potential issue), after each depth (call to any of the targeted contracts), the resulting deployed contracts, or contracts with modified state (included in the state changeset), are collected, and added to targeted_contracts. This means that, in the case of 6166, as WETH9 gets modified with the handlers due to moving tokens around, this contract will be added to targeted_contracts.
• These added contracts get cleared at the end of each run.
• This means that using target contracts does not really restrict new contracts from getting added, and in fact we're actively doing it on each depth. This is not necessarily hevm's behavior, and it's what we should probably discuss. I agree this might be a breaking change, albeit I don't necessarily think people are knowingly relying on it, because we don't explicitly state this anywhere. Using exclude* does avoid adding the specified contracts though.

super curious about you guys's thoughts—i think we need to explicitly document this.

[lucas-manuel]

Hmm interesting, my first reaction is to avoid adding these automatically, and document that if a dev wants to add contracts deployed during a run to target contracts, they should do so explicitly (in a handler for example).

Just to clarify:

the user explicitly calling it during setup, or
foundry adding all contracts deployed during setup if no targeted contracts are specified by the user.

Does this adding happen in the first option? Cause i'm thinking it could be nice if we did:

1. If a user specifies targetContracts in setUp, they have to explicitly add targets in handlers during a run
2. If they don't, all deployed contracts both in setUp and during a run will be added.

cc @Evalir @mds1

[webthethird]

• This means that using target contracts does not really restrict new contracts from getting added, and in fact we're actively doing it on each depth. This is not necessarily hevm's behavior, and it's what we should probably discuss. I agree this might be a breaking change, albeit I don't necessarily think people are knowingly relying on it, because we don't explicitly state this anywhere. Using exclude* does avoid adding the specified contracts though.

For what it's worth, I am doing some invariant testing in which this has become a problem for me. I have a contract that manages rewards for staking with a variety of tokens, and I want to test its behavior when new staking contracts are added. In one of my handlers I deploy a new underlying ERC20 as well as its staked counterpart, which requires its own handler. When trying to explicitly add the new handler using targetContract(address(newStakedTokenHandler)), I also call excludeContract for both the new staking contract and its underlying token.

After reading this thread, it seems that calling targetContract once the handler is deployed may not be necessary, though in my opinion it should be required to add it to the target_contracts list explicitly. More to the point, however, calling excludeContract does not seem to have any effect, as the fuzzer is still making direct calls to both the staking contract and the ERC20. This either causes my tests to revert and fail, since I have fail_on_revert set to true, or it introduces discrepancies with the ghost variables I'm using, again causing the tests to fail.

[mds1]

Maybe the simplest solution here is to have a global default for whether or not to add new contracts during a run. This would apply regardless of how you setup targeted/excluded contracts. Then, have a config flag to change the default, which can be set in foundry.toml, and changed per-test or per-contract with the inline config comments.

[webthethird]

@mds1 would it still be possible to explicitly add or exclude contracts during a run, if it's configured to not add new contracts automatically?

[mds1]

To that point, on second thought, maybe a better UX here is ditch my config flag idea and just add a new contract getter method, contractsToAdd. This would return the array of artifact names to add if they're seen deployed during the run. Or it can return an array of length 1 of "None" or "All"to totally enable/disable the setting

[webthethird]

Happy new year!
Is there an ETA on when a solution to this might be implemented?

[Evalir]

hey @webthethird ! i'll be picking this up again soon. Just needs a bit of a different approach as currently some tests are failing, and we probably wanna discuss the UX a bit further.

[ClaudeZsb]

Hey, guys, I just encoutered the similar case of #6166 , that in each run of an invariant test, the second call is made to non-target contract at a non-target function with very high possibility. In my case, the handler is dealing with token approval and access control. So it does interact with several pre-deployed contracts which are not supposed to be targets of invariant testing. Before reading this pr, I just have no idea of its reason.

Thanks to @Evalir for your very detailed explanation that answers my question. Waiting for updates.

Does the exclude work for bypassing this situation? So that I could set all interacted contracts to excluded.