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

no-fail CI option seems to conflict with config file exclude_informational #38

Closed
0xCLARITY opened this issue Oct 11, 2022 · 2 comments
Closed

Comments

@0xCLARITY
Copy link
Contributor

I have a slither config file that looks like this:

{
  "detectors_to_exclude": "timestamp,uninitialized-local,variable-scope",
  "exclude_dependencies": true,
  "exclude_informational": true,
  "solc": "0.8.17",

  "filter_paths": "lib",
  "solc_remaps": ["ds-test/=lib/ds-test/src/", "forge-std/=lib/forge-std/src/"]
}

With Slither 0.9.0, even running slither locally, I seem to need to pass the --no-fail-pedantic flag to get it to properly parse the exclude_informational flag in the config file as described here: crytic/slither#1408

In a fork of slither-action, I tried echoing the --no-fail-pedantic flag if the fail-on was set to config:

if [ "$FAIL_ON_LEVEL" = "config" ]; then
       echo "--no-fail-pedantic"
       return
fi

This does get it to parse the exclude_informational flag in the config file but it incorrectly marks CI as successful even when I have a high severity failure (in my test case, I have some assembly that triggers https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-shift-in-assembly).

Any ideas on what the fix is here? Or do I need to rearrange my config file / CI setup to adhere to the newest version of Slither?

@elopez
Copy link
Member

elopez commented Oct 11, 2022

HI @0xCLARITY, thanks for the report! With the current implementation in Slither, fail-pedantic is mutually exclusive with exclude-informational and exclude-optimization, on the rationale that asking slither to fail on anything >= informational and then excluding informational findings does not make much sense (although it's unfortunate that optimizations and informationals are combined on that level). If you have any feedback or ideas to share on how to improve the situation, crytic/slither#1408 would be the best place for it.

As for a way forward for your use case at this time, have you considered using fail-on: config plus something like this? All of these new command-line flags can also be provided in the config file.

{
  /* this is the default level, so disable it */
  "fail_pedantic": false,
  /* and enable other higher level, eg. low */
  "fail_low": true,

  /* and exclude informational */
  "exclude_informational": true,

  /* other options */
  /* ... */
}

@0xCLARITY
Copy link
Contributor Author

That worked! Thank you for the guidance for how to work with the current implementation of fail-on!

I'll close this issue out.

maurelian added a commit to ethereum-optimism/optimism that referenced this issue Apr 14, 2023
This change implements the information provided in these slither
issues:
crytic/slither#1408
crytic/slither-action#38

Apparently 'fail_pedantic' defaults to true, which overrides the
'exclude_*' settings and causes failures on optimization and
informational severity findings.

As is this config should:
1. Fail only on High severity findings
2. Print Medium severity findings but not fail on them.

This commit also deletes the findings in the slither db which
were less than high severity. They are now simply ignored via this
corrected config.
maurelian added a commit to ethereum-optimism/optimism that referenced this issue Apr 14, 2023
This change implements the information provided in these slither
issues:
crytic/slither#1408
crytic/slither-action#38

Apparently 'fail_pedantic' defaults to true, which overrides the
'exclude_*' settings and causes failures on optimization and
informational severity findings.

As is this config should:
1. Fail only on High severity findings
2. Print Medium severity findings but not fail on them.

This commit also deletes the findings in the slither db which
were less than high severity. They are now simply ignored via this
corrected config.
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

No branches or pull requests

2 participants