custom erros + one nit

l1-contracts/contracts/common/L1ContractErrors.sol

@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.21;
+
+error AccessToFallbackDenied(address target, address invoker);
+error AccessToFunctionDenied(address target, bytes4 selector, address invoker);
+error OnlySelfAllowed();
+error RestrictionWasNotPresent(address restriction);
+error RestrictionWasAlreadyPresent(address restriction);
+error NoCallsProvided();
+error CallNotAllowed(bytes call);
+error ChainZeroAddress();
+error NotAHyperchain(address chainAddress);
+error NotAnAdmin(address expected, address actual);
+error RemovingPermanentRestriction();
+error UnallowedImplementatioN(bytes32 imlementationHash);
+error ZeroAddress();

l1-contracts/contracts/governance/AccessControlRestriction.sol

@@ -2,6 +2,7 @@
 
 pragma solidity 0.8.24;
 
+import {AccessToFallbackDenied, AccessToFunctionDenied} from "../common/L1ContractErrors.sol";
 import {IAccessControlRestriction} from "./IAccessControlRestriction.sol";
 import {AccessControlDefaultAdminRules} from "@openzeppelin/contracts/access/AccessControlDefaultAdminRules.sol";
 import {IRestriction} from "./IRestriction.sol";
@@ -16,6 +17,9 @@ import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 /// @dev It inherits from `AccessControlDefaultAdminRules` without overriding `_setRoleAdmin` functionaity. 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.
     /// @dev Note, that the role 0 means the `DEFAULT_ADMIN_ROLE` from the `AccessControlDefaultAdminRules` contract.
@@ -54,25 +58,18 @@ contract AccessControlRestriction is IRestriction, IAccessControlRestriction, Ac
 
     /// @inheritdoc IRestriction
     function validateCall(Call calldata _call, address _invoker) external view {
-        // It is very rare that an admin needs to send value somewhere, so we require the invoker to have the DEFAULT_ADMIN_ROLE
-        if (_call.value != 0) {
-            require(hasRole(DEFAULT_ADMIN_ROLE, _invoker), "AccessControlRestriction: Access denied");
-        }
-
         // 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.
         if (_call.data.length < 4) {
-            require(
-                hasRole(requiredRolesForFallback[_call.target], _invoker),
-                "AccessControlRestriction: Fallback function is not allowed"
-            );
+            if(!hasRole(requiredRolesForFallback[_call.target], _invoker)) {
+                revert(AccessToFallbackDenied(_call.target, _invoker));
+            }
         } else {
             bytes4 selector = bytes4(_call.data[:4]);
-            require(
-                hasRole(requiredRoles[_call.target][selector], _invoker),
-                "AccessControlRestriction: Access denied"
-            );
+            if(!hasRole(requiredRoles[_call.target][selector], _invoker)) {
+                revert(AccessToFunctionDenied(_call.target, selector, _invoker));
+            }
         }
     }
 }

l1-contracts/contracts/governance/ChainAdmin.sol

@@ -2,6 +2,7 @@
 
 pragma solidity 0.8.24;
 
+import {NoCallsProvided, OnlySelfAllowed, RestrictionWasNotPresent, RestrictionWasAlreadyPresent} from "../common/L1ContractErrors.sol";
 import {IChainAdmin} from "./IChainAdmin.sol";
 import {IRestriction} from "./IRestriction.sol";
 import {Call} from "./Common.sol";
