diff --git a/l1-contracts/contracts/dev-contracts/DummyRestriction.sol b/l1-contracts/contracts/dev-contracts/DummyRestriction.sol index b4f0025b6..8ce2680db 100644 --- a/l1-contracts/contracts/dev-contracts/DummyRestriction.sol +++ b/l1-contracts/contracts/dev-contracts/DummyRestriction.sol @@ -29,7 +29,7 @@ contract DummyRestriction is IRestriction { /// @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 virtual view { + function validateCall(Call calldata _call, address _invoker) external view virtual { // nothing } } diff --git a/l1-contracts/contracts/governance/AccessControlRestriction.sol b/l1-contracts/contracts/governance/AccessControlRestriction.sol index 99cd1e7e2..6052d1e6a 100644 --- a/l1-contracts/contracts/governance/AccessControlRestriction.sol +++ b/l1-contracts/contracts/governance/AccessControlRestriction.sol @@ -16,15 +16,9 @@ import {Call} from "./Common.sol"; /// @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 -<<<<<<< HEAD -/// or not ETH should be passively stored in `ChainAdmin` account. -contract AccessControlRestriction is Restriction, IAccessControlRestriction, AccessControlDefaultAdminRules { - /// @notice Required roles to call a specific functions. -======= /// or no ETH should be passively stored in `ChainAdmin` account. -contract AccessControlRestriction is IRestriction, IAccessControlRestriction, AccessControlDefaultAdminRules { +contract AccessControlRestriction is Restriction, IAccessControlRestriction, AccessControlDefaultAdminRules { /// @notice Required roles to call a specific function. ->>>>>>> origin/vb-governance-n05 /// @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; @@ -66,7 +60,7 @@ contract AccessControlRestriction is IRestriction, IAccessControlRestriction, Ac } /// @inheritdoc Restriction - function validateCall(Call calldata _call, address _invoker) external override view { + 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. diff --git a/l1-contracts/contracts/governance/ChainAdmin.sol b/l1-contracts/contracts/governance/ChainAdmin.sol index c0aec1183..7755a1330 100644 --- a/l1-contracts/contracts/governance/ChainAdmin.sol +++ b/l1-contracts/contracts/governance/ChainAdmin.sol @@ -16,8 +16,8 @@ import {ReentrancyGuard} from "../common/ReentrancyGuard.sol"; /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev /// @notice The contract is designed to hold the `admin` role in ZKSync Chain (State Transition) contracts. -/// @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 +/// @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; @@ -107,15 +107,9 @@ contract ChainAdmin is IChainAdmin, ReentrancyGuard { /// @dev Contract might receive/hold ETH as part of the maintenance process. receive() external payable {} -<<<<<<< HEAD /// @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) internal view { -======= - /// @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) private view { ->>>>>>> origin/vb-governance-n03 address[] memory restrictions = getRestrictions(); unchecked { @@ -127,19 +121,12 @@ 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. -<<<<<<< HEAD - function _addRestriction(address _restriction) internal { -<<<<<<< HEAD - RestrictionValidator.validateRestriction(_restriction); - -======= + function _addRestriction(address _restriction) private { if (_restriction == address(0)) { revert ZeroAddress(); } ->>>>>>> origin/sb-governance-l02 -======= - function _addRestriction(address _restriction) private { ->>>>>>> origin/vb-governance-n03 + RestrictionValidator.validateRestriction(_restriction); + if (!activeRestrictions.add(_restriction)) { revert RestrictionWasAlreadyPresent(_restriction); } diff --git a/l1-contracts/contracts/governance/L2AdminFactory.sol b/l1-contracts/contracts/governance/L2AdminFactory.sol index 879153d5e..6fca4fb69 100644 --- a/l1-contracts/contracts/governance/L2AdminFactory.sol +++ b/l1-contracts/contracts/governance/L2AdminFactory.sol @@ -3,11 +3,8 @@ pragma solidity 0.8.24; import {ChainAdmin} from "./ChainAdmin.sol"; -<<<<<<< HEAD import {RestrictionValidator} from "./restriction/RestrictionValidator.sol"; -======= import {ZeroAddress} from "../common/L1ContractErrors.sol"; ->>>>>>> origin/sb-governance-l02 /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev @@ -28,35 +25,24 @@ contract L2AdminFactory { address[] public requiredRestrictions; constructor(address[] memory _requiredRestrictions) { -<<<<<<< HEAD - _validateRestrctions(_requiredRestrictions); -======= _validateZeroAddress(_requiredRestrictions); ->>>>>>> origin/sb-governance-l02 + _validateRestrctions(_requiredRestrictions); requiredRestrictions = _requiredRestrictions; } /// @notice Deploys a new L2 admin contract. /// @return admin The address of the deployed admin contract. -<<<<<<< HEAD // 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, + // 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. -<<<<<<< HEAD - _validateRestrctions(_additionalRestrictions); -======= _validateZeroAddress(_additionalRestrictions); + _validateRestrctions(_additionalRestrictions); ->>>>>>> origin/sb-governance-l02 - address[] memory restrictions = new address[](requiredRestrictions.length + _additionalRestrictions.length); -======= - function deployAdmin(address[] calldata _additionalRestrictions, bytes32 _salt) external returns (address admin) { ->>>>>>> origin/sb-governance-n01 uint256 cachedRequired = requiredRestrictions.length; uint256 cachedAdditional = _additionalRestrictions.length; - - address[] memory restrictions = new address[](cachedRequired + cachedRequired); + + address[] memory restrictions = new address[](cachedRequired + cachedAdditional); unchecked { for (uint256 i = 0; i < cachedRequired; ++i) { @@ -64,7 +50,7 @@ contract L2AdminFactory { } for (uint256 i = 0; i < cachedAdditional; ++i) { restrictions[cachedRequired + i] = _additionalRestrictions[i]; - } + } } admin = address(new ChainAdmin{salt: _salt}(restrictions)); @@ -72,28 +58,29 @@ contract L2AdminFactory { emit AdminDeployed(admin); } -<<<<<<< HEAD - /// @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) internal view { - unchecked { - uint256 length = _restrictions.length; - for(uint256 i = 0; i < length; ++i) { - RestrictionValidator.validateRestriction(_restrictions[i]); -======= /// @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) internal view { + function _validateZeroAddress(address[] memory _restrictions) private pure { unchecked { uint256 length = _restrictions.length; - for(uint256 i = 0; i < length; ++i) { + for (uint256 i = 0; i < length; ++i) { if (_restrictions[i] == address(0)) { revert ZeroAddress(); } ->>>>>>> origin/sb-governance-l02 + } + } + } + + /// @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]); } } } diff --git a/l1-contracts/contracts/governance/PermanentRestriction.sol b/l1-contracts/contracts/governance/PermanentRestriction.sol index c287dc420..633a7a897 100644 --- a/l1-contracts/contracts/governance/PermanentRestriction.sol +++ b/l1-contracts/contracts/governance/PermanentRestriction.sol @@ -25,13 +25,8 @@ import {IPermanentRestriction} from "./IPermanentRestriction.sol"; /// @notice This contract should be used by chains that wish to guarantee that certain security /// properties are preserved forever. /// @dev To be deployed as a transparent upgradable proxy, owned by a trusted decentralized governance. -<<<<<<< HEAD -/// @dev Once of the instances of such contract is to ensure that a ZkSyncHyperchain is a rollup forever. -contract PermanentRestriction is Restriction, IPermanentRestriction, Ownable2StepUpgradeable { -======= /// @dev One of the instances of such contract is enough to ensure that a ZkSyncHyperchain is a rollup forever. -contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2StepUpgradeable { ->>>>>>> origin/vb-governance-n05 +contract PermanentRestriction is Restriction, IPermanentRestriction, Ownable2StepUpgradeable { /// @notice The address of the Bridgehub contract. IBridgehub public immutable BRIDGE_HUB; @@ -134,17 +129,10 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St /// @param _call The call data. /// @dev Note that we do not need to validate the migration to the L1 layer as the admin /// is not changed in this case. -<<<<<<< HEAD - function _validateMigrationToL2(Call calldata _call) internal view { - (address admin, bool isMigration) = _getNewAdminFromMigration(_call); - if(isMigration) { - if(!allowedL2Admins[admin]) { -======= function _validateMigrationToL2(Call calldata _call) private view { - _ensureEnoughGas(); - try this.tryGetNewAdminFromMigration(_call) returns (address admin) { + (address admin, bool isMigration) = _getNewAdminFromMigration(_call); + if (isMigration) { if (!allowedL2Admins[admin]) { ->>>>>>> origin/vb-governance-n03 revert NotAllowed(admin); } } @@ -220,23 +208,9 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St } /// @notice Checks if the `msg.sender` is an admin of a certain ZkSyncHyperchain. + /// @notice Function is internal for testing purposes only. /// @param _chain The address of the chain. -<<<<<<< HEAD function _isAdminOfAChain(address _chain) internal view returns (bool) { -======= - function _isAdminOfAChain(address _chain) private view returns (bool) { - _ensureEnoughGas(); - (bool success, ) = address(this).staticcall(abi.encodeCall(this.tryCompareAdminOfAChain, (_chain, msg.sender))); - return success; - } - - /// @notice Tries to compare the admin of a chain with the potential admin. - /// @param _chain The address of the chain. - /// @param _potentialAdmin The address of the potential admin. - /// @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 { ->>>>>>> origin/vb-governance-n03 if (_chain == address(0)) { return false; } @@ -252,7 +226,7 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St if (!chainIdQuerySuccess) { // It is not a hyperchain, so we can return `false` here. return false; - } + } // Note, that here it is important to use the legacy `getHyperchain` function, so that the contract // is compatible with the legacy ones. @@ -270,25 +244,18 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St /// @notice Tries to call `IGetters.getChainId()` function on the `_chain`. /// It ensures that the returndata is of correct format and if not, it returns false. /// @param _chain The address of the potential chain - /// @return chainId The chainId of the chain. + /// @return chainId The chainId of the chain. /// @return success Whether the call was successful. - /// If the second item is `false`, the caller should ignore the first value. + /// If the second item is `false`, the caller should ignore the first value. function _getChainIdUnffallibleCall(address _chain) internal view returns (uint256 chainId, bool success) { bytes4 selector = IGetters.getChainId.selector; - + // Note, that we do use assembly here to ensure that the function does not panic in case of // either incorrect `_chain` address or in case the returndata is too large assembly { // We use scratch space here, so it is safe mstore(0, selector) - success := staticcall( - gas(), - _chain, - 0, - 4, - 0, - 0 - ) + success := staticcall(gas(), _chain, 0, 4, 0, 0) let isReturndataSizeCorrect := eq(returndatasize(), 32) @@ -306,15 +273,15 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St /// @notice Tries to get the new admin from the migration. /// @param _call The call data. /// @return Returns a tuple of the new admin and whether the transaction is indeed the migration. - /// If the second item is `false`, the caller should ignore the first value. - /// @dev If any other error is returned, it is assumed to be out of gas or some other unexpected + /// If the second item is `false`, the caller should ignore the first value. + /// @dev If any other error is returned, it is assumed to be out of gas or some other unexpected /// error that should be bubbled up by the caller. function _getNewAdminFromMigration(Call calldata _call) internal view returns (address, bool) { if (_call.target != address(BRIDGE_HUB)) { return (address(0), false); } - if(_call.data.length < 4) { + if (_call.data.length < 4) { return (address(0), false); } @@ -325,7 +292,7 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St address sharedBridge = BRIDGE_HUB.sharedBridge(); // Assuming that correctly encoded calldata is provided, the following line must never fail, - // since the correct selector was checked before. + // since the correct selector was checked before. L2TransactionRequestTwoBridgesOuter memory request = abi.decode( _call.data[4:], (L2TransactionRequestTwoBridgesOuter) @@ -349,10 +316,10 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St } // From now on, we know that the used encoding version is `NEW_ENCODING_VERSION` that is - // supported only in the new protocol version with Gateway support, so we can assume + // supported only in the new protocol version with Gateway support, so we can assume // that the methods like e.g. Bridgehub.ctmAssetIdToAddress must exist. - // This is the format of the `secondBridgeData` under the `NEW_ENCODING_VERSION`. + // This is the format of the `secondBridgeData` under the `NEW_ENCODING_VERSION`. // If it fails, it would mean that the data is not correct and the call would eventually fail anyway. (bytes32 chainAssetId, bytes memory bridgehubData) = abi.decode(encodedData, (bytes32, bytes)); @@ -371,20 +338,10 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St return (address(0), false); } - // The asset handler of CTM is the bridgehub and so the following decoding should work + // The asset handler of CTM is the bridgehub and so the following decoding should work BridgehubBurnCTMAssetData memory burnData = abi.decode(bridgehubData, (BridgehubBurnCTMAssetData)); (address l2Admin, ) = abi.decode(burnData.ctmData, (address, bytes)); -<<<<<<< HEAD return (l2Admin, true); -======= - return l2Admin; - } - - function _ensureEnoughGas() private view { - if (gasleft() < MIN_GAS_FOR_FALLABLE_CALL) { - revert NotEnoughGas(); - } ->>>>>>> origin/vb-governance-n03 } } diff --git a/l1-contracts/contracts/governance/restriction/Restriction.sol b/l1-contracts/contracts/governance/restriction/Restriction.sol index 48c199b43..010563a5e 100644 --- a/l1-contracts/contracts/governance/restriction/Restriction.sol +++ b/l1-contracts/contracts/governance/restriction/Restriction.sol @@ -11,12 +11,12 @@ import {IRestriction, RESTRICTION_MAGIC} from "./IRestriction.sol"; abstract contract Restriction is IRestriction { /// @notice A method used to check that the contract supports this interface. /// @return Returns the `RESTRICTION_MAGIC` - function getSupportsRestrictionMagic() external view returns (bytes32) { + function getSupportsRestrictionMagic() external pure returns (bytes32) { return RESTRICTION_MAGIC; } /// @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 virtual view; + function validateCall(Call calldata _call, address _invoker) external view virtual; } diff --git a/l1-contracts/contracts/governance/restriction/RestrictionValidator.sol b/l1-contracts/contracts/governance/restriction/RestrictionValidator.sol index 247b65c56..e0d110947 100644 --- a/l1-contracts/contracts/governance/restriction/RestrictionValidator.sol +++ b/l1-contracts/contracts/governance/restriction/RestrictionValidator.sol @@ -3,7 +3,7 @@ pragma solidity 0.8.24; import {NotARestriction} from "../../common/L1ContractErrors.sol"; -import { IRestriction, RESTRICTION_MAGIC } from "./IRestriction.sol"; +import {IRestriction, RESTRICTION_MAGIC} from "./IRestriction.sol"; /// @title Restriction validator /// @author Matter Labs @@ -12,8 +12,8 @@ import { IRestriction, RESTRICTION_MAGIC } from "./IRestriction.sol"; library RestrictionValidator { /// @notice Ensures that the provided address implements the restriction interface /// @dev Note that it *can not guarantee* that the corresponding address indeed implements - /// the interface completely or that it is implemented correctly. It is mainly used to - /// ensure that invalid restrictions can not be accidentally added. + /// the interface completely or that it is implemented correctly. It is mainly used to + /// ensure that invalid restrictions can not be accidentally added. function validateRestriction(address _restriction) internal view { if (IRestriction(_restriction).getSupportsRestrictionMagic() != RESTRICTION_MAGIC) { revert NotARestriction(_restriction); diff --git a/l1-contracts/test/foundry/l1/unit/concrete/Governance/ChainAdmin.t.sol b/l1-contracts/test/foundry/l1/unit/concrete/Governance/ChainAdmin.t.sol index 50a66f2e8..abfd3f0ce 100644 --- a/l1-contracts/test/foundry/l1/unit/concrete/Governance/ChainAdmin.t.sol +++ b/l1-contracts/test/foundry/l1/unit/concrete/Governance/ChainAdmin.t.sol @@ -9,12 +9,8 @@ import {IChainAdmin} from "contracts/governance/IChainAdmin.sol"; import {ChainAdmin} from "contracts/governance/ChainAdmin.sol"; import {GettersFacet} from "contracts/state-transition/chain-deps/facets/Getters.sol"; import {Call} from "contracts/governance/Common.sol"; -<<<<<<< HEAD import {DummyRestriction} from "contracts/dev-contracts/DummyRestriction.sol"; -import {NotARestriction, NoCallsProvided, RestrictionWasAlreadyPresent, RestrictionWasNotPresent, AccessToFallbackDenied, AccessToFunctionDenied} from "contracts/common/L1ContractErrors.sol"; -======= -import {ZeroAddress, NoCallsProvided, RestrictionWasAlreadyPresent, RestrictionWasNotPresent, AccessToFallbackDenied, AccessToFunctionDenied} from "contracts/common/L1ContractErrors.sol"; ->>>>>>> origin/sb-governance-l02 +import {ZeroAddress, NotARestriction, NoCallsProvided, RestrictionWasAlreadyPresent, RestrictionWasNotPresent, AccessToFallbackDenied, AccessToFunctionDenied} from "contracts/common/L1ContractErrors.sol"; import {Utils} from "test/foundry/l1/unit/concrete/Utils/Utils.sol"; contract ChainAdminTest is Test { diff --git a/l1-contracts/test/foundry/l2/unit/L2AdminFactory/L2AdminFactory.t.sol b/l1-contracts/test/foundry/l2/unit/L2AdminFactory/L2AdminFactory.t.sol index e2d1e8c87..9c11a9cf0 100644 --- a/l1-contracts/test/foundry/l2/unit/L2AdminFactory/L2AdminFactory.t.sol +++ b/l1-contracts/test/foundry/l2/unit/L2AdminFactory/L2AdminFactory.t.sol @@ -8,16 +8,16 @@ import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; import {L2AdminFactory} from "contracts/governance/L2AdminFactory.sol"; import {PermanentRestriction} from "contracts/governance/PermanentRestriction.sol"; import {IPermanentRestriction} from "contracts/governance/IPermanentRestriction.sol"; -<<<<<<< HEAD import {DummyRestriction} from "contracts/dev-contracts/DummyRestriction.sol"; import {NotARestriction} from "contracts/common/L1ContractErrors.sol"; +import {ZeroAddress} from "contracts/common/L1ContractErrors.sol"; contract L2AdminFactoryTest is Test { address validRestriction1; address validRestriction2; address invalidRestriction; - + function setUp() public { validRestriction1 = address(new DummyRestriction(true)); validRestriction2 = address(new DummyRestriction(true)); @@ -25,11 +25,6 @@ contract L2AdminFactoryTest is Test { invalidRestriction = address(new DummyRestriction(false)); } - function test_invalidInitialRestriction() public { -======= -import {ZeroAddress} from "contracts/common/L1ContractErrors.sol"; - -contract L2AdminFactoryTest is Test { function testDeployL2AdminFactoryRevertZeroAddress() public { address[] memory requiredRestrictions = new address[](2); requiredRestrictions[0] = makeAddr("required"); @@ -41,20 +36,19 @@ contract L2AdminFactoryTest is Test { function testDeployL2AdminZeroAddress() public { address[] memory requiredRestrictions = new address[](1); - requiredRestrictions[0] = makeAddr("required"); + requiredRestrictions[0] = validRestriction1; L2AdminFactory factory = new L2AdminFactory(requiredRestrictions); address[] memory additionalRestrictions = new address[](2); - additionalRestrictions[0] = makeAddr("additional"); + additionalRestrictions[0] = validRestriction2; additionalRestrictions[1] = address(0); vm.expectRevert(abi.encodeWithSelector(ZeroAddress.selector)); address admin = factory.deployAdmin(additionalRestrictions, bytes32(0)); } - function testL2AdminFactory() public { ->>>>>>> origin/sb-governance-l02 + function test_invalidInitialRestriction() public { address[] memory requiredRestrictions = new address[](1); requiredRestrictions[0] = invalidRestriction;