[Fix] G-2,3,4 and 5 - Use return vars on function and optimize reverts (

#247) * Use return vars on function and optimize reverts * use calldata array lengths directly * remove cache length on message anchoring * fix: remove duplicate line * chore: export new ABIs for LineaRollupV6 and L2MessageServiceV1 --------- Signed-off-by: Victorien Gauch <[email protected]> Co-authored-by: Victorien Gauch <[email protected]> Co-authored-by: VGau <[email protected]>
Consensys · Oct 31, 2024 · 660f849 · 660f849
1 parent 390d6ff
commit 660f849
Show file tree

Hide file tree

Showing 14 changed files with 100 additions and 69 deletions.
diff --git a/contracts/abi/L2MessageServiceV1.0.abi b/contracts/abi/L2MessageServiceV1.0.abi
@@ -975,7 +975,7 @@
     "outputs": [
       {
         "internalType": "bool",
-        "name": "",
+        "name": "pauseTypeIsPaused",
         "type": "bool"
       }
     ],
@@ -1242,7 +1242,7 @@
     "outputs": [
       {
         "internalType": "address",
-        "name": "",
+        "name": "originalSender",
         "type": "address"
       }
     ],

diff --git a/contracts/abi/LineaRollupV6.0.abi b/contracts/abi/LineaRollupV6.0.abi
@@ -84,6 +84,17 @@
     "name": "FeeTooLow",
     "type": "error"
   },
