diff --git a/l1-contracts/Dockerfile b/l1-contracts/Dockerfile index 5544b0eaa7be..f6ccb1dc6b82 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 cacd60874540..e3b27c553883 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 1969ca985208..006ddcd02286 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 000000000000..c77991cc91c4 --- /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 000000000000..2489723a9460 --- /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 000000000000..7b45948006d1 --- /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 39ec6a692bc2..568bbda37c65 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 894cd91ac6ab..46ec35674084 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; + } }