Merge pull request #7935 from ethereum-optimism/jm/log-cleanup

Liveness clean up

packages/contracts-bedrock/.gas-snapshot

@@ -308,8 +308,8 @@ LivenessGuard_CheckTx_TestFails:test_checkTransaction_callerIsNotSafe_revert() (
 LivenessGuard_Constructor_Test:test_constructor_works() (gas: 1198965)
 LivenessGuard_Getters_Test:test_getters_works() (gas: 10662)
 LivenessGuard_OwnerManagement_Test:test_addOwner_succeeds() (gas: 274366)
-LivenessGuard_OwnerManagement_Test:test_removeOwner_succeeds() (gas: 246263)
-LivenessGuard_OwnerManagement_Test:test_swapOwner_succeeds() (gas: 284880)
+LivenessGuard_OwnerManagement_Test:test_removeOwner_succeeds() (gas: 243684)
+LivenessGuard_OwnerManagement_Test:test_swapOwner_succeeds() (gas: 282299)
 LivenessGuard_ShowLiveness_Test:test_showLiveness_succeeds() (gas: 28831)
 LivenessGuard_ShowLiveness_TestFail:test_showLiveness_callIsNotSafeOwner_reverts() (gas: 18770)
 LivenessModule_CanRemove_Test:test_canRemove_works() (gas: 33026)
@@ -318,16 +318,16 @@ LivenessModule_Constructor_TestFail:test_constructor_minOwnersGreaterThanOwners_
 LivenessModule_Constructor_TestFail:test_constructor_wrongThreshold_reverts() (gas: 92925)
 LivenessModule_Get75PercentThreshold_Test:test_get75PercentThreshold_Works() (gas: 26339)
 LivenessModule_Getters_Test:test_getters_works() (gas: 14853)
-LivenessModule_RemoveOwners_Test:test_removeOwners_allOwners_succeeds() (gas: 1326177)
-LivenessModule_RemoveOwners_Test:test_removeOwners_oneOwner_succeeds() (gas: 133975)
-LivenessModule_RemoveOwners_TestFail:test_removeOwners_belowEmptiedButNotShutDown_reverts() (gas: 1278643)
-LivenessModule_RemoveOwners_TestFail:test_removeOwners_belowMinButNotEmptied_reverts() (gas: 1281685)
+LivenessModule_RemoveOwners_Test:test_removeOwners_allOwners_succeeds() (gas: 1316393)
+LivenessModule_RemoveOwners_Test:test_removeOwners_oneOwner_succeeds() (gas: 130750)
+LivenessModule_RemoveOwners_TestFail:test_removeOwners_belowEmptiedButNotShutDown_reverts() (gas: 1269598)
+LivenessModule_RemoveOwners_TestFail:test_removeOwners_belowMinButNotEmptied_reverts() (gas: 1273409)
 LivenessModule_RemoveOwners_TestFail:test_removeOwners_differentArrayLengths_reverts() (gas: 10502)
-LivenessModule_RemoveOwners_TestFail:test_removeOwners_guardChanged_reverts() (gas: 2839358)
+LivenessModule_RemoveOwners_TestFail:test_removeOwners_guardChanged_reverts() (gas: 2836129)
 LivenessModule_RemoveOwners_TestFail:test_removeOwners_invalidThreshold_reverts() (gas: 69358)
-LivenessModule_RemoveOwners_TestFail:test_removeOwners_ownerHasShownLivenessRecently_reverts() (gas: 80971)
-LivenessModule_RemoveOwners_TestFail:test_removeOwners_ownerHasSignedRecently_reverts() (gas: 617629)
-LivenessModule_RemoveOwners_TestFail:test_removeOwners_swapToFallbackOwner_reverts() (gas: 1288036)
+LivenessModule_RemoveOwners_TestFail:test_removeOwners_ownerHasShownLivenessRecently_reverts() (gas: 77749)
+LivenessModule_RemoveOwners_TestFail:test_removeOwners_ownerHasSignedRecently_reverts() (gas: 615047)
+LivenessModule_RemoveOwners_TestFail:test_removeOwners_swapToFallbackOwner_reverts() (gas: 1278252)
 LivenessModule_RemoveOwners_TestFail:test_removeOwners_wrongPreviousOwner_reverts() (gas: 73954)
 MIPS_Test:test_add_succeeds() (gas: 122932)
 MIPS_Test:test_addiSign_succeeds() (gas: 122923)

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

@@ -47,7 +47,7 @@ contract LivenessGuard_Constructor_Test is LivenessGuard_TestInit {
     function test_constructor_works() external {
         address[] memory owners = safeInstance.owners;
         livenessGuard = new WrappedGuard(safeInstance.safe);
-        for (uint256 i = 0; i < owners.length; i++) {
+        for (uint256 i; i < owners.length; i++) {
             assertEq(livenessGuard.lastLive(owners[i]), initTime);
         }
     }
@@ -242,7 +242,7 @@ contract LivenessGuard_FuzzOwnerManagement_Test is StdCheats, StdUtils, Liveness
         (address[] memory ownerAddrs, uint256[] memory ownerkeys) =
             SafeTestLib.makeAddrsAndKeys("safeTest", initialOwners);
         // record the private keys for later use
-        for (uint256 i = 0; i < ownerAddrs.length; i++) {
+        for (uint256 i; i < ownerAddrs.length; i++) {
             privateKeys[ownerAddrs[i]] = ownerkeys[i];
         }
 
@@ -251,7 +251,7 @@ contract LivenessGuard_FuzzOwnerManagement_Test is StdCheats, StdUtils, Liveness
         livenessGuard = new WrappedGuard(safeInstance.safe);
         safeInstance.setGuard(address(livenessGuard));
 
-        for (uint256 i = 0; i < changes.length; i++) {
+        for (uint256 i; i < changes.length; i++) {
             vm.warp(block.timestamp + changes[i].timeDelta);
             OwnerChange memory change = changes[i];
             address[] memory currentOwners = safeInstance.safe.getOwners();
@@ -312,16 +312,16 @@ contract LivenessGuard_FuzzOwnerManagement_Test is StdCheats, StdUtils, Liveness
 
         // Looks up the private key for each owner
         uint256[] memory unsortedOwnerPKs = new uint256[](instance.owners.length);
-        for (uint256 j = 0; j < instance.owners.length; j++) {
-            unsortedOwnerPKs[j] = privateKeys[instance.owners[j]];
+        for (uint256 i; i < instance.owners.length; i++) {
+            unsortedOwnerPKs[i] = privateKeys[instance.owners[i]];
         }
 
         // Sort the keys by address and store them in the SafeInstance
         instance.ownerPKs = SafeTestLib.sortPKsByComputedAddress(unsortedOwnerPKs);
 
         // Overwrite the SafeInstances owners array with the computed addresses from the ownerPKs array
-        for (uint256 k; k < instance.owners.length; k++) {
-            instance.owners[k] = SafeTestLib.getAddr(instance.ownerPKs[k]);
+        for (uint256 i; i < instance.owners.length; i++) {
+            instance.owners[i] = SafeTestLib.getAddr(instance.ownerPKs[i]);
         }
     }
 }

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

@@ -199,7 +199,7 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
         uint256 numOwners = safeInstance.owners.length;
 
         address[] memory ownersToRemove = new address[](numOwners);
-        for (uint256 i = 0; i < numOwners; i++) {
+        for (uint256 i; i < numOwners; i++) {
             ownersToRemove[i] = safeInstance.owners[i];
         }
         address[] memory prevOwners = safeInstance.getPrevOwners(ownersToRemove);
@@ -218,7 +218,7 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
         uint256 numOwners = safeInstance.owners.length - 2;
 
         address[] memory ownersToRemove = new address[](numOwners);
-        for (uint256 i = 0; i < numOwners; i++) {
+        for (uint256 i; i < numOwners; i++) {
             ownersToRemove[i] = safeInstance.owners[i];
         }
         address[] memory prevOwners = safeInstance.getPrevOwners(ownersToRemove);
@@ -236,7 +236,7 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
         uint256 numOwners = safeInstance.owners.length - 1;
 
         address[] memory ownersToRemove = new address[](numOwners);
-        for (uint256 i = 0; i < numOwners; i++) {
+        for (uint256 i; i < numOwners; i++) {
             ownersToRemove[i] = safeInstance.owners[i];
         }
         address[] memory prevOwners = safeInstance.getPrevOwners(ownersToRemove);
@@ -297,7 +297,7 @@ contract LivenessModule_RemoveOwners_Test is LivenessModule_TestInit {
         uint256 numOwners = safeInstance.owners.length;
 
         address[] memory ownersToRemove = new address[](numOwners);
-        for (uint256 i = 0; i < numOwners; i++) {
+        for (uint256 i; i < numOwners; i++) {
             ownersToRemove[i] = safeInstance.owners[i];
         }
         address[] memory prevOwners = safeInstance.getPrevOwners(ownersToRemove);
@@ -348,15 +348,12 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
         //
         // _numOwners must be at least 4, so that _minOwners can be set to at least 3 by the following bound() call.
         // Limiting the owner set to 20 helps to keep the runtime of the test reasonable.
-        console.log("bounding numOwners");
         numOwners_ = bound(_numOwners, 4, 20);
         // _minOwners must be at least 3, otherwise we don't have any range below _minOwners in which to test all of the
         // ShutDownBehavior options.
-        console.log("bounding minOwners");
         minOwners_ = bound(_minOwners, 3, numOwners_ - 1);
 
         // Ensure that _numLiveOwners is less than _numOwners so that we can remove at least one owner.
-        console.log("bounding numLiveOwners");
         numLiveOwners_ = bound(_numLiveOwners, 0, numOwners_ - 1);
 
         // The above bounds are a bit tricky, so we assert that the resulting parameters enable us to test all possible
@@ -402,20 +399,19 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
 
         // Create an array of live owners, and call showLiveness for each of them
         address[] memory liveOwners = new address[](numLiveOwners);
-        for (uint256 i = 0; i < numLiveOwners; i++) {
+        for (uint256 i; i < numLiveOwners; i++) {
             liveOwners[i] = safeInstance.owners[i];
             vm.prank(safeInstance.owners[i]);
             livenessGuard.showLiveness();
         }
 
         address[] memory nonLiveOwners = new address[](numOwners - numLiveOwners);
-        for (uint256 i = 0; i < numOwners - numLiveOwners; i++) {
+        for (uint256 i; i < numOwners - numLiveOwners; i++) {
             nonLiveOwners[i] = safeInstance.owners[i + numLiveOwners];
         }
 
         address[] memory prevOwners;
         if (numLiveOwners >= minOwners) {
-            console.log("No shutdown");
             // The safe will remain above the minimum number of owners, so we can remove only those owners which are not
             // live.
             prevOwners = safeInstance.getPrevOwners(nonLiveOwners);
@@ -424,10 +420,10 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
             // Validate the resulting state of the Safe
             assertEq(safeInstance.safe.getOwners().length, numLiveOwners);
             assertEq(safeInstance.safe.getThreshold(), get75PercentThreshold(numLiveOwners));
-            for (uint256 i = 0; i < numLiveOwners; i++) {
+            for (uint256 i; i < numLiveOwners; i++) {
                 assertTrue(safeInstance.safe.isOwner(liveOwners[i]));
             }
-            for (uint256 i = 0; i < nonLiveOwners.length; i++) {
+            for (uint256 i; i < nonLiveOwners.length; i++) {
                 assertFalse(safeInstance.safe.isOwner(nonLiveOwners[i]));
             }
         } else {
@@ -439,14 +435,10 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
             // The safe is below the minimum number of owners.
             // The ShutDownBehavior enum determines how we handle this case.
             if (shutDownBehavior == ShutDownBehavior.Correct) {
-                console.log("Correct Shutdown");
                 // We remove all owners, and transfer ownership to the shutDown owner.
                 // but we need to do remove the non-live owners first, so we reverse the owners array, since
                 // the first owners in the array were the ones to call showLiveness.
-                // ownersToRemove = new address[](numOwners);
-                for (uint256 i = 0; i < numOwners; i++) {
-                    // ownersToRemove[numOwners - i - 1] = safeInstance.owners[i];
-                    // ownersToRemove[i] = safeInstance.owners[numOwners - i - 1];
+                for (uint256 i; i < numOwners; i++) {
                     ownersToRemove.push(safeInstance.owners[numOwners - i - 1]);
                 }
                 prevOwners = safeInstance.getPrevOwners(ownersToRemove);
@@ -461,14 +453,11 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
                 // trigger that behavior. We initialize that value here then set it in the if statements below.
                 uint256 numOwnersToRemoveinShutDown;
                 if (shutDownBehavior == ShutDownBehavior.DoesNotRemoveAllOwners) {
-                    console.log("Shutdown DoesNotRemoveAllOwners");
                     // In the DoesNotRemoveAllOwners case, we should have more than 1 of the pre-existing owners
                     // remaining
-                    console.log("bounding numOwnersToRemoveinShutDown");
                     numOwnersToRemoveinShutDown =
                         bound(_numOwnersToRemoveinShutDown, numOwners - minOwners + 1, numOwners - 2);
-                    uint256 i = 0;
-                    for (i; i < numOwnersToRemoveinShutDown; i++) {
+                    for (uint256 i; i < numOwnersToRemoveinShutDown; i++) {
                         // Add non-live owners to remove first
                         if (i < nonLiveOwners.length) {
                             ownersToRemove.push(nonLiveOwners[i]);
@@ -483,11 +472,9 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
                     );
                     livenessModule.removeOwners(prevOwners, ownersToRemove);
                 } else if (shutDownBehavior == ShutDownBehavior.DoesNotTransferToFallbackOwner) {
-                    console.log("Shutdown DoesNotTransferToFallbackOwner");
                     // In the DoesNotRemoveAllOwners case, we should have exactly 1 pre-existing owners remaining
                     numOwnersToRemoveinShutDown = numOwners - 1;
-                    uint256 i = 0;
-                    for (i; i < numOwnersToRemoveinShutDown; i++) {
+                    for (uint256 i; i < numOwnersToRemoveinShutDown; i++) {
                         // Add non-live owners to remove first
                         if (i < nonLiveOwners.length) {
                             ownersToRemove.push(nonLiveOwners[i]);
@@ -503,8 +490,8 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
                 // For both of the incorrect behaviors, verify no change to the Safe state
                 assertEq(safeInstance.safe.getOwners().length, numOwners);
                 assertEq(safeInstance.safe.getThreshold(), get75PercentThreshold(numOwners));
-                for (uint256 j = 0; j < numOwners; j++) {
-                    assertTrue(safeInstance.safe.isOwner(safeInstance.owners[j]));
+                for (uint256 i; i < numOwners; i++) {
+                    assertTrue(safeInstance.safe.isOwner(safeInstance.owners[i]));
                 }
             }
         }

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

@@ -40,7 +40,7 @@ contract SafeSigners_Test is Test, SafeTestTools {
         uint256 numSigs = bound(_numSigs, 1, 25);
 
         (, uint256[] memory keys) = SafeTestLib.makeAddrsAndKeys("getSigsTest", numSigs);
-        for (uint256 i = 0; i < keys.length; i++) {
+        for (uint256 i; i < keys.length; i++) {
             if (sigType(keys[i]) == SigTypes.Contract) {
                 keys[i] =
                     SafeTestLib.encodeSmartContractWalletAsPK(SafeTestLib.decodeSmartContractWalletAsAddress(keys[i]));
@@ -91,7 +91,7 @@ contract SafeSigners_Test is Test, SafeTestTools {
         // the validation checks that the Safe contract performs on the value of s on contract
         // signatures. The Safe contract checks that s correctly points to additional data appended
         // after the signatures, and that the length of the data is within bounds.
-        for (uint256 i = 0; i < contractSigs; i++) {
+        for (uint256 i; i < contractSigs; i++) {
             signatures = bytes.concat(signatures, abi.encode(32, 1));
         }
 

packages/contracts-bedrock/test/safe-tools/SafeTestTools.sol

@@ -212,17 +212,10 @@ library SafeTestLib {
         address[] memory _ownersList
     )
         internal
-        view
-        returns (
-            // pure
-            address prevOwner_
-        )
+        pure
+        returns (address prevOwner_)
     {
-        // console.log("getPrevOwnerFromList");
-        for (uint256 i = 0; i < _ownersList.length; i++) {
-            // console.log(i);
-            // console.log(_owner);
-            // console.log("_ownersList[i]:", _ownersList[i]);
+        for (uint256 i; i < _ownersList.length; i++) {
             if (_ownersList[i] != _owner) continue;
             if (i == 0) {
                 prevOwner_ = SENTINEL_OWNERS;
@@ -230,8 +223,6 @@ library SafeTestLib {
             }
             prevOwner_ = _ownersList[i - 1];
         }
-
-        console.log("prevOwner_:", prevOwner_);
     }
 
     /// @dev Given an array of owners to remove, this function will return an array of the previous owners
@@ -250,7 +241,7 @@ library SafeTestLib {
         OwnerSimulator ownerSimulator = new OwnerSimulator(instance.owners, 1);
         prevOwners_ = new address[](_ownersToRemove.length);
         address[] memory currentOwners;
-        for (uint256 i = 0; i < _ownersToRemove.length; i++) {
+        for (uint256 i; i < _ownersToRemove.length; i++) {
             currentOwners = ownerSimulator.getOwners();
             prevOwners_[i] = SafeTestLib.getPrevOwnerFromList(_ownersToRemove[i], currentOwners);
 

specs/safe-liveness-checking.md

@@ -11,7 +11,11 @@
   - [Owner removal call flow](#owner-removal-call-flow)
   - [Shutdown](#shutdown)
   - [Security Properties](#security-properties)
+    - [In the guard](#in-the-guard)
+    - [In the module](#in-the-module)
   - [Interdependency between the guard and module](#interdependency-between-the-guard-and-module)
+- [Operational considerations](#operational-considerations)
+  - [Manual validation of new owner liveness](#manual-validation-of-new-owner-liveness)
   - [Deploying the liveness checking system](#deploying-the-liveness-checking-system)
   - [Modify the liveness checking system](#modify-the-liveness-checking-system)
     - [Replacing the module](#replacing-the-module)
@@ -45,7 +49,18 @@ For implementing liveness checks a `LivenessGuard` is created which receives the
 each executed transaction, and tracks the latest time at which a transaction was signed by each
 signer. This time is made publicly available by calling a `lastLive(address)(Timestamp)` method.
 
-Signers may also call the contract's `showLiveness()()` method directly in order to prove liveness.
+Owners are recorded in this mapping in one of 4 ways:
+
+1. Upon deployment, the guard reads the current set of owners from the Safe contract.
+1. When a new owner is added to the safe. Similarly, when an owner is removed from the Safe, it's
+   entry is deleted from the mapping.
+1. When a transaction is executed, the signatures on that transaction are passed to the guard and
+   used to identify the signers. If more than the required number of signatures is provided, they
+   are ignored.
+1. An owner may call the contract's `showLiveness()()` method directly in order to prove liveness.
+
+Note that the first two methods do not require the owner to actually sign anything. However these mechanisms
+are necessary to prevent new owners from being removed before they have had a chance to show liveness.
 
 ### The liveness module
 
@@ -85,25 +100,34 @@ sequenceDiagram
 
 ### Shutdown
 
-In the unlikely event that the signer set (`N`) is reduced below the allowed threshold, then (and only then) is a
-   shutdown mechanism activated which removes the existing signers, and hands control of the
-   multisig over to a predetermined entity.
+In the unlikely event that the signer set (`N`) is reduced below the allowed minimum number of
+   owners, then (and only then) is a shutdown mechanism activated which removes the existing
+   signers, and hands control of the multisig over to a predetermined entity.
 
 ### Security Properties
 
 The following security properties must be upheld:
 
+#### In the guard
+
 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 griefed such that their signing is not recorded.
-1. Signers may demonstrate liveness either by signing a transaction or by calling directly to the
+1. An owner cannot be censored or griefed such that their signing is not recorded.
+1. Owners 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. It must be impossible for the guard's `checkTransaction` or `checkAfterExecution` method to
+   permanently revert given any calldata and the current state.
+1. The guard correctly handles updates to the owners list, such that new owners are recorded, and
+   removed owners are deleted.
+   1. An `ownersBefore` enumerable set variable is used to accomplish this, it must be emptied at
+      the end of the `checkAfterExecution` call.
+
+#### In the module
+
 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.
+1. The module only removes an owner if they have not demonstrated liveness during the interval, or
+     if enough other owners have been removed to activate the shutdown mechanism.
+1. The module correctly sets the Safe's threshold upon removing a signer.
 
 Note: neither the module nor guard attempt to prevent a quorum of owners from removing either the liveness
 module or guard. There are legitimate reasons they might wish to do so. Moreover, if such a quorum
@@ -119,6 +143,14 @@ This means that the module can be removed or replaced without any affect on the
 The module however does have a dependency on the guard; if the guard is removed from the Safe, then
 the module will no longer be functional and calls to its `removeOwners` function will revert.
 
+## Operational considerations
+
+### Manual validation of new owner liveness
+
+As [noted above](#the-liveness-guard) newly added owners are recorded in the guard without
+necessarily having signed a transaction. Off-chain validation of the liveness of an address must
+therefore be done prior to adding a new owner.
+
 ### Deploying the liveness checking system
 
 [deploying]: #deploying-the-liveness-checking-system