From c15d99a146c8aa428a7fea3167c2f3d933b8f7fd Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 30 Oct 2024 17:09:13 +0000 Subject: [PATCH] feat: ownership transfer script can set arbitrary manager (#234) ## Description When the contract is deployed to a new network, we often need to update the manager and the owner to two separate addresses (for example, some administrative and the DAO respectively). These changes make this easier. ## Test Plan Updated tests. Try the updated ## Note I added the `--slow` flag so that the two transactions aren't submitted at the same time. This is to prevent the unlikely scenario where a malicious block builder wouldn't be able to just execute the second transaction by overriding the first from another attempt (which could happen if the first attempt was done with incorrect parameters). --- README.md | 6 ++--- script/TransferOwnership.s.sol | 40 ++++++++++++++--------------- test/script/TransferOwnership.t.sol | 27 +++++-------------- 3 files changed, 29 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index a71557d3..cce53150 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 ``` 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..2b53fcf2 100644 --- a/script/TransferOwnership.s.sol +++ b/script/TransferOwnership.s.sol @@ -11,7 +11,7 @@ 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"; @@ -19,7 +19,7 @@ contract TransferOwnership is NetworksJson { struct ScriptParams { address newOwner; - bool resetManager; + address newManager; ERC173 authenticatorProxy; } @@ -46,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.resetManager) { - console.log( - string.concat( - "Setting new solver manager from ", - vm.toString(authenticator.manager()), - " to ", - vm.toString(params.newOwner) - ) - ); - vm.broadcast(msg.sender); - authenticator.setManager(params.newOwner); - 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( @@ -68,11 +66,14 @@ 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 = vm.envAddress(INPUT_ENV_NEW_MANAGER); address authenticatorProxy; try vm.envAddress(INPUT_ENV_AUTHENTICATOR_PROXY) returns (address env) { @@ -95,11 +96,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..ed50785c 100644 --- a/test/script/TransferOwnership.t.sol +++ b/test/script/TransferOwnership.t.sol @@ -30,32 +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"); - } - - function test_only_transfers_proxy_ownership() public { - address newOwner = makeAddr("TestTransferOwnership: new proxy owner"); - assertEq(proxy.owner(), owner); - assertEq(proxyAsAuthenticator.manager(), owner); - - TransferOwnership.ScriptParams memory params = - TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, resetManager: false}); - - script.runWith(params); - - assertEq(proxy.owner(), newOwner, "did not change the owner"); - assertEq(proxyAsAuthenticator.manager(), owner, "changed the manager"); + assertEq(proxyAsAuthenticator.manager(), newManager, "did not change the manager"); } function test_reverts_if_no_proxy_at_target() public { @@ -63,7 +50,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 +62,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 +86,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(