+  {
+    "inputs": [
+      {
+        "internalType": "bytes32",
+        "name": "shnarf",
+        "type": "bytes32"
+      }
+    ],
+    "name": "FinalBlobNotSubmitted",
+    "type": "error"
+  },
   {
     "inputs": [
       {
@@ -403,6 +414,22 @@
     "name": "ReentrantCall",
     "type": "error"
   },
+  {
+    "inputs": [
+      {
+        "internalType": "uint8",
+        "name": "bits",
+        "type": "uint8"
+      },
+      {
+        "internalType": "uint256",
+        "name": "value",
+        "type": "uint256"
+      }
+    ],
+    "name": "SafeCastOverflowedUintDowncast",
+    "type": "error"
+  },
   {
     "inputs": [],
     "name": "StartingRootHashDoesNotMatch",
@@ -1873,7 +1900,7 @@
     "outputs": [
       {
         "internalType": "bool",
-        "name": "",
+        "name": "isClaimed",
         "type": "bool"
       }
     ],
@@ -1892,7 +1919,7 @@
     "outputs": [
       {
         "internalType": "bool",
-        "name": "",
+        "name": "pauseTypeIsPaused",
         "type": "bool"
       }
     ],
@@ -2176,7 +2203,7 @@
     "outputs": [
       {
         "internalType": "address",
-        "name": "addr",
+        "name": "originalSender",
         "type": "address"
       }
     ],

diff --git a/contracts/contracts/LineaRollup.sol b/contracts/contracts/LineaRollup.sol
@@ -214,14 +214,26 @@ contract LineaRollup is
     bytes32 _parentShnarf,
     bytes32 _finalBlobShnarf
   ) external whenTypeAndGeneralNotPaused(PauseType.BLOB_SUBMISSION) onlyRole(OPERATOR_ROLE) {
-    uint256 blobSubmissionLength = _blobSubmissions.length;
-
-    if (blobSubmissionLength == 0) {
+    if (_blobSubmissions.length == 0) {
       revert BlobSubmissionDataIsMissing();
     }
 
-    if (blobhash(blobSubmissionLength) != EMPTY_HASH) {
-      revert BlobSubmissionDataEmpty(blobSubmissionLength);
+    if (blobhash(_blobSubmissions.length) != EMPTY_HASH) {
+      revert BlobSubmissionDataEmpty(_blobSubmissions.length);
+    }
+
+    if (blobShnarfExists[_parentShnarf] == 0) {
+      revert ParentBlobNotSubmitted(_parentShnarf);
+    }
+
+    /**
+     * @dev validate we haven't submitted the last shnarf. There is a final check at the end of the function verifying,
+     * that _finalBlobShnarf was computed correctly.
+     * Note: As only the last shnarf is stored, we don't need to validate shnarfs,
+     * computed for any previous blobs in the submission (if multiple are submitted).
+     */
+    if (blobShnarfExists[_finalBlobShnarf] != 0) {
+      revert DataAlreadySubmitted(_finalBlobShnarf);
     }
 
     bytes32 currentDataEvaluationPoint;
@@ -230,13 +242,9 @@ contract LineaRollup is
     /// @dev Assigning in memory saves a lot of gas vs. calldata reading.
     BlobSubmission memory blobSubmission;
 
-    if (blobShnarfExists[_parentShnarf] == 0) {
-      revert ParentBlobNotSubmitted(_parentShnarf);
-    }
-
     bytes32 computedShnarf = _parentShnarf;
 
-    for (uint256 i; i < blobSubmissionLength; i++) {
+    for (uint256 i; i < _blobSubmissions.length; i++) {
       blobSubmission = _blobSubmissions[i];
 
       currentDataHash = blobhash(i);
@@ -270,15 +278,6 @@ contract LineaRollup is
       revert FinalShnarfWrong(_finalBlobShnarf, computedShnarf);
     }
 
-    /**
-     * @dev validate we haven't submitted the last shnarf.
-     * Note: As only the last shnarf is stored, we don't need to validate shnarfs,
-     * computed for any previous blobs in the submission (if multiple are submitted).
-     */
-    if (blobShnarfExists[computedShnarf] != 0) {
-      revert DataAlreadySubmitted(computedShnarf);
-    }
-
     /// @dev use the last shnarf as the submission to store as technically it becomes the next parent shnarf.
     blobShnarfExists[computedShnarf] = SHNARF_EXISTS_DEFAULT_VALUE;
 
@@ -301,6 +300,10 @@ contract LineaRollup is
       revert EmptySubmissionData();
     }
 
+    if (blobShnarfExists[_expectedShnarf] != 0) {
+      revert DataAlreadySubmitted(_expectedShnarf);
+    }
+
     if (blobShnarfExists[_parentShnarf] == 0) {
       revert ParentBlobNotSubmitted(_parentShnarf);
     }
@@ -321,10 +324,6 @@ contract LineaRollup is
       revert FinalShnarfWrong(_expectedShnarf, computedShnarf);
     }
 
-    if (blobShnarfExists[computedShnarf] != 0) {
-      revert DataAlreadySubmitted(computedShnarf);
-    }
-
     blobShnarfExists[computedShnarf] = SHNARF_EXISTS_DEFAULT_VALUE;
 
     emit DataSubmittedV3(_parentShnarf, computedShnarf, _submission.finalStateRootHash);

diff --git a/contracts/contracts/interfaces/IMessageService.sol b/contracts/contracts/interfaces/IMessageService.sol
@@ -89,7 +89,7 @@ interface IMessageService {
 
   /**
    * @notice Returns the original sender of the message on the origin layer.
-   * @return The original sender of the message on the origin layer.
+   * @return originalSender The original sender of the message on the origin layer.
    */
-  function sender() external view returns (address);
+  function sender() external view returns (address originalSender);
 }
diff --git a/contracts/contracts/interfaces/IPauseManager.sol b/contracts/contracts/interfaces/IPauseManager.sol
@@ -91,7 +91,7 @@ interface IPauseManager {
   /**
    * @notice Check if a pause type is enabled.
    * @param _pauseType The pause type value.
-   * @return boolean True if the pause type if enabled, false otherwise.
+   * @return pauseTypeIsPaused Returns true if the pause type if paused, false otherwise.
    */
-  function isPaused(PauseType _pauseType) external view returns (bool);
+  function isPaused(PauseType _pauseType) external view returns (bool pauseTypeIsPaused);
 }
diff --git a/contracts/contracts/interfaces/l1/IL1MessageManager.sol b/contracts/contracts/interfaces/l1/IL1MessageManager.sol
@@ -48,7 +48,7 @@ interface IL1MessageManager {
   /**
    * @notice Checks if the L2->L1 message is claimed or not.
    * @param _messageNumber The message number on L2.
-   * @return Bool indicating claimed or not.
+   * @return isClaimed Returns whether or not the message with _messageNumber has been claimed.
    */
-  function isMessageClaimed(uint256 _messageNumber) external view returns (bool);
+  function isMessageClaimed(uint256 _messageNumber) external view returns (bool isClaimed);
 }
diff --git a/contracts/contracts/lib/PauseManager.sol b/contracts/contracts/lib/PauseManager.sol
@@ -60,13 +60,12 @@ abstract contract PauseManager is Initializable, IPauseManager, AccessControlUpg
     PauseTypeRole[] calldata _pauseTypeRoleAssignments,
     PauseTypeRole[] calldata _unpauseTypeRoleAssignments
   ) internal onlyInitializing {
-    uint256 arrayLength = _pauseTypeRoleAssignments.length;
-    for (uint256 i; i < arrayLength; i++) {
+    for (uint256 i; i < _pauseTypeRoleAssignments.length; i++) {
       _pauseTypeRoles[_pauseTypeRoleAssignments[i].pauseType] = _pauseTypeRoleAssignments[i].role;
       emit PauseTypeRoleSet(_pauseTypeRoleAssignments[i].pauseType, _pauseTypeRoleAssignments[i].role);
     }
-    arrayLength = _unpauseTypeRoleAssignments.length;
-    for (uint256 i; i < arrayLength; i++) {
+
+    for (uint256 i; i < _unpauseTypeRoleAssignments.length; i++) {
       _unPauseTypeRoles[_unpauseTypeRoleAssignments[i].pauseType] = _unpauseTypeRoleAssignments[i].role;
       emit UnPauseTypeRoleSet(_unpauseTypeRoleAssignments[i].pauseType, _unpauseTypeRoleAssignments[i].role);
     }
@@ -131,9 +130,9 @@ abstract contract PauseManager is Initializable, IPauseManager, AccessControlUpg
   /**
    * @notice Check if a pause type is enabled.
    * @param _pauseType The pause type value.
-   * @return boolean True if the pause type if enabled, false otherwise.
+   * @return pauseTypeIsPaused Returns true if the pause type if paused, false otherwise.
    */
-  function isPaused(PauseType _pauseType) public view returns (bool) {
-    return (_pauseTypeStatusesBitMap & (1 << uint256(_pauseType))) != 0;
+  function isPaused(PauseType _pauseType) public view returns (bool pauseTypeIsPaused) {
+    pauseTypeIsPaused = (_pauseTypeStatusesBitMap & (1 << uint256(_pauseType))) != 0;
   }
 }
diff --git a/contracts/contracts/lib/PermissionsManager.sol b/contracts/contracts/lib/PermissionsManager.sol
@@ -17,9 +17,7 @@ abstract contract PermissionsManager is Initializable, AccessControlUpgradeable,
    * @param _roleAddresses The list of addresses and roles to assign permissions to.
    */
   function __Permissions_init(RoleAddress[] calldata _roleAddresses) internal onlyInitializing {
-    uint256 roleAddressesLength = _roleAddresses.length;
-
-    for (uint256 i; i < roleAddressesLength; i++) {
+    for (uint256 i; i < _roleAddresses.length; i++) {
       if (_roleAddresses[i].addressWithRole == address(0)) {
         revert ZeroAddressNotAllowed();
       }

diff --git a/contracts/contracts/messageService/l1/L1MessageManager.sol b/contracts/contracts/messageService/l1/L1MessageManager.sol
@@ -94,8 +94,9 @@ abstract contract L1MessageManager is L1MessageManagerV1, IL1MessageManager {
   /**
    * @notice Checks if the L2->L1 message is claimed or not.
    * @param _messageNumber The message number on L2.
+   * @return isClaimed Returns whether or not the message with _messageNumber has been claimed.
    */
-  function isMessageClaimed(uint256 _messageNumber) external view returns (bool) {
-    return _messageClaimedBitMap.get(_messageNumber);
+  function isMessageClaimed(uint256 _messageNumber) external view returns (bool isClaimed) {
+    isClaimed = _messageClaimedBitMap.get(_messageNumber);
   }
 }
diff --git a/contracts/contracts/messageService/l1/L1MessageService.sol b/contracts/contracts/messageService/l1/L1MessageService.sol
@@ -144,9 +144,9 @@ abstract contract L1MessageService is
   /**
    * @notice Claims and delivers a cross-chain message.
    * @dev The message sender address is set temporarily in the transient storage when claiming.
-   * @return addr The message sender address that is stored temporarily in the transient storage when claiming.
+   * @return originalSender The message sender address that is stored temporarily in the transient storage when claiming.
    */
-  function sender() external view returns (address addr) {
-    return TransientStorageHelpers.tloadAddress(MESSAGE_SENDER_TRANSIENT_KEY);
+  function sender() external view returns (address originalSender) {
+    originalSender = TransientStorageHelpers.tloadAddress(MESSAGE_SENDER_TRANSIENT_KEY);
   }
 }
diff --git a/contracts/contracts/messageService/l2/L2MessageManager.sol b/contracts/contracts/messageService/l2/L2MessageManager.sol
@@ -40,14 +40,12 @@ abstract contract L2MessageManager is AccessControlUpgradeable, IL2MessageManage
     uint256 _finalMessageNumber,
     bytes32 _finalRollingHash
   ) external whenTypeNotPaused(PauseType.GENERAL) onlyRole(L1_L2_MESSAGE_SETTER_ROLE) {
-    uint256 messageHashesLength = _messageHashes.length;
-
-    if (messageHashesLength == 0) {
+    if (_messageHashes.length == 0) {
       revert MessageHashesListLengthIsZero();
     }
 
-    if (messageHashesLength > 100) {
-      revert MessageHashesListLengthHigherThanOneHundred(messageHashesLength);
+    if (_messageHashes.length > 100) {
+      revert MessageHashesListLengthHigherThanOneHundred(_messageHashes.length);
     }
 
     if (_finalRollingHash == 0x0) {
@@ -63,7 +61,7 @@ abstract contract L2MessageManager is AccessControlUpgradeable, IL2MessageManage
     bytes32 rollingHash = l1RollingHashes[currentL1MessageNumber];
 
     bytes32 messageHash;
-    for (uint256 i; i < messageHashesLength; ++i) {
+    for (uint256 i; i < _messageHashes.length; ++i) {
       messageHash = _messageHashes[i];
       if (inboxL1L2MessageStatus[messageHash] == INBOX_STATUS_UNKNOWN) {
         inboxL1L2MessageStatus[messageHash] = INBOX_STATUS_RECEIVED;

diff --git a/contracts/contracts/messageService/l2/v1/L2MessageServiceV1.sol b/contracts/contracts/messageService/l2/v1/L2MessageServiceV1.sol
@@ -160,10 +160,10 @@ abstract contract L2MessageServiceV1 is
 
   /**
    * @dev The _messageSender address is set temporarily when claiming.
-   * @return _messageSender address.
+   * @return originalSender The original sender stored temporarily at the _messageSender address in storage.
    */
-  function sender() external view returns (address) {
-    return _messageSender;
+  function sender() external view returns (address originalSender) {
+    originalSender = _messageSender;
   }
 
   /**

diff --git a/contracts/contracts/messageService/lib/SparseMerkleTreeVerifier.sol b/contracts/contracts/messageService/lib/SparseMerkleTreeVerifier.sol
@@ -29,14 +29,16 @@ library SparseMerkleTreeVerifier {
    * @param _leafIndex Index of the leaf.
    * @param _root Merkle root.
    * @dev The depth of the tree is expected to be validated elsewhere beforehand.
+   * @return proofIsValid Returns if the proof is valid or not.
    */
   function _verifyMerkleProof(
     bytes32 _leafHash,
     bytes32[] calldata _proof,
     uint32 _leafIndex,
     bytes32 _root
-  ) internal pure returns (bool) {
+  ) internal pure returns (bool proofIsValid) {
     uint32 maxAllowedIndex = safeCastToUint32((2 ** _proof.length) - 1);
+
     if (_leafIndex > maxAllowedIndex) {
       revert LeafIndexOutOfBounds(_leafIndex, maxAllowedIndex);
     }
@@ -50,7 +52,7 @@ library SparseMerkleTreeVerifier {
         node = Utils._efficientKeccak(node, _proof[height]);
       }
     }
-    return node == _root;
+    proofIsValid = node == _root;
   }
 
   /**