Add unit tests for governance

ethereum/contracts/dev-contracts/EventOnFallback.sol

@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+contract EventOnFallback {
+    event Called(address msgSender, uint256 value, bytes data);
+
+    fallback() external payable {
+        emit Called(msg.sender, msg.value, msg.data);
+    }
+}

ethereum/contracts/dev-contracts/test/ReenterGovernance.sol

@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.13;
+
+import {IGovernance} from "../../governance/IGovernance.sol";
+
+contract ReenterGovernance {
+    IGovernance governance;
+
+    // Store call, predecessor and salt separately, 
+    // because Operation struct can't be stored on storage.
+    IGovernance.Call call;
+    bytes32 predecessor;
+    bytes32 salt;
+
+    // Save one value to determine whether reentrancy already happen.
+    bool alreadyReentered;
+
+    enum FunctionToCall {
+        Unset,
+        Execute,
+        ExecuteInstant,
+        Cancel
+    }
+
+    FunctionToCall functionToCall;
+
+    function initialize(IGovernance _governance, IGovernance.Operation memory _op, FunctionToCall _functionToCall) external {
+        governance = _governance;
+        require(_op.calls.length == 1, "Only 1 calls supported");
+        call = _op.calls[0];
+        predecessor = _op.predecessor;
+        salt = _op.salt;
+
+        functionToCall = _functionToCall;
+    }
+
+    fallback() external payable {
+        if (!alreadyReentered) {
+            alreadyReentered = true;
+            IGovernance.Call[] memory calls = new IGovernance.Call[](1);
+            calls[0] = call;
+            IGovernance.Operation memory op = IGovernance.Operation({calls: calls, predecessor: predecessor, salt: salt});
+
+            if (functionToCall == ReenterGovernance.FunctionToCall.Execute) {
+                governance.execute(op);
+            } else if(functionToCall == ReenterGovernance.FunctionToCall.ExecuteInstant) {
+                governance.executeInstant(op);
+            } else if(functionToCall == ReenterGovernance.FunctionToCall.Cancel) {
+                bytes32 opId = governance.hashOperation(op);
+                governance.cancel(opId);
+            } else {
+                revert("Unset function to call");
+            }   
+        }
+    }
+}

ethereum/contracts/governance/Governance.sol

@@ -3,7 +3,6 @@
 pragma solidity ^0.8.13;
 
 import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
-import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {IGovernance} from "./IGovernance.sol";
 
 /// @author Matter Labs
