Perform cross-contract taint analysis from diff of two upgrade versions #1816
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Given a list of functions from one contract, finds tainted functions/variables in other contracts
and functions that call tainted functions
in addition to tainted functions
…lity-util-cross-contract-taint
montyly
reviewed
Apr 4, 2023
| @@ -273,7 +409,7 @@ def encode_ir_for_compare(ir: Operation) -> str: | |||
| if isinstance(ir, Assignment): | |||
| return f"({encode_var_for_compare(ir.lvalue)}):=({encode_var_for_compare(ir.rvalue)})" | |||
| if isinstance(ir, Index): | |||
| return f"index({ntype(ir.index_type)})" | |||
| return f"index({ntype(ir.variable_right.type)})" | |||
There was a problem hiding this comment.
Can we add a test that highlight why this was wrong? It will help in the long term
There was a problem hiding this comment.
It only became wrong when the index_type property was removed from the Index class. Slither was just crashing when I tried to access the property, since it didn't exist anymore. Is that still something we can test?
and add TaintedFunction and TaintedVariable classes
I've updated the test in test_upgradeability_util, adding some external taint to the test contracts, and I added some in-line comments. |
|
I believe this one is ready for another review @montyly or @0xalpharush |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In
slither.utils.upgradeability, thecomparefunction found functions and variables in the contracts being compared that were tainted due to new or modified functions. But functions and variables in other contracts may also be tainted by these changes due to external calls.This PR currently adds two functions to
slither.utils.upgradeability, one of which is used bycompareto perform cross-contract taint analysis and return more information in the diff results:tainted_external_contractstakes a list of functions, i.e., the new, modified and tainted functions found incompare, and searches them for high-level calls to other contracts. It marks the corresponding functions (and public variables) as tainted, as well as any variables they write to. Then, it searches for any other functions in the external contract which read or write to tainted variables. It returns a list ofTaintedExternalContractobjects, which are typed dicts containing the contract object plus two lists of tainted functions and variables.comparecalls this function, after finding all new, modified and tainted functions in the diffed contracts, and returns the list along with all original outputs.tainted_inheriting_contractsaddresses a shortcoming oftainted_external_contracts, which is that high-level calls may take advantage of polymorphism and call functions from a base contract, causing the method to miss tainted functions and variables defined in contracts that inherit the base class.tainted_inheriting_contractstakes a list ofTaintedExternalContractobjects as input, as well as an optional list of contracts to search for inheritance, and outputs a potentially augmented list of tainted contracts with their tainted functions and variables. It searches the list of contracts (or the contracts in the compilation unit) for contracts that inherit from tainted contracts, and searches their declared functions for internal calls to previously tainted functions or reads/writes to tainted variables. It then appends the newTaintedExternalContractto the original list.These functions are not strictly related to upgradeability, so I'm not sure if they belong here or somewhere else. I am open to any suggestions/feedback!