diff --git a/l1-contracts/slither_output.md b/l1-contracts/slither_output.md index 3c8fcd33236..7f202ef2ce5 100644 --- a/l1-contracts/slither_output.md +++ b/l1-contracts/slither_output.md @@ -1,7 +1,7 @@ Summary - [pess-unprotected-setter](#pess-unprotected-setter) (1 results) (High) - [uninitialized-local](#uninitialized-local) (2 results) (Medium) - - [unused-return](#unused-return) (1 results) (Medium) + - [unused-return](#unused-return) (2 results) (Medium) - [pess-dubious-typecast](#pess-dubious-typecast) (8 results) (Medium) - [missing-zero-check](#missing-zero-check) (1 results) (Low) - [reentrancy-events](#reentrancy-events) (2 results) (Low) @@ -18,9 +18,9 @@ Summary Impact: High Confidence: Medium - [ ] ID-0 -Function [Rollup.process(bytes,bytes32,bytes,bytes)](src/core/Rollup.sol#L53-L91) is a non-protected setter archive is written +Function [Rollup.process(bytes,bytes32,bytes,bytes)](src/core/Rollup.sol#L58-L103) is a non-protected setter archive is written -src/core/Rollup.sol#L53-L91 +src/core/Rollup.sol#L58-L103 ## uninitialized-local @@ -42,36 +42,42 @@ src/core/libraries/decoders/TxsDecoder.sol#L86 Impact: Medium Confidence: Medium - [ ] ID-3 -[Rollup.process(bytes,bytes32,bytes,bytes)](src/core/Rollup.sol#L53-L91) ignores return value by [(l1ToL2Msgs,l2ToL1Msgs) = MessagesDecoder.decode(_body)](src/core/Rollup.sol#L69) +[Rollup.process(bytes,bytes32,bytes,bytes)](src/core/Rollup.sol#L58-L103) ignores return value by [NEW_INBOX.consume()](src/core/Rollup.sol#L93) -src/core/Rollup.sol#L53-L91 +src/core/Rollup.sol#L58-L103 + + + - [ ] ID-4 +[Rollup.process(bytes,bytes32,bytes,bytes)](src/core/Rollup.sol#L58-L103) ignores return value by [(l1ToL2Msgs,l2ToL1Msgs) = MessagesDecoder.decode(_body)](src/core/Rollup.sol#L74) + +src/core/Rollup.sol#L58-L103 ## pess-dubious-typecast Impact: Medium Confidence: High - - [ ] ID-4 + - [ ] ID-5 Dubious typecast in [TxsDecoder.read4(bytes,uint256)](src/core/libraries/decoders/TxsDecoder.sol#L359-L361): bytes => bytes4 casting occurs in [uint256(uint32(bytes4(slice(_data,_offset,4))))](src/core/libraries/decoders/TxsDecoder.sol#L360) src/core/libraries/decoders/TxsDecoder.sol#L359-L361 - - [ ] ID-5 + - [ ] ID-6 Dubious typecast in [MessagesDecoder.read1(bytes,uint256)](src/core/libraries/decoders/MessagesDecoder.sol#L158-L160): bytes => bytes1 casting occurs in [uint256(uint8(bytes1(_data)))](src/core/libraries/decoders/MessagesDecoder.sol#L159) src/core/libraries/decoders/MessagesDecoder.sol#L158-L160 - - [ ] ID-6 + - [ ] ID-7 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-7 + - [ ] ID-8 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) @@ -79,28 +85,28 @@ Dubious typecast in [Inbox.sendL2Message(DataStructures.L2Actor,uint32,bytes32,b src/core/messagebridge/Inbox.sol#L45-L91 - - [ ] ID-8 + - [ ] ID-9 Dubious typecast in [TxsDecoder.read1(bytes,uint256)](src/core/libraries/decoders/TxsDecoder.sol#L349-L351): bytes => bytes1 casting occurs in [uint256(uint8(bytes1(slice(_data,_offset,1))))](src/core/libraries/decoders/TxsDecoder.sol#L350) src/core/libraries/decoders/TxsDecoder.sol#L349-L351 - - [ ] ID-9 + - [ ] ID-10 Dubious typecast in [MessagesDecoder.read4(bytes,uint256)](src/core/libraries/decoders/MessagesDecoder.sol#L168-L170): bytes => bytes4 casting occurs in [uint256(uint32(bytes4(_data)))](src/core/libraries/decoders/MessagesDecoder.sol#L169) src/core/libraries/decoders/MessagesDecoder.sol#L168-L170 - - [ ] ID-10 + - [ ] ID-11 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-11 + - [ ] ID-12 Dubious typecast in [HeaderLib.decode(bytes)](src/core/libraries/HeaderLib.sol#L145-L189): bytes => bytes32 casting occurs in [header.lastArchive = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L153-L155) bytes => bytes4 casting occurs in [header.lastArchive = AppendOnlyTreeSnapshot(bytes32(_header),uint32(bytes4(_header)))](src/core/libraries/HeaderLib.sol#L153-L155) @@ -131,7 +137,7 @@ src/core/libraries/HeaderLib.sol#L145-L189 ## missing-zero-check Impact: Low Confidence: Medium - - [ ] ID-12 + - [ ] ID-13 [NewInbox.constructor(address,uint256)._rollup](src/core/messagebridge/NewInbox.sol#L41) lacks a zero-check on : - [ROLLUP = _rollup](src/core/messagebridge/NewInbox.sol#L42) @@ -141,17 +147,6 @@ src/core/messagebridge/NewInbox.sol#L41 ## reentrancy-events Impact: Low Confidence: Medium - - [ ] ID-13 -Reentrancy in [Rollup.process(bytes,bytes32,bytes,bytes)](src/core/Rollup.sol#L53-L91): - External calls: - - [inbox.batchConsume(l1ToL2Msgs,msg.sender)](src/core/Rollup.sol#L85) - - [outbox.sendL1Messages(l2ToL1Msgs)](src/core/Rollup.sol#L88) - Event emitted after the call(s): - - [L2BlockProcessed(header.globalVariables.blockNumber)](src/core/Rollup.sol#L90) - -src/core/Rollup.sol#L53-L91 - - - [ ] ID-14 Reentrancy in [NewInbox.sendL2Message(DataStructures.L2Actor,bytes32,bytes32)](src/core/messagebridge/NewInbox.sol#L62-L99): External calls: @@ -162,10 +157,22 @@ Reentrancy in [NewInbox.sendL2Message(DataStructures.L2Actor,bytes32,bytes32)](s src/core/messagebridge/NewInbox.sol#L62-L99 + - [ ] ID-15 +Reentrancy in [Rollup.process(bytes,bytes32,bytes,bytes)](src/core/Rollup.sol#L58-L103): + External calls: + - [inbox.batchConsume(l1ToL2Msgs,msg.sender)](src/core/Rollup.sol#L90) + - [NEW_INBOX.consume()](src/core/Rollup.sol#L93) + - [outbox.sendL1Messages(l2ToL1Msgs)](src/core/Rollup.sol#L100) + Event emitted after the call(s): + - [L2BlockProcessed(header.globalVariables.blockNumber)](src/core/Rollup.sol#L102) + +src/core/Rollup.sol#L58-L103 + + ## timestamp Impact: Low Confidence: Medium - - [ ] ID-15 + - [ ] ID-16 [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) @@ -173,7 +180,7 @@ Confidence: Medium src/core/messagebridge/Inbox.sol#L122-L143 - - [ ] ID-16 + - [ ] ID-17 [HeaderLib.validate(HeaderLib.Header,uint256,uint256,bytes32)](src/core/libraries/HeaderLib.sol#L108-L138) uses timestamp for comparisons Dangerous comparisons: - [_header.globalVariables.timestamp > block.timestamp](src/core/libraries/HeaderLib.sol#L122) @@ -181,7 +188,7 @@ src/core/messagebridge/Inbox.sol#L122-L143 src/core/libraries/HeaderLib.sol#L108-L138 - - [ ] ID-17 + - [ ] ID-18 [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) @@ -189,7 +196,7 @@ src/core/libraries/HeaderLib.sol#L108-L138 src/core/messagebridge/Inbox.sol#L45-L91 - - [ ] ID-18 + - [ ] ID-19 [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) @@ -200,28 +207,28 @@ src/core/messagebridge/Inbox.sol#L102-L113 ## pess-public-vs-external Impact: Low Confidence: Medium - - [ ] ID-19 + - [ ] ID-20 The following public functions could be turned into external in [FrontierMerkle](src/core/messagebridge/frontier_tree/Frontier.sol#L7-L93) contract: [FrontierMerkle.constructor(uint256)](src/core/messagebridge/frontier_tree/Frontier.sol#L19-L27) src/core/messagebridge/frontier_tree/Frontier.sol#L7-L93 - - [ ] ID-20 + - [ ] ID-21 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-21 -The following public functions could be turned into external in [Rollup](src/core/Rollup.sol#L27-L100) contract: - [Rollup.constructor(IRegistry,IAvailabilityOracle)](src/core/Rollup.sol#L39-L44) + - [ ] ID-22 +The following public functions could be turned into external in [Rollup](src/core/Rollup.sol#L30-L112) contract: + [Rollup.constructor(IRegistry,IAvailabilityOracle)](src/core/Rollup.sol#L43-L49) -src/core/Rollup.sol#L27-L100 +src/core/Rollup.sol#L30-L112 - - [ ] ID-22 + - [ ] ID-23 The following public functions could be turned into external in [Outbox](src/core/messagebridge/Outbox.sol#L21-L148) contract: [Outbox.constructor(address)](src/core/messagebridge/Outbox.sol#L29-L31) [Outbox.get(bytes32)](src/core/messagebridge/Outbox.sol#L77-L84) @@ -230,7 +237,7 @@ The following public functions could be turned into external in [Outbox](src/cor src/core/messagebridge/Outbox.sol#L21-L148 - - [ ] ID-23 + - [ ] ID-24 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) @@ -238,7 +245,7 @@ The following public functions could be turned into external in [Inbox](src/core src/core/messagebridge/Inbox.sol#L21-L231 - - [ ] ID-24 + - [ ] ID-25 The following public functions could be turned into external in [NewInbox](src/core/messagebridge/NewInbox.sol#L25-L128) contract: [NewInbox.constructor(address,uint256)](src/core/messagebridge/NewInbox.sol#L41-L52) @@ -248,7 +255,7 @@ src/core/messagebridge/NewInbox.sol#L25-L128 ## assembly Impact: Informational Confidence: High - - [ ] ID-25 + - [ ] ID-26 [MessagesDecoder.decode(bytes)](src/core/libraries/decoders/MessagesDecoder.sol#L60-L150) uses assembly - [INLINE ASM](src/core/libraries/decoders/MessagesDecoder.sol#L79-L81) - [INLINE ASM](src/core/libraries/decoders/MessagesDecoder.sol#L112-L118) @@ -256,7 +263,7 @@ Confidence: High src/core/libraries/decoders/MessagesDecoder.sol#L60-L150 - - [ ] ID-26 + - [ ] ID-27 [TxsDecoder.computeRoot(bytes32[])](src/core/libraries/decoders/TxsDecoder.sol#L291-L310) uses assembly - [INLINE ASM](src/core/libraries/decoders/TxsDecoder.sol#L298-L300) @@ -266,31 +273,31 @@ src/core/libraries/decoders/TxsDecoder.sol#L291-L310 ## dead-code Impact: Informational Confidence: Medium - - [ ] ID-27 + - [ ] ID-28 [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-28 + - [ ] ID-29 [Outbox._errNothingToConsume(bytes32)](src/core/messagebridge/Outbox.sol#L114-L116) is never used and should be removed src/core/messagebridge/Outbox.sol#L114-L116 - - [ ] ID-29 + - [ ] 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-30 + - [ ] ID-31 [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-31 + - [ ] ID-32 [Outbox._errIncompatibleEntryArguments(bytes32,uint64,uint64,uint32,uint32,uint32,uint32)](src/core/messagebridge/Outbox.sol#L129-L147) is never used and should be removed src/core/messagebridge/Outbox.sol#L129-L147 @@ -299,13 +306,13 @@ src/core/messagebridge/Outbox.sol#L129-L147 ## solc-version Impact: Informational Confidence: High - - [ ] ID-32 + - [ ] ID-33 solc-0.8.21 is not recommended for deployment ## low-level-calls Impact: Informational Confidence: High - - [ ] ID-33 + - [ ] ID-34 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) @@ -315,61 +322,61 @@ src/core/messagebridge/Inbox.sol#L148-L153 ## similar-names Impact: Informational Confidence: Medium - - [ ] ID-34 + - [ ] ID-35 Variable [Constants.LOGS_HASHES_NUM_BYTES_PER_BASE_ROLLUP](src/core/libraries/ConstantsGen.sol#L129) is too similar to [Constants.NOTE_HASHES_NUM_BYTES_PER_BASE_ROLLUP](src/core/libraries/ConstantsGen.sol#L122) src/core/libraries/ConstantsGen.sol#L129 - - [ ] ID-35 + - [ ] ID-36 Variable [Constants.L1_TO_L2_MESSAGE_LENGTH](src/core/libraries/ConstantsGen.sol#L109) is too similar to [Constants.L2_TO_L1_MESSAGE_LENGTH](src/core/libraries/ConstantsGen.sol#L110) src/core/libraries/ConstantsGen.sol#L109 - - [ ] ID-36 -Variable [Rollup.AVAILABILITY_ORACLE](src/core/Rollup.sol#L30) is too similar to [Rollup.constructor(IRegistry,IAvailabilityOracle)._availabilityOracle](src/core/Rollup.sol#L39) + - [ ] ID-37 +Variable [Rollup.AVAILABILITY_ORACLE](src/core/Rollup.sol#L33) is too similar to [Rollup.constructor(IRegistry,IAvailabilityOracle)._availabilityOracle](src/core/Rollup.sol#L43) -src/core/Rollup.sol#L30 +src/core/Rollup.sol#L33 ## constable-states Impact: Optimization Confidence: High - - [ ] ID-37 -[Rollup.lastWarpedBlockTs](src/core/Rollup.sol#L37) should be constant + - [ ] ID-38 +[Rollup.lastWarpedBlockTs](src/core/Rollup.sol#L41) should be constant -src/core/Rollup.sol#L37 +src/core/Rollup.sol#L41 ## pess-multiple-storage-read Impact: Optimization Confidence: High - - [ ] ID-38 + - [ ] ID-39 In a function [NewInbox.sendL2Message(DataStructures.L2Actor,bytes32,bytes32)](src/core/messagebridge/NewInbox.sol#L62-L99) variable [NewInbox.inProgress](src/core/messagebridge/NewInbox.sol#L37) is read multiple times src/core/messagebridge/NewInbox.sol#L62-L99 - - [ ] ID-39 + - [ ] ID-40 In a function [FrontierMerkle.root()](src/core/messagebridge/frontier_tree/Frontier.sol#L43-L76) variable [FrontierMerkle.HEIGHT](src/core/messagebridge/frontier_tree/Frontier.sol#L8) is read multiple times src/core/messagebridge/frontier_tree/Frontier.sol#L43-L76 - - [ ] ID-40 + - [ ] ID-41 In a function [NewInbox.consume()](src/core/messagebridge/NewInbox.sol#L108-L127) variable [NewInbox.inProgress](src/core/messagebridge/NewInbox.sol#L37) is read multiple times src/core/messagebridge/NewInbox.sol#L108-L127 - - [ ] ID-41 + - [ ] ID-42 In a function [NewInbox.consume()](src/core/messagebridge/NewInbox.sol#L108-L127) variable [NewInbox.toConsume](src/core/messagebridge/NewInbox.sol#L35) is read multiple times src/core/messagebridge/NewInbox.sol#L108-L127 - - [ ] ID-42 + - [ ] ID-43 In a function [FrontierMerkle.root()](src/core/messagebridge/frontier_tree/Frontier.sol#L43-L76) variable [FrontierMerkle.frontier](src/core/messagebridge/frontier_tree/Frontier.sol#L13) is read multiple times src/core/messagebridge/frontier_tree/Frontier.sol#L43-L76 diff --git a/l1-contracts/src/core/Rollup.sol b/l1-contracts/src/core/Rollup.sol index f9e22bfa801..a898c6ceab8 100644 --- a/l1-contracts/src/core/Rollup.sol +++ b/l1-contracts/src/core/Rollup.sol @@ -6,6 +6,7 @@ pragma solidity >=0.8.18; import {IRollup} from "./interfaces/IRollup.sol"; import {IAvailabilityOracle} from "./interfaces/IAvailabilityOracle.sol"; import {IInbox} from "./interfaces/messagebridge/IInbox.sol"; +import {INewInbox} from "./interfaces/messagebridge/INewInbox.sol"; import {IOutbox} from "./interfaces/messagebridge/IOutbox.sol"; import {IRegistry} from "./interfaces/messagebridge/IRegistry.sol"; @@ -14,9 +15,11 @@ import {HeaderLib} from "./libraries/HeaderLib.sol"; import {MessagesDecoder} from "./libraries/decoders/MessagesDecoder.sol"; import {Hash} from "./libraries/Hash.sol"; import {Errors} from "./libraries/Errors.sol"; +import {Constants} from "./libraries/ConstantsGen.sol"; // Contracts import {MockVerifier} from "../mock/MockVerifier.sol"; +import {NewInbox} from "./messagebridge/NewInbox.sol"; /** * @title Rollup @@ -28,6 +31,7 @@ contract Rollup is IRollup { MockVerifier public immutable VERIFIER; IRegistry public immutable REGISTRY; IAvailabilityOracle public immutable AVAILABILITY_ORACLE; + INewInbox public immutable NEW_INBOX; uint256 public immutable VERSION; bytes32 public archive; // Root of the archive tree @@ -40,6 +44,7 @@ contract Rollup is IRollup { VERIFIER = new MockVerifier(); REGISTRY = _registry; AVAILABILITY_ORACLE = _availabilityOracle; + NEW_INBOX = new NewInbox(address(this), Constants.L1_TO_L2_MSG_SUBTREE_HEIGHT); VERSION = 1; } @@ -84,6 +89,13 @@ contract Rollup is IRollup { IInbox inbox = REGISTRY.getInbox(); inbox.batchConsume(l1ToL2Msgs, msg.sender); + // TODO(#4633): enable the inHash check + NEW_INBOX.consume(); + // bytes32 inHash = NEW_INBOX.consume(); + // if (header.contentCommitment.inHash != inHash) { + // revert Errors.Rollup__InvalidInHash(inHash, header.contentCommitment.inHash); + // } + IOutbox outbox = REGISTRY.getOutbox(); outbox.sendL1Messages(l2ToL1Msgs); diff --git a/l1-contracts/test/Rollup.t.sol b/l1-contracts/test/Rollup.t.sol index 00b0020fa30..be9dc7192fa 100644 --- a/l1-contracts/test/Rollup.t.sol +++ b/l1-contracts/test/Rollup.t.sol @@ -8,6 +8,7 @@ import {DataStructures} from "../src/core/libraries/DataStructures.sol"; import {Registry} from "../src/core/messagebridge/Registry.sol"; import {Inbox} from "../src/core/messagebridge/Inbox.sol"; +import {NewInbox} from "../src/core/messagebridge/NewInbox.sol"; import {Outbox} from "../src/core/messagebridge/Outbox.sol"; import {Errors} from "../src/core/libraries/Errors.sol"; import {Rollup} from "../src/core/Rollup.sol"; @@ -22,6 +23,7 @@ contract RollupTest is DecoderBase { Inbox internal inbox; Outbox internal outbox; Rollup internal rollup; + NewInbox internal newInbox; AvailabilityOracle internal availabilityOracle; function setUp() public virtual { @@ -30,6 +32,7 @@ contract RollupTest is DecoderBase { outbox = new Outbox(address(registry)); availabilityOracle = new AvailabilityOracle(); rollup = new Rollup(registry, availabilityOracle); + newInbox = NewInbox(address(rollup.NEW_INBOX())); registry.upgrade(address(rollup), address(inbox), address(outbox)); } @@ -136,9 +139,13 @@ contract RollupTest is DecoderBase { availabilityOracle.publish(body); + uint256 toConsume = newInbox.toConsume(); + vm.record(); rollup.process(header, archive, body, bytes("")); + assertEq(newInbox.toConsume(), toConsume + 1, "Message subtree not consumed"); + (, bytes32[] memory inboxWrites) = vm.accesses(address(inbox)); (, bytes32[] memory outboxWrites) = vm.accesses(address(outbox));