diff --git a/packages/contracts-bedrock/scripts/deploy/DeployOwnership.s.sol b/packages/contracts-bedrock/scripts/deploy/DeployOwnership.s.sol index ebbc244e9c3..d340380e96f 100644 --- a/packages/contracts-bedrock/scripts/deploy/DeployOwnership.s.sol +++ b/packages/contracts-bedrock/scripts/deploy/DeployOwnership.s.sol @@ -324,14 +324,14 @@ contract DeployOwnership is Deploy { removeDeployerFromSafe({ _name: "SecurityCouncilSafe", _newThreshold: exampleCouncilConfig.safeConfig.threshold }); // Verify the module was configured correctly - (uint256 configuredPeriod, address configuredFallback) = - LivenessModule2(livenessModule).livenessSafeConfiguration(address(safe)); + LivenessModule2.ModuleConfig memory verifyConfig = + LivenessModule2(livenessModule).livenessSafeConfiguration(safe); require( - configuredPeriod == exampleCouncilConfig.livenessModuleConfig.livenessInterval, + verifyConfig.livenessResponsePeriod == exampleCouncilConfig.livenessModuleConfig.livenessInterval, "DeployOwnership: configured liveness interval must match expected value" ); require( - configuredFallback == exampleCouncilConfig.livenessModuleConfig.fallbackOwner, + verifyConfig.fallbackOwner == exampleCouncilConfig.livenessModuleConfig.fallbackOwner, "DeployOwnership: configured fallback owner must match expected value" ); diff --git a/packages/contracts-bedrock/snapshots/abi/SaferSafes.json b/packages/contracts-bedrock/snapshots/abi/SaferSafes.json index 0360375a66c..fa401d9f82a 100644 --- a/packages/contracts-bedrock/snapshots/abi/SaferSafes.json +++ b/packages/contracts-bedrock/snapshots/abi/SaferSafes.json @@ -49,7 +49,7 @@ { "inputs": [ { - "internalType": "address", + "internalType": "contract GnosisSafe", "name": "_safe", "type": "address" } @@ -62,7 +62,7 @@ { "inputs": [ { - "internalType": "address", + "internalType": "contract GnosisSafe", "name": "", "type": "address" } @@ -81,7 +81,7 @@ { "inputs": [ { - "internalType": "address", + "internalType": "contract GnosisSafe", "name": "_safe", "type": "address" } @@ -227,7 +227,7 @@ { "inputs": [ { - "internalType": "address", + "internalType": "contract GnosisSafe", "name": "_safe", "type": "address" } @@ -246,22 +246,29 @@ { "inputs": [ { - "internalType": "address", - "name": "", + "internalType": "contract GnosisSafe", + "name": "_safe", "type": "address" } ], "name": "livenessSafeConfiguration", "outputs": [ { - "internalType": "uint256", - "name": "livenessResponsePeriod", - "type": "uint256" - }, - { - "internalType": "address", - "name": "fallbackOwner", - "type": "address" + "components": [ + { + "internalType": "uint256", + "name": "livenessResponsePeriod", + "type": "uint256" + }, + { + "internalType": "address", + "name": "fallbackOwner", + "type": "address" + } + ], + "internalType": "struct LivenessModule2.ModuleConfig", + "name": "", + "type": "tuple" } ], "stateMutability": "view", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 92f1fe46624..ffcf038b244 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -208,8 +208,8 @@ "sourceCodeHash": "0x950725f8b9ad9bb3b6b5e836f67e18db824a7864bac547ee0eeba88ada3de0e9" }, "src/safe/SaferSafes.sol:SaferSafes": { - "initCodeHash": "0xde571369d6823ac315c03a862df84fa9fc0e9a7e58fd6227c5513d06ffdc6ba1", - "sourceCodeHash": "0x9d76fa66c0bd1261ab3f6c32892a0942ce4c10a5dfa169c012985dd43ea1cdde" + "initCodeHash": "0xaa17bb150c9bcf19675a33e9762b050148aceae9f6a9a6ba020fc6947ebaab39", + "sourceCodeHash": "0xc4201612048ff051ed795521efa3eece1a6556f2c514a268b180d84a2ad8b2d1" }, "src/universal/OptimismMintableERC20.sol:OptimismMintableERC20": { "initCodeHash": "0x3c85eed0d017dca8eda6396aa842ddc12492587b061e8c756a8d32c4610a9658", diff --git a/packages/contracts-bedrock/snapshots/storageLayout/SaferSafes.json b/packages/contracts-bedrock/snapshots/storageLayout/SaferSafes.json index 17b7d237e9f..c8fa41d7a3c 100644 --- a/packages/contracts-bedrock/snapshots/storageLayout/SaferSafes.json +++ b/packages/contracts-bedrock/snapshots/storageLayout/SaferSafes.json @@ -1,17 +1,17 @@ [ { "bytes": "32", - "label": "livenessSafeConfiguration", + "label": "_livenessSafeConfiguration", "offset": 0, "slot": "0", - "type": "mapping(address => struct LivenessModule2.ModuleConfig)" + "type": "mapping(contract GnosisSafe => struct LivenessModule2.ModuleConfig)" }, { "bytes": "32", "label": "challengeStartTime", "offset": 0, "slot": "1", - "type": "mapping(address => uint256)" + "type": "mapping(contract GnosisSafe => uint256)" }, { "bytes": "32", diff --git a/packages/contracts-bedrock/src/safe/LivenessModule2.sol b/packages/contracts-bedrock/src/safe/LivenessModule2.sol index 1feb5595689..fe024bc757d 100644 --- a/packages/contracts-bedrock/src/safe/LivenessModule2.sol +++ b/packages/contracts-bedrock/src/safe/LivenessModule2.sol @@ -27,10 +27,10 @@ abstract contract LivenessModule2 { } /// @notice Mapping from Safe address to its configuration. - mapping(address => ModuleConfig) public livenessSafeConfiguration; + mapping(Safe => ModuleConfig) internal _livenessSafeConfiguration; /// @notice Mapping from Safe address to active challenge start time (0 if none). - mapping(address => uint256) public challengeStartTime; + mapping(Safe => uint256) public challengeStartTime; /// @notice Reserved address used as previous owner to the first owner in a Safe. address internal constant SENTINEL_OWNER = address(0x1); @@ -95,23 +95,40 @@ abstract contract LivenessModule2 { /// @param fallbackOwner The address that claimed ownership if the Safe is unresponsive. event ChallengeSucceeded(address indexed safe, address fallbackOwner); + //////////////////////////////////////////////////////////////// + // External View Functions // + //////////////////////////////////////////////////////////////// + /// @notice Returns challenge_start_time + liveness_response_period if challenge exists, or /// 0 if not. /// @param _safe The Safe address to query. /// @return The challenge end timestamp, or 0 if no challenge. - function getChallengePeriodEnd(address _safe) public view returns (uint256) { + function getChallengePeriodEnd(Safe _safe) public view returns (uint256) { uint256 startTime = challengeStartTime[_safe]; if (startTime == 0) { return 0; } - ModuleConfig storage config = livenessSafeConfiguration[_safe]; + ModuleConfig storage config = _livenessSafeConfiguration[_safe]; return startTime + config.livenessResponsePeriod; } + /// @notice Returns the configuration for a given Safe. + /// @param _safe The Safe address to query. + /// @return The ModuleConfig for the Safe. + function livenessSafeConfiguration(Safe _safe) public view returns (ModuleConfig memory) { + return _livenessSafeConfiguration[_safe]; + } + + //////////////////////////////////////////////////////////////// + // External State-Changing Functions // + //////////////////////////////////////////////////////////////// + /// @notice Configures the module for a Safe that has already enabled it. /// @param _config The configuration parameters for the module containing the response /// period and fallback owner. function configureLivenessModule(ModuleConfig memory _config) external { + Safe callingSafe = Safe(payable(msg.sender)); + // Validate configuration parameters to ensure module can function properly. // livenessResponsePeriod must be > 0 to allow time for Safe owners to respond. if (_config.livenessResponsePeriod == 0) { @@ -123,10 +140,10 @@ abstract contract LivenessModule2 { } // Check that this module is enabled on the calling Safe. - _assertModuleEnabled(msg.sender); + _assertModuleEnabled(callingSafe); // Store the configuration for this safe - livenessSafeConfiguration[msg.sender] = _config; + _livenessSafeConfiguration[callingSafe] = _config; // Clear any existing challenge when configuring/re-configuring. // This is necessary because changing the configuration (especially @@ -137,19 +154,14 @@ abstract contract LivenessModule2 { // Additionally, a Safe that is able to successfully trigger the configuration function // is necessarily live, so cancelling the challenge also makes sense from a // theoretical standpoint. - _cancelChallenge(msg.sender); + _cancelChallenge(callingSafe); emit ModuleConfigured(msg.sender, _config.livenessResponsePeriod, _config.fallbackOwner); // Verify that any other extensions which are enabled on the Safe are configured correctly. - _checkCombinedConfig(Safe(payable(msg.sender))); + _checkCombinedConfig(callingSafe); } - /// @notice Internal helper function which can be overriden in a child contract to check if the guard's - /// configuration is valid in the context of other extensions that are enabled on the Safe. - /// @param _safe The Safe instance to check the configuration against - function _checkCombinedConfig(Safe _safe) internal view virtual; - /// @notice Clears the module configuration for a Safe. /// @dev Note: Clearing the configuration also cancels any ongoing challenges. /// This function is intended for use when a Safe wants to permanently remove @@ -160,23 +172,25 @@ abstract contract LivenessModule2 { /// Never calling clearLivenessModule() after disabling keeps configuration data persistent /// for potential future re-enabling. function clearLivenessModule() external { + Safe callingSafe = Safe(payable(msg.sender)); + // Check if the calling safe has configuration set - _assertModuleConfigured(msg.sender); + _assertModuleConfigured(callingSafe); // Check that this module is NOT enabled on the calling Safe // This prevents clearing configuration while module is still enabled - _assertModuleNotEnabled(msg.sender); + _assertModuleNotEnabled(callingSafe); // Erase the configuration data for this safe - delete livenessSafeConfiguration[msg.sender]; + delete _livenessSafeConfiguration[callingSafe]; // Also clear any active challenge - _cancelChallenge(msg.sender); - emit ModuleCleared(msg.sender); + _cancelChallenge(callingSafe); + emit ModuleCleared(address(callingSafe)); } /// @notice Challenges an enabled safe. /// @param _safe The Safe address to challenge. - function challenge(address _safe) external { + function challenge(Safe _safe) external { // Check if the calling safe has configuration set _assertModuleConfigured(_safe); @@ -184,7 +198,7 @@ abstract contract LivenessModule2 { _assertModuleEnabled(_safe); // Check that the caller is the fallback owner - if (msg.sender != livenessSafeConfiguration[_safe].fallbackOwner) { + if (msg.sender != _livenessSafeConfiguration[_safe].fallbackOwner) { revert LivenessModule2_UnauthorizedCaller(); } @@ -195,26 +209,28 @@ abstract contract LivenessModule2 { // Set the challenge start time and emit the event challengeStartTime[_safe] = block.timestamp; - emit ChallengeStarted(_safe, block.timestamp); + emit ChallengeStarted(address(_safe), block.timestamp); } /// @notice Responds to a challenge for an enabled safe, canceling it. function respond() external { + Safe callingSafe = Safe(payable(msg.sender)); + // Check if the calling safe has configuration set. - _assertModuleConfigured(msg.sender); + _assertModuleConfigured(callingSafe); // Check that this module is enabled on the calling Safe. - _assertModuleEnabled(msg.sender); + _assertModuleEnabled(callingSafe); // Check that a challenge exists - uint256 startTime = challengeStartTime[msg.sender]; + uint256 startTime = challengeStartTime[callingSafe]; if (startTime == 0) { revert LivenessModule2_ChallengeDoesNotExist(); } // Cancel the challenge without checking if response period has expired // This allows the Safe to respond at any time, providing more flexibility - _cancelChallenge(msg.sender); + _cancelChallenge(callingSafe); } /// @notice With successful challenge, removes all current owners from enabled safe, @@ -224,7 +240,7 @@ abstract contract LivenessModule2 { /// fallback owner effectively becomes its own fallback owner, maintaining /// the ability to challenge itself if needed. /// @param _safe The Safe address to transfer ownership of. - function changeOwnershipToFallback(address _safe) external { + function changeOwnershipToFallback(Safe _safe) external { // Ensure Safe is configured with this module to prevent unauthorized execution. _assertModuleConfigured(_safe); @@ -232,7 +248,7 @@ abstract contract LivenessModule2 { _assertModuleEnabled(_safe); // Only fallback owner can execute ownership transfer (per specs update) - if (msg.sender != livenessSafeConfiguration[_safe].fallbackOwner) { + if (msg.sender != _livenessSafeConfiguration[_safe].fallbackOwner) { revert LivenessModule2_UnauthorizedCaller(); } @@ -248,61 +264,67 @@ abstract contract LivenessModule2 { revert LivenessModule2_ResponsePeriodActive(); } - Safe targetSafe = Safe(payable(_safe)); - // Get current owners - address[] memory owners = targetSafe.getOwners(); + address[] memory owners = _safe.getOwners(); // Remove all owners after the first one // Note: This loop is safe as real-world Safes have limited owners (typically < 10) // Gas limits would only be a concern with hundreds/thousands of owners while (owners.length > 1) { - targetSafe.execTransactionFromModule({ - to: _safe, + _safe.execTransactionFromModule({ + to: address(_safe), value: 0, operation: Enum.Operation.Call, data: abi.encodeCall(OwnerManager.removeOwner, (SENTINEL_OWNER, owners[0], 1)) }); - owners = targetSafe.getOwners(); + owners = _safe.getOwners(); } // Now swap the remaining single owner with the fallback owner - targetSafe.execTransactionFromModule({ - to: _safe, + _safe.execTransactionFromModule({ + to: address(_safe), value: 0, operation: Enum.Operation.Call, data: abi.encodeCall( - OwnerManager.swapOwner, (SENTINEL_OWNER, owners[0], livenessSafeConfiguration[_safe].fallbackOwner) + OwnerManager.swapOwner, (SENTINEL_OWNER, owners[0], _livenessSafeConfiguration[_safe].fallbackOwner) ) }); // Sanity check: verify the fallback owner is now the only owner - address[] memory finalOwners = targetSafe.getOwners(); - if (finalOwners.length != 1 || finalOwners[0] != livenessSafeConfiguration[_safe].fallbackOwner) { + address[] memory finalOwners = _safe.getOwners(); + if (finalOwners.length != 1 || finalOwners[0] != _livenessSafeConfiguration[_safe].fallbackOwner) { revert LivenessModule2_OwnershipTransferFailed(); } // Reset the challenge state to allow a new challenge delete challengeStartTime[_safe]; - emit ChallengeSucceeded(_safe, livenessSafeConfiguration[_safe].fallbackOwner); // Disable the guard // Note that this will remove whichever guard is currently set on the Safe, // even if it is not the SaferSafes guard. This is intentional, as it is possible that the guard // itself was the cause of the liveness failure which resulted in the transfer of ownership to // the fallback owner. - targetSafe.execTransactionFromModule({ - to: _safe, + _safe.execTransactionFromModule({ + to: address(_safe), value: 0, operation: Enum.Operation.Call, data: abi.encodeCall(GuardManager.setGuard, (address(0))) }); + emit ChallengeSucceeded(address(_safe), _livenessSafeConfiguration[_safe].fallbackOwner); } + //////////////////////////////////////////////////////////////// + // Internal View Functions // + //////////////////////////////////////////////////////////////// + + /// @notice Internal helper function which can be overriden in a child contract to check if the guard's + /// configuration is valid in the context of other extensions that are enabled on the Safe. + function _checkCombinedConfig(Safe _safe) internal view virtual; + /// @notice Asserts that the module is configured for the given Safe. /// @param _safe The Safe address to check. - function _assertModuleConfigured(address _safe) internal view { - ModuleConfig storage config = livenessSafeConfiguration[_safe]; + function _assertModuleConfigured(Safe _safe) internal view { + ModuleConfig storage config = _livenessSafeConfiguration[_safe]; if (config.fallbackOwner == address(0)) { revert LivenessModule2_ModuleNotConfigured(); } @@ -310,29 +332,31 @@ abstract contract LivenessModule2 { /// @notice Asserts that the module is enabled for the given Safe. /// @param _safe The Safe address to check. - function _assertModuleEnabled(address _safe) internal view { - Safe safe = Safe(payable(_safe)); - if (!safe.isModuleEnabled(address(this))) { + function _assertModuleEnabled(Safe _safe) internal view { + if (!_safe.isModuleEnabled(address(this))) { revert LivenessModule2_ModuleNotEnabled(); } } /// @notice Asserts that the module is not enabled for the given Safe. /// @param _safe The Safe address to check. - function _assertModuleNotEnabled(address _safe) internal view { - Safe safe = Safe(payable(_safe)); - if (safe.isModuleEnabled(address(this))) { + function _assertModuleNotEnabled(Safe _safe) internal view { + if (_safe.isModuleEnabled(address(this))) { revert LivenessModule2_ModuleStillEnabled(); } } + //////////////////////////////////////////////////////////////// + // Internal State-Changing Functions // + //////////////////////////////////////////////////////////////// + /// @notice Internal function to cancel a challenge and emit the appropriate event. /// @param _safe The Safe address for which to cancel the challenge. - function _cancelChallenge(address _safe) internal { + function _cancelChallenge(Safe _safe) internal { // Early return if no challenge exists if (challengeStartTime[_safe] == 0) return; delete challengeStartTime[_safe]; - emit ChallengeCancelled(_safe); + emit ChallengeCancelled(address(_safe)); } } diff --git a/packages/contracts-bedrock/src/safe/SaferSafes.sol b/packages/contracts-bedrock/src/safe/SaferSafes.sol index a70615ba2bc..52ea97df94e 100644 --- a/packages/contracts-bedrock/src/safe/SaferSafes.sol +++ b/packages/contracts-bedrock/src/safe/SaferSafes.sol @@ -39,7 +39,7 @@ contract SaferSafes is LivenessModule2, TimelockGuard, ISemver { } uint256 timelockDelay = _safeState[_safe].timelockDelay; - uint256 livenessResponsePeriod = livenessSafeConfiguration[address(_safe)].livenessResponsePeriod; + uint256 livenessResponsePeriod = _livenessSafeConfiguration[_safe].livenessResponsePeriod; // If the timelock delay is 0, then the timelock guard is enabled but not configured. // No delay is applied to transactions, so we don't need to perform any further checks. diff --git a/packages/contracts-bedrock/src/safe/TimelockGuard.sol b/packages/contracts-bedrock/src/safe/TimelockGuard.sol index c38c1222f1f..2276cbdcf99 100644 --- a/packages/contracts-bedrock/src/safe/TimelockGuard.sol +++ b/packages/contracts-bedrock/src/safe/TimelockGuard.sol @@ -224,11 +224,6 @@ abstract contract TimelockGuard is IGuard { /// @param _safe The Safe address to query /// @return The current cancellation threshold function cancellationThreshold(Safe _safe) public view returns (uint256) { - // Return 0 if guard is not enabled - if (!_isGuardEnabled(_safe)) { - return 0; - } - return _safeState[_safe].cancellationThreshold; } @@ -254,7 +249,7 @@ abstract contract TimelockGuard is IGuard { /// @notice Returns the timelock delay for a given Safe /// @param _safe The Safe address to query /// @return The timelock delay in seconds - function timelockConfiguration(Safe _safe) public view returns (uint256) { + function timelockDelay(Safe _safe) public view returns (uint256) { return _safeState[_safe].timelockDelay; } diff --git a/packages/contracts-bedrock/test/safe/LivenessModule2.t.sol b/packages/contracts-bedrock/test/safe/LivenessModule2.t.sol index c45781b8b58..24680065089 100644 --- a/packages/contracts-bedrock/test/safe/LivenessModule2.t.sol +++ b/packages/contracts-bedrock/test/safe/LivenessModule2.t.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.15; import { Test } from "forge-std/Test.sol"; import { Enum } from "safe-contracts/common/Enum.sol"; +import { GnosisSafe as Safe } from "safe-contracts/GnosisSafe.sol"; import "test/safe-tools/SafeTestTools.sol"; import { Constants } from "src/libraries/Constants.sol"; @@ -116,10 +117,10 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni _enableModule(safeInstance, CHALLENGE_PERIOD, fallbackOwner); - (uint256 period, address fbOwner) = livenessModule2.livenessSafeConfiguration(address(safeInstance.safe)); - assertEq(period, CHALLENGE_PERIOD); - assertEq(fbOwner, fallbackOwner); - assertEq(livenessModule2.challengeStartTime(address(safeInstance.safe)), 0); + LivenessModule2.ModuleConfig memory config = livenessModule2.livenessSafeConfiguration(safeInstance.safe); + assertEq(config.livenessResponsePeriod, CHALLENGE_PERIOD); + assertEq(config.fallbackOwner, fallbackOwner); + assertEq(livenessModule2.challengeStartTime(safeInstance.safe), 0); } function test_configureLivenessModule_multipleSafes_succeeds() external { @@ -146,17 +147,17 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni _enableModule(safe3, 3 days, fallback3); // Verify each safe has independent configuration - (uint256 period1, address fb1) = livenessModule2.livenessSafeConfiguration(address(safe1.safe)); - assertEq(period1, 1 days); - assertEq(fb1, fallback1); + LivenessModule2.ModuleConfig memory config1 = livenessModule2.livenessSafeConfiguration(safe1.safe); + assertEq(config1.livenessResponsePeriod, 1 days); + assertEq(config1.fallbackOwner, fallback1); - (uint256 period2, address fb2) = livenessModule2.livenessSafeConfiguration(address(safe2.safe)); - assertEq(period2, 2 days); - assertEq(fb2, fallback2); + LivenessModule2.ModuleConfig memory config2 = livenessModule2.livenessSafeConfiguration(safe2.safe); + assertEq(config2.livenessResponsePeriod, 2 days); + assertEq(config2.fallbackOwner, fallback2); - (uint256 period3, address fb3) = livenessModule2.livenessSafeConfiguration(address(safe3.safe)); - assertEq(period3, 3 days); - assertEq(fb3, fallback3); + LivenessModule2.ModuleConfig memory config3 = livenessModule2.livenessSafeConfiguration(safe3.safe); + assertEq(config3.livenessResponsePeriod, 3 days); + assertEq(config3.fallbackOwner, fallback3); } function test_configureLivenessModule_requiresSafeModuleInstallation_reverts() external { @@ -197,10 +198,10 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni // Start a challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); // Verify challenge exists - uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)); + uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe); assertGt(challengeEndTime, 0); // Reconfigure the module, which should cancel the challenge and emit ChallengeCancelled @@ -215,7 +216,7 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni ); // Verify challenge was cancelled - challengeEndTime = livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)); + challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe); assertEq(challengeEndTime, 0); } @@ -243,9 +244,9 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni Enum.Operation.Call ); - (uint256 period, address fbOwner) = livenessModule2.livenessSafeConfiguration(address(safeInstance.safe)); - assertEq(period, 0); - assertEq(fbOwner, address(0)); + LivenessModule2.ModuleConfig memory clearedConfig = livenessModule2.livenessSafeConfiguration(safeInstance.safe); + assertEq(clearedConfig.livenessResponsePeriod, 0); + assertEq(clearedConfig.fallbackOwner, address(0)); } function test_clear_notEnabled_reverts() external { @@ -277,9 +278,9 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { emit ChallengeStarted(address(safeInstance.safe), block.timestamp); vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); - uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)); + uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe); assertEq(challengeEndTime, block.timestamp + CHALLENGE_PERIOD); } @@ -288,7 +289,7 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { vm.expectRevert(LivenessModule2.LivenessModule2_UnauthorizedCaller.selector); vm.prank(notFallback); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); } function test_challenge_moduleNotEnabled_reverts() external { @@ -296,16 +297,16 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { vm.expectRevert(LivenessModule2.LivenessModule2_ModuleNotConfigured.selector); vm.prank(fallbackOwner); - livenessModule2.challenge(newSafe); + livenessModule2.challenge(Safe(payable(newSafe))); } function test_challenge_alreadyExists_reverts() external { vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); vm.expectRevert(LivenessModule2.LivenessModule2_ChallengeAlreadyExists.selector); vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); } function test_challenge_moduleDisabledAtSafeLevel_reverts() external { @@ -331,13 +332,13 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { // Try to challenge - should revert because module is disabled at Safe level vm.expectRevert(LivenessModule2.LivenessModule2_ModuleNotEnabled.selector); vm.prank(fallbackOwner); - livenessModule2.challenge(address(disabledSafe.safe)); + livenessModule2.challenge(disabledSafe.safe); } function test_respond_succeeds() external { // Start a challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); // Cancel it vm.expectEmit(true, true, true, true); @@ -346,7 +347,7 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { _respondToChallenge(safeInstance); // Verify challenge is cancelled - uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)); + uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe); assertEq(challengeEndTime, 0); } @@ -376,7 +377,7 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { function test_respond_afterResponsePeriod_succeeds() external { // Start a challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); // Warp past challenge period vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); @@ -388,7 +389,7 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { livenessModule2.respond(); // Verify challenge was cancelled - assertEq(livenessModule2.challengeStartTime(address(safeInstance.safe)), 0); + assertEq(livenessModule2.challengeStartTime(safeInstance.safe), 0); } function test_respond_moduleNotConfigured_reverts() external { @@ -423,9 +424,10 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { ); // Verify the Safe still has configuration but module is not enabled - (uint256 period, address fbOwner) = livenessModule2.livenessSafeConfiguration(address(configuredSafe.safe)); - assertTrue(period > 0); // Configuration exists - assertTrue(fbOwner != address(0)); // Configuration exists + LivenessModule2.ModuleConfig memory configuredConfig = + livenessModule2.livenessSafeConfiguration(configuredSafe.safe); + assertTrue(configuredConfig.livenessResponsePeriod > 0); // Configuration exists + assertTrue(configuredConfig.fallbackOwner != address(0)); // Configuration exists assertFalse(configuredSafe.safe.isModuleEnabled(address(livenessModule2))); // Module not enabled // Now respond() should revert because module is not enabled @@ -435,37 +437,178 @@ contract LivenessModule2_Challenge_Test is LivenessModule2_TestInit { } } +/// @title LivenessModule2_ChangeOwnershipToFallback_Test +/// @notice Tests the ownership transfer after successful challenge +contract LivenessModule2_ChangeOwnershipToFallback_Test is LivenessModule2_TestInit { + function setUp() public override { + super.setUp(); + _enableModule(safeInstance, CHALLENGE_PERIOD, fallbackOwner); + } + + /// @notice Tests that ownership successfully transfers to fallback owner after challenge period expires + function testFuzz_changeOwnershipToFallback_succeeds(uint256 timeAfterExpiry) external { + // Bound time to reasonable values (1 second to 365 days after expiry) + timeAfterExpiry = bound(timeAfterExpiry, 1, 365 days); + + // Start a challenge + vm.prank(fallbackOwner); + livenessModule2.challenge(safeInstance.safe); + + // Warp past challenge period + vm.warp(block.timestamp + CHALLENGE_PERIOD + timeAfterExpiry); + + // Execute ownership transfer + vm.expectEmit(true, true, true, true); + emit ChallengeSucceeded(address(safeInstance.safe), fallbackOwner); + + vm.prank(fallbackOwner); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); + + // Verify ownership changed + address[] memory newOwners = safeInstance.safe.getOwners(); + assertEq(newOwners.length, 1); + assertEq(newOwners[0], fallbackOwner); + assertEq(safeInstance.safe.getThreshold(), 1); + + // Verify challenge is reset + uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe); + assertEq(challengeEndTime, 0); + } + + /// @notice Tests that changeOwnershipToFallback reverts if module is not configured + function test_changeOwnershipToFallback_moduleNotEnabled_reverts() external { + address newSafe = makeAddr("newSafe"); + + vm.prank(fallbackOwner); + vm.expectRevert(LivenessModule2.LivenessModule2_ModuleNotConfigured.selector); + livenessModule2.changeOwnershipToFallback(Safe(payable(newSafe))); + } + + /// @notice Tests that changeOwnershipToFallback reverts if no challenge exists + function test_changeOwnershipToFallback_noChallenge_reverts() external { + vm.prank(fallbackOwner); + vm.expectRevert(LivenessModule2.LivenessModule2_ChallengeDoesNotExist.selector); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); + } + + /// @notice Tests that changeOwnershipToFallback reverts if called before response period expires + function testFuzz_changeOwnershipToFallback_beforeResponsePeriod_reverts(uint256 timeElapsed) external { + // Bound time to be within response period (not yet expired) + timeElapsed = bound(timeElapsed, 0, CHALLENGE_PERIOD); + + // Start a challenge + vm.prank(fallbackOwner); + livenessModule2.challenge(safeInstance.safe); + + // Warp to a time before response period expires + vm.warp(block.timestamp + timeElapsed); + + // Try to execute before response period expires + vm.prank(fallbackOwner); + vm.expectRevert(LivenessModule2.LivenessModule2_ResponsePeriodActive.selector); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); + } + + /// @notice Tests that changeOwnershipToFallback reverts if module is disabled at Safe level + function test_changeOwnershipToFallback_moduleDisabledAtSafeLevel_reverts() external { + // Start a challenge + vm.prank(fallbackOwner); + livenessModule2.challenge(safeInstance.safe); + + // Warp past challenge period + vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); + + // Disable the module at Safe level + SafeTestLib.execTransaction( + safeInstance, + address(safeInstance.safe), + 0, + abi.encodeCall(ModuleManager.disableModule, (address(0x1), address(livenessModule2))), + Enum.Operation.Call + ); + + // Try to execute ownership transfer - should revert because module is disabled at Safe level + vm.prank(fallbackOwner); + vm.expectRevert(LivenessModule2.LivenessModule2_ModuleNotEnabled.selector); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); + } + + /// @notice Tests that only the fallback owner can execute changeOwnershipToFallback + function testFuzz_changeOwnershipToFallback_onlyFallbackOwner_succeeds(address _caller) external { + vm.assume(_caller != fallbackOwner); + + // Start a challenge + vm.prank(fallbackOwner); + livenessModule2.challenge(safeInstance.safe); + + // Warp past challenge period + vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); + + // Try from non-fallback owner - should fail + vm.prank(_caller); + vm.expectRevert(LivenessModule2.LivenessModule2_UnauthorizedCaller.selector); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); + + // Execute from fallback owner - should succeed + vm.prank(fallbackOwner); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); + + // Verify ownership changed + address[] memory newOwners = safeInstance.safe.getOwners(); + assertEq(newOwners.length, 1); + assertEq(newOwners[0], fallbackOwner); + } + + /// @notice Tests that a new challenge can be started after ownership transfer completes + function test_changeOwnershipToFallback_canRechallenge_succeeds() external { + // Start and execute first challenge + vm.prank(fallbackOwner); + livenessModule2.challenge(safeInstance.safe); + + vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); + vm.prank(fallbackOwner); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); + + // Start a new challenge (as fallback owner) + vm.prank(fallbackOwner); + livenessModule2.challenge(safeInstance.safe); + + uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe); + assertGt(challengeEndTime, 0); + } +} + /// @title LivenessModule2_GetChallengePeriodEnd_Test /// @notice Tests the getChallengePeriodEnd function and related view functionality contract LivenessModule2_GetChallengePeriodEnd_Test is LivenessModule2_TestInit { function test_safeConfigs_succeeds() external { // Before enabling - (uint256 period1, address fbOwner1) = livenessModule2.livenessSafeConfiguration(address(safeInstance.safe)); - assertEq(period1, 0); - assertEq(fbOwner1, address(0)); - assertEq(livenessModule2.challengeStartTime(address(safeInstance.safe)), 0); + LivenessModule2.ModuleConfig memory configBefore = livenessModule2.livenessSafeConfiguration(safeInstance.safe); + assertEq(configBefore.livenessResponsePeriod, 0); + assertEq(configBefore.fallbackOwner, address(0)); + assertEq(livenessModule2.challengeStartTime(safeInstance.safe), 0); // After enabling _enableModule(safeInstance, CHALLENGE_PERIOD, fallbackOwner); - (uint256 period2, address fbOwner2) = livenessModule2.livenessSafeConfiguration(address(safeInstance.safe)); - assertEq(period2, CHALLENGE_PERIOD); - assertEq(fbOwner2, fallbackOwner); - assertEq(livenessModule2.challengeStartTime(address(safeInstance.safe)), 0); + LivenessModule2.ModuleConfig memory configAfter = livenessModule2.livenessSafeConfiguration(safeInstance.safe); + assertEq(configAfter.livenessResponsePeriod, CHALLENGE_PERIOD); + assertEq(configAfter.fallbackOwner, fallbackOwner); + assertEq(livenessModule2.challengeStartTime(safeInstance.safe), 0); } function test_getChallengePeriodEnd_succeeds() external { _enableModule(safeInstance, CHALLENGE_PERIOD, fallbackOwner); // No challenge - assertEq(livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)), 0); + assertEq(livenessModule2.getChallengePeriodEnd(safeInstance.safe), 0); // With challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); - assertEq(livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)), block.timestamp + CHALLENGE_PERIOD); + livenessModule2.challenge(safeInstance.safe); + assertEq(livenessModule2.getChallengePeriodEnd(safeInstance.safe), block.timestamp + CHALLENGE_PERIOD); // After cancellation _respondToChallenge(safeInstance); - assertEq(livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)), 0); + assertEq(livenessModule2.getChallengePeriodEnd(safeInstance.safe), 0); } } diff --git a/packages/contracts-bedrock/test/safe/SaferSafes.t.sol b/packages/contracts-bedrock/test/safe/SaferSafes.t.sol index 5caa088827b..36b9532d40a 100644 --- a/packages/contracts-bedrock/test/safe/SaferSafes.t.sol +++ b/packages/contracts-bedrock/test/safe/SaferSafes.t.sol @@ -85,11 +85,10 @@ contract SaferSafes_Uncategorized_Test is SaferSafes_TestInit { saferSafes.configureTimelockGuard(timelockDelay); // Verify configurations were set - (uint256 storedLivenessResponsePeriod, address storedFallbackOwner) = - saferSafes.livenessSafeConfiguration(address(safeInstance.safe)); - assertEq(storedLivenessResponsePeriod, livenessResponsePeriod); - assertEq(storedFallbackOwner, fallbackOwner); - assertEq(saferSafes.timelockConfiguration(safeInstance.safe), timelockDelay); + LivenessModule2.ModuleConfig memory storedConfig = saferSafes.livenessSafeConfiguration(safeInstance.safe); + assertEq(storedConfig.livenessResponsePeriod, livenessResponsePeriod); + assertEq(storedConfig.fallbackOwner, fallbackOwner); + assertEq(saferSafes.timelockDelay(safeInstance.safe), timelockDelay); } function test_configure_timelockGuardFirst_succeeds() public { @@ -110,11 +109,10 @@ contract SaferSafes_Uncategorized_Test is SaferSafes_TestInit { saferSafes.configureLivenessModule(moduleConfig); // Verify configurations were set - (uint256 storedLivenessResponsePeriod, address storedFallbackOwner) = - saferSafes.livenessSafeConfiguration(address(safeInstance.safe)); - assertEq(storedLivenessResponsePeriod, livenessResponsePeriod); - assertEq(storedFallbackOwner, fallbackOwner); - assertEq(saferSafes.timelockConfiguration(safeInstance.safe), timelockDelay); + LivenessModule2.ModuleConfig memory storedConfig = saferSafes.livenessSafeConfiguration(safeInstance.safe); + assertEq(storedConfig.livenessResponsePeriod, livenessResponsePeriod); + assertEq(storedConfig.fallbackOwner, fallbackOwner); + assertEq(saferSafes.timelockDelay(safeInstance.safe), timelockDelay); } /// @notice Test that attempting to incorrectly configure the timelock guard after first configuring the liveness @@ -185,7 +183,7 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { assertEq(safeInstance.safe.getThreshold(), 1); // Verify challenge is reset - uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)); + uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe); assertEq(challengeEndTime, 0); // Verify guard is deactivated @@ -195,7 +193,7 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { function test_changeOwnershipToFallback_succeeds() external { // Start a challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); // Warp past challenge period vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); @@ -205,7 +203,7 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { emit ChallengeSucceeded(address(safeInstance.safe), fallbackOwner); vm.prank(fallbackOwner); - livenessModule2.changeOwnershipToFallback(address(safeInstance.safe)); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); _assertOwnershipChanged(); } @@ -235,7 +233,7 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { // Start a challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); // Warp past challenge period vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); @@ -245,7 +243,7 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { emit ChallengeSucceeded(address(safeInstance.safe), fallbackOwner); vm.prank(fallbackOwner); - livenessModule2.changeOwnershipToFallback(address(safeInstance.safe)); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); // These checks include ensuring that the guard is deactivated _assertOwnershipChanged(); @@ -256,30 +254,30 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { vm.prank(fallbackOwner); vm.expectRevert(LivenessModule2.LivenessModule2_ModuleNotConfigured.selector); - livenessModule2.changeOwnershipToFallback(newSafe); + livenessModule2.changeOwnershipToFallback(Safe(payable(newSafe))); } function test_changeOwnershipToFallback_noChallenge_reverts() external { vm.prank(fallbackOwner); vm.expectRevert(LivenessModule2.LivenessModule2_ChallengeDoesNotExist.selector); - livenessModule2.changeOwnershipToFallback(address(safeInstance.safe)); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); } function test_changeOwnershipToFallback_beforeResponsePeriod_reverts() external { // Start a challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); // Try to execute before response period expires vm.prank(fallbackOwner); vm.expectRevert(LivenessModule2.LivenessModule2_ResponsePeriodActive.selector); - livenessModule2.changeOwnershipToFallback(address(safeInstance.safe)); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); } function test_changeOwnershipToFallback_moduleDisabledAtSafeLevel_reverts() external { // Start a challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); // Warp past challenge period vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); @@ -296,14 +294,14 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { // Try to execute ownership transfer - should revert because module is disabled at Safe level vm.prank(fallbackOwner); vm.expectRevert(LivenessModule2.LivenessModule2_ModuleNotEnabled.selector); - livenessModule2.changeOwnershipToFallback(address(safeInstance.safe)); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); } function test_changeOwnershipToFallback_onlyFallbackOwner_succeeds() external { _enableModule(safeInstance, CHALLENGE_PERIOD, fallbackOwner); // Start a challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); // Warp past challenge period vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); @@ -312,11 +310,11 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { address randomCaller = makeAddr("randomCaller"); vm.prank(randomCaller); vm.expectRevert(LivenessModule2.LivenessModule2_UnauthorizedCaller.selector); - livenessModule2.changeOwnershipToFallback(address(safeInstance.safe)); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); // Execute from fallback owner - should succeed vm.prank(fallbackOwner); - livenessModule2.changeOwnershipToFallback(address(safeInstance.safe)); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); // Verify ownership changed address[] memory newOwners = safeInstance.safe.getOwners(); @@ -328,11 +326,11 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { _enableModule(safeInstance, CHALLENGE_PERIOD, fallbackOwner); // Start and execute first challenge vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); vm.warp(block.timestamp + CHALLENGE_PERIOD + 1); vm.prank(fallbackOwner); - livenessModule2.changeOwnershipToFallback(address(safeInstance.safe)); + livenessModule2.changeOwnershipToFallback(safeInstance.safe); // Re-configure the module vm.prank(address(safeInstance.safe)); @@ -342,9 +340,9 @@ contract SaferSafes_ChangeOwnershipToFallback_Test is SaferSafes_TestInit { // Start a new challenge (as fallback owner) vm.prank(fallbackOwner); - livenessModule2.challenge(address(safeInstance.safe)); + livenessModule2.challenge(safeInstance.safe); - uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(address(safeInstance.safe)); + uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe); assertGt(challengeEndTime, 0); } } diff --git a/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol b/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol index 2dcefe87d41..7e21f251295 100644 --- a/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol +++ b/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol @@ -265,7 +265,7 @@ abstract contract TimelockGuard_TestInit is Test, SafeTestTools { contract TimelockGuard_TimelockConfiguration_Test is TimelockGuard_TestInit { /// @notice Ensures an unconfigured Safe reports a zero timelock delay. function test_timelockConfiguration_returnsZeroForUnconfiguredSafe_succeeds() external view { - uint256 delay = timelockGuard.timelockConfiguration(safeInstance.safe); + uint256 delay = timelockGuard.timelockDelay(safeInstance.safe); assertEq(delay, 0); // configured is now determined by timelockDelay == 0 assertEq(delay == 0, true); @@ -274,7 +274,7 @@ contract TimelockGuard_TimelockConfiguration_Test is TimelockGuard_TestInit { /// @notice Validates the configuration view reflects the stored timelock delay. function test_timelockConfiguration_returnsConfigurationForConfiguredSafe_succeeds() external { _configureGuard(safeInstance, TIMELOCK_DELAY); - uint256 delay = timelockGuard.timelockConfiguration(safeInstance.safe); + uint256 delay = timelockGuard.timelockDelay(safeInstance.safe); assertEq(delay, TIMELOCK_DELAY); // configured is now determined by timelockDelay != 0 assertEq(delay != 0, true); @@ -291,7 +291,7 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit { _configureGuard(safeInstance, TIMELOCK_DELAY); - uint256 delay = timelockGuard.timelockConfiguration(safe); + uint256 delay = timelockGuard.timelockDelay(safe); assertEq(delay, TIMELOCK_DELAY); // configured is now determined by timelockDelay != 0 assertEq(delay != 0, true); @@ -322,7 +322,7 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit { _configureGuard(safeInstance, ONE_YEAR); - uint256 delay = timelockGuard.timelockConfiguration(safe); + uint256 delay = timelockGuard.timelockDelay(safe); assertEq(delay, ONE_YEAR); // configured is now determined by timelockDelay != 0 assertEq(delay != 0, true); @@ -332,7 +332,7 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit { function test_configureTimelockGuard_allowsReconfiguration_succeeds() external { // Initial configuration _configureGuard(safeInstance, TIMELOCK_DELAY); - assertEq(timelockGuard.timelockConfiguration(safe), TIMELOCK_DELAY); + assertEq(timelockGuard.timelockDelay(safe), TIMELOCK_DELAY); uint256 newDelay = TIMELOCK_DELAY + 1; @@ -350,14 +350,14 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit { emit GuardConfigured(safe, newDelay); _configureGuard(safeInstance, newDelay); - assertEq(timelockGuard.timelockConfiguration(safe), newDelay); + assertEq(timelockGuard.timelockDelay(safe), newDelay); } /// @notice Ensures setting delay to zero clears the configuration. function test_configureTimelockGuard_clearConfiguration_succeeds() external { // First configure the guard _configureGuard(safeInstance, TIMELOCK_DELAY); - assertEq(timelockGuard.timelockConfiguration(safe), TIMELOCK_DELAY); + assertEq(timelockGuard.timelockDelay(safe), TIMELOCK_DELAY); // Configure timelock delay to 0 should succeed and emit event vm.expectEmit(true, true, true, true); @@ -366,7 +366,7 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit { timelockGuard.configureTimelockGuard(0); // Timelock delay should be set to 0 - assertEq(timelockGuard.timelockConfiguration(safe), 0); + assertEq(timelockGuard.timelockDelay(safe), 0); } /// @notice Checks clearing succeeds even if the guard was never configured. @@ -382,12 +382,6 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit { /// @title TimelockGuard_CancellationThreshold_Test /// @notice Tests for cancellationThreshold function contract TimelockGuard_CancellationThreshold_Test is TimelockGuard_TestInit { - /// @notice Validates cancellation threshold is zero when the guard is disabled. - function test_cancellationThreshold_returnsZeroIfGuardNotEnabled_succeeds() external view { - uint256 threshold = timelockGuard.cancellationThreshold(Safe(payable(unguardedSafe.safe))); - assertEq(threshold, 0); - } - /// @notice Ensures an enabled but unconfigured guard yields a zero threshold. function test_cancellationThreshold_returnsZeroIfGuardNotConfigured_succeeds() external view { // Safe with guard enabled but not configured should return 0 @@ -433,7 +427,7 @@ contract TimelockGuard_ScheduleTransaction_Test is TimelockGuard_TestInit { function test_scheduleTransaction_guardNotConfigured_reverts() external { // Enable the guard on the unguarded Safe, but don't configure it _enableGuard(unguardedSafe); - assertEq(timelockGuard.timelockConfiguration(unguardedSafe.safe), 0); + assertEq(timelockGuard.timelockDelay(unguardedSafe.safe), 0); TransactionBuilder.Transaction memory dummyTx = _createDummyTransaction(unguardedSafe); vm.expectRevert(TimelockGuard.TimelockGuard_GuardNotConfigured.selector); @@ -935,7 +929,7 @@ contract TimelockGuard_ClearTimelockGuard_Test is TimelockGuard_TestInit { ); // Verify configuration is cleared - assertEq(timelockGuard.timelockConfiguration(safe), 0); + assertEq(timelockGuard.timelockDelay(safe), 0); assertEq(timelockGuard.cancellationThreshold(safe), 0); // Verify pending transaction was cancelled @@ -972,7 +966,7 @@ contract TimelockGuard_ClearTimelockGuard_Test is TimelockGuard_TestInit { assertEq(timelockGuard.pendingTransactions(Safe(payable(address(safeInstance.safe)))).length, 50); // Verify configuration is cleared - assertEq(timelockGuard.timelockConfiguration(safe), 0); + assertEq(timelockGuard.timelockDelay(safe), 0); assertEq(timelockGuard.cancellationThreshold(safe), 0); } diff --git a/packages/contracts-bedrock/test/scripts/DeployOwnership.t.sol b/packages/contracts-bedrock/test/scripts/DeployOwnership.t.sol index 42ef9288d8e..56ed0f177ba 100644 --- a/packages/contracts-bedrock/test/scripts/DeployOwnership.t.sol +++ b/packages/contracts-bedrock/test/scripts/DeployOwnership.t.sol @@ -67,12 +67,12 @@ contract DeployOwnershipTest is Test, DeployOwnership { // LivenessModule2 checks LivenessModuleConfig memory lmConfig = exampleSecurityCouncilConfig.livenessModuleConfig; - (uint256 configuredPeriod, address configuredFallback) = - LivenessModule2(livenessModule).livenessSafeConfiguration(address(securityCouncilSafe)); - assertEq(configuredPeriod, lmConfig.livenessInterval); - assertEq(configuredFallback, lmConfig.fallbackOwner); + LivenessModule2.ModuleConfig memory moduleConfig = + LivenessModule2(livenessModule).livenessSafeConfiguration(Safe(payable(securityCouncilSafe))); + assertEq(moduleConfig.livenessResponsePeriod, lmConfig.livenessInterval); + assertEq(moduleConfig.fallbackOwner, lmConfig.fallbackOwner); // Verify no active challenge exists initially - assertEq(LivenessModule2(livenessModule).getChallengePeriodEnd(address(securityCouncilSafe)), 0); + assertEq(LivenessModule2(livenessModule).getChallengePeriodEnd(Safe(payable(securityCouncilSafe))), 0); } }