diff --git a/packages/contracts-bedrock/scripts/DeployOwnership.s.sol b/packages/contracts-bedrock/scripts/DeployOwnership.s.sol index d6859b8bd03f0..a82fbbe4e0c6f 100644 --- a/packages/contracts-bedrock/scripts/DeployOwnership.s.sol +++ b/packages/contracts-bedrock/scripts/DeployOwnership.s.sol @@ -186,6 +186,14 @@ contract DeployOwnership is Deploy { _callViaSafe({ _safe: safe, _target: address(safe), _data: abi.encodeCall(GuardManager.setGuard, (guard)) }); console.log("LivenessGuard setup on SecurityCouncilSafe"); + // Deploy and add the Liveness Module. + address livenessModule = deployLivenessModule(); + _callViaSafe({ + _safe: safe, + _target: address(safe), + _data: abi.encodeCall(ModuleManager.enableModule, (livenessModule)) + }); + // Remove the deployer address (msg.sender) which was used to setup the Security Council Safe thus far // this call is also used to update the threshold. // Because deploySafe() always adds msg.sender first (if keepDeployer is true), we know that the previousOwner @@ -197,17 +205,11 @@ contract DeployOwnership is Deploy { OwnerManager.removeOwner, (SENTINEL_OWNERS, msg.sender, exampleCouncilConfig.safeConfig.threshold) ) }); - - // Deploy and add the Liveness Module. - address livenessModule = deployLivenessModule(); - vm.stopBroadcast(); - - // Since we don't have private keys for the safe owners, we instead use 'startBroadcast' to do something - // similar to pranking as the safe. This simulates a quorum of signers executing a transation from the safe to - // call it's own 'enableModule' method. - vm.startBroadcast(address(safe)); - safe.enableModule(livenessModule); - vm.stopBroadcast(); + address[] memory owners = safe.getOwners(); + require( + safe.getThreshold() == LivenessModule(livenessModule).getRequiredThreshold(owners.length), + "Safe threshold must be equal to the LivenessModule's required threshold" + ); addr_ = address(safe); console.log("Deployed and configured the Security Council Safe!"); } diff --git a/packages/contracts-bedrock/semver-lock.json b/packages/contracts-bedrock/semver-lock.json index 4325677bc9761..31548140d4dd1 100644 --- a/packages/contracts-bedrock/semver-lock.json +++ b/packages/contracts-bedrock/semver-lock.json @@ -112,8 +112,8 @@ "sourceCodeHash": "0xea3872d8f196ae3c863363dfa4b57803cb2a24b0c100244d8f861891e901e03f" }, "src/Safe/LivenessModule.sol": { - "initCodeHash": "0xa8b233f0f26f8a73b997b12ba06d64cefa8ee98d523f68cd63320e9787468fae", - "sourceCodeHash": "0x15dfd32e92577f4cb5ab05def834a5a1b183e30ca249184f282fca6441be8788" + "initCodeHash": "0x6c3d47fdce3dcd5b8499da08061f3ee4ddd126b726bedec601210667bf73db1d", + "sourceCodeHash": "0x6bdfa1a9fe8a7bc6e032c2448b911ca3bd55ece0856866042e29a20e270dd0a3" }, "src/cannon/MIPS.sol": { "initCodeHash": "0xa5d36fc67170ad87322f358f612695f642757bbf5280800d5d878da21402579a", diff --git a/packages/contracts-bedrock/src/Safe/LivenessModule.sol b/packages/contracts-bedrock/src/Safe/LivenessModule.sol index e881506da17f1..3621dbf5f4c26 100644 --- a/packages/contracts-bedrock/src/Safe/LivenessModule.sol +++ b/packages/contracts-bedrock/src/Safe/LivenessModule.sol @@ -67,10 +67,6 @@ contract LivenessModule is ISemver { MIN_OWNERS = _minOwners; address[] memory owners = _safe.getOwners(); require(_minOwners <= owners.length, "LivenessModule: minOwners must be less than the number of owners"); - require( - _safe.getThreshold() >= getRequiredThreshold(owners.length), - "LivenessModule: Insufficient threshold for the number of owners" - ); require(_thresholdPercentage > 0, "LivenessModule: thresholdPercentage must be greater than 0"); require(_thresholdPercentage <= 100, "LivenessModule: thresholdPercentage must be less than or equal to 100"); } diff --git a/packages/contracts-bedrock/test/Safe/DeployOwnership.t.sol b/packages/contracts-bedrock/test/Safe/DeployOwnership.t.sol index 0665ea227f667..9c046c0e096c2 100644 --- a/packages/contracts-bedrock/test/Safe/DeployOwnership.t.sol +++ b/packages/contracts-bedrock/test/Safe/DeployOwnership.t.sol @@ -88,5 +88,8 @@ contract DeployOwnershipTest is Test, DeployOwnership { assertEq(LivenessModule(livenessModule).livenessInterval(), lmConfig.livenessInterval); assertEq(LivenessModule(livenessModule).thresholdPercentage(), lmConfig.thresholdPercentage); assertEq(LivenessModule(livenessModule).minOwners(), lmConfig.minOwners); + + // Ensure the threshold on the safe agrees with the LivenessModule's required threshold + assertEq(securityCouncilSafe.getThreshold(), LivenessModule(livenessModule).getRequiredThreshold(owners.length)); } } diff --git a/packages/contracts-bedrock/test/Safe/LivenessGuard.t.sol b/packages/contracts-bedrock/test/Safe/LivenessGuard.t.sol index 00b4c8b39e2fe..b8d67506da6df 100644 --- a/packages/contracts-bedrock/test/Safe/LivenessGuard.t.sol +++ b/packages/contracts-bedrock/test/Safe/LivenessGuard.t.sol @@ -28,14 +28,20 @@ contract LivenessGuard_TestInit is Test, SafeTestTools { event OwnerRecorded(address owner); - uint256 initTime = 10; WrappedGuard livenessGuard; SafeInstance safeInstance; + // This needs to be non-zero so that the `lastLive` mapping can record non-zero timestamps + uint256 initTime = 10; + // These values reflect the planned state of the mainnet Security Council Safe. + uint256 threshold = 10; + uint256 ownerCount = 13; + /// @dev Sets up the test environment function setUp() public { vm.warp(initTime); - safeInstance = _setupSafe(); + (, uint256[] memory privKeys) = SafeTestLib.makeAddrsAndKeys("test-owners", ownerCount); + safeInstance = _setupSafe(privKeys, threshold); livenessGuard = new WrappedGuard(safeInstance.safe); safeInstance.setGuard(address(livenessGuard)); } @@ -88,8 +94,10 @@ contract LivenessGuard_CheckTx_Test is LivenessGuard_TestInit { // Create an array of the addresses who will sign the transaction. SafeTestTools // will generate these signatures up to the threshold by iterating over the owners array. address[] memory signers = new address[](safeInstance.threshold); - signers[0] = safeInstance.owners[0]; - signers[1] = safeInstance.owners[1]; + // copy the first threshold owners into the signers array + for (uint256 i; i < safeInstance.threshold; i++) { + signers[i] = safeInstance.owners[i]; + } // Record the timestamps before the transaction uint256[] memory beforeTimestamps = new uint256[](safeInstance.owners.length); diff --git a/packages/contracts-bedrock/test/Safe/LivenessModule.t.sol b/packages/contracts-bedrock/test/Safe/LivenessModule.t.sol index fb36791abb19c..ecbc591ca7119 100644 --- a/packages/contracts-bedrock/test/Safe/LivenessModule.t.sol +++ b/packages/contracts-bedrock/test/Safe/LivenessModule.t.sol @@ -101,23 +101,6 @@ contract LivenessModule_Constructor_TestFail is LivenessModule_TestInit { _fallbackOwner: address(0) }); } - - /// @dev Tests that the constructor fails if the minOwners is greater than the number of owners - function test_constructor_wrongThreshold_reverts() external { - uint256 wrongThreshold = livenessModule.getRequiredThreshold(safeInstance.owners.length) - 1; - vm.mockCall( - address(safeInstance.safe), abi.encodeCall(OwnerManager.getThreshold, ()), abi.encode(wrongThreshold) - ); - vm.expectRevert("LivenessModule: Insufficient threshold for the number of owners"); - new LivenessModule({ - _safe: safeInstance.safe, - _livenessGuard: livenessGuard, - _livenessInterval: LIVENESS_INTERVAL, - _thresholdPercentage: THRESHOLD_PERCENTAGE, - _minOwners: MIN_OWNERS, - _fallbackOwner: address(0) - }); - } } contract LivenessModule_Getters_Test is LivenessModule_TestInit {