crytic / slither

Static Analyzer for Solidity and Vyper
https://blog.trailofbits.com/2018/10/19/slither-a-solidity-static-analysis-framework/
GNU Affero General Public License v3.0
5.31k stars 965 forks source link

Slither doesn't resolve references across compilation units #2102

Open hacker-DOM opened 1 year ago

hacker-DOM commented 1 year ago

As discussed on telegram, it seems to me that Slither does not resolve references across compilation units. Here is a reproduction repo: https://github.com/hacker-DOM/reference-resolving. The tldr is multiple Contract instances will exist in Slither.contracts for each contract that spans multiple compilation units, and so a regular usage of the Api like finding a contract with a given name (if there's just one in my codebase) and looking for its derived contracts will report inaccurate results.

0xalpharush commented 1 year ago

There will be two contracts called A as they potentially have different behavior and are thus analyzed separately e.g. a staticcall is used for view versus call. Right now your script only looks at the first one (using next). The references are indeed missing but I do not see how this supports saying that Slither cannot be trusted on TG. If you have experienced false negatives, then we'd appreciate bug reports.

hacker-DOM commented 1 year ago

Josselin's response:

Actually this is a misuse of the python API 😅

  • First, there is two compilations units here, so two contracts named A. If you use next() over a python set, you will only see the first one
  • Then within the context of A there is no reference to a_main, so reference is correctly empty. If you want to see the reference, you need to access the function A.a_main() that lives in the contract B or C.

Functions and contracts are duplicate based on their inheritance usage, as it's needed for static analysis. Happy to continue this discussion on github to not add noise here. But we definitely have room for improvements in the documentation of slither's internals.

However saying slither cannot be trusted because you identify a potential bug sounds extreme tbh 🙂 (in particular if the bug is in your script 😅)

hacker-DOM commented 1 year ago

Guys, Slither does not have a reference resolver. This is not good practice and will lead to problems down the road. Some like https://github.com/crytic/slither/issues/2107 may be easy to solve, others not.

Slither cannot even answer the simplest questions in static analysis:

All of Slither's results are with respect to a compilation unit, which is not intuitive for users.

EDIT: I think I was a bit too courteous here. It's not just unintuitive, it's plain simply wrong. If you give 100 developers/auditors the above codebase and ask them what are the derived contracts of A (and give them unbounded time), they would all say B and C. Slither unfortunately doesn't abstract compilation units from the users and so gives answers that are just wrong. Nobody audits code with the compilation graph in their head. This will be an even bigger problem for frameworks that prefer large numbers of compilation units (eg woke lsp), bc one file might be in 10 compilation units.

If you guys understand the value of resolving references to identical objects across compilation units, you can build slither on top of woke.

hacker-DOM commented 1 year ago

Reference resolving is currently one of the hardest problems in Solidity static analysis.

montyly commented 1 year ago

What do you mean by Reference resolving is currently one of the hardest problems in Solidity static analysis. ?

I believe Slither has solved this a long time ago, and it can actually answer the two questions you asked ;) (#2107 is a UI problem, not a feature problem)

hacker-DOM commented 1 year ago

Hey @montyly , I can provide a detailed explanation early next week but just a sanity check - which @property (or other Api method) is supposed to give references / derivations across all compilation units?

((C.a_main.references will return only c_main and B.a_main.references will return only b_main))

montyly commented 1 year ago

We don't have a direct API for that, but it just requires looping over the compilation units and gather the information if it's what you are looking for ;)

However we can add user-facing API if this is useful