diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 7acb955c6cd29..f7dbfcffbe497 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/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", diff --git a/packages/contracts-bedrock/src/safe/LivenessModule2.sol b/packages/contracts-bedrock/src/safe/LivenessModule2.sol index 290d3f8017fa9..0651bd3b9b6f6 100644 --- a/packages/contracts-bedrock/src/safe/LivenessModule2.sol +++ b/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. diff --git a/packages/contracts-bedrock/src/safe/SaferSafes.sol b/packages/contracts-bedrock/src/safe/SaferSafes.sol index 5b57b3a38fc88..533e360bd51d7 100644 --- a/packages/contracts-bedrock/src/safe/SaferSafes.sol +++ b/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(); diff --git a/packages/contracts-bedrock/src/safe/TimelockGuard.sol b/packages/contracts-bedrock/src/safe/TimelockGuard.sol index 77824e0f9d08c..57ef493ddf889 100644 --- a/packages/contracts-bedrock/src/safe/TimelockGuard.sol +++ b/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 Because `_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.