SaferSafes audit fixes 2

packages/contracts-bedrock/interfaces/safe/ITimelockGuard.sol

@@ -48,7 +48,7 @@ interface ITimelockGuard {
     event GuardConfigured(address indexed safe, uint256 timelockDelay);
     event TransactionCancelled(address indexed safe, bytes32 indexed txHash);
     event TransactionScheduled(address indexed safe, bytes32 indexed txHash, uint256 executionTime);
-    event TransactionExecuted(address indexed safe, bytes32 txHash);
+    event TransactionExecuted(address indexed safe, bytes32 indexed txHash);
     event Message(string message);
     event TransactionsNotCancelled(address indexed safe, uint256 uncancelledCount);
 

packages/contracts-bedrock/snapshots/abi/SaferSafes.json

@@ -805,7 +805,7 @@
         "type": "address"
       },
       {
-        "indexed": false,
+        "indexed": true,
         "internalType": "bytes32",
         "name": "txHash",
         "type": "bytes32"

packages/contracts-bedrock/snapshots/semver-lock.json

@@ -208,8 +208,8 @@
     "sourceCodeHash": "0x7fc4789b082bc8ecd29c4c75a06058f0ff0b72f1c1028a42db6f1c35269c8865"
   },
   "src/safe/SaferSafes.sol:SaferSafes": {
-    "initCodeHash": "0xea63f8c74f2ce10cc53e460b5f7f5006a06ab5612adc7253885a1ca3df124adb",
-    "sourceCodeHash": "0x169dc281821d3951fb399deefb0b84021b8172cf32ca03526686421bb6478cb4"
+    "initCodeHash": "0x80f67f25659b54347cea5f8c250cdf66a32a647375b6703e790b05965fe9cf88",
+    "sourceCodeHash": "0x9edb7350f8c664964dc106a89e1a117ba9e9b2d2c52d57b3c1c976170a34aa3b"
   },
   "src/universal/OptimismMintableERC20.sol:OptimismMintableERC20": {
     "initCodeHash": "0x3c85eed0d017dca8eda6396aa842ddc12492587b061e8c756a8d32c4610a9658",

packages/contracts-bedrock/src/safe/LivenessModule2.sol

@@ -173,8 +173,8 @@ abstract contract LivenessModule2 {
         if (_config.livenessResponsePeriod == 0) {
             revert LivenessModule2_InvalidResponsePeriod();
         }
-        // fallbackOwner must not be zero address to have a valid ownership recipient.
-        if (_config.fallbackOwner == address(0)) {
+        // fallbackOwner must not be zero address or the safe itself to be able to become an owner.
+        if (_config.fallbackOwner == address(0) || _config.fallbackOwner == address(callingSafe)) {
             revert LivenessModule2_InvalidFallbackOwner();
         }
 
@@ -334,6 +334,9 @@ abstract contract LivenessModule2 {
         }
 
         // Now swap the remaining single owner with the fallback owner
+        // Note: If the fallback owner would be the only or the last owner in the owners list,
+        // swapOwner would internally revert in OwnerManager, but we ignore it because the final
+        // owners list would still be what we want.
         _safe.execTransactionFromModule({
             to: address(_safe),
             value: 0,

packages/contracts-bedrock/src/safe/SaferSafes.sol

@@ -26,8 +26,8 @@ import { ISemver } from "interfaces/universal/ISemver.sol";
 ///      compatibility restrictions in the LivenessModule2 and TimelockGuard contracts.
 contract SaferSafes is LivenessModule2, TimelockGuard, ISemver {
     /// @notice Semantic version.
-    /// @custom:semver 1.9.1
-    string public constant version = "1.9.1";
+    /// @custom:semver 1.10.0
+    string public constant version = "1.10.0";
 
     /// @notice Error for when the liveness response period is insufficient.
     error SaferSafes_InsufficientLivenessResponsePeriod();

packages/contracts-bedrock/src/safe/TimelockGuard.sol

@@ -190,17 +190,17 @@ abstract contract TimelockGuard is BaseGuard {
     /// @param txHash The identifier of the cancelled transaction.
     event TransactionCancelled(Safe indexed safe, bytes32 indexed txHash);
 
+    /// @notice Emitted when a transaction is executed for a Safe.
+    /// @param safe The Safe whose transaction is executed.
+    /// @param txHash The identifier of the executed transaction.
+    event TransactionExecuted(Safe indexed safe, bytes32 indexed txHash);
+
     /// @notice Emitted when the cancellation threshold is updated
     /// @param safe The Safe whose cancellation threshold is updated.
     /// @param oldThreshold The old cancellation threshold.
     /// @param newThreshold The new cancellation threshold.
     event CancellationThresholdUpdated(Safe indexed safe, uint256 oldThreshold, uint256 newThreshold);
 
-    /// @notice Emitted when a transaction is executed for a Safe.
-    /// @param safe The Safe whose transaction is executed.
-    /// @param txHash The identifier of the executed transaction.
-    event TransactionExecuted(Safe indexed safe, bytes32 txHash);
-
     /// @notice Used to emit a message, primarily to ensure that the cancelTransaction function is
     ///         is not labelled as view so that it is treated as a state-changing function.
     event Message(string message);
@@ -455,8 +455,13 @@ abstract contract TimelockGuard is BaseGuard {
             revert TimelockGuard_InvalidVersion();
         }
 
-        // Validate timelock delay - must not be longer than 1 year
-        if (_timelockDelay > 365 days) {
+        // Check that this guard is enabled on the calling Safe
+        if (!_isGuardEnabled(callingSafe)) {
+            revert TimelockGuard_GuardNotEnabled();
+        }
+
+        // Validate timelock delay - must not be zero or longer than 1 year
+        if (_timelockDelay == 0 || _timelockDelay > 365 days) {
             revert TimelockGuard_InvalidTimelockDelay();
         }
 

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

@@ -185,7 +185,7 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni
         );
     }
 
-    function test_configureLivenessModule_invalidFallbackOwner_reverts() external {
+    function test_configureLivenessModule_zeroAddressFallbackOwner_reverts() external {
         // Test with zero address
         vm.expectRevert(LivenessModule2.LivenessModule2_InvalidFallbackOwner.selector);
         vm.prank(address(safeInstance.safe));
@@ -194,6 +194,18 @@ contract LivenessModule2_ConfigureLivenessModule_Test is LivenessModule2_TestIni
         );
     }
 
+    function test_configureLivenessModule_safeAddressFallbackOwner_reverts() external {
+        // Test with safe address as fallbackOwner
+        vm.expectRevert(LivenessModule2.LivenessModule2_InvalidFallbackOwner.selector);
+        vm.prank(address(safeInstance.safe));
+        livenessModule2.configureLivenessModule(
+            LivenessModule2.ModuleConfig({
+                livenessResponsePeriod: CHALLENGE_PERIOD,
+                fallbackOwner: address(safeInstance.safe)
+            })
+        );
+    }
+
     /// @notice Checks configuration reverts when the contract is too old.
     function test_configureLivenessModule_withWrongVersion_reverts() external {
         // nosemgrep: sol-style-use-abi-encodecall

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

@@ -153,7 +153,7 @@ abstract contract TimelockGuard_TestInit is Test, SafeTestTools {
     event TransactionScheduled(Safe indexed safe, bytes32 indexed txId, uint256 when);
     event TransactionCancelled(Safe indexed safe, bytes32 indexed txId);
     event CancellationThresholdUpdated(Safe indexed safe, uint256 oldThreshold, uint256 newThreshold);
-    event TransactionExecuted(Safe indexed safe, bytes32 txHash);
+    event TransactionExecuted(Safe indexed safe, bytes32 indexed txHash);
     event Message(string message);
     event TransactionsNotCancelled(Safe indexed safe, uint256 n);
 
@@ -317,6 +317,13 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit {
         timelockGuard.configureTimelockGuard(_delay);
     }
 
+    /// @notice Ensures setting delay to zero reverts.
+    function test_configureTimelockGuard_zeroDelay_reverts() external {
+        vm.expectRevert(TimelockGuard.TimelockGuard_InvalidTimelockDelay.selector);
+        vm.prank(address(safeInstance.safe));
+        timelockGuard.configureTimelockGuard(0);
+    }
+
     /// @notice Checks configuration reverts when the contract is not 1.4.1.
     function test_configureTimelockGuard_withWrongVersion_reverts() external {
         // nosemgrep: sol-style-use-abi-encodecall
@@ -334,6 +341,13 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit {
         timelockGuard.configureTimelockGuard(TIMELOCK_DELAY);
     }
 
+    /// @notice Ensures configuration reverts when the guard has not been enabled on the Safe.
+    function test_configureTimelockGuard_guardNotEnabled_reverts() external {
+        vm.expectRevert(TimelockGuard.TimelockGuard_GuardNotEnabled.selector);
+        vm.prank(address(unguardedSafe.safe));
+        timelockGuard.configureTimelockGuard(TIMELOCK_DELAY);
+    }
+
     /// @notice Asserts the maximum valid delay configures successfully.
     function test_configureTimelockGuard_acceptsMaxValidDelay_succeeds() external {
         vm.expectEmit(true, true, true, true);
@@ -369,31 +383,6 @@ contract TimelockGuard_ConfigureTimelockGuard_Test is TimelockGuard_TestInit {
         _configureGuard(safeInstance, 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.timelockDelay(safe), TIMELOCK_DELAY);
-
-        // Configure timelock delay to 0 should succeed and emit event
-        vm.expectEmit(true, true, true, true);
-        emit GuardConfigured(safe, 0);
-        vm.prank(address(safeInstance.safe));
-        timelockGuard.configureTimelockGuard(0);
-
-        // Timelock delay should be set to 0
-        assertEq(timelockGuard.timelockDelay(safe), 0);
-    }
-
-    /// @notice Checks clearing succeeds even if the guard was never configured.
-    function test_configureTimelockGuard_notConfigured_succeeds() external {
-        // Try to clear - should succeed even if not yet configured
-        vm.expectEmit(true, true, true, true);
-        emit GuardConfigured(safe, 0);
-        vm.prank(address(safeInstance.safe));
-        timelockGuard.configureTimelockGuard(0);
-    }
 }
 
 /// @title TimelockGuard_CancellationThreshold_Test
@@ -1011,26 +1000,6 @@ contract TimelockGuard_Integration_Test is TimelockGuard_TestInit {
         dummyTx.scheduleTransaction(timelockGuard);
     }
 
-    /// @notice Test that the guard can be reset while still enabled, and then can be disabled
-    function test_integration_resetThenDisableGuard_succeeds() external {
-        TransactionBuilder.Transaction memory resetGuardTx = _createEmptyTransaction(safeInstance);
-        resetGuardTx.params.to = address(timelockGuard);
-        resetGuardTx.params.data = abi.encodeCall(TimelockGuard.configureTimelockGuard, (0));
-        resetGuardTx.updateTransaction();
-        resetGuardTx.scheduleTransaction(timelockGuard);
-
-        vm.warp(block.timestamp + TIMELOCK_DELAY);
-        resetGuardTx.executeTransaction(safeInstance.owners[0]);
-
-        TransactionBuilder.Transaction memory disableGuardTx = _createEmptyTransaction(safeInstance);
-        disableGuardTx.params.to = address(safeInstance.safe);
-        disableGuardTx.params.data = abi.encodeCall(GuardManager.setGuard, (address(0)));
-        disableGuardTx.updateTransaction();
-
-        vm.warp(block.timestamp + TIMELOCK_DELAY);
-        disableGuardTx.executeTransaction(safeInstance.owners[0]);
-    }
-
     /// @notice Test that the max cancellation threshold is not exceeded
     function test_integration_maxCancellationThresholdNotExceeded_succeeds() external {
         uint256 maxThreshold = timelockGuard.maxCancellationThreshold(safeInstance.safe);