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

Global pre-processing for leveraging callgraphs #605

Merged
merged 32 commits into from
Jul 28, 2024

Conversation

TilakMaddy
Copy link
Contributor

@TilakMaddy TilakMaddy commented Jul 20, 2024

CallGraphs can now be leveraged with help of StandardInvestigator - I have tried to make as many default assumptions as possible to keep the interface simple to use.

Job of the investigator is to derive a subgraph from the master workspace callgraph and provide hooks to interact with functions and modifiers while it does the following:

  • Navigate upstream
  • Navigate downstream
  • Navigate upstream's downstream-side-effects (These are nodes that are downstream from the upstream nodes but are not upstream themselves)

PS: You can provide a navigation style to save time and be efficient in terms of covering traversal ground

Pre-processing

  • The investigator relies on the callgraph to be present attached to workspace context.
  • We create forward callgraph then, transpose (flip the edges) it to get a reverse callgraph (for upstream navigation)
  • Then attach both to Workspace Context before the pipeline is run

Trackers

  • These are structures that consume from the investigators and every detector that makes use of the investigator is expected to maintain it's own tracker for tracking stuff across procedure boundaries.
  • Then this information is used to communicate back to the detector what to do about the investigation

Future scope

  • Inter contract analysis ? 🧐

Add the following detectors as proof of concept to this PR

@TilakMaddy TilakMaddy marked this pull request as ready for review July 21, 2024 09:30
@TilakMaddy TilakMaddy requested a review from alexroan as a code owner July 21, 2024 09:30
@TilakMaddy TilakMaddy marked this pull request as draft July 21, 2024 09:35
@TilakMaddy TilakMaddy marked this pull request as ready for review July 21, 2024 09:38
@TilakMaddy
Copy link
Contributor Author

TilakMaddy commented Jul 21, 2024

@alexroan There are bunch of unit tests for the callgraph framework in the file detect/experimental/cfg_sample.rs in addition to that the 3 detectors' tests :) In fact, I want to add more but I'll wait for your approval :)

Copy link
Contributor

@alexroan alexroan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like there are a few detectors and other features we're trying to squeeze in at once.
So that I know what I'm reviewing, could you reference all the tickets being addressed in this one please @TilakMaddy ?

aderyn_core/src/detect/low/sub_without_if.rs Outdated Show resolved Hide resolved
tests/contract-playground/src/Tower.sol Outdated Show resolved Hide resolved
@TilakMaddy
Copy link
Contributor Author

@alexroan You are right, I was focussed on the callgraph library and I didn't even realize I used the same Tower.sol file for testing the detectors as well as the callgraph. I have now seperated them and also under relevant file names.

SendEtherNoChecks.sol and CallGraphTests.sol

Copy link
Contributor

@alexroan alexroan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking very good @TilakMaddy . I have a few change requests before another review. We're getting close though, this looks powerful af.

@alexroan
Copy link
Contributor

I'm removing "Sub without If" from this so we can merge the callgraph without it blocking.

Copy link
Contributor

@alexroan alexroan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, with a follow up that I'll make a note of @TilakMaddy

@alexroan alexroan merged commit 57a1328 into dev Jul 28, 2024
8 checks passed
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.

2 participants