test(ctb): Add test for not giving to fallback owner

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

@@ -213,7 +213,8 @@ contract LivenessModule is ISemver {
             );
         }
 
-        // Check that the threshold is correct. This check is also correct when there is a single
+        // Check that"LivenessModule: must remove all owners and transfer to fallback owner if numOwners < minOwners"
+        // the threshold is correct. This check is also correct when there is a single
         // owner, because get75PercentThreshold(1) returns 1.
         uint256 threshold = SAFE.getThreshold();
         require(

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

@@ -195,7 +195,7 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
     }
 
     /// @dev Tests if removing all owners works correctly
-    function test_removeOwners_swapToFallBackOwner_reverts() external {
+    function test_removeOwners_swapToFallbackOwner_reverts() external {
         uint256 numOwners = safeInstance.owners.length;
 
         address[] memory ownersToRemove = new address[](numOwners);
@@ -230,8 +230,8 @@ contract LivenessModule_RemoveOwners_TestFail is LivenessModule_TestInit {
         livenessModule.removeOwners(prevOwners, ownersToRemove);
     }
 
-    /// @dev Tests if remove owners reverts if it removes too many owners transferring to the fallback owner
-    function test_removeOwners_belowEmptiedButNotFallback_reverts() external {
+    /// @dev Tests if remove owners reverts if it removes too many owners transferring to the shutDown owner
+    function test_removeOwners_belowEmptiedButNotShutDown_reverts() external {
         // Remove all but one owner
         uint256 numOwners = safeInstance.owners.length - 1;
 
@@ -318,36 +318,66 @@ function get75PercentThreshold(uint256 _numOwners) pure returns (uint256 thresho
 contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
     using SafeTestLib for SafeInstance;
 
-    /// @dev Override the base setUp function, to avoid instantiating an unnecessary Safe
+    address[] ownersToRemove2;
+
+    /// @dev Options for handling the event that the number of owners remaining is less than minOwners
+    enum ShutDownBehavior {
+        Correct, // Correctly removes the owners and transfers to the shutDown owner
+        DoesNotTransferToFallbackOwner, // Removes all but one owner, and does not transfer to the shutDown owner
+        DoesNotRemoveAllOwners // Leaves more than one owner when below minOwners
+    }
+
+    /// @dev This contract inherits the storage layout from the LivenessModule_TestInit contract, but we
+    ///      override the base setUp function, to avoid instantiating an unnecessary Safe and liveness checking system.
     function setUp() public override {
         vm.warp(initTime);
         fallbackOwner = makeAddr("fallbackOwner");
     }
 
-    /// @dev Tests if removing owners works correctly for various safe configurations and numbeers of live owners
-    function testFuzz_removeOwners(uint256 _numOwners, uint256 _minOwners, uint256 _numLiveOwners) external {
-        // _numOwners must be at least 3, so that _minOwners can be set to at least 2 by the following bound() call.
-        _numOwners = bound(_numOwners, 3, 20);
-        // _minOwners must be at least 2, otherwise we don't have any range below _minOwners to test the transfer to
-        // the fallback owner.
-        _minOwners = bound(_minOwners, _numOwners - 1, _numOwners - 1);
+    /// @dev Extracts the setup of the test environment into a separate function.
+    function _prepare(
+        uint256 _numOwners,
+        uint256 _minOwners,
+        uint256 _numLiveOwners
+    )
+        internal
+        returns (uint256 numOwners_, uint256 minOwners_, uint256 numLiveOwners_)
+    {
+        // First we modify the test parameters to ensure that they describe a plausible starting point.
+        //
+        // _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.
-        _numLiveOwners = bound(_numLiveOwners, 0, _numOwners - 1);
-
-        // The above bounds are a bit tricky, so we assert that the result is correct
-        assertTrue(_numOwners > _minOwners && _numOwners > _numLiveOwners && _minOwners >= 2);
+        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
+        // success and revert cases in the removeOwners function.
+        assertTrue(
+            numOwners_ > minOwners_ //
+                && numOwners_ > numLiveOwners_
+            //
+            && minOwners_ >= 3
+        );
+        //
 
         // Create a Safe with _numOwners owners
-        (, uint256[] memory keys) = SafeTestLib.makeAddrsAndKeys("rmOwnersTest", _numOwners);
-        uint256 threshold = get75PercentThreshold(_numOwners);
+        (, uint256[] memory keys) = SafeTestLib.makeAddrsAndKeys("rmOwnersTest", numOwners_);
+        uint256 threshold = get75PercentThreshold(numOwners_);
         safeInstance = _setupSafe(keys, threshold);
         livenessGuard = new LivenessGuard(safeInstance.safe);
         livenessModule = new LivenessModule({
                     _safe: safeInstance.safe,
                     _livenessGuard: livenessGuard,
                     _livenessInterval: livenessInterval,
-                    _minOwners: _minOwners,
+                    _minOwners: minOwners_,
                     _fallbackOwner: fallbackOwner
                 });
         safeInstance.setGuard(address(livenessGuard));
@@ -355,52 +385,121 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
 
         // Warp ahead so that all owners non-live
         _warpPastLivenessInterval();
+    }
+
+    /// @dev Tests if removing owners works correctly for various safe configurations and numbeers of live owners
+    function testFuzz_removeOwners(
+        uint256 _numOwners,
+        uint256 _minOwners,
+        uint256 _numLiveOwners,
+        uint256 _shutDownBehavior,
+        uint256 _numOwnersToRemoveinShutDown
+    )
+        external
+    {
+        // Prepare the test env and test params
+        (uint256 numOwners, uint256 minOwners, uint256 numLiveOwners) = _prepare(_numOwners, _minOwners, _numLiveOwners);
 
         // 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++) {
+        address[] memory liveOwners = new address[](numLiveOwners);
+        for (uint256 i = 0; i < numLiveOwners; i++) {
             liveOwners[i] = safeInstance.owners[i];
             vm.prank(safeInstance.owners[i]);
             livenessGuard.showLiveness();
         }
 
         // Create an array of non-live owners
-        address[] memory nonLiveOwners = new address[](_numOwners - _numLiveOwners);
-        for (uint256 i = 0; i < _numOwners - _numLiveOwners; i++) {
-            nonLiveOwners[i] = safeInstance.owners[i + _numLiveOwners];
+        address[] memory nonLiveOwners = new address[](numOwners - numLiveOwners);
+        for (uint256 i = 0; i < numOwners - numLiveOwners; i++) {
+            nonLiveOwners[i] = safeInstance.owners[i + numLiveOwners];
         }
 
         address[] memory prevOwners;
-        if (_numLiveOwners >= _minOwners) {
+        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);
             livenessModule.removeOwners(prevOwners, nonLiveOwners);
 
             // 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++) {
+            assertEq(safeInstance.safe.getOwners().length, numLiveOwners);
+            assertEq(safeInstance.safe.getThreshold(), get75PercentThreshold(numLiveOwners));
+            for (uint256 i = 0; i < numLiveOwners; i++) {
                 assertTrue(safeInstance.safe.isOwner(liveOwners[i]));
             }
             for (uint256 i = 0; i < nonLiveOwners.length; i++) {
                 assertFalse(safeInstance.safe.isOwner(nonLiveOwners[i]));
             }
         } else {
-            // The safe is below the minimum number of owners, so we can remove all owners,
-            // but we need to do remove the non-live owners first, so we reverse the owners array, since
-            // the first owners have signed recently.
-            address[] memory ownersToRemove = new address[](_numOwners);
-            for (uint256 i = 0; i < _numOwners; i++) {
-                ownersToRemove[_numOwners - i - 1] = safeInstance.owners[i];
+            // The number of non-live owners will push the safe below the minimum number of owners.
+            // We need to test all of the possible ShutDownBehavior options, so we'll create a ShutDownBehavior enum
+            // from the _shutDownBehavior input.
+            ShutDownBehavior shutDownBehavior =
+                ShutDownBehavior(bound(_shutDownBehavior, 0, uint256(type(ShutDownBehavior).max)));
+            // 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 have signed recently.
+                address[] memory ownersToRemove = new address[](numOwners);
+                for (uint256 i = 0; i < numOwners; i++) {
+                    ownersToRemove[numOwners - i - 1] = safeInstance.owners[i];
+                }
+                prevOwners = safeInstance.getPrevOwners(ownersToRemove);
+                livenessModule.removeOwners(prevOwners, ownersToRemove);
+
+                // Validate the resulting state of the Safe
+                assertEq(safeInstance.safe.getOwners().length, 1);
+                assertEq(safeInstance.safe.getOwners()[0], fallbackOwner);
+                assertEq(safeInstance.safe.getThreshold(), 1);
+            } else {
+                // For both of the incorrect behaviors, we need to calculate the number of owners to remove to
+                // 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++) {
+                        // Add non-live owners to remove first
+                        if (i < nonLiveOwners.length) {
+                            ownersToRemove2.push(nonLiveOwners[i]);
+                        } else {
+                            // Then add live owners to remove
+                            ownersToRemove2.push(liveOwners[i - nonLiveOwners.length]);
+                        }
+                    }
+                    prevOwners = safeInstance.getPrevOwners(ownersToRemove2);
+                    vm.expectRevert(
+                        "LivenessModule: must remove all owners and transfer to fallback owner if numOwners < minOwners"
+                    );
+                    livenessModule.removeOwners(prevOwners, ownersToRemove2);
+                } 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++) {
+                        // Add non-live owners to remove first
+                        if (i < nonLiveOwners.length) {
+                            ownersToRemove2.push(nonLiveOwners[i]);
+                        } else {
+                            // Then add live owners to remove
+                            ownersToRemove2.push(liveOwners[i - nonLiveOwners.length]);
+                        }
+                    }
+                    prevOwners = safeInstance.getPrevOwners(ownersToRemove2);
+                    vm.expectRevert("LivenessModule: must transfer ownership to fallback owner");
+                    livenessModule.removeOwners(prevOwners, ownersToRemove2);
+                }
             }
-            prevOwners = safeInstance.getPrevOwners(ownersToRemove);
-            livenessModule.removeOwners(prevOwners, ownersToRemove);
-
-            // Validate the resulting state of the Safe
-            assertEq(safeInstance.safe.getOwners().length, 1);
-            assertEq(safeInstance.safe.getOwners()[0], fallbackOwner);
-            assertEq(safeInstance.safe.getThreshold(), 1);
         }
     }
 }