@@ -153,7 +152,7 @@ contract Governance is IGovernance, Ownable2Step {
     /// @dev Both the owner and security council may cancel an operation.
     /// @param _id Proposal id value (see `hashOperation`)
     function cancel(bytes32 _id) external onlyOwnerOrSecurityCouncil {
-        require(isOperationPending(_id));
+        require(isOperationPending(_id), "Operation must be pending");
         delete timestamps[_id];
         emit OperationCancelled(_id);
     }

ethereum/contracts/zksync/DiamondInit.sol

@@ -17,10 +17,8 @@ import {L2_TO_L1_LOG_SERIALIZE_SIZE, EMPTY_STRING_KECCAK, DEFAULT_L2_LOGS_TREE_R
 /// @dev The contract is used only once to initialize the diamond proxy.
 /// @dev The deployment process takes care of this contract's initialization.
 contract DiamondInit is Base {
-    /// @dev Initialize the implementation to prevent any possibility of a Parity hack.
-    constructor() reentrancyGuardInitializer {}
-
-    /// @notice zkSync contract initialization
+    /// @notice Struct that holds all data needed for initializing zkSync Diamond Proxy.
+    /// @dev We use struct instead of raw parameters in `initialize` function to prevent "Stack too deep" error
     /// @param _verifier address of Verifier contract
     /// @param _governor address who can manage critical updates in the contract
     /// @param _admin address who can manage non-critical updates in the contract
@@ -33,50 +31,56 @@ contract DiamondInit is Base {
     /// @param _l2BootloaderBytecodeHash The hash of bootloader L2 bytecode
     /// @param _l2DefaultAccountBytecodeHash The hash of default account L2 bytecode
     /// @param _priorityTxMaxGasLimit maximum number of the L2 gas that a user can request for L1 -> L2 transactions
+    struct InitializeData {
+        IVerifier verifier;
+        address governor;
+        address admin;
+        bytes32 genesisBatchHash;
+        uint64 genesisIndexRepeatedStorageChanges;
+        bytes32 genesisBatchCommitment;
+        IAllowList allowList;
+        VerifierParams verifierParams;
+        bool zkPorterIsAvailable;
+        bytes32 l2BootloaderBytecodeHash;
+        bytes32 l2DefaultAccountBytecodeHash;
+        uint256 priorityTxMaxGasLimit;
+    }
+
+    /// @dev Initialize the implementation to prevent any possibility of a Parity hack.
+    constructor() reentrancyGuardInitializer {}
+
+    /// @notice zkSync contract initialization
     /// @return Magic 32 bytes, which indicates that the contract logic is expected to be used as a diamond proxy
     /// initializer
-    function initialize(
-        IVerifier _verifier,
-        address _governor,
-        address _admin,
-        bytes32 _genesisBatchHash,
-        uint64 _genesisIndexRepeatedStorageChanges,
-        bytes32 _genesisBatchCommitment,
-        IAllowList _allowList,
-        VerifierParams calldata _verifierParams,
-        bool _zkPorterIsAvailable,
-        bytes32 _l2BootloaderBytecodeHash,
-        bytes32 _l2DefaultAccountBytecodeHash,
-        uint256 _priorityTxMaxGasLimit
-    ) external reentrancyGuardInitializer returns (bytes32) {
-        require(address(_verifier) != address(0), "vt");
-        require(_governor != address(0), "vy");
-        require(_admin != address(0), "hc");
-        require(_priorityTxMaxGasLimit <= L2_TX_MAX_GAS_LIMIT, "vu");
+    function initialize(InitializeData calldata _initalizeData) external reentrancyGuardInitializer returns (bytes32) {
+        require(address(_initalizeData.verifier) != address(0), "vt");
+        require(_initalizeData.governor != address(0), "vy");
+        require(_initalizeData.admin != address(0), "hc");
+        require(_initalizeData.priorityTxMaxGasLimit <= L2_TX_MAX_GAS_LIMIT, "vu");
 
-        s.verifier = _verifier;
-        s.governor = _governor;
-        s.admin = _admin;
+        s.verifier = _initalizeData.verifier;
+        s.governor = _initalizeData.governor;
+        s.admin = _initalizeData.admin;
 
         // We need to initialize the state hash because it is used in the commitment of the next batch
         IExecutor.StoredBatchInfo memory storedBatchZero = IExecutor.StoredBatchInfo(
             0,
-            _genesisBatchHash,
-            _genesisIndexRepeatedStorageChanges,
+            _initalizeData.genesisBatchHash,
+            _initalizeData.genesisIndexRepeatedStorageChanges,
             0,
             EMPTY_STRING_KECCAK,
             DEFAULT_L2_LOGS_TREE_ROOT_HASH,
             0,
-            _genesisBatchCommitment
+            _initalizeData.genesisBatchCommitment
         );
 
         s.storedBatchHashes[0] = keccak256(abi.encode(storedBatchZero));
-        s.allowList = _allowList;
-        s.verifierParams = _verifierParams;
-        s.zkPorterIsAvailable = _zkPorterIsAvailable;
-        s.l2BootloaderBytecodeHash = _l2BootloaderBytecodeHash;
-        s.l2DefaultAccountBytecodeHash = _l2DefaultAccountBytecodeHash;
-        s.priorityTxMaxGasLimit = _priorityTxMaxGasLimit;
+        s.allowList = _initalizeData.allowList;
+        s.verifierParams = _initalizeData.verifierParams;
+        s.zkPorterIsAvailable = _initalizeData.zkPorterIsAvailable;
+        s.l2BootloaderBytecodeHash = _initalizeData.l2BootloaderBytecodeHash;
+        s.l2DefaultAccountBytecodeHash = _initalizeData.l2DefaultAccountBytecodeHash;
+        s.priorityTxMaxGasLimit = _initalizeData.priorityTxMaxGasLimit;
 
         // While this does not provide a protection in the production, it is needed for local testing
         // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages

ethereum/contracts/zksync/ValidatorTimelock.sol

@@ -33,7 +33,7 @@ contract ValidatorTimelock is IExecutor, Ownable2Step {
     address public immutable zkSyncContract;
 
     /// @dev The mapping of L2 batch number => timestamp when it was committed.
-    LibMap.Uint32Map committedBatchTimestamp;
+    LibMap.Uint32Map internal committedBatchTimestamp;
 
     /// @dev The address that can commit/revert/validate/execute batches.
     address public validator;

ethereum/package.json

@@ -52,8 +52,9 @@
     "test": "CONTRACT_TESTS=1 yarn hardhat test test/unit_tests/*.spec.ts --network hardhat",
     "test:foundry": "hardhat solpp && forge test",
     "test:fork": "CONTRACT_TESTS=1 TEST_CONTRACTS_FORK=1 yarn run hardhat test test/unit_tests/*.fork.ts --network hardhat",
+    "coverage:foundry": "hardhat solpp && forge coverage",
     "lint": "yarn lint:sol && yarn prettier:check",
-    "lint:sol-contracts": "solhint --max-warnings 44 contracts/**/*.sol",
+    "lint:sol-contracts": "solhint --max-warnings 40 contracts/**/*.sol",
     "lint:sol-tests": "solhint --max-warnings 0 test/**/*.sol",
     "lint:sol": "yarn lint:sol-contracts && yarn lint:sol-tests",
     "prettier:check-contracts": "prettier --check contracts/**/*.sol",

ethereum/src.ts/deploy.ts

@@ -106,7 +106,7 @@ export class Deployer {
             )
         );
         const genesisBatchHash = getHashFromEnv('CONTRACTS_GENESIS_ROOT'); // TODO: confusing name
-        const genesisRollupLeafIndex = getNumberFromEnv('CONTRACTS_GENESIS_ROLLUP_LEAF_INDEX');
+        const genesisIndexRepeatedStorageChanges = getNumberFromEnv('CONTRACTS_GENESIS_ROLLUP_LEAF_INDEX');
         const genesisBatchCommitment = getHashFromEnv('CONTRACTS_GENESIS_BATCH_COMMITMENT');
         const verifierParams = {
             recursionNodeLevelVkHash: getHashFromEnv('CONTRACTS_RECURSION_NODE_LEVEL_VK_HASH'),
@@ -117,18 +117,20 @@ export class Deployer {
         const DiamondInit = new Interface(hardhat.artifacts.readArtifactSync('DiamondInit').abi);
 
         const diamondInitCalldata = DiamondInit.encodeFunctionData('initialize', [
-            this.addresses.ZkSync.Verifier,
-            this.ownerAddress,
-            this.ownerAddress,
-            genesisBatchHash,
-            genesisRollupLeafIndex,
-            genesisBatchCommitment,
-            this.addresses.AllowList,
-            verifierParams,
-            false, // isPorterAvailable
-            L2_BOOTLOADER_BYTECODE_HASH,
-            L2_DEFAULT_ACCOUNT_BYTECODE_HASH,
-            priorityTxMaxGasLimit
+            {
+                verifier: this.addresses.ZkSync.Verifier,
+                governor: this.ownerAddress,
+                admin: this.ownerAddress,
+                genesisBatchHash,
+                genesisIndexRepeatedStorageChanges,
+                genesisBatchCommitment,
+                allowList: this.addresses.AllowList,
+                verifierParams,
+                zkPorterIsAvailable: false,
+                l2BootloaderBytecodeHash: L2_BOOTLOADER_BYTECODE_HASH,
+                l2DefaultAccountBytecodeHash: L2_DEFAULT_ACCOUNT_BYTECODE_HASH,
+                priorityTxMaxGasLimit,
+            }
         ]);
 
         // @ts-ignore

ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol

@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.17;
+
+import {GovernanceTest} from "./_Governance_Shared.t.sol";
+import {IGovernance} from "../../../../../cache/solpp-generated-contracts/governance/IGovernance.sol";
+
+contract Authorization is GovernanceTest {
+    function test_RevertWhen_SchedulingByUnauthorisedAddress() public {
+        vm.prank(randomSigner);
+        vm.expectRevert("Ownable: caller is not the owner");
+        IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, "");
+        governance.scheduleTransparent(op, 0);
+    }
+
+    function test_RevertWhen_SchedulingBySecurityCouncil() public {
+        vm.prank(securityCouncil);
+        vm.expectRevert("Ownable: caller is not the owner");
+        IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, "");
+        governance.scheduleTransparent(op, 0);
+    }
+
+    function test_RevertWhen_SchedulingShadowByUnauthorisedAddress() public {
+        vm.prank(randomSigner);
+        vm.expectRevert("Ownable: caller is not the owner");
+        governance.scheduleShadow(bytes32(0), 0);
+    }
+
+    function test_RevertWhen_SchedulingShadowBySecurityCouncil() public {
+        vm.prank(securityCouncil);
+        vm.expectRevert("Ownable: caller is not the owner");
+        governance.scheduleShadow(bytes32(0), 0);
+    }
+
+    function test_RevertWhen_ExecutingByUnauthorisedAddress() public {
+        vm.prank(randomSigner);
+        vm.expectRevert("Only the owner and security council are allowed to call this function");
+        IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, "");
+        governance.execute(op);
+    }
+
+    function test_RevertWhen_ExecutingInstantByUnauthorisedAddress() public {
+        vm.prank(randomSigner);
+        vm.expectRevert("Only security council allowed to call this function");
+        IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, "");
+        governance.executeInstant(op);
+    }
+
+    function test_RevertWhen_ExecutingInstantByOwner() public {
+        vm.prank(owner);
+        vm.expectRevert("Only security council allowed to call this function");
+        IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, "");
+        governance.executeInstant(op);
+    }
+
+    function test_RevertWhen_CancelByUnauthorisedAddress() public {
+        vm.prank(randomSigner);
+        vm.expectRevert("Only the owner and security council are allowed to call this function");
+        governance.cancel(bytes32(0));
+    }
+
+    function test_RevertWhen_UpdateDelayByUnauthorisedAddress() public {
+        vm.prank(randomSigner);
+        vm.expectRevert("Only governance contract itself allowed to call this function");
+        governance.updateDelay(0);
+    }
+
+    function test_RevertWhen_UpdateDelayByOwner() public {
+        vm.prank(owner);
+        vm.expectRevert("Only governance contract itself allowed to call this function");
+        governance.updateDelay(0);
+    }
+
+    function test_RevertWhen_UpdateDelayBySecurityCouncil() public {
+        vm.prank(securityCouncil);
+        vm.expectRevert("Only governance contract itself allowed to call this function");
+        governance.updateDelay(0);
+    }
+
+    function test_RevertWhen_UpdateSecurityCouncilByUnauthorisedAddress() public {
+        vm.prank(randomSigner);
+        vm.expectRevert("Only governance contract itself allowed to call this function");
+        governance.updateSecurityCouncil(address(0));
+    }
+
+    function test_RevertWhen_UpdateSecurityCouncilByOwner() public {
+        vm.prank(owner);
+        vm.expectRevert("Only governance contract itself allowed to call this function");
+        governance.updateSecurityCouncil(address(0));
+    }
+
+    function test_RevertWhen_UpdateSecurityCouncilBySecurityCouncil() public {
+        vm.prank(securityCouncil);
+        vm.expectRevert("Only governance contract itself allowed to call this function");
+        governance.updateSecurityCouncil(address(0));
+    }
+}