From b4dc31dd9fb02c096db6c40d848aea9f03e36a8c Mon Sep 17 00:00:00 2001 From: Lasse Herskind <16536249+LHerskind@users.noreply.github.com> Date: Fri, 26 Jan 2024 09:12:57 +0000 Subject: [PATCH] feat: Adding slither to l1-contracts (#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. --- l1-contracts/Dockerfile | 17 +- l1-contracts/README.md | 17 +- l1-contracts/package.json | 4 +- l1-contracts/slither.config.json | 3 + l1-contracts/slither_has_diff.sh | 12 + l1-contracts/slither_output.md | 249 ++++++++++++++++++ l1-contracts/src/core/libraries/Errors.sol | 3 + l1-contracts/src/core/libraries/HeaderLib.sol | 66 ++--- 8 files changed, 326 insertions(+), 45 deletions(-) create mode 100644 l1-contracts/slither.config.json create mode 100755 l1-contracts/slither_has_diff.sh create mode 100644 l1-contracts/slither_output.md diff --git a/l1-contracts/Dockerfile b/l1-contracts/Dockerfile index 5544b0eaa7b..f6ccb1dc6b8 100644 --- 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 \ No newline at end of file +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 \ No newline at end of file diff --git a/l1-contracts/README.md b/l1-contracts/README.md index cacd6087454..e3b27c55388 100644 --- 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. \ No newline at end of file diff --git a/l1-contracts/package.json b/l1-contracts/package.json index 1969ca98520..006ddcd0228 100644 --- 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 new file mode 100644 index 00000000000..c77991cc91c --- /dev/null +++ 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 new file mode 100755 index 00000000000..2489723a946 --- /dev/null +++ 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 new file mode 100644 index 00000000000..7b45948006d --- /dev/null +++ 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 index 39ec6a692bc..568bbda37c6 100644 --- 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 } diff --git a/l1-contracts/src/core/libraries/HeaderLib.sol b/l1-contracts/src/core/libraries/HeaderLib.sol index 894cd91ac6a..46ec3567408 100644 --- a/l1-contracts/src/core/libraries/HeaderLib.sol +++ b/l1-contracts/src/core/libraries/HeaderLib.sol @@ -81,38 +81,6 @@ library HeaderLib { bytes32 bodyHash; } - /** - * @notice Decodes the header - * @param _header - The header calldata - * @return The decoded header - */ - function decode(bytes calldata _header) internal pure returns (Header memory) { - require(_header.length == 376, "Invalid header length"); - - Header memory header; - - header.globalVariables.chainId = uint256(bytes32(_header[:0x20])); - header.globalVariables.version = uint256(bytes32(_header[0x20:0x40])); - header.globalVariables.blockNumber = uint256(bytes32(_header[0x40:0x60])); - header.globalVariables.timestamp = uint256(bytes32(_header[0x60:0x80])); - header.stateReference.l1ToL2MessageTree = - AppendOnlyTreeSnapshot(bytes32(_header[0x80:0xa0]), uint32(bytes4(_header[0xa0:0xa4]))); - header.stateReference.partialStateReference.noteHashTree = - AppendOnlyTreeSnapshot(bytes32(_header[0xa4:0xc4]), uint32(bytes4(_header[0xc4:0xc8]))); - header.stateReference.partialStateReference.nullifierTree = - AppendOnlyTreeSnapshot(bytes32(_header[0xc8:0xe8]), uint32(bytes4(_header[0xe8:0xec]))); - header.stateReference.partialStateReference.contractTree = - AppendOnlyTreeSnapshot(bytes32(_header[0xec:0x10c]), uint32(bytes4(_header[0x10c:0x110]))); - header.stateReference.partialStateReference.publicDataTree = - AppendOnlyTreeSnapshot(bytes32(_header[0x110:0x130]), uint32(bytes4(_header[0x130:0x134]))); - header.lastArchive = - AppendOnlyTreeSnapshot(bytes32(_header[0x134:0x154]), uint32(bytes4(_header[0x154:0x158]))); - - header.bodyHash = bytes32(_header[0x158:0x178]); - - return header; - } - /** * @notice Validates the header * @param _header - The decoded header @@ -151,4 +119,38 @@ library HeaderLib { revert Errors.Rollup__InvalidArchive(_archive, _header.lastArchive.root); } } + + /** + * @notice Decodes the header + * @param _header - The header calldata + * @return The decoded header + */ + function decode(bytes calldata _header) internal pure returns (Header memory) { + if (_header.length != 376) { + revert Errors.HeaderLib__InvalidHeaderSize(376, _header.length); + } + + Header memory header; + + header.globalVariables.chainId = uint256(bytes32(_header[:0x20])); + header.globalVariables.version = uint256(bytes32(_header[0x20:0x40])); + header.globalVariables.blockNumber = uint256(bytes32(_header[0x40:0x60])); + header.globalVariables.timestamp = uint256(bytes32(_header[0x60:0x80])); + header.stateReference.l1ToL2MessageTree = + AppendOnlyTreeSnapshot(bytes32(_header[0x80:0xa0]), uint32(bytes4(_header[0xa0:0xa4]))); + header.stateReference.partialStateReference.noteHashTree = + AppendOnlyTreeSnapshot(bytes32(_header[0xa4:0xc4]), uint32(bytes4(_header[0xc4:0xc8]))); + header.stateReference.partialStateReference.nullifierTree = + AppendOnlyTreeSnapshot(bytes32(_header[0xc8:0xe8]), uint32(bytes4(_header[0xe8:0xec]))); + header.stateReference.partialStateReference.contractTree = + AppendOnlyTreeSnapshot(bytes32(_header[0xec:0x10c]), uint32(bytes4(_header[0x10c:0x110]))); + header.stateReference.partialStateReference.publicDataTree = + AppendOnlyTreeSnapshot(bytes32(_header[0x110:0x130]), uint32(bytes4(_header[0x130:0x134]))); + header.lastArchive = + AppendOnlyTreeSnapshot(bytes32(_header[0x134:0x154]), uint32(bytes4(_header[0x154:0x158]))); + + header.bodyHash = bytes32(_header[0x158:0x178]); + + return header; + } }