Merge pull request #8391 from ethereum-optimism/refcell/fix-slither-triaging-and-docs

fix(ctb): Slither Triaging and Docs

.circleci/config.yml

@@ -477,8 +477,8 @@ jobs:
           patterns: contracts-bedrock
       - run:
           name: slither
-          command: |
-            pnpm slither:check || echo "Slither failed"
+          command: pnpm slither:check
+          working_directory: packages/contracts-bedrock
 
   contracts-bedrock-validate-spaces:
     docker:

packages/contracts-bedrock/README.md

@@ -94,3 +94,18 @@ to reduce the overhead of maintaining multiple ways to set up the state as well
 The L1 contract addresses are held in `deployments/hardhat/.deploy` and the L2 test state is held in a `.testdata` directory. The L1 addresses are used to create the L2 state
 and it is possible for stale addresses to be pulled into the L2 state, causing tests to fail. Stale addresses may happen if the order of the L1 deployments happen differently
 since some contracts are deployed using `CREATE`. Run `pnpm clean` and rerun the tests if they are failing for an unknown reason.
+
+### 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`.

packages/contracts-bedrock/scripts/slither.sh

@@ -4,6 +4,7 @@ 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 )"
@@ -44,20 +45,35 @@ if [ -e "$SLITHER_REPORT" ]; then
     echo "Created backup of previous slither report at $SLITHER_REPORT_BACKUP"
 fi
 
-# Slither's triage mode will run an 'interview' in the terminal, allowing you to review each of
-# its findings, and specify which should be ignored in future runs of slither. This will update
-# (or create) the slither.db.json file. This DB is a cleaner alternative to adding slither-disable
-# comments throughout the codebase.
-# Triage mode should only be run manually, and can be used to update the db when new findings are
-# causing a CI failure.
-# See slither.config.json for slither settings
+# 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"
-  # Run slither and store the output in a variable to be used later
   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 [[ ! -z "$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
+    json=$(cat $SLITHER_TRIAGE_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.
+    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
+# If slither failed to generate a report, exit with an error.
+if [ ! -f "$SLITHER_REPORT" ]; then
   echo "Slither output:\n$SLITHER_OUTPUT"
   echo "Slither failed to generate a report."
   if [ -e "$SLITHER_REPORT_BACKUP" ]; then
@@ -66,23 +82,26 @@ if [[ -z "$TRIAGE_MODE" ]]; then
   fi
   echo "Exiting with error."
   exit 1
-  fi
+fi
 
-  echo "Slither ran successfully, generating minimzed report..."
-  json=$(cat $SLITHER_REPORT)
-  updated_json=$(cat $SLITHER_REPORT | jq -r '[.results.detectors[] | .description as $description | .check as $check | .impact as $impact | .confidence as $confidence | (.elements[] | .type as $type | .name as $name | (.source_mapping | { "impact": $impact, "confidence": $confidence, "check": $check, "description": $description, "type": $type, "name": $name, start, length, filename_relative } ))]')
-  echo "$updated_json" > $SLITHER_REPORT
+# 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..."
+json=$(cat $SLITHER_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"
-else
-  echo "Running slither in triage mode"
-  slither . --triage-mode
-
-  # The slither json report contains a `filename_absolute` property which includes the full
-  # local path to source code on the machine where it was generated. This property breaks
-  # cross-platform report comparisons, so it's removed here.
-  mv $SLITHER_REPORT temp-slither-report.json
-  jq 'walk(if type == "object" then del(.filename_absolute) else . end)' temp-slither-report.json > $SLITHER_REPORT
-  rm -f temp-slither-report.json
 fi
 
 # Delete the backup of the previous slither report if it exists

packages/contracts-bedrock/slither.config.json

@@ -9,7 +9,7 @@
     "solc_disable_warnings": false,
     "disable_color": false,
     "exclude_dependencies": true,
-    "filter_paths": "(lib/|src/vendor|src/cannon/MIPS.sol)",
+    "filter_paths": "(lib/|src/vendor|src/cannon/MIPS.sol|src/EAS/EAS.sol)",
     "legacy_ast": false,
     "foundry_out_directory": "artifacts"
 }