@@ -21,7 +22,9 @@ contract ChainAdmin is IChainAdmin, ReentrancyGuard {
     /// @dev All functions that require access-control should use `onlySelf` modifier, while the access control logic
     /// should be implemented in the restriction contracts.
     modifier onlySelf() {
-        require(msg.sender == address(this), "Only self");
+        if(msg.sender != address(this)) {
+            revert OnlySelfAllowed();
+        }
         _;
     }
 
@@ -57,7 +60,9 @@ contract ChainAdmin is IChainAdmin, ReentrancyGuard {
 
     /// @inheritdoc IChainAdmin
     function removeRestriction(address _restriction) external onlySelf {
-        require(activeRestrictions.remove(_restriction), "restriction was not present");
+        if(!activeRestrictions.remove(_restriction)) {
+            revert RestrictionWasNotPresent();
+        }
         emit RestrictionRemoved(_restriction);
     }
 
@@ -77,8 +82,9 @@ contract ChainAdmin is IChainAdmin, ReentrancyGuard {
     /// @dev Even though all the validation from external modules is executed via `staticcall`, the function
     /// is marked as `nonReentrant` to prevent reentrancy attacks in case the staticcall restriction is lifted in the future.
     function multicall(Call[] calldata _calls, bool _requireSuccess) external payable nonReentrant {
-        // solhint-disable-next-line gas-custom-errors
-        require(_calls.length > 0, "No calls provided");
+        if (_calls.length == 0) {
+            revert NoCallsProvided();
+        }
         // solhint-disable-next-line gas-length-in-loops
         for (uint256 i = 0; i < _calls.length; ++i) {
             _validateCall(_calls[i]);
@@ -113,7 +119,9 @@ contract ChainAdmin is IChainAdmin, ReentrancyGuard {
     /// @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 {
-        require(activeRestrictions.add(_restriction), "restriction already present");
+        if(!activeRestrictions.add(_restriction)) {
+            revert RestrictionWasAlreadyPresent(_restriction);
+        }
         emit RestrictionAdded(_restriction);
     }
 }

l1-contracts/contracts/governance/PermanentRestriction.sol

@@ -2,6 +2,8 @@
 
 pragma solidity 0.8.24;
 
+import {CallNotAllowed, ChainZeroAddress, NotAHyperchain, NotAnAdmin, RemovingPermanentRestriction, ZeroAddress} from "../common/L1ContractErrors.sol";
+
 import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
 
 import {Call} from "./Common.sol";
@@ -37,7 +39,9 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St
         BRIDGE_HUB = _bridgehub;
 
         // solhint-disable-next-line gas-custom-errors, reason-string
-        require(_initialOwner != address(0), "Initial owner should be non zero address");
+        if (_initialOwner == address(0)) {
+            revert ZeroAddress();
+        }
         _transferOwnership(_initialOwner);
     }
 
@@ -104,7 +108,9 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St
             return;
         }
 
-        require(allowedCalls[_call.data], "not allowed");
+        if(!allowedCalls[_call.data]) {
+            revert CallNotAllowed(_call.data);
+        }
     }
 
     /// @notice Validates the correctness of the new admin.
@@ -117,11 +123,16 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St
         assembly {
             implementationCodeHash := extcodehash(newChainAdmin)
         }
-        require(allowedAdminImplementations[implementationCodeHash], "Unallowed implementation");
+
+        if(!allowedAdminImplementations[implementationCodeHash]) {
+            revert UnallowedImplementatioN(implementationCodeHash);
+        }
 
         // Since the implementation is known to be correct (from the checks above), we
         // can safely trust the returned value from the call below
-        require(IChainAdmin(newChainAdmin).isRestrictionActive(address(this)), "This restriction is permanent");
+        if (!IChainAdmin(newChainAdmin).isRestrictionActive(address(this))) {
+            revert RemovingPermanentRestriction();
+        }
     }
 
     /// @notice Validates the removal of the restriction.
@@ -138,7 +149,9 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St
 
         address removedRestriction = abi.decode(_call.data[4:], (address));
 
-        require(removedRestriction != address(this), "This restriction is permanent");
+        if (removedRestriction == address(this)) {
+            revert RemovingPermanentRestriction();
+        }
     }
 
     /// @notice Checks if the `msg.sender` is an admin of a certain ZkSyncHyperchain.
@@ -154,18 +167,24 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St
     /// @dev This function reverts if the `_chain` is not a ZkSyncHyperchain or the `_potentialAdmin` is not the
     /// admin of the chain.
     function tryCompareAdminOfAChain(address _chain, address _potentialAdmin) external view {
-        require(_chain != address(0), "Address 0 is never a chain");
+        if (_chain == address(0)) {
+            revert ChainZeroAddress();
+        }
 
         // Unfortunately there is no easy way to double check that indeed the `_chain` is a ZkSyncHyperchain.
         // So we do the following:
         // - Query it for `chainId`. If it reverts, it is not a ZkSyncHyperchain.
         // - Query the Bridgehub for the Hyperchain with the given `chainId`.
         // - We compare the corresponding addresses
         uint256 chainId = IZkSyncHyperchain(_chain).getChainId();
-        require(BRIDGE_HUB.getHyperchain(chainId) == _chain, "Not a Hyperchain");
+        if (BRIDGE_HUB.getHyperchain(chainId) != _chain) {
+            revert NotAHyperchain(_chain);
+        }
 
         // Now, the chain is known to be a hyperchain, so it should implement the corresponding interface
         address admin = IZkSyncHyperchain(_chain).getAdmin();
-        require(admin == _potentialAdmin, "Not an admin");
+        if (admin != _potentialAdmin) {
+            revert NotAnAdmin(admin, _potentialAdmin);
+        }
     }
 }