diff --git a/l1-contracts/Dockerfile b/l1-contracts/Dockerfile index f6ccb1dc6b8..f5e51731f9f 100644 --- a/l1-contracts/Dockerfile +++ b/l1-contracts/Dockerfile @@ -1,6 +1,6 @@ # Building requires foundry. 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 +RUN apk update && apk add git jq bash nodejs npm yarn python3 py3-pip && pip3 install slither-analyzer slitherin WORKDIR /usr/src/l1-contracts COPY . . RUN git init diff --git a/l1-contracts/README.md b/l1-contracts/README.md index e3b27c55388..258fb4cdedd 100644 --- a/l1-contracts/README.md +++ b/l1-contracts/README.md @@ -76,7 +76,7 @@ yarn lint --- -# Slither +# Slither & Slitherin 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 @@ -85,6 +85,6 @@ 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 assume that you already have slither installed. You can install it with `pip3 install slither-analyzer slitherin`. 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/slither_output.md b/l1-contracts/slither_output.md index 7b45948006d..db4b5b391fd 100644 --- a/l1-contracts/slither_output.md +++ b/l1-contracts/slither_output.md @@ -1,19 +1,32 @@ Summary + - [pess-unprotected-setter](#pess-unprotected-setter) (1 results) (High) - [uninitialized-local](#uninitialized-local) (1 results) (Medium) - [unused-return](#unused-return) (1 results) (Medium) + - [pess-dubious-typecast](#pess-dubious-typecast) (8 results) (Medium) - [reentrancy-events](#reentrancy-events) (1 results) (Low) - [timestamp](#timestamp) (4 results) (Low) + - [pess-public-vs-external](#pess-public-vs-external) (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) + - [pess-magic-number](#pess-magic-number) (1 results) (Informational) - [constable-states](#constable-states) (1 results) (Optimization) +## pess-unprotected-setter +Impact: High +Confidence: Medium + - [ ] ID-0 +Function [Rollup.process(bytes,bytes32,bytes32,bytes,bytes)](src/core/Rollup.sol#L54-L94) is a non-protected setter archive is written + +src/core/Rollup.sol#L54-L94 + + ## uninitialized-local Impact: Medium Confidence: Medium - - [ ] ID-0 + - [ ] ID-1 [HeaderLib.decode(bytes).header](src/core/libraries/HeaderLib.sol#L133) is a local variable never initialized src/core/libraries/HeaderLib.sol#L133 @@ -22,16 +35,92 @@ src/core/libraries/HeaderLib.sol#L133 ## unused-return Impact: Medium Confidence: Medium - - [ ] ID-1 + - [ ] ID-2 [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 +## pess-dubious-typecast +Impact: Medium +Confidence: High + - [ ] ID-3 +Dubious typecast in [TxsDecoder.read4(bytes,uint256)](src/core/libraries/decoders/TxsDecoder.sol#L298-L300): + bytes => bytes4 casting occurs in [uint256(uint32(bytes4(slice(_data,_offset,4))))](src/core/libraries/decoders/TxsDecoder.sol#L299) + +src/core/libraries/decoders/TxsDecoder.sol#L298-L300 + + + - [ ] ID-4 +Dubious typecast in [Decoder.read4(bytes,uint256)](src/core/libraries/decoders/Decoder.sol#L415-L417): + bytes => bytes4 casting occurs in [uint256(uint32(bytes4(slice(_data,_offset,4))))](src/core/libraries/decoders/Decoder.sol#L416) + +src/core/libraries/decoders/Decoder.sol#L415-L417 + + + - [ ] ID-5 +Dubious typecast in [Outbox.sendL1Messages(bytes32[])](src/core/messagebridge/Outbox.sol#L38-L46): + uint256 => uint32 casting occurs in [version = uint32(REGISTRY.getVersionFor(msg.sender))](src/core/messagebridge/Outbox.sol#L40) + +src/core/messagebridge/Outbox.sol#L38-L46 + + + - [ ] ID-6 +Dubious typecast in [Inbox.sendL2Message(DataStructures.L2Actor,uint32,bytes32,bytes32)](src/core/messagebridge/Inbox.sol#L45-L91): + uint256 => uint64 casting occurs in [fee = uint64(msg.value)](src/core/messagebridge/Inbox.sol#L64) + uint256 => uint32 casting occurs in [entries.insert(key,fee,uint32(_recipient.version),_deadline,_errIncompatibleEntryArguments)](src/core/messagebridge/Inbox.sol#L76) + +src/core/messagebridge/Inbox.sol#L45-L91 + + + - [ ] ID-7 +Dubious typecast in [HeaderLib.decode(bytes)](src/core/libraries/HeaderLib.sol#L128-L167): + bytes => bytes32 casting occurs in [header.lastArchive = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L136-L138) + bytes => bytes4 casting occurs in [header.lastArchive = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L136-L138) + bytes => bytes32 casting occurs in [header.bodyHash = bytes32(_header)](src/core/libraries/HeaderLib.sol#L141) + bytes => bytes32 casting occurs in [header.stateReference.l1ToL2MessageTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L144-L146) + bytes => bytes4 casting occurs in [header.stateReference.l1ToL2MessageTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L144-L146) + bytes => bytes32 casting occurs in [header.stateReference.partialStateReference.noteHashTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L147-L149) + bytes => bytes4 casting occurs in [header.stateReference.partialStateReference.noteHashTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L147-L149) + bytes => bytes32 casting occurs in [header.stateReference.partialStateReference.nullifierTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L150-L152) + bytes => bytes4 casting occurs in [header.stateReference.partialStateReference.nullifierTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L150-L152) + bytes => bytes32 casting occurs in [header.stateReference.partialStateReference.contractTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L153-L155) + bytes => bytes4 casting occurs in [header.stateReference.partialStateReference.contractTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L153-L155) + bytes => bytes32 casting occurs in [header.stateReference.partialStateReference.publicDataTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L156-L158) + bytes => bytes4 casting occurs in [header.stateReference.partialStateReference.publicDataTree = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L156-L158) + bytes => bytes32 casting occurs in [header.globalVariables.chainId = uint256(bytes32(_header))](src/core/libraries/HeaderLib.sol#L161) + bytes => bytes32 casting occurs in [header.globalVariables.version = uint256(bytes32(_header))](src/core/libraries/HeaderLib.sol#L162) + bytes => bytes32 casting occurs in [header.globalVariables.blockNumber = uint256(bytes32(_header))](src/core/libraries/HeaderLib.sol#L163) + bytes => bytes32 casting occurs in [header.globalVariables.timestamp = uint256(bytes32(_header))](src/core/libraries/HeaderLib.sol#L164) + +src/core/libraries/HeaderLib.sol#L128-L167 + + + - [ ] ID-8 +Dubious typecast in [MessagesDecoder.read4(bytes,uint256)](src/core/libraries/decoders/MessagesDecoder.sol#L110-L112): + bytes => bytes4 casting occurs in [uint256(uint32(bytes4(_data)))](src/core/libraries/decoders/MessagesDecoder.sol#L111) + +src/core/libraries/decoders/MessagesDecoder.sol#L110-L112 + + + - [ ] ID-9 +Dubious typecast in [Inbox.batchConsume(bytes32[],address)](src/core/messagebridge/Inbox.sol#L122-L143): + uint256 => uint32 casting occurs in [expectedVersion = uint32(REGISTRY.getVersionFor(msg.sender))](src/core/messagebridge/Inbox.sol#L128) + +src/core/messagebridge/Inbox.sol#L122-L143 + + + - [ ] ID-10 +Dubious typecast in [Decoder.getL2BlockNumber(bytes)](src/core/libraries/decoders/Decoder.sol#L132-L134): + bytes => bytes32 casting occurs in [uint256(bytes32(_l2Block))](src/core/libraries/decoders/Decoder.sol#L133) + +src/core/libraries/decoders/Decoder.sol#L132-L134 + + ## reentrancy-events Impact: Low Confidence: Medium - - [ ] ID-2 + - [ ] ID-11 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) @@ -45,7 +134,7 @@ src/core/Rollup.sol#L54-L94 ## timestamp Impact: Low Confidence: Medium - - [ ] ID-3 + - [ ] ID-12 [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) @@ -53,7 +142,7 @@ Confidence: Medium src/core/messagebridge/Inbox.sol#L122-L143 - - [ ] ID-4 + - [ ] ID-13 [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) @@ -61,7 +150,7 @@ src/core/messagebridge/Inbox.sol#L122-L143 src/core/messagebridge/Inbox.sol#L45-L91 - - [ ] ID-5 + - [ ] ID-14 [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) @@ -69,7 +158,7 @@ src/core/messagebridge/Inbox.sol#L45-L91 src/core/libraries/HeaderLib.sol#L91-L121 - - [ ] ID-6 + - [ ] ID-15 [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) @@ -77,24 +166,58 @@ src/core/libraries/HeaderLib.sol#L91-L121 src/core/messagebridge/Inbox.sol#L102-L113 +## pess-public-vs-external +Impact: Low +Confidence: Medium + - [ ] ID-16 +The following public functions could be turned into external in [Registry](src/core/messagebridge/Registry.sol#L22-L129) contract: + [Registry.constructor()](src/core/messagebridge/Registry.sol#L29-L33) + +src/core/messagebridge/Registry.sol#L22-L129 + + + - [ ] ID-17 +The following public functions could be turned into external in [Rollup](src/core/Rollup.sol#L27-L103) contract: + [Rollup.constructor(IRegistry,IAvailabilityOracle)](src/core/Rollup.sol#L39-L44) + +src/core/Rollup.sol#L27-L103 + + + - [ ] ID-18 +The following public functions could be turned into external in [Outbox](src/core/messagebridge/Outbox.sol#L21-L149) contract: + [Outbox.constructor(address)](src/core/messagebridge/Outbox.sol#L29-L31) + [Outbox.get(bytes32)](src/core/messagebridge/Outbox.sol#L78-L85) + [Outbox.contains(bytes32)](src/core/messagebridge/Outbox.sol#L92-L94) + +src/core/messagebridge/Outbox.sol#L21-L149 + + + - [ ] ID-19 +The following public functions could be turned into external in [Inbox](src/core/messagebridge/Inbox.sol#L21-L231) contract: + [Inbox.constructor(address)](src/core/messagebridge/Inbox.sol#L30-L32) + [Inbox.contains(bytes32)](src/core/messagebridge/Inbox.sol#L174-L176) + +src/core/messagebridge/Inbox.sol#L21-L231 + + ## assembly Impact: Informational Confidence: High - - [ ] ID-7 + - [ ] ID-20 [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 + - [ ] ID-21 [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 + - [ ] ID-22 [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) @@ -102,14 +225,14 @@ src/core/libraries/decoders/TxsDecoder.sol#L71-L184 src/core/libraries/decoders/Decoder.sol#L164-L301 - - [ ] ID-10 + - [ ] ID-23 [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 + - [ ] ID-24 [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) @@ -120,79 +243,79 @@ src/core/libraries/decoders/MessagesDecoder.sol#L52-L102 ## dead-code Impact: Informational Confidence: Medium - - [ ] ID-12 + - [ ] ID-25 [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 + - [ ] ID-26 [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 + - [ ] ID-27 [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 + - [ ] ID-28 [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 + - [ ] ID-29 [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 + - [ ] ID-30 [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 + - [ ] ID-31 [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 + - [ ] ID-32 [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 + - [ ] ID-33 [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 + - [ ] ID-34 [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 + - [ ] ID-35 [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 + - [ ] ID-36 [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 + - [ ] ID-37 [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 @@ -201,13 +324,13 @@ src/core/messagebridge/Outbox.sol#L130-L148 ## solc-version Impact: Informational Confidence: High - - [ ] ID-25 + - [ ] ID-38 solc-0.8.21 is not recommended for deployment ## low-level-calls Impact: Informational Confidence: High - - [ ] ID-26 + - [ ] ID-39 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) @@ -217,7 +340,7 @@ src/core/messagebridge/Inbox.sol#L148-L153 ## similar-names Impact: Informational Confidence: Medium - - [ ] ID-27 + - [ ] ID-40 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 @@ -226,22 +349,33 @@ src/core/Rollup.sol#L30 ## unused-state Impact: Informational Confidence: High - - [ ] ID-28 + - [ ] ID-41 [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 + - [ ] ID-42 [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 +## pess-magic-number +Impact: Informational +Confidence: High + - [ ] ID-43 +Magic number 376 is used multiple times in: + [_header.length != 376](src/core/libraries/HeaderLib.sol#L129) + [Errors.HeaderLib__InvalidHeaderSize(376,_header.length)](src/core/libraries/HeaderLib.sol#L130) + +src/core/libraries/HeaderLib.sol#L129 + + ## constable-states Impact: Optimization Confidence: High - - [ ] ID-30 + - [ ] ID-44 [Rollup.lastWarpedBlockTs](src/core/Rollup.sol#L37) should be constant src/core/Rollup.sol#L37