From b1c59688f9f4411bfd4bdf60427d986c356facf4 Mon Sep 17 00:00:00 2001 From: koloz193 Date: Wed, 24 Jul 2024 09:09:43 -0400 Subject: [PATCH] OZ L-02 Inconsistent Input Validation (#570) --- .../contracts/bridgehub/Bridgehub.sol | 20 ++++++++++++++++++- .../contracts/governance/Governance.sol | 1 + .../state-transition/ValidatorTimelock.sol | 5 ++++- .../chain-deps/facets/Admin.sol | 5 ++++- .../contracts/upgrades/UpgradeHyperchains.sol | 6 ++++++ 5 files changed, 34 insertions(+), 3 deletions(-) diff --git a/l1-contracts/contracts/bridgehub/Bridgehub.sol b/l1-contracts/contracts/bridgehub/Bridgehub.sol index 82ded9072..c389c4cc7 100644 --- a/l1-contracts/contracts/bridgehub/Bridgehub.sol +++ b/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(); diff --git a/l1-contracts/contracts/governance/Governance.sol b/l1-contracts/contracts/governance/Governance.sol index 95c2f764b..de811ad74 100644 --- a/l1-contracts/contracts/governance/Governance.sol +++ b/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(); diff --git a/l1-contracts/contracts/state-transition/ValidatorTimelock.sol b/l1-contracts/contracts/state-transition/ValidatorTimelock.sol index 9d2081129..7b428cb7b 100644 --- a/l1-contracts/contracts/state-transition/ValidatorTimelock.sol +++ b/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; } diff --git a/l1-contracts/contracts/state-transition/chain-deps/facets/Admin.sol b/l1-contracts/contracts/state-transition/chain-deps/facets/Admin.sol index 418b1e8c1..ed08c095b 100644 --- a/l1-contracts/contracts/state-transition/chain-deps/facets/Admin.sol +++ b/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 diff --git a/l1-contracts/contracts/upgrades/UpgradeHyperchains.sol b/l1-contracts/contracts/upgrades/UpgradeHyperchains.sol index 40670459e..44495bab8 100644 --- a/l1-contracts/contracts/upgrades/UpgradeHyperchains.sol +++ b/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;