Improve SaferSafes test suite. (#17949)

maurelian · graphite-app[bot] · claude · web-flow · commit 4e57f1e91b2b · 2025-10-21T18:28:19.000Z
* Add SaferSafes as child of the module and guard * Add ISaferSafes * Test comment and assertion fixes * Improve comments * Make LivenessModule2 and TimelockGuard abstract Move semver to SaferSafes semver lock * fix test contract name * Move semver to SaferSafes * Disable the guard and module upon ownership transfer * Add _disableThisGuard function * Update tests * Add config resets * fmt * fix test_changeOwnershipToFallback_canRechallenge_succeeds * Simplify by clearing config directly * Put _disableThisGuard into child contract * Add timelockDelay reset on _disableThisGuard * semver-lock * Move _disableThisGuard logic into TimelockGuard * clear livenessSafeConfig at tend of _disableThisModule * Clarify use of SENTINEL_OWNER * Fix the ordering of the disableGuard and disableModule calls * semver-lock * remove unused imports * rename _disableThisGuard to _disableGuard * bump semver * Add test to remove unrelated guard * Add SENTINEL_MODULE constant * Clean up using ternary if * Reset cancellationThreshold to 0 on changeOwnership * Fix moduleFound if/else handling * Clear pending transactions * Pre-pr fixes * Add test contract to test name lint exclusions * fix name of test contract * Move _disableGuard impl into TimelockGuard * Add missing natspec * Add gas limit testing on changeOwnershipToFallback * Remove interfaces for abstract contracts * Move state changes out into internal _clearLivenessModule * Improve names on the internal _disableX methods * Add clearTimelockGuard function * Add _disableGuard helper to TLG tests * Limit number of transactions cancelled to 100 * Revert "Remove interfaces for abstract contracts" This reverts commit bd03288. * Move livenessModule2 address into TestUtils Reduces diff a bit * Reduce diff somewhat * Remove unused arg * Update packages/contracts-bedrock/src/safe/TimelockGuard.sol Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> * Fix iface * update abi for iface fix * Do not clear or disable the module during ownership transfer * Fix inaccurate comment on _disableAndClearGuard * Further improve comment * remove unused import * fix test name * Do not clear guard during changeOwnershipToFallback * Remove unused SENTINEL_MODULE var * Remove dangling comment * Revert "Remove dangling comment" This reverts commit d266d12. * Fix whitespace * remove unnecessary internal _clearTimelockGuard function It's no longer reused in the change ownership call. * Address feedback * Add missing assertion * Move guard slot into constants * semver-lock * Remove LivenessModule from semver-lock * fix: fmt, semver-lock, unused imports * Remove unused variable * fix semver lock by resetting old LivenessModule * fix unused import * Require that msgSender be an owner of the Safe * fix compiler error * Fix placement of _msgSender check * semver-lock * Add TimelockGuard_NotOwner test * Bump semver * Add test comment, make into fuzz test * Improvements to SaferSafes styling (#17903) * Add public getter livenessSafeConfiguration to return a struct rather than tuple * Use Safe as input type to mappings and functions on LivenessModule2 * Add dividers based on function type * fmt * snapshots * Remove conditional return of 0 in the cancellationThreshold if the guard is not enabled * rename timelockConfiguration func to timelockDelay * semver-lock * Add missing natspec on tests and convert to fuzzing where possible * fix import and abi snapshot * fix: off by one error in challenge period test * fix test name * refactor(test): split clearLivenessModule tests into separate contract - Create LivenessModule2_ClearLivenessModule_Test contract - Rename test_clear_* to test_clearLivenessModule_* for consistency - Update contract title to match function under test * test(LivenessModule2): add challenge cancellation to clearLivenessModule test Enhance test_clearLivenessModule_succeeds to verify that clearing also properly cancels any active challenge, covering the _cancelChallenge code path. * test(LivenessModule2): add guard removal check to ownership transfer test Integrate guard removal verification into existing fuzz test rather than creating a separate test. Verifies that any guard set on the Safe is properly removed during the ownership transfer process. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Remove pointless check Slop that got through * Convert to fuzz tests * test(TimelockGuard): enhance test coverage and convert to fuzz tests - Convert configureTimelockGuard tests to fuzz tests for broader coverage - Add comprehensive checkAfterExecution tests for all code paths - Remove redundant assertions and comments - Add test to verify transaction cannot be re-executed after success - Improve test documentation and structure 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix test name to match function name * feat: two extra tests to fuzz * fix: fmt --------- Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: JosepBove <josep@oplabs.co>
diff --git a/packages/contracts-bedrock/test/safe/LivenessModule2.t.sol b/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
diff --git a/packages/contracts-bedrock/test/safe/SaferSafes.t.sol b/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";
diff --git a/packages/contracts-bedrock/test/safe/TimelockGuard.t.sol b/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 {
