feat(3450): complete contracts recommendation (#65)

* Feat/3450 Complete contracts recommendation V2

* fix: linting issue in CoordinatorConfigTest file

---------

Co-authored-by: count-sum <[email protected]>
Co-authored-by: thedarkjester <[email protected]>

config/common/smart-contract-errors.toml

@@ -42,6 +42,7 @@
 "3b174434" = "MessageHashesListLengthHigherThanOneHundred"
 "ca389c44" = "InvalidProofOrProofVerificationRanOutOfGas"
 # L2 Message Service
+"6446cc9c" = "MessageHashesListLengthIsZero"
 "d39e75f9" = "L1MessageNumberSynchronizationWrong"
 "7557a60a" = "L1RollingHashSynchronizationWrong"
 "36a4bb94" = "FinalRollingHashIsZero"

contracts/contracts/LineaRollup.sol

@@ -172,6 +172,10 @@ contract LineaRollup is
       revert BlobSubmissionDataIsMissing();
     }
 
+    if (blobhash(blobSubmissionLength) != EMPTY_HASH) {
+      revert BlobSubmissionDataEmpty(blobSubmissionLength);
+    }
+
     bytes32 currentDataEvaluationPoint;
     bytes32 currentDataHash;
     uint256 lastFinalizedBlockNumber = currentL2BlockNumber;

contracts/contracts/interfaces/l1/ILineaRollup.sol

