feat: Adding slither to l1-contracts (AztecProtocol#4226)

- Fixed linting in CI, seemed to be entirely skipped - Updated linter issues in `HeaderLib` which were missed. - Use slither to generate automatic report with potential issues - Add slither to the CI, and match report with file. Fail CI if diff - Updates to newer version of foundry for better slither compatability Slither was running super slow in the CI but found in their issues that doing a fresh forge clean and then building only the contract we want to check (e.g., don't compile tests and external imports) there was a massive speedup.
Swoir · Jan 26, 2024 · b4dc31d · b4dc31d
1 parent 1e05f33
commit b4dc31d
Show file tree

Hide file tree

Showing 8 changed files with 326 additions and 45 deletions.
diff --git a/l1-contracts/Dockerfile b/l1-contracts/Dockerfile
@@ -1,14 +1,6 @@
-# Linting requires node.
-FROM node:18.19.0-alpine
-RUN apk update && apk add --no-cache build-base git python3 curl bash jq
-WORKDIR /usr/src/l1-contracts
-COPY . .
-RUN yarn && yarn lint
-
 # Building requires foundry.
-FROM ghcr.io/foundry-rs/foundry:nightly-c331b5eeee1b4151ef7354a081667e2d770b37f5
-# Required for foundry
-RUN apk update && apk add git jq bash
+FROM ghcr.io/foundry-rs/foundry:nightly-4a643801d0b3855934cdec778e33e79c79971783
+RUN apk update && apk add git jq bash nodejs npm yarn python3 py3-pip && pip3 install slither-analyzer
 WORKDIR /usr/src/l1-contracts
 COPY . .
 RUN git init
@@ -17,4 +9,7 @@ RUN forge install --no-commit \
   https://github.com/foundry-rs/forge-std \
   https://github.com/openzeppelin/openzeppelin-contracts
 # Run build and tests
-RUN forge clean && forge fmt --check && forge build && forge test
+RUN forge clean && forge fmt --check && forge build && forge test
+RUN yarn && yarn lint
+RUN git add . && yarn slither && yarn slither-has-diff
+RUN forge build
diff --git a/l1-contracts/README.md b/l1-contracts/README.md
@@ -66,10 +66,25 @@ For now, this include bytecode for contract deployment, but over time this will
 
 We use an extended version of solhint (https://github.com/LHerskind/solhint) to include custom rules. These custom rules relate to how errors should be named, using custom errors instead of reverts etc, see `.solhint.json` for more specifics about the rules.
 
-The linter is the only node module we need which is the reason behind not going full yarn-project on it. It is not part of the docker image, but can be run once in a while to make sure we are on track.
+The linter is the only node module we need which is the reason behind not going full yarn-project on it.
 
 To run the linter, simply run:
 
 ```bash
 yarn lint
 ```
+
+---
+
+# Slither
+
+We use slither as an automatic way to find blunders and common vulnerabilities in our contracts. It is not part of the docker image due to its slowness, but it can be run using the following command to generate a markdown file with the results:
+```bash
+yarn slither
+```
+
+When this command is run in CI, it will fail if the markdown file generated in docker don't match the one in the repository. 
+
+We assume that you already have slither installed. You can install it with `pip3 install slither-analyzer`. It is kept out of the bootstrap script as it is not a requirement for people who just want to run tests or are uninterested in the contracts.
+
+> We are not running the `naming-convention` detector because we have our own rules for naming which is enforced by the linter.
diff --git a/l1-contracts/package.json b/l1-contracts/package.json
@@ -7,6 +7,8 @@
     "solhint": "https://github.com/LHerskind/solhint#master"
   },
   "scripts": {
-    "lint": "solhint --config ./.solhint.json --fix \"src/**/*.sol\""
+    "lint": "solhint --config ./.solhint.json --fix \"src/**/*.sol\"",
+    "slither": "forge clean && forge build --build-info --skip '*/test/**' --force && slither . --checklist --ignore-compile --show-ignored-findings --config-file ./slither.config.json | tee slither_output.md",
+    "slither-has-diff": "./slither_has_diff.sh"
   }
 }
diff --git a/l1-contracts/slither.config.json b/l1-contracts/slither.config.json
@@ -0,0 +1,3 @@
+{
+  "detectors_to_exclude": "naming-convention"
+}
diff --git a/l1-contracts/slither_has_diff.sh b/l1-contracts/slither_has_diff.sh
@@ -0,0 +1,12 @@
+#!/bin/bash
+
+FILE="slither_output.md"
+
+DIFF_OUTPUT=$(git diff -- "$FILE")
+
+if [ -z "$DIFF_OUTPUT" ]; then
+    echo "No difference found."
+else
+    echo "Difference found!"
+    exit 1 
+fi
diff --git a/l1-contracts/slither_output.md b/l1-contracts/slither_output.md
@@ -0,0 +1,249 @@
+Summary
+ - [uninitialized-local](#uninitialized-local) (1 results) (Medium)
+ - [unused-return](#unused-return) (1 results) (Medium)
+ - [reentrancy-events](#reentrancy-events) (1 results) (Low)
+ - [timestamp](#timestamp) (4 results) (Low)
+ - [assembly](#assembly) (5 results) (Informational)
+ - [dead-code](#dead-code) (13 results) (Informational)
+ - [solc-version](#solc-version) (1 results) (Informational)
+ - [low-level-calls](#low-level-calls) (1 results) (Informational)
+ - [similar-names](#similar-names) (1 results) (Informational)
+ - [unused-state](#unused-state) (2 results) (Informational)
+ - [constable-states](#constable-states) (1 results) (Optimization)
+## uninitialized-local
+Impact: Medium
+Confidence: Medium
+ - [ ] ID-0
+[HeaderLib.decode(bytes).header](src/core/libraries/HeaderLib.sol#L133) is a local variable never initialized
+
+src/core/libraries/HeaderLib.sol#L133
+
+
+## unused-return
+Impact: Medium
+Confidence: Medium
+ - [ ] ID-1
+[Rollup.process(bytes,bytes32,bytes32,bytes,bytes)](src/core/Rollup.sol#L54-L94) ignores return value by [(inHash,l1ToL2Msgs,l2ToL1Msgs) = MessagesDecoder.decode(_body)](src/core/Rollup.sol#L71-L72)
+
+src/core/Rollup.sol#L54-L94
+
+
+## reentrancy-events
+Impact: Low
+Confidence: Medium
+ - [ ] ID-2
+Reentrancy in [Rollup.process(bytes,bytes32,bytes32,bytes,bytes)](src/core/Rollup.sol#L54-L94):
+	External calls:
+	- [inbox.batchConsume(l1ToL2Msgs,msg.sender)](src/core/Rollup.sol#L88)
+	- [outbox.sendL1Messages(l2ToL1Msgs)](src/core/Rollup.sol#L91)
+	Event emitted after the call(s):
+	- [L2BlockProcessed(header.globalVariables.blockNumber)](src/core/Rollup.sol#L93)
+
+src/core/Rollup.sol#L54-L94
+
+
+## timestamp
+Impact: Low
+Confidence: Medium
+ - [ ] ID-3
+[Inbox.batchConsume(bytes32[],address)](src/core/messagebridge/Inbox.sol#L122-L143) uses timestamp for comparisons
+	Dangerous comparisons:
+	- [block.timestamp > entry.deadline](src/core/messagebridge/Inbox.sol#L136)
+
+src/core/messagebridge/Inbox.sol#L122-L143
+
+
+ - [ ] ID-4
+[Inbox.sendL2Message(DataStructures.L2Actor,uint32,bytes32,bytes32)](src/core/messagebridge/Inbox.sol#L45-L91) uses timestamp for comparisons
+	Dangerous comparisons:
+	- [_deadline <= block.timestamp](src/core/messagebridge/Inbox.sol#L54)
+
+src/core/messagebridge/Inbox.sol#L45-L91
+
+
+ - [ ] ID-5
+[HeaderLib.validate(HeaderLib.Header,uint256,uint256,bytes32)](src/core/libraries/HeaderLib.sol#L91-L121) uses timestamp for comparisons
+	Dangerous comparisons:
+	- [_header.globalVariables.timestamp > block.timestamp](src/core/libraries/HeaderLib.sol#L105)
+
+src/core/libraries/HeaderLib.sol#L91-L121
+
+
+ - [ ] ID-6
+[Inbox.cancelL2Message(DataStructures.L1ToL2Msg,address)](src/core/messagebridge/Inbox.sol#L102-L113) uses timestamp for comparisons
+	Dangerous comparisons:
+	- [block.timestamp <= _message.deadline](src/core/messagebridge/Inbox.sol#L108)
+
+src/core/messagebridge/Inbox.sol#L102-L113
+
+
+## assembly
+Impact: Informational
+Confidence: High
+ - [ ] ID-7
+[Decoder.computeRoot(bytes32[])](src/core/libraries/decoders/Decoder.sol#L373-L392) uses assembly
+	- [INLINE ASM](src/core/libraries/decoders/Decoder.sol#L380-L382)
+
+src/core/libraries/decoders/Decoder.sol#L373-L392
+
+
+ - [ ] ID-8
+[TxsDecoder.decode(bytes)](src/core/libraries/decoders/TxsDecoder.sol#L71-L184) uses assembly
+	- [INLINE ASM](src/core/libraries/decoders/TxsDecoder.sol#L98-L104)
+
+src/core/libraries/decoders/TxsDecoder.sol#L71-L184
+
+
+ - [ ] ID-9
+[Decoder.computeConsumables(bytes)](src/core/libraries/decoders/Decoder.sol#L164-L301) uses assembly
+	- [INLINE ASM](src/core/libraries/decoders/Decoder.sol#L196-L202)
+	- [INLINE ASM](src/core/libraries/decoders/Decoder.sol#L289-L295)
+
+src/core/libraries/decoders/Decoder.sol#L164-L301
+
+
+ - [ ] ID-10
+[TxsDecoder.computeRoot(bytes32[])](src/core/libraries/decoders/TxsDecoder.sol#L256-L275) uses assembly
+	- [INLINE ASM](src/core/libraries/decoders/TxsDecoder.sol#L263-L265)
+
+src/core/libraries/decoders/TxsDecoder.sol#L256-L275
+
+
+ - [ ] ID-11
+[MessagesDecoder.decode(bytes)](src/core/libraries/decoders/MessagesDecoder.sol#L52-L102) uses assembly
+	- [INLINE ASM](src/core/libraries/decoders/MessagesDecoder.sol#L81-L83)
+	- [INLINE ASM](src/core/libraries/decoders/MessagesDecoder.sol#L94-L96)
+
+src/core/libraries/decoders/MessagesDecoder.sol#L52-L102
+
+
+## dead-code
+Impact: Informational
+Confidence: Medium
+ - [ ] ID-12
+[Decoder.computeConsumables(bytes)](src/core/libraries/decoders/Decoder.sol#L164-L301) is never used and should be removed
+
+src/core/libraries/decoders/Decoder.sol#L164-L301
+
+
+ - [ ] ID-13
+[Inbox._errIncompatibleEntryArguments(bytes32,uint64,uint64,uint32,uint32,uint32,uint32)](src/core/messagebridge/Inbox.sol#L212-L230) is never used and should be removed
+
+src/core/messagebridge/Inbox.sol#L212-L230
+
+
+ - [ ] ID-14
+[Decoder.slice(bytes,uint256,uint256)](src/core/libraries/decoders/Decoder.sol#L401-L407) is never used and should be removed
+
+src/core/libraries/decoders/Decoder.sol#L401-L407
+
+
+ - [ ] ID-15
+[Outbox._errNothingToConsume(bytes32)](src/core/messagebridge/Outbox.sol#L115-L117) is never used and should be removed
+
+src/core/messagebridge/Outbox.sol#L115-L117
+
+
+ - [ ] ID-16
+[Decoder.computeRoot(bytes32[])](src/core/libraries/decoders/Decoder.sol#L373-L392) is never used and should be removed
+
+src/core/libraries/decoders/Decoder.sol#L373-L392
+
+
+ - [ ] ID-17
+[Hash.sha256ToField(bytes32)](src/core/libraries/Hash.sol#L59-L61) is never used and should be removed
+
+src/core/libraries/Hash.sol#L59-L61
+
+
+ - [ ] ID-18
+[Decoder.computeKernelLogsHash(uint256,bytes)](src/core/libraries/decoders/Decoder.sol#L335-L365) is never used and should be removed
+
+src/core/libraries/decoders/Decoder.sol#L335-L365
+
+
+ - [ ] ID-19
+[Decoder.read4(bytes,uint256)](src/core/libraries/decoders/Decoder.sol#L415-L417) is never used and should be removed
+
+src/core/libraries/decoders/Decoder.sol#L415-L417
+
+
+ - [ ] ID-20
+[Decoder.computeStateHash(uint256,uint256,bytes)](src/core/libraries/decoders/Decoder.sol#L146-L154) is never used and should be removed
+
+src/core/libraries/decoders/Decoder.sol#L146-L154
+
+
+ - [ ] ID-21
+[Decoder.computePublicInputHash(bytes,bytes32,bytes32)](src/core/libraries/decoders/Decoder.sol#L118-L125) is never used and should be removed
+
+src/core/libraries/decoders/Decoder.sol#L118-L125
+
+
+ - [ ] ID-22
+[Inbox._errNothingToConsume(bytes32)](src/core/messagebridge/Inbox.sol#L197-L199) is never used and should be removed
+
+src/core/messagebridge/Inbox.sol#L197-L199
+
+
+ - [ ] ID-23
+[Decoder.getL2BlockNumber(bytes)](src/core/libraries/decoders/Decoder.sol#L132-L134) is never used and should be removed
+
+src/core/libraries/decoders/Decoder.sol#L132-L134
+
+
+ - [ ] ID-24
+[Outbox._errIncompatibleEntryArguments(bytes32,uint64,uint64,uint32,uint32,uint32,uint32)](src/core/messagebridge/Outbox.sol#L130-L148) is never used and should be removed
+
+src/core/messagebridge/Outbox.sol#L130-L148
+
+
+## solc-version
+Impact: Informational
+Confidence: High
+ - [ ] ID-25
+solc-0.8.21 is not recommended for deployment
+
+## low-level-calls
+Impact: Informational
+Confidence: High
+ - [ ] ID-26
+Low level call in [Inbox.withdrawFees()](src/core/messagebridge/Inbox.sol#L148-L153):
+	- [(success) = msg.sender.call{value: balance}()](src/core/messagebridge/Inbox.sol#L151)
+
+src/core/messagebridge/Inbox.sol#L148-L153
+
+
+## similar-names
+Impact: Informational
+Confidence: Medium
+ - [ ] ID-27
+Variable [Rollup.AVAILABILITY_ORACLE](src/core/Rollup.sol#L30) is too similar to [Rollup.constructor(IRegistry,IAvailabilityOracle)._availabilityOracle](src/core/Rollup.sol#L39)
+
+src/core/Rollup.sol#L30
+
+
+## unused-state
+Impact: Informational
+Confidence: High
+ - [ ] ID-28
+[Decoder.END_TREES_BLOCK_HEADER_OFFSET](src/core/libraries/decoders/Decoder.sol#L103-L104) is never used in [Decoder](src/core/libraries/decoders/Decoder.sol#L72-L418)
+
+src/core/libraries/decoders/Decoder.sol#L103-L104
+
+
+ - [ ] ID-29
+[Decoder.BLOCK_HEADER_OFFSET](src/core/libraries/decoders/Decoder.sol#L107-L108) is never used in [Decoder](src/core/libraries/decoders/Decoder.sol#L72-L418)
+
+src/core/libraries/decoders/Decoder.sol#L107-L108
+
+
+## constable-states
+Impact: Optimization
+Confidence: High
+ - [ ] ID-30
+[Rollup.lastWarpedBlockTs](src/core/Rollup.sol#L37) should be constant 
+
+src/core/Rollup.sol#L37
+
+
diff --git a/l1-contracts/src/core/libraries/Errors.sol b/l1-contracts/src/core/libraries/Errors.sol
@@ -59,4 +59,7 @@ library Errors {
   // Registry
   error Registry__RollupNotRegistered(address rollup); // 0xa1fee4cf
   error Registry__RollupAlreadyRegistered(address rollup); // 0x3c34eabf
+
+  // HeaderLib
+  error HeaderLib__InvalidHeaderSize(uint256 expected, uint256 actual); // 0xf3ccb247
 }