Merge pull request #17 from HerodotusDev/audit

Audit

.gitignore

@@ -1,3 +1,4 @@
 target
+.DS_Store
 
 l1/broadcast/**

l1/src/L1MessagesSender.sol

@@ -1,9 +1,8 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.13;
+pragma solidity 0.8.21;
 
 import {Ownable} from "openzeppelin/access/Ownable.sol";
 
-import {FormatWords64} from "./lib/FormatWords64.sol";
 import {IStarknetCore} from "./interfaces/IStarknetCore.sol";
 
 import {IAggregatorsFactory} from "./interfaces/IAggregatorsFactory.sol";

l1/src/interfaces/IAggregator.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.13;
+pragma solidity 0.8.21;
 
 interface IAggregator {
     /// @notice Returns the current root hash of the Keccak Merkle Mountain Range (MMR) tree

l1/src/interfaces/IAggregatorsFactory.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.13;
+pragma solidity 0.8.21;
 
 import {IAggregator} from "./IAggregator.sol";
 

l1/src/interfaces/IStarknetCore.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
-pragma solidity ^0.8.13;
+pragma solidity 0.8.21;
 
 interface IStarknetCore {
     /**

multicall/deploy.toml

@@ -1,9 +1,9 @@
 [[call]]
 call_type = "deploy"
-class_hash = "0x710fb6c249e9c76996f01e170218e31aec7498a8ae618842a0f67b66987c436"
+class_hash = "0x4618004201b8cc18d82ef9e909e8d653cd5ef0327f4b9f40d014eaeb93d8598"
 inputs = [
     "0x1",
-    "0x2",
+    "0x18E4A8e2badB5f5950758F46f8108E2C5d357b07",
     "0x0",
     "0x007327d012f432a9f940228ae6b01032ea4742d8bba8599d7a34e1d5c120e983",
 ]
@@ -12,14 +12,20 @@ unique = false
 
 [[call]]
 call_type = "deploy"
-class_hash = "0x4965a6ea3851cde0fd10952802af3300fac851122051c3743644aabe901ee13"
-inputs = ["0x1"]
+class_hash = "0x229789b99574e92c01f99e9d6f16a3cc6326085733504a8b7ca367606b40ab4"
+inputs = ["commitments_inbox"]
 id = "headers_store"
 unique = false
 
 [[call]]
 call_type = "deploy"
-class_hash = "0xfa77018c244261ab65551a07c0f6e3fab9592027e4cd456b9d074f2b94a9ae"
-inputs = ["0x1"]
+class_hash = "0x4438ef289f10b9c687089b5ad218d8465a9266af74834356dbd78a35cfaf618"
+inputs = ["headers_store"]
 id = "evm_facts_registry"
 unique = false
+
+[[call]]
+call_type = "invoke"
+contract_address = "commitments_inbox"
+function = "set_headers_store"
+inputs = ["headers_store"]

src/core/commitments_inbox.cairo

@@ -1,56 +1,59 @@
-// SPDX-License-Identifier: GPL-3.0
-
 use starknet::{ContractAddress, EthAddress};
 
-//
-// Interface
-//
-
 #[starknet::interface]
 trait ICommitmentsInbox<TContractState> {
-    // Getters
+    // @notice Returns the address of the HeadersStore contract
+    // @return The address of the headers store contract
     fn get_headers_store(self: @TContractState) -> ContractAddress;
+
+    // @notice Returns the address of the L1 message sender
+    // @return The address of the L1 message sender
     fn get_l1_message_sender(self: @TContractState) -> EthAddress;
+
+    // @notice Returns the address of the owner
+    // @return The address of the owner
     fn get_owner(self: @TContractState) -> ContractAddress;
 
-    // Setters
+    // @notice Sets the address of the HeadersStore contract
+    // @dev This function is only callable by the owner
+    // @param headers_store The address of the headers store contract
     fn set_headers_store(ref self: TContractState, headers_store: ContractAddress);
+
+    // @notice Sets the address of the L1 message sender
+    // @dev This function is only callable by the owner
+    // @param l1_message_sender The address of the L1 message sender
     fn set_l1_message_sender(ref self: TContractState, l1_message_sender: EthAddress);
 
-    // Ownership
+    // @notice Transfers ownership of the contract to a new address
+    // @dev This function is only callable by the owner
+    // @param new_owner The address of the new owner
     fn transfer_ownership(ref self: TContractState, new_owner: ContractAddress);
+
+    // @notice Renounces ownership of the contract, once renounced it cannot be reversed
+    // @dev This function is only callable by the owner
     fn renounce_ownership(ref self: TContractState);
 
-    // Receives a hash commitment from the contract owner
-    fn receive_commitment_owner(ref self: TContractState, blockhash: u256, block_number: u256);
+    // @notice receives a parent blockhash and the corresponding block number, simulating L1 messaging, and sends it to the HeadersStore
+    // @dev This function is only callable by the owner, ownership will be renounced in mainnet
+    fn receive_commitment_owner(ref self: TContractState, parent_hash: u256, block_number: u256);
 }
 
-//
-// Contract
-//
-
+// @notice The contract that receives the commitments from L1, both individual blocks and proven MMRs, sending them to the HeadersStore
+// @dev The contract ownership will be renounced in mainnet, it is only used for testing purposes
 #[starknet::contract]
 mod CommitmentsInbox {
     use starknet::{ContractAddress, get_caller_address, EthAddress};
     use herodotus_eth_starknet::core::headers_store::{
         IHeadersStoreDispatcherTrait, IHeadersStoreDispatcher
     };
 
-    //
-    // Storage
-    //
-
     #[storage]
     struct Storage {
         headers_store: ContractAddress,
         l1_message_sender: EthAddress,
         owner: ContractAddress
     }
 
-    //
-    // Events
-    //
-
     #[event]
     #[derive(Drop, starknet::Event)]
     enum Event {
@@ -73,7 +76,7 @@ mod CommitmentsInbox {
 
     #[derive(Drop, starknet::Event)]
     struct CommitmentReceived {
-        blockhash: u256,
+        parent_hash: u256,
         block_number: u256
     }
 
@@ -102,35 +105,46 @@ mod CommitmentsInbox {
 
     #[external(v0)]
     impl CommitmentsInbox of super::ICommitmentsInbox<ContractState> {
+        // @inheritdoc ICommitmentsInbox
         fn get_headers_store(self: @ContractState) -> ContractAddress {
             self.headers_store.read()
         }
 
+        // @inheritdoc ICommitmentsInbox
         fn get_l1_message_sender(self: @ContractState) -> EthAddress {
             self.l1_message_sender.read()
         }
 
+        // @inheritdoc ICommitmentsInbox
         fn get_owner(self: @ContractState) -> ContractAddress {
             self.owner.read()
         }
 
+        // @inheritdoc ICommitmentsInbox
         fn set_headers_store(ref self: ContractState, headers_store: ContractAddress) {
             let caller = get_caller_address();
             assert(self.owner.read() == caller, 'Only owner');
             self.headers_store.write(headers_store);
         }
 
+        // @inheritdoc ICommitmentsInbox
         fn set_l1_message_sender(ref self: ContractState, l1_message_sender: EthAddress) {
             let caller = get_caller_address();
             assert(self.owner.read() == caller, 'Only owner');
             self.l1_message_sender.write(l1_message_sender);
         }
 
+        // @inheritdoc ICommitmentsInbox
         fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) {
             let caller = get_caller_address();
             assert(self.owner.read() == caller, 'Only owner');
             self.owner.write(new_owner);
 
+            if new_owner.is_zero() {
+                self.emit(Event::OwnershipRenounced(OwnershipRenounced { previous_owner: caller }));
+                return;
+            }
+
             self
                 .emit(
                     Event::OwnershipTransferred(
@@ -139,6 +153,7 @@ mod CommitmentsInbox {
                 );
         }
 
+        // @inheritdoc ICommitmentsInbox
         fn renounce_ownership(ref self: ContractState) {
             let caller = get_caller_address();
             assert(self.owner.read() == caller, 'Only owner');
@@ -147,29 +162,42 @@ mod CommitmentsInbox {
             self.emit(Event::OwnershipRenounced(OwnershipRenounced { previous_owner: caller }));
         }
 
-        fn receive_commitment_owner(ref self: ContractState, blockhash: u256, block_number: u256) {
+        // @inheritdoc ICommitmentsInbox
+        fn receive_commitment_owner(
+            ref self: ContractState, parent_hash: u256, block_number: u256
+        ) {
             let caller = get_caller_address();
             assert(self.owner.read() == caller, 'Only owner');
 
             let contract_address = self.headers_store.read();
-            IHeadersStoreDispatcher { contract_address }.receive_hash(blockhash, block_number);
+            IHeadersStoreDispatcher { contract_address }.receive_hash(parent_hash, block_number);
 
-            self.emit(Event::CommitmentReceived(CommitmentReceived { blockhash, block_number }));
+            self.emit(Event::CommitmentReceived(CommitmentReceived { parent_hash, block_number }));
         }
     }
 
+    // @notice receives a parent blockhash and the corresponding block number from L1, and sends it to the HeadersStore
+    // @param from_address The address of the sender, checking that it is the L1 contract
+    // @param blockhash The parent blockhash of the block
+    // @param block_number The block number of the block
     #[l1_handler]
     fn receive_commitment(
-        ref self: ContractState, from_address: felt252, blockhash: u256, block_number: u256
+        ref self: ContractState, from_address: felt252, parent_hash: u256, block_number: u256
     ) {
         assert(from_address == self.l1_message_sender.read().into(), 'Invalid sender');
 
         let contract_address = self.headers_store.read();
-        IHeadersStoreDispatcher { contract_address }.receive_hash(blockhash, block_number);
+        IHeadersStoreDispatcher { contract_address }.receive_hash(parent_hash, block_number);
 
-        self.emit(Event::CommitmentReceived(CommitmentReceived { blockhash, block_number }));
+        self.emit(Event::CommitmentReceived(CommitmentReceived { parent_hash, block_number }));
     }
 
+    // @notice receives an MMR root and the last position from L1, and sends it to the HeadersStore
+    // @dev This MMR was built offchain and verified on L1
+    // @param from_address The address of the sender, checking that it is the L1 contract
+    // @param root The root of the MMR
+    // @param last_pos The last position of the MMR
+    // @param aggregator_id The aggregator id of the proven MMR
     #[l1_handler]
     fn receive_mmr(
         ref self: ContractState,