@@ -194,10 +194,15 @@ interface ILineaRollup {
   error EmptyBlobDataAtIndex(uint256 index);
 
   /**
-   * @dev Thrown when the data for multiple blobs' submission has length zero.
+   * @dev Thrown when the data for multiple blobs submission has length zero.
    */
   error BlobSubmissionDataIsMissing();
 
+  /**
+   * @dev Thrown when a blob has been submitted but there is no data for it.
+   */
+  error BlobSubmissionDataEmpty(uint256 emptyBlobIndex);
+
   /**
    * @dev Thrown when the starting block in the data item is out of sequence with the last block number.
    */

contracts/contracts/interfaces/l2/IL2MessageManager.sol

@@ -16,6 +16,11 @@ interface IL2MessageManager {
    */
   event RollingHashUpdated(uint256 indexed messageNumber, bytes32 indexed rollingHash);
 
+  /**
+   * @dev Reverts when the message hashes array length is zero.
+   */
+  error MessageHashesListLengthIsZero();
+
   /**
    * @dev Reverts when message number synchronization is mismatched.
    */

contracts/contracts/messageService/MessageServiceBase.sol

@@ -18,6 +18,13 @@ abstract contract MessageServiceBase is Initializable, IGenericErrors {
   /// @dev Keep 10 free storage slots for future implementation updates to avoid storage collision.
   uint256[10] private __base_gap;
 
+  /**
+   * @dev Event emitted when the remote sender is set.
+   * @param remoteSender The address of the new remote sender.
+   * @param setter The address of the account that set the remote sender.
+   */
+  event RemoteSenderSet(address indexed remoteSender, address indexed setter);
+
   /**
    * @dev Thrown when the caller address is not the message service address
    */
@@ -71,6 +78,7 @@ abstract contract MessageServiceBase is Initializable, IGenericErrors {
 
   /**
    * @notice Sets the remote sender
+   * @dev This function sets the remote sender address and emits the RemoteSenderSet event.
    * @param _remoteSender The authorized remote sender address, cannot be empty.
    */
   function _setRemoteSender(address _remoteSender) internal {
@@ -79,5 +87,6 @@ abstract contract MessageServiceBase is Initializable, IGenericErrors {
     }
 
     remoteSender = _remoteSender;
+    emit RemoteSenderSet(_remoteSender, msg.sender);
   }
 }

contracts/contracts/messageService/l2/L2MessageManager.sol

@@ -40,6 +40,10 @@ abstract contract L2MessageManager is AccessControlUpgradeable, IL2MessageManage
   ) external whenTypeNotPaused(GENERAL_PAUSE_TYPE) onlyRole(L1_L2_MESSAGE_SETTER_ROLE) {
     uint256 messageHashesLength = _messageHashes.length;
 
+    if (messageHashesLength == 0) {
+      revert MessageHashesListLengthIsZero();
+    }
+
     if (messageHashesLength > 100) {
       revert MessageHashesListLengthHigherThanOneHundred(messageHashesLength);
     }

contracts/contracts/messageService/l2/v1/L2MessageServiceV1.sol

@@ -152,7 +152,7 @@ abstract contract L2MessageServiceV1 is
     uint256 previousMinimumFee = minimumFeeInWei;
     minimumFeeInWei = _feeInWei;
 
-    emit MinimumFeeChanged(previousMinimumFee, minimumFeeInWei, msg.sender);
+    emit MinimumFeeChanged(previousMinimumFee, _feeInWei, msg.sender);
   }
 
   /**

contracts/contracts/messageService/lib/RateLimiter.sol

@@ -60,20 +60,17 @@ contract RateLimiter is Initializable, IRateLimiter, AccessControlUpgradeable {
    */
   function _addUsedAmount(uint256 _usedAmount) internal {
     if (_usedAmount != 0) {
-      uint256 currentPeriodAmountTemp;
-
       if (currentPeriodEnd < block.timestamp) {
         currentPeriodEnd = block.timestamp + periodInSeconds;
-        currentPeriodAmountTemp = _usedAmount;
       } else {
-        currentPeriodAmountTemp = currentPeriodAmountInWei + _usedAmount;
+        _usedAmount += currentPeriodAmountInWei;
       }
 
-      if (currentPeriodAmountTemp > limitInWei) {
+      if (_usedAmount > limitInWei) {
         revert RateLimitExceeded();
       }
 
-      currentPeriodAmountInWei = currentPeriodAmountTemp;
+      currentPeriodAmountInWei = _usedAmount;
     }
   }
 

contracts/contracts/messageService/lib/SparseMerkleTreeVerifier.sol

@@ -1,12 +1,16 @@
 // SPDX-License-Identifier: AGPL-3.0
 pragma solidity 0.8.26;
 
+import { Utils } from "../../lib/Utils.sol";
+
 /**
  * @title Library to verify sparse merkle proofs and to get the leaf hash value
  * @author ConsenSys Software Inc.
  * @custom:security-contact [email protected]
  */
 library SparseMerkleTreeVerifier {
+  using Utils for *;
+
   /**
    * @dev Custom error for when the leaf index is out of bounds.
    */
@@ -35,24 +39,11 @@ library SparseMerkleTreeVerifier {
 
     for (uint256 height; height < _proof.length; ++height) {
       if (((_leafIndex >> height) & 1) == 1) {
-        node = _efficientKeccak(_proof[height], node);
+        node = Utils._efficientKeccak(_proof[height], node);
       } else {
-        node = _efficientKeccak(node, _proof[height]);
+        node = Utils._efficientKeccak(node, _proof[height]);
       }
     }
     return node == _root;
   }
-
-  /**
-   * @notice Performs a gas optimized keccak hash
-   * @param _left Left value.
-   * @param _right Right value.
-   */
-  function _efficientKeccak(bytes32 _left, bytes32 _right) internal pure returns (bytes32 value) {
-    assembly {
-      mstore(0x00, _left)
-      mstore(0x20, _right)
-      value := keccak256(0x00, 0x40)
-    }
-  }
 }

contracts/contracts/test-contracts/TestMessageServiceBase.sol

@@ -17,4 +17,8 @@ contract TestMessageServiceBase is MessageServiceBase {
     __MessageServiceBase_init(_messageService);
     _setRemoteSender(_remoteSender);
   }
+
+  function testSetRemoteSender(address _remoteSender) external {
+    _setRemoteSender(_remoteSender);
+  }
 }

contracts/contracts/test-contracts/TestSparseMerkleTreeVerifier.sol

@@ -2,9 +2,11 @@
 pragma solidity 0.8.26;
 
 import { SparseMerkleTreeVerifier } from "../messageService/lib/SparseMerkleTreeVerifier.sol";
+import { Utils } from "../lib/Utils.sol";
 
 contract TestSparseMerkleTreeVerifier {
   using SparseMerkleTreeVerifier for *;
+  using Utils for *;
 
   function verifyMerkleProof(
     bytes32 _leafHash,
@@ -16,7 +18,7 @@ contract TestSparseMerkleTreeVerifier {
   }
 
   function efficientKeccak(bytes32 _left, bytes32 _right) external pure returns (bytes32 value) {
-    return SparseMerkleTreeVerifier._efficientKeccak(_left, _right);
+    return Utils._efficientKeccak(_left, _right);
   }
 
   function getLeafHash(

contracts/test/L2MessageManager.ts

@@ -77,6 +77,16 @@ describe("L2MessageManager", () => {
       );
     });
 
+    it("Should revert if message hashes array length is zero", async () => {
+      const messageHashes: [] = [];
+
+      const anchorCall = l2MessageManager
+        .connect(l1l2MessageSetter)
+        .anchorL1L2MessageHashes(messageHashes, 1, 100, HASH_ZERO);
+
+      await expectRevertWithCustomError(l2MessageManager, anchorCall, "MessageHashesListLengthIsZero");
+    });
+
     it("Should update rolling hash and messages emitting events", async () => {
       const messageHashes = generateNKeccak256Hashes("message", 100);
       const expectedRollingHash = calculateRollingHashFromCollection(ethers.ZeroHash, messageHashes.slice(0, 100));

contracts/test/LineaRollup.ts

@@ -1106,6 +1106,50 @@ describe("Linea Rollup contract", () => {
       );
     });
 
+    it("Should revert if there is less data than blobs", async () => {
+      const operatorHDSigner = getWalletForIndex(2);
+      const lineaRollupAddress = await lineaRollup.getAddress();
+
+      const {
+        blobDataSubmission: blobSubmission,
+        compressedBlobs: compressedBlobs,
+        parentShnarf: parentShnarf,
+        finalShnarf: finalShnarf,
+      } = generateBlobDataSubmission(0, 2, true);
+
+      const encodedCall = lineaRollup.interface.encodeFunctionData("submitBlobs", [
+        [blobSubmission[0]],
+        parentShnarf,
+        finalShnarf,
+      ]);
+
+      const { maxFeePerGas, maxPriorityFeePerGas } = await ethers.provider.getFeeData();
+      const nonce = await operatorHDSigner.getNonce();
+
+      const transaction = Transaction.from({
+        data: encodedCall,
+        maxPriorityFeePerGas: maxPriorityFeePerGas!,
+        maxFeePerGas: maxFeePerGas!,
+        to: lineaRollupAddress,
+        chainId: (await ethers.provider.getNetwork()).chainId,
+        type: 3,
+        nonce: nonce,
+        value: 0,
+        gasLimit: 5_000_000,
+        kzg,
+        maxFeePerBlobGas: 1n,
+        blobs: compressedBlobs,
+      });
+
+      const signedTx = await operatorHDSigner.signTransaction(transaction);
+      await expectRevertWithCustomError(
+        lineaRollup,
+        ethers.provider.broadcastTransaction(signedTx),
+        "BlobSubmissionDataEmpty",
+        [1],
+      );
+    });
+
     it("Should fail to finalize with not enough gas for the rollup (pre-verifier)", async () => {
       // Submit 2 blobs
       await sendBlobTransaction(0, 2);

contracts/test/MessageServiceBase.ts

@@ -13,7 +13,7 @@ import {
   unpauseTypeRoles,
 } from "./utils/constants";
 import { deployUpgradableFromFactory } from "./utils/deployment";
-import { expectRevertWithCustomError, expectRevertWithReason } from "./utils/helpers";
+import { expectEvent, expectRevertWithCustomError, expectRevertWithReason } from "./utils/helpers";
 
 describe("MessageServiceBase", () => {
   let messageServiceBase: TestMessageServiceBase;
@@ -85,6 +85,18 @@ describe("MessageServiceBase", () => {
     });
   });
 
+  describe("RemoteSenderSet event", () => {
+    it("Should emit RemoteSenderSet event when testSetRemoteSender is called", async () => {
+      const newRemoteSender = ethers.Wallet.createRandom().address;
+      await expectEvent(
+        messageServiceBase,
+        messageServiceBase.testSetRemoteSender(newRemoteSender),
+        "RemoteSenderSet",
+        [newRemoteSender, admin.address],
+      );
+    });
+  });
+
   describe("onlyMessagingService() modifier", () => {
     it("Should revert if msg.sender is not the message service address", async () => {
       await expectRevertWithCustomError(

coordinator/app/src/test/kotlin/net/consensys/zkevm/coordinator/app/config/CoordinatorConfigTest.kt

@@ -180,6 +180,7 @@ class CoordinatorConfigTest {
         "3b174434" to "MessageHashesListLengthHigherThanOneHundred",
         "ca389c44" to "InvalidProofOrProofVerificationRanOutOfGas",
         // L2 Message Service
+        "6446cc9c" to "MessageHashesListLengthIsZero",
         "d39e75f9" to "L1MessageNumberSynchronizationWrong",
         "7557a60a" to "L1RollingHashSynchronizationWrong",
         "36a4bb94" to "FinalRollingHashIsZero"