Improve SaferSafes test suite.

packages/contracts-bedrock/test/safe/LivenessModule2.t.sol

@@ -10,6 +10,7 @@ import { Constants } from "src/libraries/Constants.sol";
 import { LivenessModule2 } from "src/safe/LivenessModule2.sol";
 import { SaferSafes } from "src/safe/SaferSafes.sol";
 import { ModuleManager } from "safe-contracts/base/ModuleManager.sol";
+import { GuardManager } from "safe-contracts/base/GuardManager.sol";
 
 /// @title LivenessModule2_TestUtils
 /// @notice Reusable helper methods for LivenessModule2 tests.
@@ -108,8 +109,8 @@ contract LivenessModule2_TestInit is LivenessModule2_TestUtils {
     }
 }
 
-/// @title LivenessModule2_Configure_Test
-/// @notice Tests configuring and clearing the module
+/// @title LivenessModule2_ConfigureLivenessModule_Test
+/// @notice Tests configuring the module
 contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestInit {
     function test_configureLivenessModule_succeeds() external {
         vm.expectEmit(true, true, true, true);
@@ -219,10 +220,19 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni
         challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe);
         assertEq(challengeEndTime, 0);
     }
+}
 
-    function test_clear_succeeds() external {
+/// @title LivenessModule2_ClearLivenessModule_Test
+/// @notice Tests clearing the module configuration
+contract LivenessModule2_ClearLivenessModule_Test is LivenessModule2_TestInit {
+    function test_clearLivenessModule_succeeds() external {
         _enableModule(safeInstance, CHALLENGE_PERIOD, fallbackOwner);
 
+        // Start a challenge to test that clearing also cancels it
+        vm.prank(fallbackOwner);
+        livenessModule2.challenge(safeInstance.safe);
+        assertGt(livenessModule2.challengeStartTime(safeInstance.safe), 0);
+
         // First disable the module at the Safe level
         SafeTestLib.execTransaction(
             safeInstance,
@@ -232,6 +242,9 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni
             Enum.Operation.Call
         );
 
+        // Clear should emit ChallengeCancelled then ModuleCleared
+        vm.expectEmit(true, true, true, true);
+        emit ChallengeCancelled(address(safeInstance.safe));
         vm.expectEmit(true, true, true, true);
         emit ModuleCleared(address(safeInstance.safe));
 
@@ -247,15 +260,16 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni
         LivenessModule2.ModuleConfig memory clearedConfig = livenessModule2.livenessSafeConfiguration(safeInstance.safe);
         assertEq(clearedConfig.livenessResponsePeriod, 0);
         assertEq(clearedConfig.fallbackOwner, address(0));
+        assertEq(livenessModule2.challengeStartTime(safeInstance.safe), 0);
     }
 
-    function test_clear_notEnabled_reverts() external {
+    function test_clearLivenessModule_notConfigured_reverts() external {
         vm.expectRevert(LivenessModule2.LivenessModule2_ModuleNotConfigured.selector);
         vm.prank(address(safeInstance.safe));
         livenessModule2.clearLivenessModule();
     }
 
-    function test_clear_moduleStillEnabled_reverts() external {
+    function test_clearLivenessModule_moduleStillEnabled_reverts() external {
         _enableModule(safeInstance, CHALLENGE_PERIOD, fallbackOwner);
 
         // Try to clear while module is still enabled (should revert)
@@ -450,6 +464,17 @@ contract LivenessModule2_ChangeOwnershipToFallback_Test is LivenessModule2_TestI
         // Bound time to reasonable values (1 second to 365 days after expiry)
         timeAfterExpiry = bound(timeAfterExpiry, 1, 365 days);
 
+        // Set a guard to verify it gets removed
+        address mockGuard = makeAddr("mockGuard");
+        SafeTestLib.execTransaction(
+            safeInstance,
+            address(safeInstance.safe),
+            0,
+            abi.encodeCall(GuardManager.setGuard, (mockGuard)),
+            Enum.Operation.Call
+        );
+        assertEq(_getGuard(safeInstance), mockGuard);
+
         // Start a challenge
         vm.prank(fallbackOwner);
         livenessModule2.challenge(safeInstance.safe);
@@ -473,6 +498,9 @@ contract LivenessModule2_ChangeOwnershipToFallback_Test is LivenessModule2_TestI
         // Verify challenge is reset
         uint256 challengeEndTime = livenessModule2.getChallengePeriodEnd(safeInstance.safe);
         assertEq(challengeEndTime, 0);
+
+        // Verify guard was removed
+        assertEq(_getGuard(safeInstance), address(0));
     }
 
     /// @notice Tests that changeOwnershipToFallback reverts if module is not configured

packages/contracts-bedrock/test/safe/SaferSafes.t.sol

@@ -2,6 +2,7 @@
 pragma solidity 0.8.15;
 
 import { Enum } from "safe-contracts/common/Enum.sol";
+import { GnosisSafe as Safe } from "safe-contracts/GnosisSafe.sol";
 import "test/safe-tools/SafeTestTools.sol";
 
 import { SaferSafes } from "src/safe/SaferSafes.sol";

packages/contracts-bedrock/test/safe/TimelockGuard.t.sol

@@ -261,49 +261,46 @@ abstract contract TimelockGuard_TestInit is Test, SafeTestTools {
 }
 
 /// @title TimelockGuard_TimelockDelay_Test
-/// @notice Tests for timelockConfiguration function
+/// @notice Tests for TimelockDelay function
 contract TimelockGuard_TimelockDelay_Test is TimelockGuard_TestInit {
     /// @notice Ensures an unconfigured Safe reports a zero timelock delay.
-    function test_timelockConfiguration_returnsZeroForUnconfiguredSafe_succeeds() external view {
+    function test_timelockDelay_returnsZeroForUnconfiguredSafe_succeeds() external view {
         uint256 delay = timelockGuard.timelockDelay(safeInstance.safe);
         assertEq(delay, 0);
-        // configured is now determined by timelockDelay == 0
-        assertEq(delay == 0, true);
     }
 
-    /// @notice Validates the configuration view reflects the stored timelock delay.
-    function test_timelockConfiguration_returnsConfigurationForConfiguredSafe_succeeds() external {
-        _configureGuard(safeInstance, TIMELOCK_DELAY);
-        uint256 delay = timelockGuard.timelockDelay(safeInstance.safe);
-        assertEq(delay, TIMELOCK_DELAY);
-        // configured is now determined by timelockDelay != 0
-        assertEq(delay != 0, true);
+    /// @notice Fuzz test: Validates the configuration view reflects the stored timelock delay for any valid delay.
+    function testFuzz_timelockDelay_returnsConfigurationForConfiguredSafe_succeeds(uint256 _delay_) external {
+        _delay_ = bound(_delay_, 1, ONE_YEAR); // Restrict to valid range
+        _configureGuard(safeInstance, _delay_);
+        uint256 delay_ = timelockGuard.timelockDelay(safeInstance.safe);
+        assertEq(delay_, _delay_);
     }
 }
 
 /// @title TimelockGuard_ConfigureTimelockGuard_Test
 /// @notice Tests for configureTimelockGuard function
 contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit {
-    /// @notice Verifies the guard can be configured with a standard delay.
-    function test_configureTimelockGuard_succeeds() external {
+    /// @notice Verifies the guard can be configured with various valid delays.
+    function testFuzz_configureTimelockGuard_validDelay_succeeds(uint256 _delay) external {
+        _delay = bound(_delay, 1, ONE_YEAR);
+
         vm.expectEmit(true, true, true, true);
-        emit GuardConfigured(safe, TIMELOCK_DELAY);
+        emit GuardConfigured(safe, _delay);
 
-        _configureGuard(safeInstance, TIMELOCK_DELAY);
+        _configureGuard(safeInstance, _delay);
 
         uint256 delay = timelockGuard.timelockDelay(safe);
-        assertEq(delay, TIMELOCK_DELAY);
-        // configured is now determined by timelockDelay != 0
-        assertEq(delay != 0, true);
+        assertEq(delay, _delay);
     }
 
     /// @notice Confirms delays above the maximum revert during configuration.
-    function test_configureTimelockGuard_revertsIfDelayTooLong_reverts() external {
-        uint256 tooLongDelay = ONE_YEAR + 1;
+    function testFuzz_configureTimelockGuard_delayTooLong_reverts(uint256 _delay) external {
+        _delay = bound(_delay, ONE_YEAR + 1, type(uint256).max);
 
         vm.expectRevert(TimelockGuard.TimelockGuard_InvalidTimelockDelay.selector);
         vm.prank(address(safeInstance.safe));
-        timelockGuard.configureTimelockGuard(tooLongDelay);
+        timelockGuard.configureTimelockGuard(_delay);
     }
 
     /// @notice Checks configuration reverts when the contract is too old.
@@ -324,8 +321,6 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit {
 
         uint256 delay = timelockGuard.timelockDelay(safe);
         assertEq(delay, ONE_YEAR);
-        // configured is now determined by timelockDelay != 0
-        assertEq(delay != 0, true);
     }
 
     /// @notice Demonstrates the guard can be reconfigured to a new delay.
@@ -733,6 +728,75 @@ contract TimelockGuard_CheckTransaction_Test is TimelockGuard_TestInit {
     }
 }
 
+/// @title TimelockGuard_CheckAfterExecution_Test
+/// @notice Tests for checkAfterExecution function
+contract TimelockGuard_CheckAfterExecution_Test is TimelockGuard_TestInit {
+    function setUp() public override {
+        super.setUp();
+        _configureGuard(safeInstance, TIMELOCK_DELAY);
+    }
+
+    /// @notice Verifies successful execution updates state and resets threshold.
+    function test_checkAfterExecution_successfulExecution_succeeds() external {
+        TransactionBuilder.Transaction memory dummyTx = _createDummyTransaction(safeInstance);
+        dummyTx.scheduleTransaction(timelockGuard);
+
+        uint256 expectedExecutionTime = block.timestamp + TIMELOCK_DELAY;
+        vm.warp(expectedExecutionTime);
+
+        // Verify initial cancellation threshold
+        uint256 initialThreshold = timelockGuard.cancellationThreshold(safeInstance.safe);
+        assertEq(initialThreshold, 1);
+
+        // Call checkAfterExecution with successful execution
+        vm.expectEmit(true, true, true, true);
+        emit TransactionExecuted(safeInstance.safe, dummyTx.hash);
+        vm.prank(address(safeInstance.safe));
+        timelockGuard.checkAfterExecution(dummyTx.hash, true);
+
+        // Verify transaction state changed to Executed
+        TimelockGuard.ScheduledTransaction memory scheduledTx =
+            timelockGuard.scheduledTransaction(safeInstance.safe, dummyTx.hash);
+        assertEq(uint256(scheduledTx.state), uint256(TimelockGuard.TransactionState.Executed));
+
+        // Verify transaction removed from pending list
+        TimelockGuard.ScheduledTransaction[] memory pending = timelockGuard.pendingTransactions(safeInstance.safe);
+        assertEq(pending.length, 0);
+
+        // Verify cancellation threshold was reset to 1
+        assertEq(timelockGuard.cancellationThreshold(safeInstance.safe), 1);
+
+        // Verify transaction cannot be executed again
+        vm.expectRevert(TimelockGuard.TimelockGuard_TransactionAlreadyExecuted.selector);
+        dummyTx.executeTransaction(safeInstance.owners[0]);
+    }
+
+    /// @notice Verifies transaction state remains unchanged on execution failure.
+    function test_checkAfterExecution_failedExecution_succeeds() external {
+        TransactionBuilder.Transaction memory dummyTx = _createDummyTransaction(safeInstance);
+        dummyTx.scheduleTransaction(timelockGuard);
+
+        uint256 expectedExecutionTime = block.timestamp + TIMELOCK_DELAY;
+        vm.warp(expectedExecutionTime);
+
+        // Call checkAfterExecution with failed execution
+        vm.prank(address(safeInstance.safe));
+        timelockGuard.checkAfterExecution(dummyTx.hash, false);
+
+        // Verify transaction state remains unchanged
+        TimelockGuard.ScheduledTransaction memory scheduledTx =
+            timelockGuard.scheduledTransaction(safeInstance.safe, dummyTx.hash);
+        assertEq(uint256(scheduledTx.state), uint256(TimelockGuard.TransactionState.Pending));
+        assertEq(uint256(scheduledTx.executionTime), expectedExecutionTime);
+    }
+
+    /// @notice Fuzz test: Verifies unconfigured guard allows checkAfterExecution for any _hash.
+    function testFuzz_checkAfterExecution_unconfiguredGuard_succeeds(bytes32 _hash) external {
+        vm.prank(address(unguardedSafe.safe));
+        timelockGuard.checkAfterExecution(_hash, true);
+    }
+}
+
 /// @title TimelockGuard_MaxCancellationThreshold_Test
 /// @notice Tests for the maxCancellationThreshold function in TimelockGuard
 contract TimelockGuard_MaxCancellationThreshold_Test is TimelockGuard_TestInit {