Triage database and `fail_on` not being respected

[mds1]

In https://github.com/ethereum-optimism/optimism/pull/9405 I'm trying to use slither-action in a monorepo, which seems related to the cause of my issues:

1. Running slither . locally (after triage and with config file) there are 3 findings, as expected.
2. In CI, slither has 50 findings https://github.com/ethereum-optimism/optimism/actions/runs/7808753591/job/21299473436?pr=9405#step:5:6521
3. The CI jobs are listed as passing with no new findings even though:
   1. There were 50 medium findings (https://github.com/ethereum-optimism/optimism/pull/9405/checks?check_run_id=21299634075)
   2. The CI file has fail-on: config and the config file has "fail_on": "medium"

For (3), I'm pretty sure I just need to change the repo code scanning settings to alert on Medium or higher (currently set to High or higher). But I'm hesitant to do this until (2) is fixed because I don't want to pollute repo with unexpected findings

So the two main issues here are:

1. Why is slither.db.json not being recognized? I confirmed this is the cause because I get 50 findings when running locally without the DB, but I don't see any other path settings for configuring it to recognize this
2. Why are the Slither Analysis and Code scanning results / Slither jobs listed as success, given my fail-on config? The config file itself seems to be detected, since there are no info/low/optimization findings in the outputs

[elopez]

Hi @mds1 ! Currently, slither.db.json is kind of hardcoded and gets loaded from the directory where Slither is run from1. However, your slither.db.json file lives in the project directory. In the first case you presented, the project directory matches the current directory, so it works as expected. But in the second case, Slither is run from the repo root with packages/contracts-bedrock as a project target, so no slither.db.json is found and no triaging is done.

We're chatting with @0xalpharush about adding an option/flag to control slither.db.json name and location, I'll update here when I have more to share.

For (3), keep in mind the highlighted bit; I see your PR has no Solidity code changes so maybe that's why nothing is shown? Do they appear if you click "View all branch alerts"?

Regarding why your jobs pass, I see you're using continue-on-error: true on your Slither step. That's possibly inhibiting the failure you expect. Consider removing it and using if: always()2 on the SARIF step instead to have it always upload the results, even if Slither failed.

[mds1]

We're chatting with @0xalpharush about adding an option/flag to control slither.db.json name and location, I'll update here when I have more to share.

Great, thank you! Let me know if you need me to test anything

For (3), keep in mind the highlighted bit; I see your PR has no Solidity code changes so maybe that's why nothing is shown? Do they appear if you click "View all branch alerts"?

Ahh that makes sense, confirmed I do see all 50 findings in the security tab after clicking that link. So comments are only added to PRs for new findings introduced by code changes in that PR?

Regarding why your jobs pass, I see you're using continue-on-error: true on your Slither step. That's possibly inhibiting the failure you expect. Consider removing it and using if: always()2 on the SARIF step instead to have it always upload the results, even if Slither failed.

Made that change and the "Slither Analysis" job now fails and "Code scanning results / Slither" passes.

For reference, I went with continue-on-error: true since that's what's in the README here, but re-reading perhaps I misunderstood that section.

Specifically what I'm confused about is there are now two CI tasks that show up on PRs: "Slither Analysis" and "Code scanning results / Slither". I want CI to fail if there are untriaged findings, so what are the options for this? Presumably various configs would results in one or both of those tasks failing.

Related to this is: What's the relation between triaging findings in the Slither DB and dismissing them from the repo's security tab in the UI? What's the best way to keep things in sync?

[elopez]

For (3), keep in mind the highlighted bit; I see your PR has no Solidity code changes so maybe that's why nothing is shown? Do they appear if you click "View all branch alerts"?

Ahh that makes sense, confirmed I do see all 50 findings in the security tab after clicking that link. So comments are only added to PRs for new findings introduced by code changes in that PR?

GitHub docs1 confirm that's the expected behavior (emphasis mine):

For all configurations of code scanning, the check that contains the results of code scanning is: Code scanning results. The results for each analysis tool used are shown separately. Any new alerts on lines of code changed in the pull request are shown as annotations.

Regarding why your jobs pass, I see you're using continue-on-error: true on your Slither step. That's possibly inhibiting the failure you expect. Consider removing it and using if: always()2 on the SARIF step instead to have it always upload the results, even if Slither failed.

Made that change and the "Slither Analysis" job now fails and "Code scanning results / Slither" passes.

"Slither Analysis" is the workflow that runs Slither itself, so it seems fail_on: medium is working as expected now 👍 The other one is a "synthetic" check created by GitHub when it processes the SARIF data, and whether it fails or not is controlled by some settings in the repository.

For reference, I went with continue-on-error: true since that's what's in the README here, but re-reading perhaps I misunderstood that section.

Oh, that footer note refers to a caveat when using an old Slither version (<=0.8.3). You can check the SARIF examples later on the README for a better picture.

Specifically what I'm confused about is there are now two CI tasks that show up on PRs: "Slither Analysis" and "Code scanning results / Slither". I want CI to fail if there are untriaged findings, so what are the options for this? Presumably various configs would results in one or both of those tasks failing.

To recap, the failure threshold of "Slither Analysis" can be controlled with fail_on, and the failure threshold of "Code scanning results / Slither" can be controlled with one of the toggles on the GitHub repository settings.

If you're going to be using the GitHub integration, another way you can do things is using fail_on: none and setting the "failing threshold" on GitHub settings themselves. Then "Slither Analysis" would fail if Slither has some sort of issue (e.g. compilation failure, Slither crash, etc), and "Code scanning results / Slither" would fail if you have findings worth blocking the PR.

Related to this is: What's the relation between triaging findings in the Slither DB and dismissing them from the repo's security tab in the UI? What's the best way to keep things in sync?

I believe triaging them in the DB would make them stop being reported, which I imagine GitHub will interpret as "getting resolved". The DB would also be useful for local Slither runs, whereas dismissing on GitHub wouldn't. If you're not going to be running Slither locally, then handling everything on the GitHub UI may be simpler.

[elopez]

@mds1 I created PR crytic/slither#2298 to make the DB file configurable. If you want to try it out now, you can use something like the following (note the slither-version referencing the PR branch, and the new argument in slither-args)

      - name: Run Slither
        uses: crytic/slither-action@v0.3.1
        id: slither
        with:
          target: packages/contracts-bedrock
          slither-config: packages/contracts-bedrock/slither.config.json
          fail-on: config
          sarif: results.sarif
          slither-version: dev-triage-db
          slither-args: --triage-database packages/contracts-bedrock/slither.db.json

[mds1]

Looks like 11 findings in the CI run compared to 3 locally. However, slither exited with status 255; aborting so perhaps a flake and will re-run it now.

I've see that 255 error a few times locally too, have not been able to consistently reproduce

Screenshot of local results

[elopez]

255 / -1 is the exit code that Slither uses to report a failure (e.g. in your case, a finding >= medium is present), so that's to be expected if one of the 11 findings it found is medium or higher.

As to why the filtering does not match what you see locally, that could be a separate bug. Do you see a pattern on the "extra" 8 issues? are they being produced by a specific detector?

[mds1]

255 / -1 is the exit code that Slither uses to report a failure (e.g. in your case, a finding >= medium is present), so that's to be expected if one of the 11 findings it found is medium or higher.

I see, fwiw the logging does make it seem like an error, no big deal though

And yes they are all https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-assembly-return. In the DB I see them with a check of incorrect-return which matches what's in the docs. It looks like that check was renamed at one point but the URL (which has incorrect-assembly-return) printed was not updated, unsure if that's related

[elopez]

I tried a few things locally but so far I've been unable to reproduce 11 findings; I get just 3 like you do locally. Is there anything noteworthy about your CI environment that we might be missing? (e.g. any foundry environment variables set on the repo/org level, or something like that)

edit: I forked the repo to try to match the environment as closely as possible, and it's reporting 3 findings 🤔 so it's possible it's something in particular in the repo/org configuration.

[mds1]

Thanks, any ideas at what env vars or config might affect this?: https://github.com/ethereum-optimism/optimism/pull/9405/files#diff-073755564356b30d97f5d5fd4e8dc4fc398cf0d25a45d96195f76794575ff1cfR1

If I use act to run the action locally with act -j slither-analyze, it also has 3 findings...

My best guess currently is that it might be a solc version issue? Locally with act I see:

| [-] SOLCVER was not set; guessing.
| [-] Guessed 0.8.0.
| Installing solc '0.8.0'...
| Version '0.8.0' installed.
| Switched global version to 0.8.0

And in CI I see:

[-] SOLCVER was not set; guessing.
[-] Guessed 0.8.15.
Installing solc '0.8.15'...
Version '0.8.15' installed.
Switched global version to 0.8.15

We intentionally don't set a version in foundry.toml to let foundry auto-detect, which is required since we use a few different solc versions depending on the contract

[mds1]

Oh wait, I bet it's a foundry version mismatch—need to make sure slither is compiling with the foundry version we specify in versions.json

[elopez]

If you're using Foundry, that solc compiler doesn't get used, so it's not that.

I think I figured it out @mds1 , it looks like it's just newer changes in develop that introduce these new findings. I merged origin/develop on your branch and suddenly I have 11 findings 😅 GitHub merges your branch with the target branch when running CI, so it tracks.

[mds1]

Ohh interesting! Nice catch, thank you! Confirmed I get the same 11 after rebasing on develop. That does make sense because the file those findings in was recently changed, so presumably slither invalidates the prior triage results since line numbers changed? The change just removed two unused error declarations so it would be nice if the triage result was preserved, but since it seems the DB is line-based, I see why it's hard to preserve

[elopez]

I'm not too familiar with how the triage matching works internally, but it might be something worth discussing over at the Slither repo.

Remember you can also do finding triaging with comments in the code, which might be more resilient to changes: https://github.com/crytic/slither/wiki/Usage#triage-mode

[mds1]

Created https://github.com/crytic/slither/issues/2300 to track

Otherwise, all my questions here are resolved. I'll leave this open, but feel free to close whenever you feel is appropriate. Thanks a lot for all your help!

[elopez]

Awesome! --triage-database was merged and is now available on the latest Slither release, so I'll close this issue.