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

Compare upgrades in slither.utils.upgradeability #1699

Closed
wants to merge 24 commits into from

Conversation

webthethird
Copy link
Contributor

@webthethird webthethird commented Feb 27, 2023

Based on @montyly's recommendation re: adding what I had been doing in my diff_with_v2.py directly within Slither, instead of as a new upgradeability check.

This adds a new slither.utils.upgradeability containing a function compare(v1: Contract, v2: Contract), which returns a dictionary of lists of Variables/Functions, and a helper function is_function_modified(f1: Function, f2: Function), which returns True if the function is modified or False otherwise.

The dictionary returned by the compare function has the following lists:

{
    "missing-vars-in-v2": list[Variable],
    "new-variables": list[Variable],
    "tainted-variables": list[Variable],
    "new-functions": list[Function],
    "modified-functions": list[Function],
    "tainted-functions": list[Function]
}

In this context, a tainted variable is a preexisting variable which is read or written by a new or modified function, and similarly a tainted function is an unmodified function that calls a modified function.

I made sure to address the feedback he gave me:

  • You need to check of immutable here
  • An issue with the comparison with get_summary, is that it might miss a change in the logic of the function. Like if a condition is inverted for example. One thing that we could use is function.source_mapping.content_hash , this returns an hash for the function source code. So if the source code differ you can detect it. It’s fast, however a downside is that if there is a change in a name, or in a comment, it will lead to a false positive. A better approach would be to compare the CFG of the two function; because we are looking for strict equivalence here, it should not be too difficult to do. Even a naive approach to walk through the CFG and compare the IR opcode should give good result I think.

montyly and others added 15 commits February 13, 2023 15:25
- update references to compilation frameworks
- add link to documentation site
- move installation instructions front and center
- explain usage for projects with dependencies
in `slither.utils.upgradeability`
 cut down on branches and nested blocks
 by adding an `is_function_modified` helper function
@webthethird
Copy link
Contributor Author

This was blocked by a bug in the CI tests, but all checks are passing now

@webthethird
Copy link
Contributor Author

This is now included in PR #1757

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants