Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slither doesn't resolve references across compilation units #2102

Open
hacker-DOM opened this issue Aug 31, 2023 · 7 comments
Open

Slither doesn't resolve references across compilation units #2102

hacker-DOM opened this issue Aug 31, 2023 · 7 comments
Labels
documentation question Further information is requested

Comments

@hacker-DOM
Copy link
Contributor

hacker-DOM commented Aug 31, 2023

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
Copy link
Contributor

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.

@0xalpharush 0xalpharush added question Further information is requested documentation labels Aug 31, 2023
@hacker-DOM
Copy link
Contributor Author

hacker-DOM commented Sep 1, 2023

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
Copy link
Contributor Author

hacker-DOM commented Sep 1, 2023

Guys, Slither does not have a reference resolver. This is not good practice and will lead to problems down the road. Some like #2107 may be easy to solve, others not.

Slither cannot even answer the simplest questions in static analysis:

  • where is this function called?
  • where is this contract derived?

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
Copy link
Contributor Author

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

@montyly
Copy link
Member

montyly commented Sep 5, 2023

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
Copy link
Contributor Author

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
Copy link
Member

montyly commented Sep 20, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants