From f95794055283dc633b041bd647144fa6ac6c4488 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 30 Oct 2024 14:25:03 +0000 Subject: [PATCH 1/4] feat: ownership transfer script can set arbitrary manager --- README.md | 6 ++--- script/TransferOwnership.s.sol | 35 ++++++++++++++++++++--------- test/script/TransferOwnership.t.sol | 17 ++++++++------ 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index a71557d3..aba91df3 100644 --- a/README.md +++ b/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 diff --git a/script/TransferOwnership.s.sol b/script/TransferOwnership.s.sol index 656cfac8..e0cd870b 100644 --- a/script/TransferOwnership.s.sol +++ b/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 { diff --git a/test/script/TransferOwnership.t.sol b/test/script/TransferOwnership.t.sol index 77511606..5b6114aa 100644 --- a/test/script/TransferOwnership.t.sol +++ b/test/script/TransferOwnership.t.sol @@ -30,18 +30,19 @@ 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 { @@ -49,8 +50,10 @@ contract TestTransferOwnership is Test { 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( From 4fa9311a4a8b3f8582d3e2aa7f84f30d4b3b7dab Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 30 Oct 2024 14:37:25 +0000 Subject: [PATCH 2/4] fix: linting issue, case --- test/script/TransferOwnership.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/script/TransferOwnership.t.sol b/test/script/TransferOwnership.t.sol index 5b6114aa..6baace8f 100644 --- a/test/script/TransferOwnership.t.sol +++ b/test/script/TransferOwnership.t.sol @@ -50,10 +50,10 @@ contract TestTransferOwnership is Test { 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"); + address noManager = script.NO_MANAGER(); + require(owner != noManager, "Invalid test setup, owner should not coincide with NO_MANAGER flag address"); TransferOwnership.ScriptParams memory params = - TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, newManager: NO_MANAGER}); + TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, newManager: noManager}); script.runWith(params); From 14c8f9f8fe9199bcd81191b3eede1d217c2440cf Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 30 Oct 2024 14:59:53 +0000 Subject: [PATCH 3/4] chore: remove unnecessary optional manager feature --- script/TransferOwnership.s.sol | 39 +++++++++-------------------- test/script/TransferOwnership.t.sol | 16 ------------ 2 files changed, 12 insertions(+), 43 deletions(-) diff --git a/script/TransferOwnership.s.sol b/script/TransferOwnership.s.sol index e0cd870b..2b53fcf2 100644 --- a/script/TransferOwnership.s.sol +++ b/script/TransferOwnership.s.sol @@ -15,14 +15,10 @@ contract TransferOwnership is NetworksJson { // 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; - /// Contains either the value `NO_MANAGER` if the manager should not be - /// updated or the address of the new manager. address newManager; ERC173 authenticatorProxy; } @@ -50,19 +46,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.newManager != NO_MANAGER) { - console.log( - string.concat( - "Setting new solver manager from ", - vm.toString(authenticator.manager()), - " to ", - vm.toString(params.newManager) - ) - ); - vm.broadcast(msg.sender); - authenticator.setManager(params.newManager); - console.log("Set new solver manager account."); - } + console.log( + string.concat( + "Setting new solver manager from ", + vm.toString(authenticator.manager()), + " to ", + vm.toString(params.newManager) + ) + ); + vm.broadcast(msg.sender); + authenticator.setManager(params.newManager); + console.log("Set new solver manager account."); console.log( string.concat( @@ -79,16 +73,7 @@ contract TransferOwnership is NetworksJson { function paramsFromEnv() internal view returns (ScriptParams memory) { address newOwner = vm.envAddress(INPUT_ENV_NEW_OWNER); - - 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 newManager = vm.envAddress(INPUT_ENV_NEW_MANAGER); address authenticatorProxy; try vm.envAddress(INPUT_ENV_AUTHENTICATOR_PROXY) returns (address env) { diff --git a/test/script/TransferOwnership.t.sol b/test/script/TransferOwnership.t.sol index 6baace8f..ed50785c 100644 --- a/test/script/TransferOwnership.t.sol +++ b/test/script/TransferOwnership.t.sol @@ -45,22 +45,6 @@ contract TestTransferOwnership is Test { 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 noManager = script.NO_MANAGER(); - require(owner != noManager, "Invalid test setup, owner should not coincide with NO_MANAGER flag address"); - TransferOwnership.ScriptParams memory params = - TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, newManager: noManager}); - - script.runWith(params); - - assertEq(proxy.owner(), newOwner, "did not change the owner"); - assertEq(proxyAsAuthenticator.manager(), owner, "changed the manager"); - } - function test_reverts_if_no_proxy_at_target() public { address notAProxy = makeAddr("not a proxy"); TransferOwnership.ScriptParams memory params = TransferOwnership.ScriptParams({ From d3d1a69e411b7edd0a9238c44c60a09da904268d Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 30 Oct 2024 15:02:31 +0000 Subject: [PATCH 4/4] chore: update readme regarding now-mandatory parameter --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index aba91df3..cce53150 100644 --- a/README.md +++ b/README.md @@ -143,7 +143,7 @@ The following parameters can be set: ```sh export ETH_RPC_URL='https://rpc.url.example.com' export NEW_OWNER=0x1111111111111111111111111111111111111111 -export NEW_MANAGER=0x2222222222222222222222222222222222222222 # optional parameter, the manager does not change if this variable is unset +export NEW_MANAGER=0x2222222222222222222222222222222222222222 ``` To test run the script from a specific owner (sender):