Slither pipeline integration

[jcsec-security]

Description

Addition of the Slither static analyzer to the pipeline of the repo. Highlights of the current configuration:

• Runs on PRs.
• Only checks for medium or high-risk findings.
• There are two silenced detectors "uninitialized local variables" and "unused returns".
  • Ideally, the second one should be included in the analysis, so we should review the impact of solving the current hits (basically adding some require(success) around).
• It uses foundry for compilation instead of pnpm, creating the foundry.toml file as part of the GH action.
  • I have tried compiling it through our recommended steps and then running slither afterwards, but it always ends up erroring... Ideally this should replicate the exact tool chain we used so this remains as a TODO

In addition, AAFactory.sol was slightly modified to solve one slither finding, which doesn't look exploitable. Please check this doesn't affect the current functionality.

Additional context

https://github.com/crytic/slither

[jcsec-security]

Removed the exclusion on unused-returns while working on fixing all instances, as discussed.
Pending ones atm:

• .excessivelySafeCall
• .decodeSignature
• .systemCallWithPropagatedRevert
• IExecutionHook(...).preExecutionHook

[cpb8010]

This looks great! I didn't notice this until you mentioned it in Slack!

[jcsec-security]

@cpb8010 @ly0va Do you think adding require() in the pending scenarios mentioned above could have some negative impact?

[cpb8010]

@cpb8010 @ly0va Do you think adding require() in the pending scenarios mentioned above could have some negative impact?

No, lyova added them here: #260

[jcsec-security]

I mean these ones:

• .excessivelySafeCall
• .decodeSignature
• .systemCallWithPropagatedRevert

[cpb8010]

• .excessivelySafeCall

So this one specifically I'm not sure under what circumstances it returns a failure (assuming if the call reverts). We initially had this so you could remove a module or hook without the risk of the uninstall reverting. If we check the result we should just remove the call since we have a backup method.

[jcsec-security]

I think it is 100% valid as a solution to the scenario you mentioned, we should add a note as comment but I don´t think we should remove it. We could also turn it into a flag to be passed as an additional param during uninstall (force?) so we can actually differentiate between desired bypass and silently failing.