Merge pull request #60 from matter-labs/sb-codehawks-batch

Codehawks batch

da-contracts/contracts/CalldataDA.sol

@@ -70,7 +70,7 @@ abstract contract CalldataDA {
 
         // Now, we need to double check that the provided input was indeed returned by the L2 DA validator.
         if (keccak256(_operatorDAInput[:ptr]) != _l2DAValidatorOutputHash) {
-            revert InvalidL2DAOutputHash();
+            revert InvalidL2DAOutputHash(_l2DAValidatorOutputHash);
         }
 
         // The rest of the output was provided specifically by the operator

da-contracts/contracts/DAContractsErrors.sol

@@ -25,8 +25,8 @@ error InvalidNumberOfBlobs(uint256 blobsProvided, uint256 maxBlobsSupported);
 // 0xcd384e46
 error InvalidBlobsHashes(uint256 operatorDAInputLength, uint256 blobsProvided);
 
-// 0xe9e79528
-error InvalidL2DAOutputHash();
+// 0xd2531c15
+error InvalidL2DAOutputHash(bytes32 l2DAValidatorOutputHash);
 
 // 0x3db6e664
 error OneBlobWithCalldata();

da-contracts/contracts/IL1Messenger.sol

@@ -7,5 +7,5 @@ pragma solidity 0.8.24;
  * @notice The interface of the L1 Messenger contract, responsible for sending messages to L1.
  */
 interface IL1Messenger {
-    function sendToL1(bytes memory _message) external returns (bytes32);
+    function sendToL1(bytes calldata _message) external returns (bytes32);
 }

da-contracts/contracts/RollupL1DAValidator.sol

@@ -149,9 +149,7 @@ contract RollupL1DAValidator is IL1DAValidator, CalldataDA {
         // we iterate over the `_operatorDAInput`, while advancing the pointer by `BLOB_DA_INPUT_SIZE` each time
         for (uint256 i = 0; i < _blobsProvided; ++i) {
             bytes calldata commitmentData = _operatorDAInput[:PUBDATA_COMMITMENT_SIZE];
-            bytes32 prepublishedCommitment = bytes32(
-                _operatorDAInput[PUBDATA_COMMITMENT_SIZE:PUBDATA_COMMITMENT_SIZE + 32]
-            );
+            bytes32 prepublishedCommitment = bytes32(_operatorDAInput[PUBDATA_COMMITMENT_SIZE:BLOB_DA_INPUT_SIZE]);
 
             if (prepublishedCommitment != bytes32(0)) {
                 // We double check that this commitment has indeed been published.

l1-contracts/contracts/bridge/L1Nullifier.sol

@@ -246,7 +246,7 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
     /// @param _l1AssetRouter The address of the asset router.
     function setL1AssetRouter(address _l1AssetRouter) external onlyOwner {
         if (address(l1AssetRouter) != address(0)) {
-            revert AddressAlreadySet(address(_l1AssetRouter));
+            revert AddressAlreadySet(address(l1AssetRouter));
         }
         if (_l1AssetRouter == address(0)) {
             revert ZeroAddress();

l1-contracts/contracts/bridge/L2WrappedBaseTokenStore.sol

@@ -3,7 +3,7 @@ pragma solidity ^0.8.0;
 
 import {Ownable2Step} from "@openzeppelin/contracts-v4/access/Ownable2Step.sol";
 
-import {ZeroAddress, Unauthorized} from "../common/L1ContractErrors.sol";
+import {ZeroAddress, Unauthorized, WrappedBaseTokenAlreadyRegistered} from "../common/L1ContractErrors.sol";
 
 /// @title L2WrappedBaseTokenStore
 /// @author Matter Labs
@@ -69,6 +69,9 @@ contract L2WrappedBaseTokenStore is Ownable2Step {
         if (_l2WBaseToken == address(0)) {
             revert ZeroAddress();
         }
+        if (l2WBaseTokenAddress[_chainId] != address(0)) {
+            revert WrappedBaseTokenAlreadyRegistered();
+        }
         _setWBaseTokenAddress(_chainId, _l2WBaseToken);
     }
 

l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol

@@ -22,7 +22,7 @@ import {DataEncoding} from "../../common/libraries/DataEncoding.sol";
 import {AddressAliasHelper} from "../../vendor/AddressAliasHelper.sol";
 import {TWO_BRIDGES_MAGIC_VALUE, ETH_TOKEN_ADDRESS} from "../../common/Config.sol";
 import {NativeTokenVaultAlreadySet} from "../L1BridgeContractErrors.sol";
-import {LegacyBridgeUsesNonNativeToken, NonEmptyMsgValue, UnsupportedEncodingVersion, AssetIdNotSupported, AssetHandlerDoesNotExist, Unauthorized, ZeroAddress, TokenNotSupported, AddressAlreadyUsed} from "../../common/L1ContractErrors.sol";
+import {LegacyBridgeUsesNonNativeToken, NonEmptyMsgValue, UnsupportedEncodingVersion, AssetIdNotSupported, AssetHandlerDoesNotExist, Unauthorized, ZeroAddress, TokenNotSupported, AddressAlreadyUsed, TokensWithFeesNotSupported} from "../../common/L1ContractErrors.sol";
 import {L2_ASSET_ROUTER_ADDR} from "../../common/L2ContractAddresses.sol";
 
 import {IBridgehub, L2TransactionRequestTwoBridgesInner, L2TransactionRequestDirect} from "../../bridgehub/IBridgehub.sol";
@@ -436,8 +436,14 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
             weCanTransfer = true;
         }
         if (weCanTransfer) {
+            uint256 balanceBefore = l1Token.balanceOf(address(nativeTokenVault));
             // slither-disable-next-line arbitrary-send-erc20
             l1Token.safeTransferFrom(_originalCaller, address(nativeTokenVault), _amount);
+            uint256 balanceAfter = l1Token.balanceOf(address(nativeTokenVault));
+
+            if (balanceAfter - balanceBefore != _amount) {
+                revert TokensWithFeesNotSupported();
+            }
             return true;
         }
         return false;
@@ -542,8 +548,6 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
                 revert LegacyBridgeUsesNonNativeToken();
             }
 
-            IERC20(_l1Token).forceApprove(address(nativeTokenVault), _amount);
-
             // Note, that starting from here `bridgeData` starts denoting bridgeMintData.
             bridgeData = _burn({
                 _chainId: ERA_CHAIN_ID,

l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol

@@ -281,7 +281,9 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
         uint256 _amount,
         bool _isNative
     ) internal override {
-        if ((_isNative) || (originChainId[_assetId] != _chainId)) {
+        // Note, that we do not update balances for chains where the assetId comes from,
+        // since these chains can mint new instances of the token.
+        if (!_hasInfiniteBalance(_isNative, _assetId, _chainId)) {
             chainBalance[_chainId][_assetId] += _amount;
         }
     }
@@ -292,12 +294,24 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
         uint256 _amount,
         bool _isNative
     ) internal override {
-        if ((_isNative) || (originChainId[_assetId] != _chainId)) {
+        // Note, that we do not update balances for chains where the assetId comes from,
+        // since these chains can mint new instances of the token.
+        if (!_hasInfiniteBalance(_isNative, _assetId, _chainId)) {
             // Check that the chain has sufficient balance
             if (chainBalance[_chainId][_assetId] < _amount) {
                 revert InsufficientChainBalance();
             }
             chainBalance[_chainId][_assetId] -= _amount;
         }
     }
+
+    /// @dev Returns whether a chain `_chainId` has infinite balance for an asset `_assetId`, i.e.
+    /// it can be minted by it.
+    /// @param _isNative Whether the asset is native to the L1 chain.
+    /// @param _assetId The asset id
+    /// @param _chainId An id of a chain which we test against.
+    /// @return Whether The chain `_chainId` has infinite balance of the token
+    function _hasInfiniteBalance(bool _isNative, bytes32 _assetId, uint256 _chainId) private view returns (bool) {
+        return !_isNative && originChainId[_assetId] == _chainId;
+    }
 }

l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol

@@ -224,7 +224,8 @@ abstract contract NativeTokenVault is
     }
 
     function tryRegisterTokenFromBurnData(bytes calldata _data, bytes32 _expectedAssetId) external {
-        (uint256 amount, address receiver, address tokenAddress) = DataEncoding.decodeBridgeBurnData(_data);
+        // slither-disable-next-line unused-return
+        (, , address tokenAddress) = DataEncoding.decodeBridgeBurnData(_data);
 
         if (tokenAddress == address(0)) {
             revert ZeroAddress();

l1-contracts/contracts/bridgehub/CTMDeploymentTracker.sol

@@ -8,7 +8,6 @@ import {IBridgehub, L2TransactionRequestTwoBridgesInner} from "./IBridgehub.sol"
 import {ICTMDeploymentTracker} from "./ICTMDeploymentTracker.sol";
 
 import {IAssetRouterBase} from "../bridge/asset-router/IAssetRouterBase.sol";
-import {ReentrancyGuard} from "../common/ReentrancyGuard.sol";
 import {TWO_BRIDGES_MAGIC_VALUE} from "../common/Config.sol";
 import {L2_BRIDGEHUB_ADDR} from "../common/L2ContractAddresses.sol";
 import {OnlyBridgehub, CTMNotRegistered, NotOwnerViaRouter, NoEthAllowed, NotOwner, WrongCounterPart} from "./L1BridgehubErrors.sol";
@@ -20,7 +19,7 @@ bytes1 constant CTM_DEPLOYMENT_TRACKER_ENCODING_VERSION = 0x01;
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 /// @dev Contract to be deployed on L1, can link together other contracts based on AssetInfo.
-contract CTMDeploymentTracker is ICTMDeploymentTracker, ReentrancyGuard, Ownable2StepUpgradeable {
+contract CTMDeploymentTracker is ICTMDeploymentTracker, Ownable2StepUpgradeable {
     /// @dev Bridgehub smart contract that is used to operate with L2 via asynchronous L2 <-> L1 communication.
     IBridgehub public immutable override BRIDGE_HUB;
 
@@ -45,15 +44,15 @@ contract CTMDeploymentTracker is ICTMDeploymentTracker, ReentrancyGuard, Ownable
 
     /// @dev Contract is expected to be used as proxy implementation on L1.
     /// @dev Initialize the implementation to prevent Parity hack.
-    constructor(IBridgehub _bridgehub, IAssetRouterBase _l1AssetRouter) reentrancyGuardInitializer {
+    constructor(IBridgehub _bridgehub, IAssetRouterBase _l1AssetRouter) {
         _disableInitializers();
         BRIDGE_HUB = _bridgehub;
         L1_ASSET_ROUTER = _l1AssetRouter;
     }
 
     /// @notice used to initialize the contract
     /// @param _owner the owner of the contract
-    function initialize(address _owner) external reentrancyGuardInitializer {
+    function initialize(address _owner) external {
         _transferOwnership(_owner);
     }
 

l1-contracts/contracts/bridgehub/L1BridgehubErrors.sol

@@ -53,9 +53,6 @@ error ChainExists();
 // 0x913183d8
 error MessageRootNotRegistered();
 
-// 0x8b7d939c
-error TooManyChains(uint256 cachedChainCount, uint256 maxNumberOfChains);
-
 // 0x7f4316f3
 error NoEthAllowed();
 

l1-contracts/contracts/bridgehub/MessageRoot.sol

@@ -6,14 +6,11 @@ import {DynamicIncrementalMerkle} from "../common/libraries/DynamicIncrementalMe
 
 import {IBridgehub} from "./IBridgehub.sol";
 import {IMessageRoot} from "./IMessageRoot.sol";
-import {ReentrancyGuard} from "../common/ReentrancyGuard.sol";
-import {OnlyBridgehub, OnlyChain, ChainExists, MessageRootNotRegistered, TooManyChains} from "./L1BridgehubErrors.sol";
+import {OnlyBridgehub, OnlyChain, ChainExists, MessageRootNotRegistered} from "./L1BridgehubErrors.sol";
 import {FullMerkle} from "../common/libraries/FullMerkle.sol";
 
 import {MessageHashing} from "../common/libraries/MessageHashing.sol";
 
-import {MAX_NUMBER_OF_ZK_CHAINS} from "../common/Config.sol";
-
 // Chain tree consists of batch commitments as their leaves. We use hash of "new bytes(96)" as the hash of an empty leaf.
 bytes32 constant CHAIN_TREE_EMPTY_ENTRY_HASH = bytes32(
     0x46700b4d40ac5c35af2c22dda2787a91eb567b06c924a8fb8ae9a05b20c08c21
@@ -27,7 +24,7 @@ bytes32 constant SHARED_ROOT_TREE_EMPTY_HASH = bytes32(
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 /// @dev The MessageRoot contract is responsible for storing the cross message roots of the chains and the aggregated root of all chains.
-contract MessageRoot is IMessageRoot, ReentrancyGuard {
+contract MessageRoot is IMessageRoot {
     using FullMerkle for FullMerkle.FullTree;
     using DynamicIncrementalMerkle for DynamicIncrementalMerkle.Bytes32PushTree;
 
@@ -75,13 +72,13 @@ contract MessageRoot is IMessageRoot, ReentrancyGuard {
     /// @dev Contract is expected to be used as proxy implementation on L1, but as a system contract on L2.
     /// This means we call the _initialize in both the constructor and the initialize functions.
     /// @dev Initialize the implementation to prevent Parity hack.
-    constructor(IBridgehub _bridgehub) reentrancyGuardInitializer {
+    constructor(IBridgehub _bridgehub) {
         BRIDGE_HUB = _bridgehub;
         _initialize();
     }
 
     /// @dev Initializes a contract for later use. Expected to be used in the proxy on L1, on L2 it is a system contract without a proxy.
-    function initialize() external reentrancyGuardInitializer {
+    function initialize() external {
         _initialize();
     }
 
@@ -151,10 +148,9 @@ contract MessageRoot is IMessageRoot, ReentrancyGuard {
     /// @param _chainId the chainId of the chain
     function _addNewChain(uint256 _chainId) internal {
         uint256 cachedChainCount = chainCount;
-        if (cachedChainCount >= MAX_NUMBER_OF_ZK_CHAINS) {
-            revert TooManyChains(cachedChainCount, MAX_NUMBER_OF_ZK_CHAINS);
-        }
 
+        // Since only the bridgehub can add new chains to the message root, it is expected that
+        // it will be responsible for ensuring that the number of chains does not exceed the limit.
         ++chainCount;
         chainIndex[_chainId] = cachedChainCount;
         chainIndexToId[cachedChainCount] = _chainId;

l1-contracts/contracts/common/L1ContractErrors.sol

@@ -330,8 +330,6 @@ error NewDeadlineExceedsMaxDeadline();
 error AlreadyPermanentRollup();
 // 0x92daded2
 error InvalidDAForPermanentRollup();
-// 0x6e3331f5
-error IncorrectPricingMode();
 // 0xd0266e26
 error NotSettlementLayer();
 // 0x7a4902ad
@@ -378,6 +376,10 @@ error NoLegacySharedBridge();
 error TooHighDeploymentNonce();
 // 0x78d2ed02
 error ChainAlreadyLive();
+// 0x4e98b356
+error MigrationsNotPaused();
+// 0xf20c5c2a
+error WrappedBaseTokenAlreadyRegistered();
 
 // 0xde4c0b96
 error InvalidNTVBurnData();

l1-contracts/contracts/common/L2ContractAddresses.sol

@@ -61,7 +61,7 @@ interface IL2Messenger {
     /// @notice Sends an arbitrary length message to L1.
     /// @param _message The variable length message to be sent to L1.
     /// @return Returns the keccak256 hashed value of the message.
-    function sendToL1(bytes memory _message) external returns (bytes32);
+    function sendToL1(bytes calldata _message) external returns (bytes32);
 }
 
 /// @dev An l2 system contract address, used in the assetId calculation for native assets.

l1-contracts/contracts/common/interfaces/IL1Messenger.sol

@@ -7,5 +7,5 @@ pragma solidity 0.8.24;
  * @notice The interface of the L1 Messenger contract, responsible for sending messages to L1.
  */
 interface IL1Messenger {
-    function sendToL1(bytes memory _message) external returns (bytes32);
+    function sendToL1(bytes calldata _message) external returns (bytes32);
 }

l1-contracts/contracts/governance/AccessControlRestriction.sol

@@ -65,6 +65,8 @@ contract AccessControlRestriction is Restriction, IAccessControlRestriction, Acc
         // `requiredRoles` and `requiredRolesForFallback` is 0, the default admin is by default a required
         // role for all the functions.
         if (_call.data.length < 4) {
+            // Note, that the following restriction protects only for targets that were compiled after
+            // Solidity v0.4.18, since before a substring of selector could still call the function.
             if (!hasRole(requiredRolesForFallback[_call.target], _invoker)) {
                 revert AccessToFallbackDenied(_call.target, _invoker);
             }

l1-contracts/contracts/governance/L2AdminFactory.sol

@@ -17,7 +17,7 @@ import {RestrictionValidator} from "./restriction/RestrictionValidator.sol";
 contract L2AdminFactory {
     /// @notice Emitted when an admin is deployed on the L2.
     /// @param admin The address of the newly deployed admin.
-    event AdminDeployed(address admin);
+    event AdminDeployed(address indexed admin);
 
     /// @dev We use storage instead of immutable variables due to the
     /// specifics of the zkEVM environment, where storage is actually cheaper.
@@ -53,6 +53,8 @@ contract L2AdminFactory {
         // this factory with `seed1` produces the same address as some other random factory with `seed2`,
         // allowing to deploy a malicious contract.
         admin = address(new ChainAdmin(restrictions));
+
+        emit AdminDeployed(address(admin));
     }
 
     /// @notice Checks that the provided list of restrictions is correct.

l1-contracts/contracts/state-transition/ChainTypeManager.sol

@@ -14,17 +14,16 @@ import {IChainTypeManager, ChainTypeManagerInitializeData, ChainCreationParams}
 import {IZKChain} from "./chain-interfaces/IZKChain.sol";
 import {FeeParams} from "./chain-deps/ZKChainStorage.sol";
 import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable-v4/access/Ownable2StepUpgradeable.sol";
-import {ReentrancyGuard} from "../common/ReentrancyGuard.sol";
 import {L2_TO_L1_LOG_SERIALIZE_SIZE, DEFAULT_L2_LOGS_TREE_ROOT_HASH, EMPTY_STRING_KECCAK} from "../common/Config.sol";
 import {InitialForceDeploymentMismatch, AdminZero, OutdatedProtocolVersion} from "./L1StateTransitionErrors.sol";
-import {ChainAlreadyLive, Unauthorized, ZeroAddress, HashMismatch, GenesisUpgradeZero, GenesisBatchHashZero, GenesisIndexStorageZero, GenesisBatchCommitmentZero} from "../common/L1ContractErrors.sol";
+import {ChainAlreadyLive, Unauthorized, ZeroAddress, HashMismatch, GenesisUpgradeZero, GenesisBatchHashZero, GenesisIndexStorageZero, GenesisBatchCommitmentZero, MigrationsNotPaused} from "../common/L1ContractErrors.sol";
 import {SemVer} from "../common/libraries/SemVer.sol";
 import {IBridgehub} from "../bridgehub/IBridgehub.sol";
 
 /// @title State Transition Manager contract
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
-contract ChainTypeManager is IChainTypeManager, ReentrancyGuard, Ownable2StepUpgradeable {
+contract ChainTypeManager is IChainTypeManager, Ownable2StepUpgradeable {
     using EnumerableMap for EnumerableMap.UintToAddressMap;
 
     /// @notice Address of the bridgehub
@@ -65,12 +64,14 @@ contract ChainTypeManager is IChainTypeManager, ReentrancyGuard, Ownable2StepUpg
 
     /// @dev Contract is expected to be used as proxy implementation.
     /// @dev Initialize the implementation to prevent Parity hack.
-    constructor(address _bridgehub) reentrancyGuardInitializer {
+    constructor(address _bridgehub) {
         BRIDGE_HUB = _bridgehub;
 
         // While this does not provide a protection in the production, it is needed for local testing
         // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages
         assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32);
+
+        _disableInitializers();
     }
 
     /// @notice only the bridgehub can call
@@ -115,7 +116,7 @@ contract ChainTypeManager is IChainTypeManager, ReentrancyGuard, Ownable2StepUpg
     }
 
     /// @dev initialize
-    function initialize(ChainTypeManagerInitializeData calldata _initializeData) external reentrancyGuardInitializer {
+    function initialize(ChainTypeManagerInitializeData calldata _initializeData) external {
         if (_initializeData.owner == address(0)) {
             revert ZeroAddress();
         }
@@ -227,6 +228,10 @@ contract ChainTypeManager is IChainTypeManager, ReentrancyGuard, Ownable2StepUpg
         uint256 _oldProtocolVersionDeadline,
         uint256 _newProtocolVersion
     ) external onlyOwner {
+        if (!IBridgehub(BRIDGE_HUB).migrationPaused()) {
+            revert MigrationsNotPaused();
+        }
+
         bytes32 newCutHash = keccak256(abi.encode(_cutData));
         uint256 previousProtocolVersion = protocolVersion;
         upgradeCutHash[_oldProtocolVersion] = newCutHash;

l1-contracts/contracts/state-transition/L1StateTransitionErrors.sol

@@ -83,8 +83,8 @@ error PubdataTooSmall(uint256 pubdataInputLength, uint256 blobCommitmentSize);
 // 0xcba35a08
 error PubdataTooLong(uint256 pubdataLength, uint256 blobSizeBytes);
 
-// 0x1b0f562e
-error InvalidPubdataHash();
+// 0x5513177c
+error InvalidPubdataHash(bytes32 fullPubdataHash, bytes32 pubdata);
 
 // 0x5717f940
 error InvalidPubdataSource(uint8 pubdataSource);

l1-contracts/contracts/state-transition/chain-deps/DiamondInit.sol

@@ -8,7 +8,7 @@ import {L2_TO_L1_LOG_SERIALIZE_SIZE, MAX_GAS_PER_TRANSACTION} from "../../common
 import {InitializeData, IDiamondInit} from "../chain-interfaces/IDiamondInit.sol";
 import {PriorityQueue} from "../libraries/PriorityQueue.sol";
 import {PriorityTree} from "../libraries/PriorityTree.sol";
-import {ZeroAddress, TooMuchGas} from "../../common/L1ContractErrors.sol";
+import {ZeroAddress, EmptyAssetId, TooMuchGas} from "../../common/L1ContractErrors.sol";
 
 /// @author Matter Labs
 /// @dev The contract is used only once to initialize the diamond proxy.
@@ -43,7 +43,7 @@ contract DiamondInit is ZKChainBase, IDiamondInit {
             revert ZeroAddress();
         }
         if (_initializeData.baseTokenAssetId == bytes32(0)) {
-            revert ZeroAddress();
+            revert EmptyAssetId();
         }
         if (_initializeData.blobVersionedHashRetriever == address(0)) {
             revert ZeroAddress();