Add `forge lint`

[jpopesculian]

This is a tracker for additional static analysis beyond what solc outputs. Static analysis can apply to things like safety, optimization and code style. Currently a few projects implement different components and aspects

• https://github.com/crytic/slither
• https://github.com/eth-sri/securify2
• https://github.com/trailofbits/manticore
• https://github.com/ConsenSys/mythril

Most of these tools seem to focus on security. For example securify2 tries to tackle the SWC registry where the solidity compiler fails

I took a brief look at Slither to see the complexity of implementing their Detector list. Each detector loops through the nodes in Slither's own intermediate representation known as SlithIR. SlithIR gets built by visiting the AST output given by crytic-compile. The IR attempts to provide additional data to the AST by adding variable references, scopes and the type information for builtin functions and variables. Internally, the Slither uses a visitor pattern for converting the AST to the IR, which seems to translate fairly nicely to the visitor exposed for solang-parser Parse Tree. I think the real challenge will be to create an incremental IR representation from the Parse Tree ignoring completeness but offering correctness so that we may build the static analysis tool over time. Luckily, the simplicity of Solidity scopes, as well as the strong typing of solidity, should make this a lot easier.

[jpopesculian]

I forgot to add the obvious solhint as well which splits up rules into "Security", "Best Practices", "Style Guide" and "Miscellaneous" found here. It seems to me by briefly looking through their codebase and the rule-set, that these are pretty much directly applied to the AST and don't even require another IR

[jpopesculian]

I started compiling a list of possible rule sets to implement here https://www.notion.so/3f9a35e6262746c18dd263fa01ddf0bd?v=ce13246b8bf545abb8e1886d14ebeff2 and I think it would be super simple with just the AST to implement the solhint rules which aren't covered by solc. And then we can work on the higher impact low-hanging fruit from slither and start augmenting the parse tree with references where needed

[montyly]

Hey. I am the author of Slither.

This looks interesting, however I am curious about the motivations behind rewriting what is already existing, versus contributing directly to Slither.

Building a static analysis framework is a large amount of work. Slither has support for all Solidity versions from 0.4, is maintained, has its own intermediate representation allowing precise and fast analyses. Building an IR is difficult, and we are at the frontier of the academic research with SlithIR (which is an IR with SSA dedicated for smart contracts). We also have a lot of advanced features that are already developed (including introspection for upgradeability, data dependencies, code mutation, …)

Rewriting all its logic and core seems counterproductive for the community, and fragmenting the security tools will not help developers and auditors in the long term.

If there are things that are missing in Slither, we can add them. I believe that the space would benefit from collaboration instead of competitive work. I am more than happy to discuss this here or over a call.

[jpopesculian]

Hi @montyly! I think @gakonst would be the best at answering this question, as my opinion here does not necessarily reflect the opinion of the project. That being said, I think the goal here is NOT to replace Slither. This was just an investigation of possible features for a possible tool. The goal of foundry as I have understood it, is to deliver a unified toolkit with a single binary. The power of having linting here in combination with a formatter for example would be able to do easy things such as automatically fixing things like non-underscored locally-scoped variables and such. This may contribute to other things in the toolkit down the road like maybe an LSP or the like. And of course we would like to expand the usefulness of that tool as much as possible, but I don't think we would have the time or resources to expand that scope to where Slither is. I think we just want to reach as many people as possible with easy tools for best practices and security

[gakonst]

+1 on not trying to redo all of Slither/SlithIR's work, but if we can get a reasonable set of features just by parsing with solang parser it'd be quite nice. And agree that this has downstream implications on exposing over LSP, auto-fixing etc.

At the limit we could also start detecting if Slither is present on the system and run it on each forge build maybe

[mds1]

One feature request to add to forge lint is an option for named imports, e.g. preferring import {X} from "A.sol" over import "A.sol"

[gakonst]

cc @0xkitsune re: using https://github.com/0xKitsune/solstat/ perhaps

[0xKitsune]

Hey, yeah I would be happy to integrate solstat into Foundry's build pipeline. Solstat uses the solang-parser to parse solidity contracts and then runs static analysis on the AST to look for various optimizations, QA and exploits. Right now, there is an emphasis on optimizations and QA, with only a few very low severity exploits being identified. There are quite a few new patterns that are queued up to be integrated into the next version, which range from a variety of optimizations, QA and vulns.

It would be really helpful to hear what solstat would need for a smooth integration. For example, is there a preferred way to output the identified patterns that would integrate nicely into Foundry? Right now the build is modular and flexible, so it shouldn't be too hard to adjust to what is needed.

Im also open to any thoughts/ideas that might be useful for a Foundry integration. With all this info, I can prioritize getting the updates integrated and a new version pushed.

[zerosnacks]

From @slvDev in #7668

Description

A static analyzer should be a combination of known issues, common anti-patterns, informational (eg "write better code"), and gas optimization.

• known issues
• common anti-patterns: eg Unvalidated result of ecrecover()
• informational: Style Guides, Complex calculation in expression, etc
• gas optimization: eg Optimize storage/struct layout

Suggested lint options

• --include - run only on specified files
• --exclude - exclude specified files
• --include-path - run linter on files matching the specified glob pattern.
• --exclude-path - run linter on files that do not match the specified glob pattern.
• --format: json | markdown - generate report in json or formatted markdown format
• --only-severity: high | med | low | info | gas - use only selected severity for output (can be multiple)
• --ignore-severity: high | med | low | info | gas - ignore selected severity (can be multiple)
• --with-description - usually description is large, should disable it by default to avoid too long console output.
• --show-description: detector title - show description of detector

Suggested output style

Informational: Non-specific Imports
Description(optional): This form is not recommended for use, because it unpredictably pollutes the namespace.
If you add new top-level items inside “filename”, they automatically appear in all files that import like this from “filename”.
It is better to import specific symbols explicitly.
  --> src/Counter.sol:
   |
 3 |   import "./ICounter.sol";
   |           ^^^^^^^^^^^^^^