feat: OZ ZKChain: Governance Audit Fixes Combines (#1044)

Co-authored-by: Stanislav Breadless <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>
Co-authored-by: vladbochok <[email protected]>
Co-authored-by: Raid Ateir <[email protected]>

l1-contracts/contracts/common/L1ContractErrors.sol

@@ -13,12 +13,6 @@ error RestrictionWasNotPresent(address restriction);
 error RestrictionWasAlreadyPresent(address restriction);
 // 0x3331e9c0
 error CallNotAllowed(bytes call);
-// 0x59e1b0d2
-error ChainZeroAddress();
-// 0xff4bbdf1
-error NotAHyperchain(address chainAddress);
-// 0xa3decdf3
-error NotAnAdmin(address expected, address actual);
 // 0xf6fd7071
 error RemovingPermanentRestriction();
 // 0xfcb9b2e1
@@ -242,8 +236,6 @@ error NonSequentialBatch();
 error NonSequentialVersion();
 // 0x4ef79e5a
 error NonZeroAddress(address);
-// 0xdd629f86
-error NotEnoughGas();
 // 0xdd7e3621
 error NotInitializedReentrancyGuard();
 // 0xdf17e316
@@ -407,12 +399,10 @@ error IncorrectBatchBounds(
 );
 // 0x64107968
 error AssetHandlerNotRegistered(bytes32 assetId);
-// 0x10f30e75
-error NotBridgehub(address addr);
-// 0x2554babc
-error InvalidAddress(address expected, address actual);
 // 0xfa5cd00f
 error NotAllowed(address addr);
+// 0x64846fe4
+error NotARestriction(address addr);
 
 enum SharedBridgeKey {
     PostUpgradeFirstBatch,

l1-contracts/contracts/dev-contracts/DummyRestriction.sol

@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity 0.8.24;
+
+import {Call} from "../governance/Common.sol";
+import {IRestriction, RESTRICTION_MAGIC} from "../governance/restriction/IRestriction.sol";
+
+/// @title Restriction contract interface
+/// @author Matter Labs
+/// @custom:security-contact [email protected]
+contract DummyRestriction is IRestriction {
+    bool immutable correctMagic;
+
+    constructor(bool useCorrectMagic) {
+        correctMagic = useCorrectMagic;
+    }
+
+    /// @notice A method used to check that the contract supports this interface.
+    /// @return Returns the `RESTRICTION_MAGIC`
+    function getSupportsRestrictionMagic() external view returns (bytes32) {
+        if (correctMagic) {
+            return RESTRICTION_MAGIC;
+        } else {
+            // Invalid magic
+            return bytes32(0);
+        }
+    }
+
+    /// @notice Ensures that the invoker has the required role to call the function.
+    /// @param _call The call data.
+    /// @param _invoker The address of the invoker.
+    function validateCall(Call calldata _call, address _invoker) external view virtual {
+        // nothing
+    }
+}

l1-contracts/contracts/governance/AccessControlRestriction.sol

@@ -2,23 +2,23 @@
 
 pragma solidity 0.8.24;
 
-import {AccessToFallbackDenied, AccessToFunctionDenied} from "../common/L1ContractErrors.sol";
+import {AccessToFallbackDenied, AccessToFunctionDenied, ZeroAddress} from "../common/L1ContractErrors.sol";
 import {IAccessControlRestriction} from "./IAccessControlRestriction.sol";
 import {AccessControlDefaultAdminRules} from "@openzeppelin/contracts-v4/access/AccessControlDefaultAdminRules.sol";
-import {IRestriction} from "./IRestriction.sol";
+import {Restriction} from "./restriction/Restriction.sol";
 import {Call} from "./Common.sol";
 
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 /// @notice The Restriction that is designed to provide the access control logic for the `ChainAdmin` contract.
-/// @dev It inherits from `AccessControlDefaultAdminRules` without overriding `_setRoleAdmin` functionaity. In other
+/// @dev It inherits from `AccessControlDefaultAdminRules` without overriding `_setRoleAdmin` functionality. In other
 /// words, the `DEFAULT_ADMIN_ROLE` is the only role that can manage roles. This is done for simplicity.
 /// @dev An instance of this restriction should be deployed separately for each `ChainAdmin` contract.
 /// @dev IMPORTANT: this function does not validate the ability of the invoker to use `msg.value`. Thus,
 /// either all callers with access to functions should be trusted to not steal ETH from the `ChainAdmin` account
-/// or not ETH should be passively stored in `ChainAdmin` account.
-contract AccessControlRestriction is IRestriction, IAccessControlRestriction, AccessControlDefaultAdminRules {
-    /// @notice Required roles to call a specific functions.
+/// or no ETH should be passively stored in `ChainAdmin` account.
+contract AccessControlRestriction is Restriction, IAccessControlRestriction, AccessControlDefaultAdminRules {
+    /// @notice Required roles to call a specific function.
     /// @dev Note, that the role 0 means the `DEFAULT_ADMIN_ROLE` from the `AccessControlDefaultAdminRules` contract.
     mapping(address target => mapping(bytes4 selector => bytes32 requiredRole)) public requiredRoles;
 
@@ -39,6 +39,9 @@ contract AccessControlRestriction is IRestriction, IAccessControlRestriction, Ac
         bytes4 _selector,
         bytes32 _requiredRole
     ) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        if (_target == address(0)) {
+            revert ZeroAddress();
+        }
         requiredRoles[_target][_selector] = _requiredRole;
 
         emit RoleSet(_target, _selector, _requiredRole);
@@ -48,13 +51,16 @@ contract AccessControlRestriction is IRestriction, IAccessControlRestriction, Ac
     /// @param _target The address of the contract.
     /// @param _requiredRole The required role.
     function setRequiredRoleForFallback(address _target, bytes32 _requiredRole) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        if (_target == address(0)) {
+            revert ZeroAddress();
+        }
         requiredRolesForFallback[_target] = _requiredRole;
 
         emit FallbackRoleSet(_target, _requiredRole);
     }
 
-    /// @inheritdoc IRestriction
-    function validateCall(Call calldata _call, address _invoker) external view {
+    /// @inheritdoc Restriction
+    function validateCall(Call calldata _call, address _invoker) external view override {
         // Note, that since `DEFAULT_ADMIN_ROLE` is 0 and the default storage value for the
         // `requiredRoles` and `requiredRolesForFallback` is 0, the default admin is by default a required
         // role for all the functions.

l1-contracts/contracts/governance/ChainAdmin.sol

@@ -4,9 +4,10 @@ pragma solidity 0.8.24;
 
 // solhint-disable gas-length-in-loops
 
-import {NoCallsProvided, OnlySelfAllowed, RestrictionWasNotPresent, RestrictionWasAlreadyPresent} from "../common/L1ContractErrors.sol";
+import {ZeroAddress, NoCallsProvided, OnlySelfAllowed, RestrictionWasNotPresent, RestrictionWasAlreadyPresent} from "../common/L1ContractErrors.sol";
 import {IChainAdmin} from "./IChainAdmin.sol";
-import {IRestriction} from "./IRestriction.sol";
+import {Restriction} from "./restriction/Restriction.sol";
+import {RestrictionValidator} from "./restriction/RestrictionValidator.sol";
 import {Call} from "./Common.sol";
 
 import {EnumerableSet} from "@openzeppelin/contracts-v4/utils/structs/EnumerableSet.sol";
@@ -15,11 +16,19 @@ import {ReentrancyGuard} from "../common/ReentrancyGuard.sol";
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 /// @notice The contract is designed to hold the `admin` role in ZKSync Chain (State Transition) contracts.
-/// The owner of the contract can perform any external calls and also save the information needed for
-/// the blockchain node to accept the protocol upgrade.
+/// @dev Note, that it does not implement any form of access control by default, but instead utilizes
+/// so called "restrictions": contracts that implement the `IRestriction` interface and ensure that
+/// particular restrictions are ensured for the contract, including access control, security invariants, etc.
 contract ChainAdmin is IChainAdmin, ReentrancyGuard {
     using EnumerableSet for EnumerableSet.AddressSet;
 
+    /// @notice Mapping of protocol versions to their expected upgrade timestamps.
+    /// @dev Needed for the offchain node administration to know when to start building batches with the new protocol version.
+    mapping(uint256 protocolVersion => uint256 upgradeTimestamp) public protocolVersionToUpgradeTimestamp;
+
+    /// @notice The set of active restrictions.
+    EnumerableSet.AddressSet internal activeRestrictions;
+
     /// @notice Ensures that only the `ChainAdmin` contract itself can call the function.
     /// @dev All functions that require access-control should use `onlySelf` modifier, while the access control logic
     /// should be implemented in the restriction contracts.
@@ -38,13 +47,6 @@ contract ChainAdmin is IChainAdmin, ReentrancyGuard {
         }
     }
 
-    /// @notice Mapping of protocol versions to their expected upgrade timestamps.
-    /// @dev Needed for the offchain node administration to know when to start building batches with the new protocol version.
-    mapping(uint256 protocolVersion => uint256 upgradeTimestamp) public protocolVersionToUpgradeTimestamp;
-
-    /// @notice The set of active restrictions.
-    EnumerableSet.AddressSet internal activeRestrictions;
-
     /// @notice Returns the list of active restrictions.
     function getRestrictions() public view returns (address[] memory) {
         return activeRestrictions.values();
@@ -105,21 +107,26 @@ contract ChainAdmin is IChainAdmin, ReentrancyGuard {
     /// @dev Contract might receive/hold ETH as part of the maintenance process.
     receive() external payable {}
 
-    /// @notice Function that returns the current admin can perform the call.
-    /// @dev By default it always returns true, but can be overridden in derived contracts.
-    function _validateCall(Call calldata _call) internal view {
+    /// @notice Function that ensures that the current admin can perform the call.
+    /// @dev Reverts in case the call can not be performed. Successfully executes otherwise.
+    function _validateCall(Call calldata _call) private view {
         address[] memory restrictions = getRestrictions();
 
         unchecked {
             for (uint256 i = 0; i < restrictions.length; ++i) {
-                IRestriction(restrictions[i]).validateCall(_call, msg.sender);
+                Restriction(restrictions[i]).validateCall(_call, msg.sender);
             }
         }
     }
 
     /// @notice Adds a new restriction to the active restrictions set.
     /// @param _restriction The address of the restriction contract to be added.
-    function _addRestriction(address _restriction) internal {
+    function _addRestriction(address _restriction) private {
+        if (_restriction == address(0)) {
+            revert ZeroAddress();
+        }
+        RestrictionValidator.validateRestriction(_restriction);
+
         if (!activeRestrictions.add(_restriction)) {
             revert RestrictionWasAlreadyPresent(_restriction);
         }

l1-contracts/contracts/governance/L2AdminFactory.sol

@@ -3,6 +3,8 @@
 pragma solidity 0.8.24;
 
 import {ChainAdmin} from "./ChainAdmin.sol";
+import {RestrictionValidator} from "./restriction/RestrictionValidator.sol";
+import {ZeroAddress} from "../common/L1ContractErrors.sol";
 
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
@@ -14,29 +16,72 @@ import {ChainAdmin} from "./ChainAdmin.sol";
 /// @dev The contract is immutable, in case the restrictions need to be changed,
 /// a new contract should be deployed.
 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);
 
     /// @dev We use storage instead of immutable variables due to the
     /// specifics of the zkEVM environment, where storage is actually cheaper.
     address[] public requiredRestrictions;
 
     constructor(address[] memory _requiredRestrictions) {
+        _validateZeroAddress(_requiredRestrictions);
+        _validateRestrctions(_requiredRestrictions);
         requiredRestrictions = _requiredRestrictions;
     }
 
     /// @notice Deploys a new L2 admin contract.
     /// @return admin The address of the deployed admin contract.
-    function deployAdmin(address[] calldata _additionalRestrictions, bytes32 _salt) external returns (address admin) {
-        address[] memory restrictions = new address[](requiredRestrictions.length + _additionalRestrictions.length);
+    // solhint-disable-next-line gas-calldata-parameters
+    function deployAdmin(address[] memory _additionalRestrictions, bytes32 _salt) external returns (address admin) {
+        // Even though the chain admin will likely perform similar checks,
+        // we keep those here just in case, since it is not expensive, while allowing to fail fast.
+        _validateZeroAddress(_additionalRestrictions);
+        _validateRestrctions(_additionalRestrictions);
+
         uint256 cachedRequired = requiredRestrictions.length;
-        for (uint256 i = 0; i < cachedRequired; ++i) {
-            restrictions[i] = requiredRestrictions[i];
-        }
         uint256 cachedAdditional = _additionalRestrictions.length;
-        for (uint256 i = 0; i < cachedAdditional; ++i) {
-            restrictions[requiredRestrictions.length + i] = _additionalRestrictions[i];
+
+        address[] memory restrictions = new address[](cachedRequired + cachedAdditional);
+
+        unchecked {
+            for (uint256 i = 0; i < cachedRequired; ++i) {
+                restrictions[i] = requiredRestrictions[i];
+            }
+            for (uint256 i = 0; i < cachedAdditional; ++i) {
+                restrictions[cachedRequired + i] = _additionalRestrictions[i];
+            }
         }
 
         admin = address(new ChainAdmin{salt: _salt}(restrictions));
+
+        emit AdminDeployed(admin);
+    }
+
+    /// @notice Checks that the provided list of restrictions does not contain
+    /// any zero addresses.
+    /// @param _restrictions List of the restrictions to check.
+    /// @dev In case either of the restrictions is zero address, the function reverts.
+    function _validateZeroAddress(address[] memory _restrictions) private pure {
+        unchecked {
+            uint256 length = _restrictions.length;
+            for (uint256 i = 0; i < length; ++i) {
+                if (_restrictions[i] == address(0)) {
+                    revert ZeroAddress();
+                }
+            }
+        }
+    }
+
+    /// @notice Checks that the provided list of restrictions is correct.
+    /// @param _restrictions List of the restrictions to check.
+    /// @dev In case either of the restrictions is not correct, the function reverts.
+    function _validateRestrctions(address[] memory _restrictions) private view {
+        unchecked {
+            uint256 length = _restrictions.length;
+            for (uint256 i = 0; i < length; ++i) {
+                RestrictionValidator.validateRestriction(_restrictions[i]);
+            }
+        }
     }
 }