OZ L-02 Inconsistent Input Validation (#570)

l1-contracts/contracts/bridgehub/Bridgehub.sol

@@ -13,7 +13,7 @@ import {IZkSyncHyperchain} from "../state-transition/chain-interfaces/IZkSyncHyp
 import {ETH_TOKEN_ADDRESS, TWO_BRIDGES_MAGIC_VALUE, BRIDGEHUB_MIN_SECOND_BRIDGE_ADDRESS} from "../common/Config.sol";
 import {BridgehubL2TransactionRequest, L2Message, L2Log, TxStatus} from "../common/Messaging.sol";
 import {AddressAliasHelper} from "../vendor/AddressAliasHelper.sol";
-import {Unauthorized, STMAlreadyRegistered, STMNotRegistered, TokenAlreadyRegistered, TokenNotRegistered, ZeroChainId, ChainIdTooBig, SharedBridgeNotSet, BridgeHubAlreadyRegistered, AddressTooLow, MsgValueMismatch, WrongMagicValue} from "../common/L1ContractErrors.sol";
+import {Unauthorized, STMAlreadyRegistered, STMNotRegistered, TokenAlreadyRegistered, TokenNotRegistered, ZeroChainId, ChainIdTooBig, SharedBridgeNotSet, BridgeHubAlreadyRegistered, AddressTooLow, MsgValueMismatch, WrongMagicValue, ZeroAddress} from "../common/L1ContractErrors.sol";
 
 contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, PausableUpgradeable {
     /// @notice all the ether is held by the weth bridge
@@ -55,6 +55,9 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus
     /// @dev Please note, if the owner wants to enforce the admin change it must execute both `setPendingAdmin` and
     /// `acceptAdmin` atomically. Otherwise `admin` can set different pending admin and so fail to accept the admin rights.
     function setPendingAdmin(address _newPendingAdmin) external onlyOwnerOrAdmin {
+        if (_newPendingAdmin == address(0)) {
+            revert ZeroAddress();
+        }
         // Save previous value into the stack to put it into the event later
         address oldPendingAdmin = pendingAdmin;
         // Change pending admin
@@ -89,6 +92,9 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus
 
     /// @notice State Transition can be any contract with the appropriate interface/functionality
     function addStateTransitionManager(address _stateTransitionManager) external onlyOwner {
+        if (_stateTransitionManager == address(0)) {
+            revert ZeroAddress();
+        }
         if (stateTransitionManagerIsRegistered[_stateTransitionManager]) {
             revert STMAlreadyRegistered();
         }
@@ -98,6 +104,9 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus
     /// @notice State Transition can be any contract with the appropriate interface/functionality
     /// @notice this stops new Chains from using the STF, old chains are not affected
     function removeStateTransitionManager(address _stateTransitionManager) external onlyOwner {
+        if (_stateTransitionManager == address(0)) {
+            revert ZeroAddress();
+        }
         if (!stateTransitionManagerIsRegistered[_stateTransitionManager]) {
             revert STMNotRegistered();
         }
@@ -115,6 +124,9 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus
     /// @notice To set shared bridge, only Owner. Not done in initialize, as
     /// the order of deployment is Bridgehub, Shared bridge, and then we call this
     function setSharedBridge(address _sharedBridge) external onlyOwner {
+        if (_sharedBridge == address(0)) {
+            revert ZeroAddress();
+        }
         sharedBridge = IL1SharedBridge(_sharedBridge);
     }
 
@@ -135,6 +147,12 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus
         if (_chainId > type(uint48).max) {
             revert ChainIdTooBig();
         }
+        if (_stateTransitionManager == address(0)) {
+            revert ZeroAddress();
+        }
+        if (_baseToken == address(0)) {
+            revert ZeroAddress();
+        }
 
         if (!stateTransitionManagerIsRegistered[_stateTransitionManager]) {
             revert STMNotRegistered();

l1-contracts/contracts/governance/Governance.sol

@@ -39,6 +39,7 @@ contract Governance is IGovernance, Ownable2Step {
     /// @param _admin The address to be assigned as the admin of the contract.
     /// @param _securityCouncil The address to be assigned as the security council of the contract.
     /// @param _minDelay The initial minimum delay (in seconds) to be set for operations.
+    /// @dev We allow for a zero address for _securityCouncil because it can be set later
     constructor(address _admin, address _securityCouncil, uint256 _minDelay) {
         if (_admin == address(0)) {
             revert ZeroAddress();

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

@@ -6,7 +6,7 @@ import {Ownable2Step} from "@openzeppelin/contracts-v4/access/Ownable2Step.sol";
 import {LibMap} from "./libraries/LibMap.sol";
 import {IExecutor} from "./chain-interfaces/IExecutor.sol";
 import {IStateTransitionManager} from "./IStateTransitionManager.sol";
-import {Unauthorized, TimeNotReached} from "../common/L1ContractErrors.sol";
+import {Unauthorized, TimeNotReached, ZeroAddress} from "../common/L1ContractErrors.sol";
 
 /// @author Matter Labs
 /// @custom:security-contact security@matterlabs.dev
@@ -79,6 +79,9 @@ contract ValidatorTimelock is IExecutor, Ownable2Step {
 
     /// @dev Sets a new state transition manager.
     function setStateTransitionManager(IStateTransitionManager _stateTransitionManager) external onlyOwner {
+        if (address(_stateTransitionManager) == address(0)) {
+            revert ZeroAddress();
+        }
         stateTransitionManager = _stateTransitionManager;
     }
 

l1-contracts/contracts/state-transition/chain-deps/facets/Admin.sol

@@ -8,7 +8,7 @@ import {MAX_GAS_PER_TRANSACTION} from "../../../common/Config.sol";
 import {FeeParams, PubdataPricingMode} from "../ZkSyncHyperchainStorage.sol";
 import {ZkSyncHyperchainBase} from "./ZkSyncHyperchainBase.sol";
 import {IStateTransitionManager} from "../../IStateTransitionManager.sol";
-import {Unauthorized, TooMuchGas, PubdataPerBatchIsLessThanTxn, InvalidPubdataPricingMode, ProtocolIdMismatch, ChainAlreadyLive, HashMismatch, ProtocolIdNotGreater, DenominatorIsZero, DiamondAlreadyFrozen, DiamondNotFrozen} from "../../../common/L1ContractErrors.sol";
+import {Unauthorized, TooMuchGas, PubdataPerBatchIsLessThanTxn, InvalidPubdataPricingMode, ProtocolIdMismatch, ChainAlreadyLive, HashMismatch, ProtocolIdNotGreater, DenominatorIsZero, DiamondAlreadyFrozen, DiamondNotFrozen, ZeroAddress} from "../../../common/L1ContractErrors.sol";
 
 // While formally the following import is not used, it is needed to inherit documentation from it
 import {IZkSyncHyperchainBase} from "../../chain-interfaces/IZkSyncHyperchainBase.sol";
@@ -22,6 +22,9 @@ contract AdminFacet is ZkSyncHyperchainBase, IAdmin {
 
     /// @inheritdoc IAdmin
     function setPendingAdmin(address _newPendingAdmin) external onlyAdmin {
+        if (_newPendingAdmin == address(0)) {
+            revert ZeroAddress();
+        }
         // Save previous value into the stack to put it into the event later
         address oldPendingAdmin = s.pendingAdmin;
         // Change pending admin

l1-contracts/contracts/upgrades/UpgradeHyperchains.sol

@@ -34,6 +34,12 @@ contract UpgradeHyperchains is BaseZkSyncUpgrade {
         if (sharedBridgeAddress == address(0)) {
             revert ZeroAddress();
         }
+        if (chainAdmin == address(0)) {
+            revert ZeroAddress();
+        }
+        if (validatorTimelock == address(0)) {
+            revert ZeroAddress();
+        }
 
         s.chainId = chainId;
         s.bridgehub = bridgehubAddress;