Add Slither analysis to pull requests

[0xJem]

Slither will be run on each pull request and listed under the "Checks" section.

e.g. https://github.com/0xJem/olympus-contracts/pull/1/checks?check_run_id=5675243069

[ghost]

Seriously cool!

that is a lot of results though >.> i removed a couple that were obv from the severe ones / that won't get replaced

I'd check out the name duplications and if not just ignore all and pray

[0xJem]

From @ind-igo :

i think we may not want it as a check on the repo at the moment though. i think moving forward, we want the action setup on new repos

Right now, the slither output is not showing up under the security tab. I think we need to enable code scanning for that to work: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/setting-up-code-scanning-for-a-repository

@ind-igo @appleseed-iii if you have admin access to the repo, can you enable that?

Secondly, I can't see a way to disable a failing check (code scanning results) causing the failure of the entire build. If we can get the security reports running, I can see if it's possible to remove the Slither output as a "check" on the PR.

[ghost]

From @ind-igo :

i think we may not want it as a check on the repo at the moment though. i think moving forward, we want the action setup on new repos

Right now, the slither output is not showing up under the security tab. I think we need to enable code scanning for that to work: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/setting-up-code-scanning-for-a-repository

@ind-igo @appleseed-iii if you have admin access to the repo, can you enable that?

Secondly, I can't see a way to disable a failing check (code scanning results) causing the failure of the entire build. If we can get the security reports running, I can see if it's possible to remove the Slither output as a "check" on the PR.

Wait, disabling the failing checks you mean as in stop slither output because of the old contracts? Yeah it's a problem. I mean I don't see the purpose of actually changing the code in repo since the code should reflect what is actually deployed. Is there no config with which you can specify file paths to check out contracts?

[appleseed-iii]

just fyi, I'll leave this to @ind-igo and the rest of ya'll to decide so I don't step on any of this team's processes.

[0xJem]

@PARABRAHMAN0 @ind-igo I'm looking at how we can reduce the number of warnings, a concern that was raised earlier.

One suggestion was to not analyse contracts that have already been deployed. This would require maintaining a list of filenames as an exclude list, and would require devs to remember to update it after deployment. Is that feasible?

The other options is slither's triage mode: https://github.com/crytic/slither/wiki/Usage#triage-mode
This would require a dev to run the command (slither . --triage-mode) and tell slither to ignore any warnings that are no longer relevant (or which can't be acted upon). This will get written to a JSON file and committed to the repo, and will prevent the slither action in github from reporting on that issue.

Which would you prefer?