feat: test for reentrancy in invariant tests

[mds1]

Component

Forge

Describe the feature you would like

In #1572 invariant testing will be added to forge. This basically executes a sequence of random calls in attempt to break some invariant.

This is really valuable but it can't test reentrancy, which is common class of vulnerabilities. @brockelmore had an idea for testing this, which is:

• For each state mutating function, generate a contract that has a fallback method which calls that method.
• Add each of these contracts to the list of potential callers.
• If one of your contracts ever transfers control back to the caller, this contract will reenter when it's the caller and hopefully break your invariant.

One downside is that each of these contracts would have no state in the system until its first call, which isn't always ideal. One way to resolve this is to generate a single contract that can call all methods with any calldata, and randomly choose which it will reenter. That contract will more frequently be the caller and likely to have state in the system than if we generated many contracts.

In general, there's many variants of reentrancy vulns and the above won't cover all of them. This is very much a work-in-progress idea that we'd need to spec out more before implementing, but wanted to open this issue so we don't lose track of it. Ideas welcome 🙂

Additional context

No response

[onbjerg]

Was this solved?

[mds1]

There's a simple version of reentrancy invariant testing that @joshieDo added and is currently off by default, but the approach isn't perfect (not sure any approach will be though). IMO worth keeping this open just to track the idea / as a place to drop any ideas on the best way(s) to do this

[grandizzy]

Here's a sample on how reentrancy can be tested in invariants (adapted from https://github.com/rappie/echidna-rari-hack): https://github.com/grandizzy/foundry-rari-hack

@mds1 do we need more of these or OK to close ticket for now and improve as we go? (would probably make sense to follow up with more testing with call_override = true setting)

[mds1]

From a quick read of that repo it seems it manually sets up a handler for reentrancy testing. If that's the approach we want to recommend (since I don't think call_override was very useful) then we can close this and just update docs accordingly (if not already done). But if we want to improve the built-in call_override way of reentrancy testing then we should either keep this open or make sure that's tracked in another issue

[grandizzy]

Yeah, had some testing for same scenario with call_override without any good results, there's more effort to put in to make it work.

Agree, let's keep this one to make call_override robust