From 4d3f73fa380bd2c64bda94ba2b513092f70d8db8 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sat, 30 Sep 2023 17:57:09 +0200 Subject: [PATCH 01/13] Add tests for Governance --- .../dev-contracts/EventOnFallback.sol | 11 + .../concrete/Governance/Authorization.t.sol | 96 ++++++++ .../unit/concrete/Governance/Executing.t.sol | 225 ++++++++++++++++++ .../concrete/Governance/OperationStatus.t.sol | 193 +++++++++++++++ .../concrete/Governance/SelfUpgrades.t.sol | 53 +++++ .../Governance/_Governance_Shared.t.sol | 58 +++++ 6 files changed, 636 insertions(+) create mode 100644 ethereum/contracts/dev-contracts/EventOnFallback.sol create mode 100644 ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol create mode 100644 ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol create mode 100644 ethereum/test/foundry/unit/concrete/Governance/OperationStatus.t.sol create mode 100644 ethereum/test/foundry/unit/concrete/Governance/SelfUpgrades.t.sol create mode 100644 ethereum/test/foundry/unit/concrete/Governance/_Governance_Shared.t.sol diff --git a/ethereum/contracts/dev-contracts/EventOnFallback.sol b/ethereum/contracts/dev-contracts/EventOnFallback.sol new file mode 100644 index 000000000..0db70fa64 --- /dev/null +++ b/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); + } +} diff --git a/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol new file mode 100644 index 000000000..c52bade12 --- /dev/null +++ b/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(bytes("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(bytes("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(bytes("Ownable: caller is not the owner")); + governance.scheduleShadow(bytes32(0), 0); + } + + function test_RevertWhen_SchedulingShadowBySecurityCouncil() public { + vm.prank(securityCouncil); + vm.expectRevert(bytes("Ownable: caller is not the owner")); + governance.scheduleShadow(bytes32(0), 0); + } + + function test_RevertWhen_ExecutingByUnauthorisedAddress() public { + vm.prank(randomSigner); + vm.expectRevert(bytes("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(bytes("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(bytes("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(bytes("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(bytes("Only governance contract itself allowed to call this function")); + governance.updateDelay(0); + } + + function test_RevertWhen_UpdateDelayByOwner() public { + vm.prank(owner); + vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + governance.updateDelay(0); + } + + function test_RevertWhen_UpdateDelayBySecurityCouncil() public { + vm.prank(securityCouncil); + vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + governance.updateDelay(0); + } + + function test_RevertWhen_UpdateSecurityCouncilByUnauthorisedAddress() public { + vm.prank(randomSigner); + vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + governance.updateSecurityCouncil(address(0)); + } + + function test_RevertWhen_UpdateSecurityCouncilByOwner() public { + vm.prank(owner); + vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + governance.updateSecurityCouncil(address(0)); + } + + function test_RevertWhen_UpdateSecurityCouncilBySecurityCouncil() public { + vm.prank(owner); + vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + governance.updateSecurityCouncil(address(0)); + } +} diff --git a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol new file mode 100644 index 000000000..e2a6a953c --- /dev/null +++ b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol @@ -0,0 +1,225 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.17; + +import {GovernanceTest} from "./_Governance_Shared.t.sol"; +import {Utils} from "../Utils/Utils.sol"; +import {IGovernance} from "../../../../../cache/solpp-generated-contracts/governance/IGovernance.sol"; + +contract ExecutingTest is GovernanceTest { + function test_ScheduleTransparentAndExecute() public { + vm.startPrank(owner); + + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + governance.scheduleTransparent(op, 1000); + vm.warp(block.timestamp + 1000); + executeOpAndCheck(op); + } + + function test_ScheduleTransparentAndExecuteInstant() public { + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + vm.prank(owner); + governance.scheduleTransparent(op, 1000000); + vm.prank(securityCouncil); + executeInstantOpAndCheck(op); + } + + function test_ScheduleShadowAndExecute() public { + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + bytes32 opId = governance.hashOperation(op); + vm.startPrank(owner); + governance.scheduleShadow(opId, 100000); + vm.warp(block.timestamp + 100001); + vm.startPrank(securityCouncil); + executeOpAndCheck(op); + } + + function test_ScheduleShadowAndExecuteInstant() public { + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + bytes32 opId = governance.hashOperation(op); + vm.startPrank(owner); + governance.scheduleShadow(opId, 100000); + vm.startPrank(securityCouncil); + executeInstantOpAndCheck(op); + } + + function test_RevertWhen_ExecutingOperationBeforeDeadline() public { + vm.startPrank(owner); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + governance.scheduleTransparent(op, 10000); + vm.expectRevert(bytes("Operation must be ready before execution")); + governance.execute(op); + } + + function test_RevertWhen_ExecutingOperationWithDifferentTarget() public { + vm.startPrank(owner); + IGovernance.Operation memory validOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "" + ); + governance.scheduleTransparent(validOp, 0); + + IGovernance.Operation memory invalidOp = operationWithOneCallZeroSaltAndPredecessor(address(0), 0, ""); + vm.expectRevert(bytes("Operation must be ready before execution")); + governance.execute(invalidOp); + } + + function test_RevertWhen_ExecutingOperationWithDifferentValue() public { + vm.startPrank(owner); + IGovernance.Operation memory validOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "" + ); + governance.scheduleTransparent(validOp, 0); + + IGovernance.Operation memory invalidOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 1, + "" + ); + vm.expectRevert(bytes("Operation must be ready before execution")); + governance.execute(invalidOp); + } + + function test_RevertWhen_ExecutingOperationWithDifferentData() public { + vm.startPrank(owner); + IGovernance.Operation memory validOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "" + ); + governance.scheduleTransparent(validOp, 0); + + IGovernance.Operation memory invalidOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "00" + ); + vm.expectRevert(bytes("Operation must be ready before execution")); + governance.execute(invalidOp); + } + + function test_RevertWhen_ExecutingOperationWithDifferentPredecessor() public { + vm.startPrank(owner); + // Executing one operation to get a valid executed predecessor + IGovernance.Operation memory executedOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "1122" + ); + governance.scheduleTransparent(executedOp, 0); + executeOpAndCheck(executedOp); + + // Schedule & execute operation with 0 predecessor + IGovernance.Operation memory validOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "" + ); + governance.scheduleTransparent(validOp, 0); + executeOpAndCheck(validOp); + + // Schedule operation with predecessor of `executedOp` operation + IGovernance.Operation memory invalidOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "" + ); + invalidOp.predecessor = governance.hashOperation(executedOp); + + // Failed to execute operation that wasn't scheduled + vm.expectRevert(bytes("Operation must be ready before execution")); + governance.execute(invalidOp); + } + + function test_RevertWhen_ExecutingOperationWithDifferentSalt() public { + vm.startPrank(owner); + IGovernance.Operation memory validOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "" + ); + governance.scheduleTransparent(validOp, 0); + + IGovernance.Operation memory invalidOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "" + ); + invalidOp.salt = Utils.randomBytes32("wrongSalt"); + vm.expectRevert(bytes("Operation must be ready before execution")); + governance.execute(invalidOp); + } + + function test_RevertWhen_ExecutingOperationWithNonExecutedPredecessor() public { + vm.startPrank(owner); + + IGovernance.Operation memory invalidOp = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "1122" + ); + invalidOp.predecessor = Utils.randomBytes32("randomPredecessor"); + governance.scheduleTransparent(invalidOp, 0); + vm.expectRevert(bytes("Predecessor operation not completed")); + governance.execute(invalidOp); + } + + function test_RevertWhen_ScheduleOperationOnceAndExecuteTwice() public { + vm.startPrank(owner); + + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "1122" + ); + governance.scheduleTransparent(op, 0); + executeOpAndCheck(op); + + vm.expectRevert(bytes("Operation must be ready before execution")); + governance.execute(op); + } + + function test_RevertWhen_ExecutingOperationAfterCanceling() public { + vm.startPrank(owner); + + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "1122" + ); + governance.scheduleTransparent(op, 0); + governance.cancel(governance.hashOperation(op)); + vm.expectRevert(bytes("Operation must be ready before execution")); + governance.execute(op); + } + + function test_ExecutingOperationAfterRescheduling() public { + vm.startPrank(owner); + + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "1122" + ); + governance.scheduleTransparent(op, 0); + governance.cancel(governance.hashOperation(op)); + governance.scheduleTransparent(op, 0); + executeOpAndCheck(op); + } + + function test_ExecutingOperationTwice() public { + vm.startPrank(owner); + + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "1122" + ); + governance.scheduleTransparent(op, 0); + governance.cancel(governance.hashOperation(op)); + governance.scheduleTransparent(op, 0); + executeOpAndCheck(op); + } +} diff --git a/ethereum/test/foundry/unit/concrete/Governance/OperationStatus.t.sol b/ethereum/test/foundry/unit/concrete/Governance/OperationStatus.t.sol new file mode 100644 index 000000000..1f603635d --- /dev/null +++ b/ethereum/test/foundry/unit/concrete/Governance/OperationStatus.t.sol @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.17; + +import {GovernanceTest} from "./_Governance_Shared.t.sol"; +import {Utils} from "../Utils/Utils.sol"; +import {IGovernance} from "../../../../../cache/solpp-generated-contracts/governance/IGovernance.sol"; + +contract OperationStatusTest is GovernanceTest { + function test_RandomIdIsNotOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + bool isOperation = governance.isOperation(randomId); + require(!isOperation, "Should not be operation"); + } + + function test_RandomIdIsNotPendingOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + bool isOperationPending = governance.isOperationPending(randomId); + require(!isOperationPending, "Should not be pending operation"); + } + + function test_RandomIdIsNotReadyOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + bool isOperationReady = governance.isOperationReady(randomId); + require(!isOperationReady, "Should not be ready operation"); + } + + function test_RandomIdIsNotDoneOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + bool isOperationDone = governance.isOperationDone(randomId); + require(!isOperationDone, "Should not be done operation"); + } + + function test_RandomIdIsHasUnsetStatus() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + IGovernance.OperationState opState = governance.getOperationState(randomId); + require(opState == IGovernance.OperationState.Unset, "Operation status should be Unset"); + } + + function test_ScheduledOperationIsOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + + bool isOperation = governance.isOperation(randomId); + require(isOperation, "Should be operation"); + } + + function test_ScheduledOperationIsPendingOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + + bool isOperationPending = governance.isOperationPending(randomId); + require(isOperationPending, "Should be pending operation"); + } + + function test_ScheduledOperationIsNotReadyOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + + bool isOperationReady = governance.isOperationReady(randomId); + require(!isOperationReady, "Should not be ready operation"); + } + + function test_ScheduledOperationIsNotDone() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + + bool isOperationDone = governance.isOperationDone(randomId); + require(!isOperationDone, "Should not be done operation"); + } + + function test_ScheduledOperationHasWaitingStatus() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + + IGovernance.OperationState opState = governance.getOperationState(randomId); + require(opState == IGovernance.OperationState.Waiting, "Operation status is not Waiting"); + } + + function test_ScheduledOperationAfterDelayIsOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + vm.warp(block.timestamp + 1000); + + bool isOperation = governance.isOperation(randomId); + require(isOperation, "Should be operation"); + } + + function test_ScheduledOperationAfterDelayIsPendingOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + vm.warp(block.timestamp + 1000); + + bool isOperationPending = governance.isOperationPending(randomId); + require(isOperationPending, "Should be pending operation"); + } + + function test_ScheduledOperationAfterDelayIsReadyOperation() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + vm.warp(block.timestamp + 1000); + + bool isOperationReady = governance.isOperationReady(randomId); + require(isOperationReady, "Operation should be ready"); + } + + function test_ScheduledOperationAfterDelayIsNotDone() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + vm.warp(block.timestamp + 1000); + + bool isOperationDone = governance.isOperationDone(randomId); + require(!isOperationDone, "Operation should not be done"); + } + + function test_ScheduledOperationAfterDelayHasReadyStatus() public { + bytes32 randomId = Utils.randomBytes32("randomId"); + vm.prank(owner); + governance.scheduleShadow(randomId, 1000); + vm.warp(block.timestamp + 1000); + + IGovernance.OperationState opState = governance.getOperationState(randomId); + require(opState == IGovernance.OperationState.Ready, "Operation status should be Ready"); + } + + function test_ProcessedOperationIsOperation() public { + vm.startPrank(owner); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + governance.scheduleTransparent(op, 1000); + vm.warp(block.timestamp + 1000); + executeOpAndCheck(op); + + bytes32 opId = governance.hashOperation(op); + bool isOperation = governance.isOperation(opId); + require(isOperation, "Should be operation"); + } + + function test_ScheduledOperationAfterDelayIsNotPendingOperation() public { + vm.startPrank(owner); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + governance.scheduleTransparent(op, 1000); + vm.warp(block.timestamp + 1000); + executeOpAndCheck(op); + + bytes32 opId = governance.hashOperation(op); + bool isOperationPending = governance.isOperationPending(opId); + require(!isOperationPending, "Should not be pending operation"); + } + + function test_ProcessedOperationAfterDelayIsNotReadyOperation() public { + vm.startPrank(owner); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + governance.scheduleTransparent(op, 1000); + vm.warp(block.timestamp + 1000); + executeOpAndCheck(op); + + bytes32 opId = governance.hashOperation(op); + bool isOperationReady = governance.isOperationReady(opId); + require(!isOperationReady, "Should be not ready operation"); + } + + function test_ProcessedOperationAfterDelayIsDone() public { + vm.startPrank(owner); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + governance.scheduleTransparent(op, 1000); + vm.warp(block.timestamp + 1000); + executeOpAndCheck(op); + + bytes32 opId = governance.hashOperation(op); + bool isOperationDone = governance.isOperationDone(opId); + require(isOperationDone, "Operation should be done"); + } + + function test_ProcessedOperationAfterDelayHasReadyStatus() public { + vm.startPrank(owner); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); + governance.scheduleTransparent(op, 1000); + vm.warp(block.timestamp + 1000); + executeOpAndCheck(op); + + bytes32 opId = governance.hashOperation(op); + IGovernance.OperationState opState = governance.getOperationState(opId); + require(opState == IGovernance.OperationState.Done, "Operation status should be done"); + } +} diff --git a/ethereum/test/foundry/unit/concrete/Governance/SelfUpgrades.t.sol b/ethereum/test/foundry/unit/concrete/Governance/SelfUpgrades.t.sol new file mode 100644 index 000000000..57583bef4 --- /dev/null +++ b/ethereum/test/foundry/unit/concrete/Governance/SelfUpgrades.t.sol @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.17; + +import {GovernanceTest} from "./_Governance_Shared.t.sol"; +import {Utils} from "../Utils/Utils.sol"; +import {IGovernance} from "../../../../../cache/solpp-generated-contracts/governance/IGovernance.sol"; + +contract SeflUpgradesTest is GovernanceTest { + event ChangeSecurityCouncil(address _securityCouncilBefore, address _securityCouncilAfter); + + event ChangeMinDelay(uint256 _delayBefore, uint256 _delayAfter); + + function test_UpgradeDelay() public { + vm.startPrank(owner); + uint256 delayBefore = governance.minDelay(); + uint256 newDelay = 100000; + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(governance), + 0, + abi.encodeCall(IGovernance.updateDelay, (newDelay)) + ); + + governance.scheduleTransparent(op, 0); + // Check event + vm.expectEmit(false, false, false, true); + emit ChangeMinDelay(delayBefore, newDelay); + governance.execute(op); + uint256 delayAfter = governance.minDelay(); + require(delayBefore != delayAfter, "Delays are the same"); + require(newDelay == delayAfter, "Delay should have been changed"); + } + + function test_UpgradeSecurityCouncil() public { + vm.startPrank(owner); + address securityCouncilBefore = governance.securityCouncil(); + address newSecurityCouncil = address(bytes20(Utils.randomBytes32("newSecurityCouncil"))); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(governance), + 0, + abi.encodeCall(IGovernance.updateSecurityCouncil, (newSecurityCouncil)) + ); + + governance.scheduleTransparent(op, 0); + + // Check event + vm.expectEmit(false, false, false, true); + emit ChangeSecurityCouncil(securityCouncilBefore, newSecurityCouncil); + governance.execute(op); + address securityCouncilAfter = governance.securityCouncil(); + require(securityCouncilBefore != securityCouncilAfter, "Security councils are the same"); + require(newSecurityCouncil == securityCouncilAfter, "SC should have been changed"); + } +} diff --git a/ethereum/test/foundry/unit/concrete/Governance/_Governance_Shared.t.sol b/ethereum/test/foundry/unit/concrete/Governance/_Governance_Shared.t.sol new file mode 100644 index 000000000..04bee2358 --- /dev/null +++ b/ethereum/test/foundry/unit/concrete/Governance/_Governance_Shared.t.sol @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {Governance} from "../../../../../cache/solpp-generated-contracts/governance/Governance.sol"; +import {IGovernance} from "../../../../../cache/solpp-generated-contracts/governance/IGovernance.sol"; +import {EventOnFallback} from "../../../../../cache/solpp-generated-contracts/dev-contracts/EventOnFallback.sol"; + +contract GovernanceTest is Test, EventOnFallback { + address internal owner; + address internal securityCouncil; + address internal randomSigner; + Governance internal governance; + EventOnFallback internal eventOnFallback; + + constructor() { + owner = makeAddr("owner"); + securityCouncil = makeAddr("securityCouncil"); + randomSigner = makeAddr("randomSigner"); + + governance = new Governance(owner, securityCouncil, 0); + eventOnFallback = new EventOnFallback(); + } + + function setUp() external { + vm.warp(100000000); + } + + function executeOpAndCheck(IGovernance.Operation memory op) internal { + _checkEventBeforeExecution(op); + governance.execute(op); + } + + function executeInstantOpAndCheck(IGovernance.Operation memory op) internal { + _checkEventBeforeExecution(op); + governance.executeInstant(op); + } + + function _checkEventBeforeExecution(IGovernance.Operation memory op) private { + for (uint256 i = 0; i < op.calls.length; i++) { + require(op.calls[i].target == address(eventOnFallback), "EventOnFallbak target expected"); + // Check event + vm.expectEmit(false, false, false, true); + emit Called(address(governance), op.calls[i].value, op.calls[i].data); + } + } + + function operationWithOneCallZeroSaltAndPredecessor( + address _target, + uint256 _value, + bytes memory _data + ) internal returns (IGovernance.Operation memory) { + IGovernance.Call[] memory calls = new IGovernance.Call[](1); + calls[0] = IGovernance.Call({target: _target, value: _value, data: _data}); + return IGovernance.Operation({calls: calls, salt: bytes32(0), predecessor: bytes32(0)}); + } +} From e4df32a4c9bab22bfe8dbb2e63e1e9ae78d084fd Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sat, 30 Sep 2023 18:17:11 +0200 Subject: [PATCH 02/13] fix(tests): Restore tests for executing operation twice --- .../test/foundry/unit/concrete/Governance/Executing.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol index e2a6a953c..0443f225e 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol @@ -209,7 +209,7 @@ contract ExecutingTest is GovernanceTest { executeOpAndCheck(op); } - function test_ExecutingOperationTwice() public { + function test_RevertWhen_ExecutingOperationTwice() public { vm.startPrank(owner); IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( @@ -218,8 +218,8 @@ contract ExecutingTest is GovernanceTest { "1122" ); governance.scheduleTransparent(op, 0); - governance.cancel(governance.hashOperation(op)); - governance.scheduleTransparent(op, 0); executeOpAndCheck(op); + vm.expectRevert(bytes("Operation with this proposal id already exists")); + governance.scheduleTransparent(op, 0); } } From 7cecea9194ff4fd2328ca93a68116cbda4b989c4 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sun, 1 Oct 2023 14:22:51 +0200 Subject: [PATCH 03/13] fix(tests): Fix coverage in Foundry tests --- ethereum/contracts/zksync/DiamondInit.sol | 72 ++++++++++--------- ethereum/package.json | 1 + ethereum/src.ts/deploy.ts | 28 ++++---- .../unit_tests/l1_erc20_bridge_test.spec.ts | 32 +++++---- .../unit_tests/l1_weth_bridge_test.spec.ts | 32 +++++---- .../test/unit_tests/l2-upgrade.test.spec.ts | 26 +++---- ethereum/test/unit_tests/mailbox_test.spec.ts | 32 +++++---- ethereum/test/unit_tests/proxy_test.spec.ts | 26 +++---- 8 files changed, 133 insertions(+), 116 deletions(-) diff --git a/ethereum/contracts/zksync/DiamondInit.sol b/ethereum/contracts/zksync/DiamondInit.sol index 9140af05e..40adeae86 100644 --- a/ethereum/contracts/zksync/DiamondInit.sol +++ b/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 diff --git a/ethereum/package.json b/ethereum/package.json index 8abd508cc..5f06dc1f5 100644 --- a/ethereum/package.json +++ b/ethereum/package.json @@ -52,6 +52,7 @@ "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-tests": "solhint --max-warnings 0 test/**/*.sol", diff --git a/ethereum/src.ts/deploy.ts b/ethereum/src.ts/deploy.ts index f08ddeda4..3ea44754f 100644 --- a/ethereum/src.ts/deploy.ts +++ b/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 diff --git a/ethereum/test/unit_tests/l1_erc20_bridge_test.spec.ts b/ethereum/test/unit_tests/l1_erc20_bridge_test.spec.ts index 399e2742d..ce6a63458 100644 --- a/ethereum/test/unit_tests/l1_erc20_bridge_test.spec.ts +++ b/ethereum/test/unit_tests/l1_erc20_bridge_test.spec.ts @@ -50,22 +50,24 @@ describe(`L1ERC20Bridge tests`, function () { dummyHash.set([1, 0, 0, 1]); const dummyAddress = ethers.utils.hexlify(ethers.utils.randomBytes(20)); const diamondInitData = diamondInit.interface.encodeFunctionData('initialize', [ - dummyAddress, - await owner.getAddress(), - await owner.getAddress(), - ethers.constants.HashZero, - 0, - ethers.constants.HashZero, - allowList.address, { - recursionCircuitsSetVksHash: ethers.constants.HashZero, - recursionLeafLevelVkHash: ethers.constants.HashZero, - recursionNodeLevelVkHash: ethers.constants.HashZero - }, - false, - dummyHash, - dummyHash, - 10000000 + verifier: dummyAddress, + governor: await owner.getAddress(), + admin: await owner.getAddress(), + genesisBatchHash: ethers.constants.HashZero, + genesisIndexRepeatedStorageChanges: 0, + genesisBatchCommitment: ethers.constants.HashZero, + allowList: allowList.address, + verifierParams: { + recursionCircuitsSetVksHash: ethers.constants.HashZero, + recursionLeafLevelVkHash: ethers.constants.HashZero, + recursionNodeLevelVkHash: ethers.constants.HashZero + }, + zkPorterIsAvailable: false, + l2BootloaderBytecodeHash: dummyHash, + l2DefaultAccountBytecodeHash: dummyHash, + priorityTxMaxGasLimit: 10000000, + } ]); const facetCuts = [ diff --git a/ethereum/test/unit_tests/l1_weth_bridge_test.spec.ts b/ethereum/test/unit_tests/l1_weth_bridge_test.spec.ts index 5306aedae..f7047c25e 100644 --- a/ethereum/test/unit_tests/l1_weth_bridge_test.spec.ts +++ b/ethereum/test/unit_tests/l1_weth_bridge_test.spec.ts @@ -82,22 +82,24 @@ describe('WETH Bridge tests', () => { dummyHash.set([1, 0, 0, 1]); const dummyAddress = ethers.utils.hexlify(ethers.utils.randomBytes(20)); const diamondInitData = diamondInit.interface.encodeFunctionData('initialize', [ - dummyAddress, - await owner.getAddress(), - await owner.getAddress(), - ethers.constants.HashZero, - 0, - ethers.constants.HashZero, - allowList.address, { - recursionCircuitsSetVksHash: ethers.constants.HashZero, - recursionLeafLevelVkHash: ethers.constants.HashZero, - recursionNodeLevelVkHash: ethers.constants.HashZero - }, - false, - dummyHash, - dummyHash, - 10000000 + verifier: dummyAddress, + governor: await owner.getAddress(), + admin: await owner.getAddress(), + genesisBatchHash: ethers.constants.HashZero, + genesisIndexRepeatedStorageChanges: 0, + genesisBatchCommitment: ethers.constants.HashZero, + allowList: allowList.address, + verifierParams: { + recursionCircuitsSetVksHash: ethers.constants.HashZero, + recursionLeafLevelVkHash: ethers.constants.HashZero, + recursionNodeLevelVkHash: ethers.constants.HashZero + }, + zkPorterIsAvailable: false, + l2BootloaderBytecodeHash: dummyHash, + l2DefaultAccountBytecodeHash: dummyHash, + priorityTxMaxGasLimit: 10000000 + } ]); const facetCuts = [ diff --git a/ethereum/test/unit_tests/l2-upgrade.test.spec.ts b/ethereum/test/unit_tests/l2-upgrade.test.spec.ts index 460799510..791ba7ad6 100644 --- a/ethereum/test/unit_tests/l2-upgrade.test.spec.ts +++ b/ethereum/test/unit_tests/l2-upgrade.test.spec.ts @@ -88,18 +88,20 @@ describe('L2 upgrade test', function () { recursionNodeLevelVkHash: ethers.constants.HashZero }; const diamondInitData = diamondInit.interface.encodeFunctionData('initialize', [ - verifier, - await owner.getAddress(), - await owner.getAddress(), - ethers.constants.HashZero, - 0, - ethers.constants.HashZero, - allowList.address, - verifierParams, - false, - dummyHash, - dummyHash, - 10000000 + { + verifier, + governor: await owner.getAddress(), + admin: await owner.getAddress(), + genesisBatchHash: ethers.constants.HashZero, + genesisIndexRepeatedStorageChanges: 0, + genesisBatchCommitment: ethers.constants.HashZero, + allowList: allowList.address, + verifierParams, + zkPorterIsAvailable: false, + l2BootloaderBytecodeHash: dummyHash, + l2DefaultAccountBytecodeHash: dummyHash, + priorityTxMaxGasLimit: 10000000 + } ]); const facetCuts = [ diff --git a/ethereum/test/unit_tests/mailbox_test.spec.ts b/ethereum/test/unit_tests/mailbox_test.spec.ts index 10199c18d..ba6458c92 100644 --- a/ethereum/test/unit_tests/mailbox_test.spec.ts +++ b/ethereum/test/unit_tests/mailbox_test.spec.ts @@ -60,22 +60,24 @@ describe('Mailbox tests', function () { dummyHash.set([1, 0, 0, 1]); const dummyAddress = ethers.utils.hexlify(ethers.utils.randomBytes(20)); const diamondInitData = diamondInit.interface.encodeFunctionData('initialize', [ - dummyAddress, - dummyAddress, - dummyAddress, - ethers.constants.HashZero, - 0, - ethers.constants.HashZero, - allowList.address, { - recursionCircuitsSetVksHash: ethers.constants.HashZero, - recursionLeafLevelVkHash: ethers.constants.HashZero, - recursionNodeLevelVkHash: ethers.constants.HashZero - }, - false, - dummyHash, - dummyHash, - 10000000 + verifier: dummyAddress, + governor: dummyAddress, + admin: dummyAddress, + genesisBatchHash: ethers.constants.HashZero, + genesisIndexRepeatedStorageChanges: 0, + genesisBatchCommitment: ethers.constants.HashZero, + allowList: allowList.address, + verifierParams: { + recursionCircuitsSetVksHash: ethers.constants.HashZero, + recursionLeafLevelVkHash: ethers.constants.HashZero, + recursionNodeLevelVkHash: ethers.constants.HashZero + }, + zkPorterIsAvailable: false, + l2BootloaderBytecodeHash: dummyHash, + l2DefaultAccountBytecodeHash: dummyHash, + priorityTxMaxGasLimit: 10000000 + } ]); const facetCuts = [ diff --git a/ethereum/test/unit_tests/proxy_test.spec.ts b/ethereum/test/unit_tests/proxy_test.spec.ts index ce52454a1..ef352eaa3 100644 --- a/ethereum/test/unit_tests/proxy_test.spec.ts +++ b/ethereum/test/unit_tests/proxy_test.spec.ts @@ -68,18 +68,20 @@ describe('Diamond proxy tests', function () { recursionCircuitsSetVksHash: ethers.constants.HashZero }; const diamondInitCalldata = diamondInit.interface.encodeFunctionData('initialize', [ - '0x03752D8252d67f99888E741E3fB642803B29B155', - governorAddress, - governorAddress, - '0x02c775f0a90abf7a0e8043f2fdc38f0580ca9f9996a895d05a501bfeaa3b2e21', - 0, - '0x0000000000000000000000000000000000000000000000000000000000000000', - '0x70a0F165d6f8054d0d0CF8dFd4DD2005f0AF6B55', - dummyVerifierParams, - false, - '0x0100000000000000000000000000000000000000000000000000000000000000', - '0x0100000000000000000000000000000000000000000000000000000000000000', - 500000 // priority tx max L2 gas limit + { + verifier: '0x03752D8252d67f99888E741E3fB642803B29B155', + governor: governorAddress, + admin: governorAddress, + genesisBatchHash: '0x02c775f0a90abf7a0e8043f2fdc38f0580ca9f9996a895d05a501bfeaa3b2e21', + genesisIndexRepeatedStorageChanges: 0, + genesisBatchCommitment: ethers.constants.HashZero, + allowList: '0x70a0F165d6f8054d0d0CF8dFd4DD2005f0AF6B55', + verifierParams: dummyVerifierParams, + zkPorterIsAvailable: false, + l2BootloaderBytecodeHash: '0x0100000000000000000000000000000000000000000000000000000000000000', + l2DefaultAccountBytecodeHash: '0x0100000000000000000000000000000000000000000000000000000000000000', + priorityTxMaxGasLimit: 500000 + } ]); const diamondCutData = diamondCut(facetCuts, diamondInit.address, diamondInitCalldata); From 33d8e29fc6775249f1c75e7ebe5df7ccb2ae90ee Mon Sep 17 00:00:00 2001 From: Vlad Bochok <41153528+vladbochok@users.noreply.github.com> Date: Sun, 1 Oct 2023 16:35:17 +0200 Subject: [PATCH 04/13] Update ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol Co-authored-by: Stanislav Bezkorovainyi --- ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol index 0443f225e..23178a38f 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol @@ -28,7 +28,7 @@ contract ExecutingTest is GovernanceTest { bytes32 opId = governance.hashOperation(op); vm.startPrank(owner); governance.scheduleShadow(opId, 100000); - vm.warp(block.timestamp + 100001); + vm.warp(block.timestamp + 100000); vm.startPrank(securityCouncil); executeOpAndCheck(op); } From 581ecd6d40a296e06051e2c32e19b67b0c56f2e7 Mon Sep 17 00:00:00 2001 From: Vlad Bochok <41153528+vladbochok@users.noreply.github.com> Date: Sun, 1 Oct 2023 16:35:23 +0200 Subject: [PATCH 05/13] Update ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol Co-authored-by: Stanislav Bezkorovainyi --- .../test/foundry/unit/concrete/Governance/Authorization.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol index c52bade12..3db0d3601 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol @@ -89,7 +89,7 @@ contract Authorization is GovernanceTest { } function test_RevertWhen_UpdateSecurityCouncilBySecurityCouncil() public { - vm.prank(owner); + vm.prank(securityCouncil); vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); governance.updateSecurityCouncil(address(0)); } From c5b9f82c9e296e02d83c0785cb570f8a107c3ce5 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sun, 1 Oct 2023 19:59:45 +0200 Subject: [PATCH 06/13] Add test for failed execution of operation --- .../unit/concrete/Governance/Executing.t.sol | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol index 0443f225e..994b2e113 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol @@ -222,4 +222,17 @@ contract ExecutingTest is GovernanceTest { vm.expectRevert(bytes("Operation with this proposal id already exists")); governance.scheduleTransparent(op, 0); } + + function test_RevertWhen_ExecutingOperationFailed() public { + vm.startPrank(owner); + + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(revertFallback), + 0, + "" + ); + governance.scheduleTransparent(op, 0); + vm.expectRevert(bytes("")); + governance.execute(op); + } } From ee2e7b669fda884de2a9d93daa8e94662d93787f Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sun, 1 Oct 2023 20:00:43 +0200 Subject: [PATCH 07/13] Refactoring --- .../concrete/Governance/OperationStatus.t.sol | 40 +++++++++---------- .../concrete/Governance/SelfUpgrades.t.sol | 8 ++-- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/ethereum/test/foundry/unit/concrete/Governance/OperationStatus.t.sol b/ethereum/test/foundry/unit/concrete/Governance/OperationStatus.t.sol index 1f603635d..43a6849fb 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/OperationStatus.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/OperationStatus.t.sol @@ -9,31 +9,31 @@ contract OperationStatusTest is GovernanceTest { function test_RandomIdIsNotOperation() public { bytes32 randomId = Utils.randomBytes32("randomId"); bool isOperation = governance.isOperation(randomId); - require(!isOperation, "Should not be operation"); + assertFalse(isOperation); } function test_RandomIdIsNotPendingOperation() public { bytes32 randomId = Utils.randomBytes32("randomId"); bool isOperationPending = governance.isOperationPending(randomId); - require(!isOperationPending, "Should not be pending operation"); + assertFalse(isOperationPending); } function test_RandomIdIsNotReadyOperation() public { bytes32 randomId = Utils.randomBytes32("randomId"); bool isOperationReady = governance.isOperationReady(randomId); - require(!isOperationReady, "Should not be ready operation"); + assertFalse(isOperationReady); } function test_RandomIdIsNotDoneOperation() public { bytes32 randomId = Utils.randomBytes32("randomId"); bool isOperationDone = governance.isOperationDone(randomId); - require(!isOperationDone, "Should not be done operation"); + assertFalse(isOperationDone); } function test_RandomIdIsHasUnsetStatus() public { bytes32 randomId = Utils.randomBytes32("randomId"); IGovernance.OperationState opState = governance.getOperationState(randomId); - require(opState == IGovernance.OperationState.Unset, "Operation status should be Unset"); + assertTrue(opState == IGovernance.OperationState.Unset); } function test_ScheduledOperationIsOperation() public { @@ -42,7 +42,7 @@ contract OperationStatusTest is GovernanceTest { governance.scheduleShadow(randomId, 1000); bool isOperation = governance.isOperation(randomId); - require(isOperation, "Should be operation"); + assertTrue(isOperation); } function test_ScheduledOperationIsPendingOperation() public { @@ -51,7 +51,7 @@ contract OperationStatusTest is GovernanceTest { governance.scheduleShadow(randomId, 1000); bool isOperationPending = governance.isOperationPending(randomId); - require(isOperationPending, "Should be pending operation"); + assertTrue(isOperationPending); } function test_ScheduledOperationIsNotReadyOperation() public { @@ -60,7 +60,7 @@ contract OperationStatusTest is GovernanceTest { governance.scheduleShadow(randomId, 1000); bool isOperationReady = governance.isOperationReady(randomId); - require(!isOperationReady, "Should not be ready operation"); + assertFalse(isOperationReady); } function test_ScheduledOperationIsNotDone() public { @@ -69,7 +69,7 @@ contract OperationStatusTest is GovernanceTest { governance.scheduleShadow(randomId, 1000); bool isOperationDone = governance.isOperationDone(randomId); - require(!isOperationDone, "Should not be done operation"); + assertFalse(isOperationDone); } function test_ScheduledOperationHasWaitingStatus() public { @@ -78,7 +78,7 @@ contract OperationStatusTest is GovernanceTest { governance.scheduleShadow(randomId, 1000); IGovernance.OperationState opState = governance.getOperationState(randomId); - require(opState == IGovernance.OperationState.Waiting, "Operation status is not Waiting"); + assertTrue(opState == IGovernance.OperationState.Waiting); } function test_ScheduledOperationAfterDelayIsOperation() public { @@ -88,7 +88,7 @@ contract OperationStatusTest is GovernanceTest { vm.warp(block.timestamp + 1000); bool isOperation = governance.isOperation(randomId); - require(isOperation, "Should be operation"); + assertTrue(isOperation); } function test_ScheduledOperationAfterDelayIsPendingOperation() public { @@ -98,7 +98,7 @@ contract OperationStatusTest is GovernanceTest { vm.warp(block.timestamp + 1000); bool isOperationPending = governance.isOperationPending(randomId); - require(isOperationPending, "Should be pending operation"); + assertTrue(isOperationPending); } function test_ScheduledOperationAfterDelayIsReadyOperation() public { @@ -108,7 +108,7 @@ contract OperationStatusTest is GovernanceTest { vm.warp(block.timestamp + 1000); bool isOperationReady = governance.isOperationReady(randomId); - require(isOperationReady, "Operation should be ready"); + assertTrue(isOperationReady); } function test_ScheduledOperationAfterDelayIsNotDone() public { @@ -118,7 +118,7 @@ contract OperationStatusTest is GovernanceTest { vm.warp(block.timestamp + 1000); bool isOperationDone = governance.isOperationDone(randomId); - require(!isOperationDone, "Operation should not be done"); + assertFalse(isOperationDone); } function test_ScheduledOperationAfterDelayHasReadyStatus() public { @@ -128,7 +128,7 @@ contract OperationStatusTest is GovernanceTest { vm.warp(block.timestamp + 1000); IGovernance.OperationState opState = governance.getOperationState(randomId); - require(opState == IGovernance.OperationState.Ready, "Operation status should be Ready"); + assertTrue(opState == IGovernance.OperationState.Ready); } function test_ProcessedOperationIsOperation() public { @@ -140,7 +140,7 @@ contract OperationStatusTest is GovernanceTest { bytes32 opId = governance.hashOperation(op); bool isOperation = governance.isOperation(opId); - require(isOperation, "Should be operation"); + assertTrue(isOperation); } function test_ScheduledOperationAfterDelayIsNotPendingOperation() public { @@ -152,7 +152,7 @@ contract OperationStatusTest is GovernanceTest { bytes32 opId = governance.hashOperation(op); bool isOperationPending = governance.isOperationPending(opId); - require(!isOperationPending, "Should not be pending operation"); + assertFalse(isOperationPending); } function test_ProcessedOperationAfterDelayIsNotReadyOperation() public { @@ -164,7 +164,7 @@ contract OperationStatusTest is GovernanceTest { bytes32 opId = governance.hashOperation(op); bool isOperationReady = governance.isOperationReady(opId); - require(!isOperationReady, "Should be not ready operation"); + assertFalse(isOperationReady); } function test_ProcessedOperationAfterDelayIsDone() public { @@ -176,7 +176,7 @@ contract OperationStatusTest is GovernanceTest { bytes32 opId = governance.hashOperation(op); bool isOperationDone = governance.isOperationDone(opId); - require(isOperationDone, "Operation should be done"); + assertTrue(isOperationDone); } function test_ProcessedOperationAfterDelayHasReadyStatus() public { @@ -188,6 +188,6 @@ contract OperationStatusTest is GovernanceTest { bytes32 opId = governance.hashOperation(op); IGovernance.OperationState opState = governance.getOperationState(opId); - require(opState == IGovernance.OperationState.Done, "Operation status should be done"); + assertTrue(opState == IGovernance.OperationState.Done); } } diff --git a/ethereum/test/foundry/unit/concrete/Governance/SelfUpgrades.t.sol b/ethereum/test/foundry/unit/concrete/Governance/SelfUpgrades.t.sol index 57583bef4..e4948ccb2 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/SelfUpgrades.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/SelfUpgrades.t.sol @@ -26,8 +26,8 @@ contract SeflUpgradesTest is GovernanceTest { emit ChangeMinDelay(delayBefore, newDelay); governance.execute(op); uint256 delayAfter = governance.minDelay(); - require(delayBefore != delayAfter, "Delays are the same"); - require(newDelay == delayAfter, "Delay should have been changed"); + assertTrue(delayBefore != delayAfter); + assertTrue(newDelay == delayAfter); } function test_UpgradeSecurityCouncil() public { @@ -47,7 +47,7 @@ contract SeflUpgradesTest is GovernanceTest { emit ChangeSecurityCouncil(securityCouncilBefore, newSecurityCouncil); governance.execute(op); address securityCouncilAfter = governance.securityCouncil(); - require(securityCouncilBefore != securityCouncilAfter, "Security councils are the same"); - require(newSecurityCouncil == securityCouncilAfter, "SC should have been changed"); + assertTrue(securityCouncilBefore != securityCouncilAfter); + assertTrue(newSecurityCouncil == securityCouncilAfter); } } From f7fb805a10005dfdb1a3f9378ff39b27782d1e0d Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sun, 1 Oct 2023 23:55:46 +0200 Subject: [PATCH 08/13] Add tests on reentrancy --- .../unit/concrete/Governance/Reentrancy.t.sol | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 ethereum/test/foundry/unit/concrete/Governance/Reentrancy.t.sol diff --git a/ethereum/test/foundry/unit/concrete/Governance/Reentrancy.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Reentrancy.t.sol new file mode 100644 index 000000000..3a735b2b6 --- /dev/null +++ b/ethereum/test/foundry/unit/concrete/Governance/Reentrancy.t.sol @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.17; + +// solhint-disable max-line-length + +import {StdStorage, stdStorage} from "forge-std/Test.sol"; +import {GovernanceTest} from "./_Governance_Shared.t.sol"; +import {IGovernance} from "../../../../../cache/solpp-generated-contracts/governance/IGovernance.sol"; +import {ReenterGovernance} from "../../../../../cache/solpp-generated-contracts/dev-contracts/test/ReenterGovernance.sol"; + +contract ReentrancyTest is GovernanceTest { + using stdStorage for StdStorage; + + function test_ExecuteOperationWithReentrancy() public { + // Set governance owner to be equal to the forwarder contract. + stdstore.target(address(governance)).sig(governance.owner.selector).checked_write(address(forwarder)); + vm.startPrank(address(forwarder)); + + // Schedule operation, but don't execute it. + IGovernance.Operation memory op1 = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "eeeeeee" + ); + governance.scheduleTransparent(op1, 0); + + // Prepare data for the second operation, which will call forwarder + // to reenter governance with the execution of the first operation. + bytes memory executeData = abi.encodeCall(IGovernance.execute, (op1)); + bytes memory forwardData = abi.encodeCall(forwarder.forward, (address(governance), executeData)); + IGovernance.Operation memory op2 = operationWithOneCallZeroSaltAndPredecessor( + address(forwarder), + 0, + forwardData + ); + // Schedule & execute the second operation. + governance.scheduleTransparent(op2, 0); + governance.execute(op2); + // Check that both operations were executed. + assertTrue(governance.isOperationDone(governance.hashOperation(op2))); + assertTrue(governance.isOperationDone(governance.hashOperation(op1))); + } + + function test_ExecuteInstantOperationWithReentrancy() public { + // Set governance owner & security council to be equal to the forwarder contract. + stdstore.target(address(governance)).sig(governance.owner.selector).checked_write(address(forwarder)); + vm.startPrank(address(forwarder)); + stdstore.target(address(governance)).sig(governance.securityCouncil.selector).checked_write(address(forwarder)); + vm.startPrank(address(forwarder)); + + // Schedule operation, but don't execute it. + IGovernance.Operation memory op1 = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "eeeeeee" + ); + governance.scheduleTransparent(op1, 0); + + // Prepare data for the second operation, which will call forwarder + // to reenter governance with the execution of the first operation. + bytes memory executeData = abi.encodeCall(IGovernance.execute, (op1)); + bytes memory forwardData = abi.encodeCall(forwarder.forward, (address(governance), executeData)); + IGovernance.Operation memory op2 = operationWithOneCallZeroSaltAndPredecessor( + address(forwarder), + 0, + forwardData + ); + // Schedule & execute instant the second operation. + governance.scheduleTransparent(op2, 0); + governance.executeInstant(op2); + // Check that both operations were executed. + assertTrue(governance.isOperationDone(governance.hashOperation(op2))); + assertTrue(governance.isOperationDone(governance.hashOperation(op1))); + } + + function test_RevertedWith_ExecuteTheSameOperationTwice() public { + // Initialize contract that will reenter Governance on executing operation. + ReenterGovernance reenterGovernance = new ReenterGovernance(); + // Grant owner access for reenterGovernance. + stdstore.target(address(governance)).sig(governance.owner.selector).checked_write(address(reenterGovernance)); + + // Schedule operation, that will reenter `Governance.execute` for the same op. + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(reenterGovernance), 0, ""); + reenterGovernance.initialize(governance, op, ReenterGovernance.FunctionToCall.Execute); + + vm.startPrank(address(reenterGovernance)); + + governance.scheduleTransparent(op, 0); + vm.expectRevert("Operation must be ready after execution"); + governance.execute(op); + } + + function test_RevertedWith_ExecuteInstantTheSameOperationTwice() public { + // Initialize contract that will reenter Governance on executing operation. + ReenterGovernance reenterGovernance = new ReenterGovernance(); + // Grant owner and security council access for reenterGovernance. + stdstore.target(address(governance)).sig(governance.owner.selector).checked_write(address(reenterGovernance)); + stdstore.target(address(governance)).sig(governance.securityCouncil.selector).checked_write( + address(reenterGovernance) + ); + + // Schedule operation, that will reenter `Governance.executeInstant` for the same op. + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(reenterGovernance), 0, ""); + reenterGovernance.initialize(governance, op, ReenterGovernance.FunctionToCall.ExecuteInstant); + + vm.startPrank(address(reenterGovernance)); + + governance.scheduleTransparent(op, 0); + vm.expectRevert("Operation must be pending after execution"); + governance.executeInstant(op); + } + + function test_RevertedWith_ExecuteOperationThatWillCancelItself() public { + // Initialize contract that will reenter Governance on executing operation. + ReenterGovernance reenterGovernance = new ReenterGovernance(); + // Grant owner access for reenterGovernance. + stdstore.target(address(governance)).sig(governance.owner.selector).checked_write(address(reenterGovernance)); + + // Schedule operation, that will reenter `Governance.execute` for the same op. + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(reenterGovernance), 0, ""); + reenterGovernance.initialize(governance, op, ReenterGovernance.FunctionToCall.Cancel); + + vm.startPrank(address(reenterGovernance)); + + governance.scheduleTransparent(op, 0); + vm.expectRevert("Operation must be ready after execution"); + governance.execute(op); + } + + function test_RevertedWith_ExecuteInstantOperationThatWillCancelItself() public { + // Initialize contract that will reenter Governance on executing operation. + ReenterGovernance reenterGovernance = new ReenterGovernance(); + // Grant owner and security council access for reenterGovernance. + stdstore.target(address(governance)).sig(governance.owner.selector).checked_write(address(reenterGovernance)); + stdstore.target(address(governance)).sig(governance.securityCouncil.selector).checked_write( + address(reenterGovernance) + ); + + // Schedule operation, that will reenter `Governance.executeInstant` for the same op. + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(reenterGovernance), 0, ""); + reenterGovernance.initialize(governance, op, ReenterGovernance.FunctionToCall.Cancel); + + vm.startPrank(address(reenterGovernance)); + + governance.scheduleTransparent(op, 0); + vm.expectRevert("Operation must be pending after execution"); + governance.executeInstant(op); + } +} From e6af6db265fad59cc476d37722350a7e982ae5bf Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sun, 1 Oct 2023 23:56:09 +0200 Subject: [PATCH 09/13] Add testcases for receive/fallback functions --- .../unit/concrete/Governance/Fallback.t.sol | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 ethereum/test/foundry/unit/concrete/Governance/Fallback.t.sol diff --git a/ethereum/test/foundry/unit/concrete/Governance/Fallback.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Fallback.t.sol new file mode 100644 index 000000000..532805ace --- /dev/null +++ b/ethereum/test/foundry/unit/concrete/Governance/Fallback.t.sol @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.17; + +import {GovernanceTest} from "./_Governance_Shared.t.sol"; + +contract ExecutingTest is GovernanceTest { + function test_SendEtherToGovernance() public { + startHoax(randomSigner); + payable(address(governance)).transfer(100); + } + + function test_RevertWhen_CallWithRandomData() public { + startHoax(randomSigner); + (bool success, ) = address(governance).call{value: 100}("11223344"); + assertFalse(success); + } +} From 98441cc4b8e0894a5d3ced645ae54e32db6cf5d3 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sun, 1 Oct 2023 23:56:48 +0200 Subject: [PATCH 10/13] 100% Governance coverage + refactoring --- .../dev-contracts/test/ReenterGovernance.sol | 57 ++++++++++++++ ethereum/contracts/governance/Governance.sol | 3 +- .../concrete/Governance/Authorization.t.sol | 28 +++---- .../unit/concrete/Governance/Executing.t.sol | 76 +++++++++++++++---- .../Governance/_Governance_Shared.t.sol | 6 ++ 5 files changed, 139 insertions(+), 31 deletions(-) create mode 100644 ethereum/contracts/dev-contracts/test/ReenterGovernance.sol diff --git a/ethereum/contracts/dev-contracts/test/ReenterGovernance.sol b/ethereum/contracts/dev-contracts/test/ReenterGovernance.sol new file mode 100644 index 000000000..9f25054c9 --- /dev/null +++ b/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"); + } + } + } +} diff --git a/ethereum/contracts/governance/Governance.sol b/ethereum/contracts/governance/Governance.sol index ccc60ba9c..b702cab42 100644 --- a/ethereum/contracts/governance/Governance.sol +++ b/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); } diff --git a/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol index c52bade12..867c72969 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/Authorization.t.sol @@ -7,90 +7,90 @@ import {IGovernance} from "../../../../../cache/solpp-generated-contracts/govern contract Authorization is GovernanceTest { function test_RevertWhen_SchedulingByUnauthorisedAddress() public { vm.prank(randomSigner); - vm.expectRevert(bytes("Ownable: caller is not the owner")); + 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(bytes("Ownable: caller is not the owner")); + 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(bytes("Ownable: caller is not the owner")); + vm.expectRevert("Ownable: caller is not the owner"); governance.scheduleShadow(bytes32(0), 0); } function test_RevertWhen_SchedulingShadowBySecurityCouncil() public { vm.prank(securityCouncil); - vm.expectRevert(bytes("Ownable: caller is not the owner")); + vm.expectRevert("Ownable: caller is not the owner"); governance.scheduleShadow(bytes32(0), 0); } function test_RevertWhen_ExecutingByUnauthorisedAddress() public { vm.prank(randomSigner); - vm.expectRevert(bytes("Only the owner and security council are allowed to call this function")); + 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(bytes("Only security council allowed to call this function")); + 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(bytes("Only security council allowed to call this function")); + 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(bytes("Only the owner and security council are allowed to call this function")); + 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(bytes("Only governance contract itself allowed to call this function")); + vm.expectRevert("Only governance contract itself allowed to call this function"); governance.updateDelay(0); } function test_RevertWhen_UpdateDelayByOwner() public { vm.prank(owner); - vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + vm.expectRevert("Only governance contract itself allowed to call this function"); governance.updateDelay(0); } function test_RevertWhen_UpdateDelayBySecurityCouncil() public { vm.prank(securityCouncil); - vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + vm.expectRevert("Only governance contract itself allowed to call this function"); governance.updateDelay(0); } function test_RevertWhen_UpdateSecurityCouncilByUnauthorisedAddress() public { vm.prank(randomSigner); - vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + 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(bytes("Only governance contract itself allowed to call this function")); + vm.expectRevert("Only governance contract itself allowed to call this function"); governance.updateSecurityCouncil(address(0)); } function test_RevertWhen_UpdateSecurityCouncilBySecurityCouncil() public { vm.prank(owner); - vm.expectRevert(bytes("Only governance contract itself allowed to call this function")); + vm.expectRevert("Only governance contract itself allowed to call this function"); governance.updateSecurityCouncil(address(0)); } } diff --git a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol index 994b2e113..ac7257e76 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol @@ -1,11 +1,14 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.17; +import {StdStorage, stdStorage} from "forge-std/Test.sol"; import {GovernanceTest} from "./_Governance_Shared.t.sol"; import {Utils} from "../Utils/Utils.sol"; import {IGovernance} from "../../../../../cache/solpp-generated-contracts/governance/IGovernance.sol"; contract ExecutingTest is GovernanceTest { + using stdStorage for StdStorage; + function test_ScheduleTransparentAndExecute() public { vm.startPrank(owner); @@ -46,7 +49,7 @@ contract ExecutingTest is GovernanceTest { vm.startPrank(owner); IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(eventOnFallback), 0, ""); governance.scheduleTransparent(op, 10000); - vm.expectRevert(bytes("Operation must be ready before execution")); + vm.expectRevert("Operation must be ready before execution"); governance.execute(op); } @@ -60,7 +63,7 @@ contract ExecutingTest is GovernanceTest { governance.scheduleTransparent(validOp, 0); IGovernance.Operation memory invalidOp = operationWithOneCallZeroSaltAndPredecessor(address(0), 0, ""); - vm.expectRevert(bytes("Operation must be ready before execution")); + vm.expectRevert("Operation must be ready before execution"); governance.execute(invalidOp); } @@ -78,7 +81,7 @@ contract ExecutingTest is GovernanceTest { 1, "" ); - vm.expectRevert(bytes("Operation must be ready before execution")); + vm.expectRevert("Operation must be ready before execution"); governance.execute(invalidOp); } @@ -96,7 +99,7 @@ contract ExecutingTest is GovernanceTest { 0, "00" ); - vm.expectRevert(bytes("Operation must be ready before execution")); + vm.expectRevert("Operation must be ready before execution"); governance.execute(invalidOp); } @@ -129,7 +132,7 @@ contract ExecutingTest is GovernanceTest { invalidOp.predecessor = governance.hashOperation(executedOp); // Failed to execute operation that wasn't scheduled - vm.expectRevert(bytes("Operation must be ready before execution")); + vm.expectRevert("Operation must be ready before execution"); governance.execute(invalidOp); } @@ -148,7 +151,7 @@ contract ExecutingTest is GovernanceTest { "" ); invalidOp.salt = Utils.randomBytes32("wrongSalt"); - vm.expectRevert(bytes("Operation must be ready before execution")); + vm.expectRevert("Operation must be ready before execution"); governance.execute(invalidOp); } @@ -162,7 +165,7 @@ contract ExecutingTest is GovernanceTest { ); invalidOp.predecessor = Utils.randomBytes32("randomPredecessor"); governance.scheduleTransparent(invalidOp, 0); - vm.expectRevert(bytes("Predecessor operation not completed")); + vm.expectRevert("Predecessor operation not completed"); governance.execute(invalidOp); } @@ -177,10 +180,34 @@ contract ExecutingTest is GovernanceTest { governance.scheduleTransparent(op, 0); executeOpAndCheck(op); - vm.expectRevert(bytes("Operation must be ready before execution")); + vm.expectRevert("Operation must be ready before execution"); governance.execute(op); } + function test_RevertWhen_ExecutingNonScheduledOperation() public { + vm.startPrank(owner); + + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "1122" + ); + vm.expectRevert("Operation must be ready before execution"); + governance.execute(op); + } + + function test_RevertWhen_ExecutingInstantNonScheduledOperation() public { + vm.startPrank(securityCouncil); + + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( + address(eventOnFallback), + 0, + "1122" + ); + vm.expectRevert("Operation must be pending before execution"); + governance.executeInstant(op); + } + function test_RevertWhen_ExecutingOperationAfterCanceling() public { vm.startPrank(owner); @@ -191,7 +218,7 @@ contract ExecutingTest is GovernanceTest { ); governance.scheduleTransparent(op, 0); governance.cancel(governance.hashOperation(op)); - vm.expectRevert(bytes("Operation must be ready before execution")); + vm.expectRevert("Operation must be ready before execution"); governance.execute(op); } @@ -219,20 +246,39 @@ contract ExecutingTest is GovernanceTest { ); governance.scheduleTransparent(op, 0); executeOpAndCheck(op); - vm.expectRevert(bytes("Operation with this proposal id already exists")); + vm.expectRevert("Operation with this proposal id already exists"); governance.scheduleTransparent(op, 0); } function test_RevertWhen_ExecutingOperationFailed() public { vm.startPrank(owner); - IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor( - address(revertFallback), - 0, - "" - ); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(revertFallback), 0, ""); governance.scheduleTransparent(op, 0); vm.expectRevert(bytes("")); governance.execute(op); } + + function test_CancelExistingOperation() public { + vm.startPrank(owner); + + governance.scheduleShadow(bytes32(0), 0); + governance.cancel(bytes32(0)); + } + + function test_RevertWhen_CancelNonExistingOperation() public { + vm.startPrank(owner); + + vm.expectRevert("Operation must be pending"); + governance.cancel(bytes32(0)); + } + + function test_RevertWhen_ScheduleOperationWithDelayLessThanMinimumOne() public { + vm.startPrank(owner); + stdstore.target(address(governance)).sig(governance.minDelay.selector).checked_write(1000); + IGovernance.Operation memory op = operationWithOneCallZeroSaltAndPredecessor(address(revertFallback), 0, ""); + + vm.expectRevert("Proposed delay is less than minimum delay"); + governance.scheduleTransparent(op, 0); + } } diff --git a/ethereum/test/foundry/unit/concrete/Governance/_Governance_Shared.t.sol b/ethereum/test/foundry/unit/concrete/Governance/_Governance_Shared.t.sol index 04bee2358..6b072b02c 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/_Governance_Shared.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/_Governance_Shared.t.sol @@ -6,6 +6,8 @@ import {Test} from "forge-std/Test.sol"; import {Governance} from "../../../../../cache/solpp-generated-contracts/governance/Governance.sol"; import {IGovernance} from "../../../../../cache/solpp-generated-contracts/governance/IGovernance.sol"; import {EventOnFallback} from "../../../../../cache/solpp-generated-contracts/dev-contracts/EventOnFallback.sol"; +import {Forwarder} from "../../../../../cache/solpp-generated-contracts/dev-contracts/Forwarder.sol"; +import {RevertFallback} from "../../../../../cache/solpp-generated-contracts/dev-contracts/RevertFallback.sol"; contract GovernanceTest is Test, EventOnFallback { address internal owner; @@ -13,6 +15,8 @@ contract GovernanceTest is Test, EventOnFallback { address internal randomSigner; Governance internal governance; EventOnFallback internal eventOnFallback; + Forwarder internal forwarder; + RevertFallback internal revertFallback; constructor() { owner = makeAddr("owner"); @@ -21,6 +25,8 @@ contract GovernanceTest is Test, EventOnFallback { governance = new Governance(owner, securityCouncil, 0); eventOnFallback = new EventOnFallback(); + forwarder = new Forwarder(); + revertFallback = new RevertFallback(); } function setUp() external { From 38de92280cfb2a406834932ca0efdd6a11203810 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sun, 1 Oct 2023 23:57:05 +0200 Subject: [PATCH 11/13] Fix warning in Validator timelock --- ethereum/contracts/zksync/ValidatorTimelock.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/contracts/zksync/ValidatorTimelock.sol b/ethereum/contracts/zksync/ValidatorTimelock.sol index 71222228c..69e56561a 100644 --- a/ethereum/contracts/zksync/ValidatorTimelock.sol +++ b/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; From f6956df356029c76c8b5892943134413ae12e1d2 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Sun, 1 Oct 2023 23:57:24 +0200 Subject: [PATCH 12/13] Descrease number or allowed warnings --- ethereum/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/package.json b/ethereum/package.json index 5f06dc1f5..4f0a45650 100644 --- a/ethereum/package.json +++ b/ethereum/package.json @@ -54,7 +54,7 @@ "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", From bf1d435a5011526839a63da75d70a8083619b22a Mon Sep 17 00:00:00 2001 From: vladbochok Date: Mon, 2 Oct 2023 00:06:11 +0200 Subject: [PATCH 13/13] Fix review --- ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol index 4986e7f90..3db473607 100644 --- a/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol +++ b/ethereum/test/foundry/unit/concrete/Governance/Executing.t.sol @@ -114,14 +114,13 @@ contract ExecutingTest is GovernanceTest { governance.scheduleTransparent(executedOp, 0); executeOpAndCheck(executedOp); - // Schedule & execute operation with 0 predecessor + // Schedule operation with 0 predecessor IGovernance.Operation memory validOp = operationWithOneCallZeroSaltAndPredecessor( address(eventOnFallback), 0, "" ); governance.scheduleTransparent(validOp, 0); - executeOpAndCheck(validOp); // Schedule operation with predecessor of `executedOp` operation IGovernance.Operation memory invalidOp = operationWithOneCallZeroSaltAndPredecessor(