foundry-rs
feat(`forge-lint`): add `UncheckedReentrancy` lint
Component
Forge
Describe the feature you would like
I'd like to propose a new lint rule called UncheckedReentrancy for forge lint. This rule should flag functions that lack a nonReentrant-like modifier and contain external calls followed by state mutations (which suggests a reentrancy bug).
Most libraries seam to use nonReentrant:
Additional context
Likely wanna support nonReentrantView as well.
https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5800
i've been looking at how Slither handles reentrancy, and they don't offer formal verification, but their heuristic is quite robust. We want to strive for maximum quality, so i would like to achieve parity with their safety guarantees.
Slither statically analyzes a contract's and produces a CFG to find the classic "read -> call -> write" reentrancy pattern. Thanks to the CFG approach, they properly handle reentrancy guards based on the contract logic (like actually based on the state updates/checks, not relying on a modifier's name).
the process is as follows:
- build the CFG: maps the "unwrapped" (accounts for internal calls and modifiers) function into a graph of all possible execution paths.
- traverse all the possible paths in the graph
- track state variables that are read along a path.
- when it finds a dangerous external call (looks like they exclude transfer() and send()), it does 2 things:
- takes a snapshot of all state variables that have been read up to that point.
- analyzes the execution path if the fn were to be re-entered at this point (if it can reach the call again, it means that there is no lock)
- finally, it continues travesing after the call. If it finds a write to any state variable that is present in its snapshot it flags a potential reentrancy. (edited)
imo, we should follow their path, and if we don't want to fall short, we can't rely on just the Visitor pattern. Instead, we should use the full HIR output (from all combined sources) and build a CFG, which is non-trivial. Because of that we will drop the "first-issue" label and plan to do it ourselves later on.
ref: https://github.com/crytic/slither/tree/dac531400a2574b8acfcf11967375727a08f49ad/slither/detectors/reentrancy
@0xClandestine would you like to contribute to this one?
I don't have a lot of time right now, probably best somebody else takes this on.
fyi this is not possible yet, we need solar to to output a CFG (control flow graph) first, right now it only lowers to HIR