ctb: Add removeDeployerFromSafe method to deploy scripts (#10620)

* ctb: Add removeDeployerFromSafe() method

* Delete extra line

* ctb: Add check to ensure deploy is removed from Safe

packages/contracts-bedrock/scripts/Deploy.s.sol

@@ -8,6 +8,7 @@ import { console2 as console } from "forge-std/console2.sol";
 import { stdJson } from "forge-std/StdJson.sol";
 
 import { GnosisSafe as Safe } from "safe-contracts/GnosisSafe.sol";
+import { OwnerManager } from "safe-contracts/base/OwnerManager.sol";
 import { GnosisSafeProxyFactory as SafeProxyFactory } from "safe-contracts/proxies/GnosisSafeProxyFactory.sol";
 import { Enum as SafeOps } from "safe-contracts/common/Enum.sol";
 
@@ -426,6 +427,13 @@ contract Deploy is Deployer {
         addr_ = deploySafe(_name, owners, 1, true);
     }
 
+    /// @notice Deploy a new Safe contract. If the keepDeployer option is used to enable further setup actions, then
+    ///         the removeDeployerFromSafe() function should be called on that safe after setup is complete.
+    ///         Note this function does not have the broadcast modifier.
+    /// @param _name The name of the Safe to deploy.
+    /// @param _owners The owners of the Safe.
+    /// @param _threshold The threshold of the Safe.
+    /// @param _keepDeployer Wether or not the deployer address will be added as an owner of the Safe.
     function deploySafe(
         string memory _name,
         address[] memory _owners,
@@ -462,6 +470,24 @@ contract Deploy is Deployer {
         console.log("New safe: %s deployed at %s\n    Note that this safe is owned by the deployer key", _name, addr_);
     }
 
+    /// @notice If the keepDeployer option was used with deploySafe(), this function can be used to remove the deployer.
+    ///         Note this function does not have the broadcast modifier.
+    function removeDeployerFromSafe(string memory _name, uint256 _newThreshold) public {
+        Safe safe = Safe(mustGetAddress(_name));
+
+        // The sentinel address is used to mark the start and end of the linked list of owners in the Safe.
+        address sentinelOwners = address(0x1);
+
+        // Because deploySafe() always adds msg.sender first (if keepDeployer is true), we know that the previousOwner
+        // will be sentinelOwners.
+        _callViaSafe({
+            _safe: safe,
+            _target: address(safe),
+            _data: abi.encodeCall(OwnerManager.removeOwner, (sentinelOwners, msg.sender, _newThreshold))
+        });
+        console.log("Removed deployer owner from ", _name);
+    }
+
     /// @notice Deploy the AddressManager
     function deployAddressManager() public broadcast returns (address addr_) {
         console.log("Deploying AddressManager");

packages/contracts-bedrock/scripts/DeployOwnership.s.sol

@@ -50,9 +50,6 @@ struct GuardianConfig {
     DeputyGuardianModuleConfig deputyGuardianModuleConfig;
 }
 
-// The sentinel address is used to mark the start and end of the linked list of owners in the Safe.
-address constant SENTINEL_OWNERS = address(0x1);
-
 /// @title Deploy
 /// @notice Script used to deploy and configure the Safe contracts which are used to manage the Superchain,
 ///         as the ProxyAdminOwner and other roles in the system. Note that this script is not executable in a
@@ -84,7 +81,7 @@ contract DeployOwnership is Deploy {
     }
 
     /// @notice Returns a GuardianConfig similar to that of the Guardian Safe on Mainnet.
-    function _getExampleGuardianConfig() internal returns (GuardianConfig memory guardianConfig_) {
+    function _getExampleGuardianConfig() internal view returns (GuardianConfig memory guardianConfig_) {
         address[] memory exampleGuardianOwners = new address[](1);
         exampleGuardianOwners[0] = mustGetAddress("SecurityCouncilSafe");
         guardianConfig_ = GuardianConfig({
@@ -161,7 +158,6 @@ contract DeployOwnership is Deploy {
     /// @notice Deploy a DeputyGuardianModule for use on the Security Council Safe.
     ///         Note this function does not have the broadcast modifier.
     function deployDeputyGuardianModule() public returns (address addr_) {
-        SecurityCouncilConfig memory councilConfig = _getExampleCouncilConfig();
         Safe councilSafe = Safe(payable(mustGetAddress("SecurityCouncilSafe")));
         DeputyGuardianModuleConfig memory deputyGuardianModuleConfig =
             _getExampleGuardianConfig().deputyGuardianModuleConfig;
@@ -203,23 +199,16 @@ contract DeployOwnership is Deploy {
 
     /// @notice Configure the Guardian Safe with the DeputyGuardianModule.
     function configureGuardianSafe() public broadcast returns (address addr_) {
-        Safe safe = Safe(payable(mustGetAddress("GuardianSafe")));
+        addr_ = mustGetAddress("GuardianSafe");
         address deputyGuardianModule = deployDeputyGuardianModule();
         _callViaSafe({
-            _safe: safe,
-            _target: address(safe),
+            _safe: Safe(payable(addr_)),
+            _target: addr_,
             _data: abi.encodeCall(ModuleManager.enableModule, (deputyGuardianModule))
         });
 
-        // Remove the deployer address (msg.sender) which was used to setup the Security Council Safe thus far
-        // this call is also used to update the threshold.
-        // Because deploySafe() always adds msg.sender first (if keepDeployer is true), we know that the previousOwner
-        // will be SENTINEL_OWNERS.
-        _callViaSafe({
-            _safe: safe,
-            _target: address(safe),
-            _data: abi.encodeCall(OwnerManager.removeOwner, (SENTINEL_OWNERS, msg.sender, 1))
-        });
+        // Finalize configuration by removing the additional deployer key.
+        removeDeployerFromSafe({ _name: "GuardianSafe", _newThreshold: 1 });
         console.log("DeputyGuardianModule enabled on GuardianSafe");
     }
 
@@ -242,22 +231,15 @@ contract DeployOwnership is Deploy {
             _data: abi.encodeCall(ModuleManager.enableModule, (livenessModule))
         });
 
-        // Remove the deployer address (msg.sender) which was used to setup the Security Council Safe thus far
-        // this call is also used to update the threshold.
-        // Because deploySafe() always adds msg.sender first (if keepDeployer is true), we know that the previousOwner
-        // will be SENTINEL_OWNERS.
-        _callViaSafe({
-            _safe: safe,
-            _target: address(safe),
-            _data: abi.encodeCall(
-                OwnerManager.removeOwner, (SENTINEL_OWNERS, msg.sender, exampleCouncilConfig.safeConfig.threshold)
-            )
-        });
+        // Finalize configuration by removing the additional deployer key.
+        removeDeployerFromSafe({ _name: "SecurityCouncilSafe", _newThreshold: exampleCouncilConfig.safeConfig.threshold });
+
         address[] memory owners = safe.getOwners();
         require(
             safe.getThreshold() == LivenessModule(livenessModule).getRequiredThreshold(owners.length),
             "Safe threshold must be equal to the LivenessModule's required threshold"
         );
+
         addr_ = address(safe);
         console.log("Deployed and configured the Security Council Safe!");
     }

packages/contracts-bedrock/test/Safe/DeployOwnership.t.sol

@@ -36,6 +36,7 @@ contract DeployOwnershipTest is Test, DeployOwnership {
 
         address[] memory safeOwners = _safe.getOwners();
         assertEq(_safeConfig.owners.length, safeOwners.length);
+        assertFalse(_safe.isOwner(msg.sender));
         for (uint256 i = 0; i < safeOwners.length; i++) {
             assertEq(safeOwners[i], _safeConfig.owners[i]);
         }