feat: ownership transfer script can set arbitrary manager

Author: fedgiac

README.md

@@ -142,8 +142,8 @@ The following parameters can be set:
 
 ```sh
 export ETH_RPC_URL='https://rpc.url.example.com'
-export NEW_OWNER=0x1111111111111111111111111111111111111111 
-export RESET_MANAGER=true # true if the new owner should also become the manager, false otherwise
+export NEW_OWNER=0x1111111111111111111111111111111111111111
+export NEW_MANAGER=0x2222222222222222222222222222222222222222 # optional parameter, the manager does not change if this variable is unset 
 ```
 
 To test run the script from a specific owner (sender):
@@ -155,7 +155,7 @@ forge script script/TransferOwnership.s.sol:TransferOwnership --rpc-url "$ETH_RP
 To actually execute the transaction:
 
 ```sh
-forge script script/TransferOwnership.s.sol:TransferOwnership --rpc-url "$ETH_RPC_URL" --private-key 0x0000000000000000000000000000000000000000000000000000000000000001 --broadcast
+forge script script/TransferOwnership.s.sol:TransferOwnership --rpc-url "$ETH_RPC_URL" --private-key 0x0000000000000000000000000000000000000000000000000000000000000001 --broadcast --slow
 ```
 
 ## Releases

script/TransferOwnership.s.sol

@@ -11,15 +11,19 @@ import {NetworksJson} from "./lib/NetworksJson.sol";
 contract TransferOwnership is NetworksJson {
     // Required input
     string private constant INPUT_ENV_NEW_OWNER = "NEW_OWNER";
-    string private constant INPUT_ENV_RESET_MANAGER = "RESET_MANAGER";
+    string private constant INPUT_ENV_NEW_MANAGER = "NEW_MANAGER";
     // Optional input
     string private constant INPUT_ENV_AUTHENTICATOR_PROXY = "AUTHENTICATOR_PROXY";
 
+    address public constant NO_MANAGER = address(0);
+
     NetworksJson internal networksJson;
 
     struct ScriptParams {
         address newOwner;
-        bool resetManager;
+        /// Contains either the value `NO_MANAGER` if the manager should not be
+        /// updated or the address of the new manager.
+        address newManager;
         ERC173 authenticatorProxy;
     }
 
@@ -46,17 +50,17 @@ contract TransferOwnership is NetworksJson {
 
         // Make sure to reset the manager BEFORE transferring ownership, or else
         // we will not be able to do it once we lose permissions.
-        if (params.resetManager) {
+        if (params.newManager != NO_MANAGER) {
             console.log(
                 string.concat(
                     "Setting new solver manager from ",
                     vm.toString(authenticator.manager()),
                     " to ",
-                    vm.toString(params.newOwner)
+                    vm.toString(params.newManager)
                 )
             );
             vm.broadcast(msg.sender);
-            authenticator.setManager(params.newOwner);
+            authenticator.setManager(params.newManager);
             console.log("Set new solver manager account.");
         }
 
@@ -68,11 +72,23 @@ contract TransferOwnership is NetworksJson {
         vm.broadcast(msg.sender);
         params.authenticatorProxy.transferOwnership(params.newOwner);
         console.log("Set new owner of the authenticator proxy.");
+
+        console.log(string.concat("Final owner:   ", vm.toString(params.authenticatorProxy.owner())));
+        console.log(string.concat("Final manager: ", vm.toString(authenticator.manager())));
     }
 
     function paramsFromEnv() internal view returns (ScriptParams memory) {
         address newOwner = vm.envAddress(INPUT_ENV_NEW_OWNER);
-        bool resetManager = vm.envBool(INPUT_ENV_RESET_MANAGER);
+
+        address newManager;
+        try vm.envAddress(INPUT_ENV_NEW_MANAGER) returns (address env) {
+            if (env == NO_MANAGER) {
+                revert(string.concat("Invalid parameter: cannot update the manager to address ", vm.toString(env)));
+            }
+            newManager = env;
+        } catch {
+            newManager = NO_MANAGER;
+        }
 
         address authenticatorProxy;
         try vm.envAddress(INPUT_ENV_AUTHENTICATOR_PROXY) returns (address env) {
@@ -95,11 +111,8 @@ contract TransferOwnership is NetworksJson {
             }
         }
 
-        return ScriptParams({
-            newOwner: newOwner,
-            resetManager: resetManager,
-            authenticatorProxy: ERC173(authenticatorProxy)
-        });
+        return
+            ScriptParams({newOwner: newOwner, newManager: newManager, authenticatorProxy: ERC173(authenticatorProxy)});
     }
 
     function checkIsProxy(address candidate) internal view {

test/script/TransferOwnership.t.sol

@@ -30,27 +30,30 @@ contract TestTransferOwnership is Test {
         proxyAsAuthenticator = GPv2AllowListAuthentication(deployed);
     }
 
-    function test_transfers_proxy_ownership_and_resets_manager() public {
+    function test_transfers_proxy_ownership_and_updates_manager() public {
         address newOwner = makeAddr("TestTransferOwnership: new proxy owner");
+        address newManager = makeAddr("TestTransferOwnership: new authenticator manager");
         assertEq(proxy.owner(), owner);
         assertEq(proxyAsAuthenticator.manager(), owner);
 
         TransferOwnership.ScriptParams memory params =
-            TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, resetManager: true});
+            TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, newManager: newManager});
 
         script.runWith(params);
 
         assertEq(proxy.owner(), newOwner, "did not change the owner");
-        assertEq(proxyAsAuthenticator.manager(), newOwner, "did not change the manager");
+        assertEq(proxyAsAuthenticator.manager(), newManager, "did not change the manager");
     }
 
     function test_only_transfers_proxy_ownership() public {
         address newOwner = makeAddr("TestTransferOwnership: new proxy owner");
         assertEq(proxy.owner(), owner);
         assertEq(proxyAsAuthenticator.manager(), owner);
 
+        address NO_MANAGER = script.NO_MANAGER();
+        require(owner != NO_MANAGER, "Invalid test setup, owner should not coincide with NO_MANAGER flag address");
         TransferOwnership.ScriptParams memory params =
-            TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, resetManager: false});
+            TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, newManager: NO_MANAGER});
 
         script.runWith(params);
 
@@ -63,7 +66,7 @@ contract TestTransferOwnership is Test {
         TransferOwnership.ScriptParams memory params = TransferOwnership.ScriptParams({
             newOwner: makeAddr("some owner"),
             authenticatorProxy: ERC173(notAProxy),
-            resetManager: false
+            newManager: makeAddr("some manager")
         });
 
         vm.expectRevert(bytes(string.concat("No code at target authenticator proxy ", vm.toString(notAProxy), ".")));
@@ -75,7 +78,7 @@ contract TestTransferOwnership is Test {
         TransferOwnership.ScriptParams memory params = TransferOwnership.ScriptParams({
             newOwner: makeAddr("some owner"),
             authenticatorProxy: ERC173(noERC173Proxy),
-            resetManager: false
+            newManager: makeAddr("some manager")
         });
         vm.etch(noERC173Proxy, hex"1337");
         vm.mockCall(
@@ -99,7 +102,7 @@ contract TestTransferOwnership is Test {
         TransferOwnership.ScriptParams memory params = TransferOwnership.ScriptParams({
             newOwner: makeAddr("some owner"),
             authenticatorProxy: ERC173(revertingProxy),
-            resetManager: false
+            newManager: makeAddr("some manager")
         });
         vm.etch(revertingProxy, hex"1337");
         vm.mockCallRevert(