Safer Safes audit fixes (no bytecode changes)

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

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

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

@@ -279,12 +279,16 @@ abstract contract LivenessModule2 {
         _cancelChallenge(callingSafe);
     }
 
-    /// @notice With successful challenge, removes all current owners from enabled safe,
-    ///         appoints fallback as sole owner, and sets its quorum to 1.
-    /// @dev Note: After ownership transfer, the fallback owner becomes the sole owner
-    ///      and is also still configured as the fallback owner. This means the
-    ///      fallback owner effectively becomes its own fallback owner, maintaining
-    ///      the ability to challenge itself if needed.
+    /// @notice With successful challenge, removes all current owners from enabled safe, appoints
+    ///         fallback as sole owner, and sets its quorum to 1.
+    /// @dev After ownership transfer, the fallback owner becomes the sole owner and is also still
+    ///      configured as the fallback owner. If the fallback owner would become unable to sign,
+    ///      it would not be able challenge the safe again. For this reason, it is important that
+    ///      the fallback owner has a way to preserve its own liveness.
+    ///
+    ///      It is of critical importance that this function never reverts. If it were to do so,
+    ///      the Safe would be permanently bricked. For this reason, the external calls from this
+    ///      function are allowed to fail silently instead of reverting.
     /// @param _safe The Safe address to transfer ownership of.
     function changeOwnershipToFallback(Safe _safe) external {
         // Ensure Safe is configured with this module to prevent unauthorized execution.

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.0
-    string public constant version = "1.9.0";
+    /// @custom:semver 1.9.1
+    string public constant version = "1.9.1";
 
     /// @notice Error for when the liveness response period is insufficient.
     error SaferSafes_InsufficientLivenessResponsePeriod();

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

@@ -212,13 +212,15 @@ abstract contract TimelockGuard is BaseGuard {
     /// @notice Returns the blocking threshold, which is defined as the minimum number of owners
     ///         that must coordinate to block a transaction from being executed by refusing to
     ///         sign.
+    /// @dev Becase `_safe.getOwners()` loops through the owners list, it could run out of gas if
+    ///      there are a lot of owners.
     /// @param _safe The Safe address to query
     /// @return The current blocking threshold
     function _blockingThreshold(Safe _safe) internal view returns (uint256) {
         return _safe.getOwners().length - _safe.getThreshold() + 1;
     }
 
-    /// @notice Internal helper to get the guard address from a Safe
+    /// @notice Internal helper to check if TimelockGuard is enabled for a Safe
     /// @param _safe The Safe address
     /// @return The current guard address
     function _isGuardEnabled(Safe _safe) internal view returns (bool) {
@@ -233,7 +235,7 @@ abstract contract TimelockGuard is BaseGuard {
         return _safeStates[_safe][_safeConfigNonces[_safe]];
     }
 
-    /// @notice Internal helper function which can be overriden in a child contract to check if the
+    /// @notice Internal helper function which can be overridden 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;
@@ -346,7 +348,8 @@ abstract contract TimelockGuard is BaseGuard {
 
         // Limit execution of transactions to owners of the Safe only.
         // This ensures that an attacker cannot simply collect valid signatures, but must also
-        // control a private key.
+        // control a private key. It is accepted as a trade-off that paymasters, relayers or UX
+        // wrappers cannot execute transactions with the TimelockGuard enabled.
         if (!callingSafe.isOwner(_msgSender)) {
             revert TimelockGuard_NotOwner();
         }
@@ -475,6 +478,9 @@ abstract contract TimelockGuard is BaseGuard {
     ///      1. Safe disables the guard via GuardManager.setGuard(address(0)).
     ///      2. Safe calls this clearTimelockGuard() function to remove stored configuration.
     ///      3. If Safe later re-enables the guard, it must call configureTimelockGuard() again.
+    ///      Warning: Clearing the configuration allows all transactions previously scheduled to be
+    ///      scheduled again, including cancelled transactions. It is strongly recommended to
+    ///      manually increment the Safe's nonce when a scheduled transaction is cancelled.
     function clearTimelockGuard() external {
         Safe callingSafe = Safe(payable(msg.sender));
 
@@ -496,6 +502,11 @@ abstract contract TimelockGuard is BaseGuard {
     ///      existing signature generation tools. Owners can use any method to sign the a
     ///      transaction, including signing with a private key, calling the Safe's approveHash
     ///      function, or EIP1271 contract signatures.
+    ///      The Safe doesn't increase its nonce when a transaction is cancelled in the Timelock.
+    ///      This means that it is possible to add the very same transaction a second time to the
+    ///      safe queue, but it won't be possible to schedule it again in the Timelock. It is
+    ///      recommended that the safe nonce is manually incremented when a scheduled transaction
+    ///      is cancelled.
     /// @param _safe The Safe address to schedule the transaction for.
     /// @param _nonce The nonce of the Safe for the transaction being scheduled.
     /// @param _params The parameters of the transaction being scheduled.
@@ -594,6 +605,9 @@ abstract contract TimelockGuard is BaseGuard {
     ///      of checkNSignatures is that owners can use any method to sign the cancellation
     ///      transaction inputs, including signing with a private key, calling the Safe's
     ///      approveHash function, or EIP1271 contract signatures.
+    ///
+    ///      It is allowed to cancel transactions from a disabled TimelockGuard, as a way of
+    ///      clearing the queue that wouldn't be as blunt as calling `clearTimelockConfiguration`.
     /// @param _safe The Safe address to cancel the transaction for.
     /// @param _txHash The hash of the transaction being cancelled.
     /// @param _nonce The nonce of the Safe for the transaction being cancelled.