ci: add slither via slither-action (#9405)

* chore: remove old slither setup

* ci: add github actions slither ci file

* chore: simplify slither config

* chore: add initial slither db

* style: fmt slither db for easier diffing

* chore: more config tweaks + updated db

* chore: helper scripts and docs

* ci: fix slither target path

* ci: bump upload-sarif version

* fix: prevent slither-action from trying to install JS deps

* fix: provide path to slither config

* ci: change from continue-on-error to 'if: always'

source: https://github.com/crytic/slither-action/issues/70\#issuecomment-1932266372

* ci: test new path to triage database

* chore: retriage after rebasing against develop

* style: minify slither db since we won't use it

* chore: updates to use github code scanning instead of triage db

* ci: fail code scanning upload step, not slither step

* doc: clarify CI jobs

* chore: rebase and update slither gb

* fix: remove accidentally committed lockfile from prior testing

.github/workflows/slither.yml

+name: 'Slither Analysis'
+
+on:
+  workflow_dispatch:
+  pull_request:
+  push:
+    branches:
+      - develop
+
+jobs:
+  slither-analyze:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      # Workaround to prevent slither-action from trying to install JS deps.
+      # Without this step, it detects the `package.json``, and since there is no
+      # lockfile it defaults `npm install` which fails due to the preinstall
+      # script to enforce pnpm. https://github.com/crytic/slither-action/issues/44#issuecomment-1338183656
+      - name: Remove package.json
+        run: rm packages/contracts-bedrock/package.json
+
+      - name: Run Slither
+        uses: crytic/slither-action@v0.3.1
+        id: slither
+        with:
+          target: packages/contracts-bedrock
+          slither-config: packages/contracts-bedrock/slither.config.json
+          fail-on: config
+          sarif: results.sarif
+          # https://github.com/crytic/slither-action/issues/70#issuecomment-1932948435
+          slither-version: dev-triage-db
+          slither-args: --triage-database packages/contracts-bedrock/slither.db.json
+
+      - name: Upload SARIF file
+        uses: github/codeql-action/upload-sarif@v3
+        if: always()
+        with:
+          sarif_file: ${{ steps.slither.outputs.sarif }}

packages/contracts-bedrock/.gitignore

@@ -6,7 +6,6 @@ broadcast
 kout-deployment
 kout-proofs
 test/kontrol/logs
-slither-report.json
 
 # Metrics
 coverage.out

packages/contracts-bedrock/README.md

@@ -97,15 +97,17 @@ since some contracts are deployed using `CREATE`. Run `pnpm clean` and rerun the
 
 ### Static Analysis
 
-`contracts-bedrock` uses [slither](https://github.com/crytic/slither) as its primary static analysis tool. When opening a pr that includes changes to `contracts-bedrock`, you should
-verify that slither did not detect any new issues by running `pnpm slither:check`.
-
-If there are new issues, you should triage them.
-Run `pnpm slither:triage` to step through findings.
-You should _carefully_ walk through these findings, specifying which to triage/ignore (default is to keep all, outputting them into `slither-report.json`).
-Findings can be triaged into `slither.db.json` or kept in the `slither-report.json`.
-You should triage issues with extreme _care_ and security sign-off.
-
-After issues are triaged, or an updated slither report is generated, make sure to check in your changes to git.
-Once checked in, the changes can be verified by running `pnpm slither:check`.
-This will fail if there are issues missing from the `slither-report.json` that are _not_ triaged into `slither.db.json`.
+`contracts-bedrock` uses [slither](https://github.com/crytic/slither) as its primary static analysis tool.
+Slither will be run against PRs as part of CI, and new findings will be reported as a comment on the PR.
+CI will fail if there are any new findings of medium or higher severity, as configured in the repo's Settings > Code Security and Analysis > Code Scanning > Protection rules setting.
+
+There are two corresponding jobs in CI: one calls "Slither Analysis" and one called "Code scanning results / Slither".
+The former will always pass if Slither runs successfully, and the latter will fail if there are any new findings of medium or higher severity.
+
+Existing findings can be found in the repo's Security tab > [Code Scanning](https://github.com/ethereum-optimism/optimism/security/code-scanning) section.
+You can view findings for a specific PR using the `pr:{number}` filter, such [`pr:9405`](https://github.com/ethereum-optimism/optimism/security/code-scanning?query=is:open+pr:9405).
+
+For each finding, either fix it locally and push a new commit, or dismiss it through the PR comment's UI.
+
+Note that you can run slither locally by running `slither .`, but because it does not contain the triaged results from GitHub, it will be noisy.
+Instead, you should run `slither ./path/to/contract.sol` to run it against a specific file.

packages/contracts-bedrock/package.json

@@ -29,9 +29,6 @@
     "gas-snapshot": "pnpm build:go-ffi && pnpm gas-snapshot:no-build",
     "snapshots": "npx tsx scripts/generate-snapshots.ts && ./test/kontrol/scripts/make-summary-deployment.sh",
     "snapshots:check": "./scripts/check-snapshots.sh",
-    "slither": "./scripts/slither.sh",
-    "slither:check": "pnpm slither && git diff --exit-code",
-    "slither:triage": "TRIAGE_MODE=1 ./scripts/slither.sh",
     "semver-lock": "forge script scripts/SemverLock.s.sol",
     "validate-deploy-configs": "./scripts/check-deploy-configs.sh",
     "validate-spacers:no-build": "npx tsx scripts/validate-spacers.ts",

packages/contracts-bedrock/scripts/slither.sh

-#!/usr/bin/env bash
-
-set -e
-
-SLITHER_REPORT="slither-report.json"
-SLITHER_REPORT_BACKUP="slither-report.json.temp"
-SLITHER_TRIAGE_REPORT="slither.db.json"
-
-# Get the absolute path of the parent directory of this script
-DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && cd .. && pwd )"
-echo "Running slither in $DIR"
-cd "$DIR"
-
-# Clean up any previous artifacts.
-# We do not check if pnpm is installed since it is used across the monorepo
-# and must be installed as a prerequisite.
-pnpm clean
-
-# Check if slither is installed
-# If not, provide instructions to install with `pip3 install slither-analyzer` and exit
-if ! command -v slither &> /dev/null
-then
-    echo "Slither could not be found. Please install slither by running:"
-    echo "pip3 install slither-analyzer"
-    exit
-fi
-
-
-# Check if jq is installed and exit otherwise
-if ! command -v jq &> /dev/null
-then
-    echo "jq could not be found. Please install jq."
-    echo "On Mac: brew install jq"
-    echo "On Ubuntu: sudo apt-get install jq"
-    echo "For other platforms: https://stedolan.github.io/jq/download/"
-    exit
-fi
-
-# Print the slither version
-echo "Slither version: $(slither --version)"
-
-# Copy the slither report if it exists to a temp file
-if [ -e "$SLITHER_REPORT" ]; then
-    mv $SLITHER_REPORT $SLITHER_REPORT_BACKUP
-    echo "Created backup of previous slither report at $SLITHER_REPORT_BACKUP"
-fi
-
-# Slither normal mode will run slither and put all findings in the slither-report.json file.
-# See slither.config.json for slither settings and to disable specific detectors.
-if [[ -z "$TRIAGE_MODE" ]]; then
-  echo "Running slither in normal mode"
-  SLITHER_OUTPUT=$(slither . 2>&1 || true)
-fi
-
-# Slither's triage mode will run an 'interview' in the terminal.
-# This allows you to review each finding, and specify which to ignore in future runs.
-# Findings to keep are output to the slither-report.json output file.
-# Checking in a json file is cleaner than adding slither-disable comments throughout the codebase.
-# See slither.config.json for slither settings and to disable specific detectors.
-if [[ -n "$TRIAGE_MODE" ]]; then
-  echo "Running slither in triage mode"
-  SLITHER_OUTPUT=$(slither . --triage-mode --json $SLITHER_REPORT || true)
-
-  # If the slither report was generated successfully, and the slither triage exists, clean up the triaged output.
-  if [ -f "$SLITHER_REPORT" ] && [ -f  "$SLITHER_TRIAGE_REPORT" ]; then
-    # The following jq command selects a subset of fields in each of the slither triage report description and element objects.
-    # This significantly slims down the output json, on the order of 100 magnitudes smaller.
-    updated_json=$(cat $SLITHER_TRIAGE_REPORT | jq -r '[.[] | .id as $id | .description as $description | .check as $check | .impact as $impact | .confidence as $confidence | (.elements[] | .type as $type | .name as $name | (.source_mapping | { "id": $id, "impact": $impact, "confidence": $confidence, "check": $check, "description": $description, "type": $type, "name": $name, start, length, filename_relative } ))]')
-    echo "$updated_json" > $SLITHER_TRIAGE_REPORT
-    echo "Slither traige report updated at $DIR/$SLITHER_TRIAGE_REPORT"
-  fi
-fi
-
-# If slither failed to generate a report, exit with an error.
-if [ ! -f "$SLITHER_REPORT" ]; then
-  echo "Slither output:"
-  echo "$SLITHER_OUTPUT"
-  echo "Slither failed to generate a report."
-  if [ -e "$SLITHER_REPORT_BACKUP" ]; then
-      mv $SLITHER_REPORT_BACKUP $SLITHER_REPORT
-      echo "Restored previous slither report from $SLITHER_REPORT_BACKUP"
-  fi
-  echo "Exiting with error."
-  exit 1
-fi
-
-# If slither successfully generated a report, clean up the report.
-# The following jq command selects a subset of fields in each of the slither triage report description and element objects.
-# This significantly slims down the output json, on the order of 100 magnitudes smaller.
-echo "Slither ran successfully, generating minimzed report..."
-updated_json=$(cat $SLITHER_REPORT | jq -r '[.results.detectors[] | .id as $id | .description as $description | .check as $check | .impact as $impact | .confidence as $confidence | (.elements[] | .type as $type | .name as $name | (.source_mapping | { "id": $id, "impact": $impact, "confidence": $confidence, "check": $check, "description": $description, "type": $type, "name": $name, start, length, filename_relative } ))]')
-echo "$updated_json" > $SLITHER_REPORT
-echo "Slither report stored at $DIR/$SLITHER_REPORT"
-
-# Remove any items in the slither report that are also in the slither triage report.
-# This prevents the same finding from being reported twice.
-# Iterate over the slither-report.json file and remove any items that are in the slither.db.json file
-# by matching on the id field.
-if [ -f "$SLITHER_TRIAGE_REPORT" ]; then
-  echo "Removing triaged items from slither report..."
-  jq -s '.[0] as $slither_report | .[1] as $slither_triage_report | $slither_report - ($slither_report - $slither_triage_report)' $SLITHER_REPORT $SLITHER_TRIAGE_REPORT > $SLITHER_REPORT.temp
-  mv $SLITHER_REPORT.temp $SLITHER_REPORT
-  echo "Slither report stored at $DIR/$SLITHER_REPORT"
-fi
-
-# Delete the backup of the previous slither report if it exists
-if [ -e "$SLITHER_REPORT_BACKUP" ]; then
-    rm $SLITHER_REPORT_BACKUP
-    echo "Deleted backup of previous slither report at $SLITHER_REPORT_BACKUP"
-fi

packages/contracts-bedrock/slither.config.json

 {
-    "detectors_to_exclude": "incorrect-shift-in-assembly,assembly,timestamp,solc-version,missing-zero-check,immutable-states,arbitrary-send-eth,too-many-digits,divide-before-multiply,conformance-to-solidity-naming-conventions,low-level-calls,reentrancy-events,cache-array-length,unused-return,cyclomatic-complexity,calls-loop,reentrancy-unlimited-gas,reentrancy-eth,reentrancy-benign,costly-loop,events-maths,incorrect-equality",
+  "detectors_to_exclude": "arbitrary-send-eth,incorrect-equality,naming-convention,solc-version",
+  "exclude_dependencies": true,
   "exclude_informational": true,
-    "exclude_optimization": true,
   "exclude_low": true,
-    "json": "slither-report.json",
-    "exclude_medium": false,
-    "exclude_high": false,
-    "solc_disable_warnings": false,
-    "disable_color": false,
-    "exclude_dependencies": true,
-    "filter_paths": "(lib/|src/vendor|src/cannon/MIPS.sol|src/EAS/EAS.sol)",
-    "legacy_ast": false,
+  "exclude_optimization": true,
+  "fail_on": "none",
+  "filter_paths": "(src/vendor|src/cannon/MIPS.sol|src/EAS/EAS.sol)",
   "foundry_out_directory": "artifacts"
 }