feat: Address feedback

packages/contracts-bedrock/src/Safe/LivenessGuard.sol

@@ -23,7 +23,7 @@ contract LivenessGuard is ISemver, BaseGuard {
 
     /// @notice Emitted when an owner is recorded.
     /// @param owner The owner's address.
-    event OwnerRecorded(bytes32 indexed txHash, address owner);
+    event OwnerRecorded(address owner);
 
     /// @notice Semantic version.
     /// @custom:semver 1.0.0
@@ -49,12 +49,18 @@ contract LivenessGuard is ISemver, BaseGuard {
         for (uint256 i = 0; i < owners.length; i++) {
             address owner = owners[i];
             lastLive[owner] = block.timestamp;
-            emit OwnerRecorded(0x0, owner);
+            emit OwnerRecorded(owner);
         }
     }
 
+    /// @notice Getter function for the Safe contract instance
+    /// @return safe_ The Safe contract instance
+    function safe() public view returns (Safe safe_) {
+        safe_ = SAFE;
+    }
+
     /// @notice Internal function to ensure that only the Safe can call certain functions.
-    function _onlySafe() internal view {
+    function _requireOnlySafe() internal view {
         require(msg.sender == address(SAFE), "LivenessGuard: only Safe can call this function");
     }
 
@@ -76,7 +82,7 @@ contract LivenessGuard is ISemver, BaseGuard {
         external
     {
         msgSender; // silence unused variable warning
-        _onlySafe();
+        _requireOnlySafe();
 
         // Cache the set of owners prior to execution.
         // This will be used in the checkAfterExecution method.
@@ -88,7 +94,7 @@ contract LivenessGuard is ISemver, BaseGuard {
 
         // This call will reenter to the Safe which is calling it. This is OK because it is only reading the
         // nonce, and using the getTransactionHash() method.
-        bytes32 txHash = Safe(payable(msg.sender)).getTransactionHash({
+        bytes32 txHash = SAFE.getTransactionHash({
             to: to,
             value: value,
             data: data,
@@ -98,7 +104,7 @@ contract LivenessGuard is ISemver, BaseGuard {
             gasPrice: gasPrice,
             gasToken: gasToken,
             refundReceiver: refundReceiver,
-            _nonce: Safe(payable(msg.sender)).nonce() - 1
+            _nonce: SAFE.nonce() - 1
         });
 
         uint256 threshold = SAFE.getThreshold();
@@ -107,7 +113,7 @@ contract LivenessGuard is ISemver, BaseGuard {
 
         for (uint256 i = 0; i < signers.length; i++) {
             lastLive[signers[i]] = block.timestamp;
-            emit OwnerRecorded(txHash, signers[i]);
+            emit OwnerRecorded(signers[i]);
         }
     }
 
@@ -118,7 +124,7 @@ contract LivenessGuard is ISemver, BaseGuard {
     ///      1. Add new owners to the lastLive mapping
     ///      2. Delete removed owners from the lastLive mapping
     function checkAfterExecution(bytes32, bool) external {
-        _onlySafe();
+        _requireOnlySafe();
         // Get the current set of owners
         address[] memory ownersAfter = SAFE.getOwners();
 
@@ -132,6 +138,7 @@ contract LivenessGuard is ISemver, BaseGuard {
                 lastLive[ownerAfter] = block.timestamp;
             }
         }
+
         // Now iterate over the remaining ownersBefore entries. Any remaining addresses are no longer an owner, so we
         // delete them from the lastLive mapping.
         uint256 ownersBeforeLength = ownersBefore.length();
@@ -147,12 +154,6 @@ contract LivenessGuard is ISemver, BaseGuard {
         require(SAFE.isOwner(msg.sender), "LivenessGuard: only Safe owners may demonstrate liveness");
         lastLive[msg.sender] = block.timestamp;
 
-        emit OwnerRecorded(0x0, msg.sender);
-    }
-
-    /// @notice Getter function for the Safe contract instance
-    /// @return safe_ The Safe contract instance
-    function safe() public view returns (Safe safe_) {
-        safe_ = SAFE;
+        emit OwnerRecorded(msg.sender);
     }
 }

packages/contracts-bedrock/src/Safe/LivenessModule.sol

@@ -55,7 +55,7 @@ contract LivenessModule is ISemver {
         FALLBACK_OWNER = _fallbackOwner;
         MIN_OWNERS = _minOwners;
         address[] memory owners = _safe.getOwners();
-        require(_minOwners < owners.length, "LivenessModule: minOwners must be less than the number of owners");
+        require(_minOwners <= owners.length, "LivenessModule: minOwners must be less than the number of owners");
         require(
             _safe.getThreshold() == get75PercentThreshold(owners.length),
             "LivenessModule: Safe must have a threshold of 75% of the number of owners"
@@ -70,7 +70,7 @@ contract LivenessModule is ISemver {
     function removeOwners(address[] memory _previousOwners, address[] memory _ownersToRemove) external {
         require(_previousOwners.length == _ownersToRemove.length, "LivenessModule: arrays must be the same length");
 
-        // We will remove at least one owner, so we'll initialize the newOwners count to the current number of owners
+        // We will remove at least one owner, so we'll initialize the ownersCount count to the current number of owners
         uint256 ownersCount = SAFE.getOwners().length;
         for (uint256 i = 0; i < _previousOwners.length; i++) {
             ownersCount--;
@@ -81,8 +81,7 @@ contract LivenessModule is ISemver {
             _removeOwner({
                 _prevOwner: _previousOwners[i],
                 _ownerToRemove: _ownersToRemove[i],
-                _newOwnersCount: ownersCount,
-                _newThreshold: get75PercentThreshold(ownersCount)
+                _newOwnersCount: ownersCount
             });
         }
         _verifyFinalState();
@@ -92,18 +91,11 @@ contract LivenessModule is ISemver {
     /// @param _prevOwner Owner that pointed to the owner to be removed in the linked list
     /// @param _ownerToRemove Owner address to be removed.
     /// @param _newOwnersCount New number of owners after removal.
-    /// @param _newThreshold New threshold.
-    function _removeOwner(
-        address _prevOwner,
-        address _ownerToRemove,
-        uint256 _newOwnersCount,
-        uint256 _newThreshold
-    )
-        internal
-    {
+    function _removeOwner(address _prevOwner, address _ownerToRemove, uint256 _newOwnersCount) internal {
         if (_newOwnersCount > 0) {
+            uint256 newThreshold = get75PercentThreshold(_newOwnersCount);
             // Remove the owner and update the threshold
-            _removeOwnerSafeCall({ _prevOwner: _prevOwner, _owner: _ownerToRemove, _threshold: _newThreshold });
+            _removeOwnerSafeCall({ _prevOwner: _prevOwner, _owner: _ownerToRemove, _threshold: newThreshold });
         } else {
             // There is only one owner left. The Safe will not allow a safe with no owners, so we will
             // need to swap owners instead.

packages/contracts-bedrock/test/LivenessGuard.t.sol

@@ -13,7 +13,7 @@ import { LivenessGuard } from "src/Safe/LivenessGuard.sol";
 contract LivenessGuard_TestInit is Test, SafeTestTools {
     using SafeTestLib for SafeInstance;
 
-    event OwnerRecorded(bytes32 indexed txHash, address signer);
+    event OwnerRecorded(address owner);
 
     uint256 initTime = 10;
     LivenessGuard livenessGuard;
@@ -81,7 +81,7 @@ contract LivenessGuard_CheckTx_Test is LivenessGuard_TestInit {
         for (uint256 i; i < signers.length; i++) {
             // Don't check topic1 so that we can avoid the ugly txHash calculation.
             vm.expectEmit(false, true, true, true, address(livenessGuard));
-            emit OwnerRecorded(0x0, signers[i]);
+            emit OwnerRecorded(signers[i]);
         }
         vm.expectCall(address(safeInstance.safe), abi.encodeWithSignature("nonce()"));
         vm.expectCall(address(safeInstance.safe), abi.encodeCall(OwnerManager.getThreshold, ()));
@@ -116,7 +116,7 @@ contract LivenessGuard_ShowLiveness_Test is LivenessGuard_TestInit {
         address caller = safeInstance.owners[0];
 
         vm.expectEmit(address(livenessGuard));
-        emit OwnerRecorded(0x0, caller);
+        emit OwnerRecorded(caller);
 
         vm.prank(caller);
         livenessGuard.showLiveness();

specs/safe-liveness-checking.md

@@ -92,13 +92,14 @@ The following security properties must be upheld:
 
 1. Signatures are assigned to the correct signer.
 1. Non-signers are unable to create a record of having signed.
-1. A signer cannot be censored or grieffed such that their signing is not recorded.
+1. A signer cannot be censored or griefed such that their signing is not recorded.
 1. Signers may demonstrate liveness either by signing a transaction or by calling directly to the
    guard.
 1. The module only removes a signer if they have demonstrated liveness during the interval, or
      if necessary to convert the safe to a 1 of 1.
 1. The module sets the correct 75% threshold upon removing a signer.
 1. During a shutdown the module correctly removes all signers, and converts the safe to a 1 of 1.
+1. It must be impossible for the guard's checkTransaction or checkAfterExecution to permanently revert given any calldata and the current state.
 
 ### Interdependency between the Guard and Module
 
@@ -108,7 +109,7 @@ be set on the Safe, and the Module will be unable to function if the Guard is re
 
 ### Deployment
 
-The module are guard are intended to be deployed and installed on the safe in the following sequence:
+The module and guard are intended to be deployed and installed on the safe in the following sequence:
 
 1. Deploy the guard contract, this will set a timestamp for each existing owner on the Safe.
 1. Deploy the module.