Slither CI

[jubeira]

Description

This PR:

• Adds Slither to CI.
• Adjusts the triaged results for the current state of interfaces and solidity-utils.
• Improves the scripts to run slither.
  • Now it can be executed from within each package without activating the virtual environment manually.
  • It makes use of the existing build commands to build first (only if required). Without this change it always had to build the packages before running the analyzer, which is inefficient.
  • Breaking change: yarn build does the same as before, but yarn compile does not remove the artifacts anymore.

I've added a section with the results of the Slither triage. Maybe we can improve these templates and add a table or something standard to describe what we are skipping, since the DB files are not human-readable.
To verify, download the changes and revert the commits with the triage results.

On top of that: I've considered having one config file for each package instead of having a global one. E.g. in interfaces we are always going to skip the compiler version check. But ultimately I think it's not a huge improvement, since it'd create more config files. I also considered creating a hidden .slither folder inside each package to store the config file and the DB with the triage results, but apparently the DB's path cannot be configured :(

Type of change

• Bug fix
• New feature
• Breaking change
• Dependency changes
• Code refactor / cleanup
• Documentation or wording changes
• Other

Checklist:

• The diff is legible and has no extraneous changes
• N/A Complex code has been commented, including external interfaces
• N/A Tests have 100% code coverage
• The base branch is either main, or there's a description of how to merge

Issue Resolution

N/A

Slither triage

See complete results without the changes to the DBs: slither-triage.txt.

Summary

Summary of skipped detections below:

Interfaces

• Pragma version^0.8.0 (contracts/solidity-utils/helpers/ITemporarilyPausable.sol#3) allows old versions

Solidity-utils:

• LogExpMath performs a multiplication on the result of a division (multiple instances)
• FixedPoint uses assembly (multiple instances)
• Function LogExpMath is not in mixedCase (multiple instances)
• LogExpMath uses literals with too many digits (multiple instances)
• FixedPoint.MIN_POW_BASE_FREE_EXPONENT (contracts/math/FixedPoint.sol#23) is never used in FixedPoint

[jubeira]

Note: I had to run yarn slither:triage 4 times before I got to store all the findings in the DB; apparently you can't triage everything at once?

Eventually you end up running yarn slither without findings locally, and it also works in CI.
There's a field in the DB with the absolute file path, but apparently it's not using it? Otherwise it wouldn't work in CI I suppose.
See crytic/slither#1138 (comment).

[jubeira]

I found out that running the analyzer repeatedly might yield different results.
In particular, in 17b6c3f I ran the analyzer 10 times and got the following:

• No errors 7 times
• 1 error 2 times
• 2 errors 1 time

I updated 13f71b4 by running yarn slither:triage and skipping the findings (more performs a multiplication on the result of a division issues) until it didn't find anything 10 times in a row.

All in all, this looks somewhat brittle. It should always be analyzing the same files with the same inputs and skipping the same problems, but the results vary from run to run.
I could be wrong, but it looks like a bug in slither itself.

EDIT: looks like a bug indeed; I've left a public example for the maintainers to review in balancer/balancer-v2-monorepo#2514.

[ylv-io]

🚢 🚢 🚢
It is indeed disappointing that Slither behaves in a non-deterministic manner. We can certainly hope for improvements in future versions. Nevertheless, I believe it's a valuable addition to our toolkit and CI pipeline. Let's make it a practice to run it prior to every major release, even with its current limitations.

[jubeira]

One last thing: if you run yarn build and then yarn slither, the compiler won't run because the cache exists, but slither will fail because the build info for the contracts is deleted with build.

You can make it work again by clearing the cache and running yarn slither again.
If this is an inconvenient, I'd either remove the rm -rf artifacts/build-info step from yarn build, or add a new command that forces the build with slither.