Skip to content

Commit

Permalink
feat: ownership transfer script can set arbitrary manager (#234)
Browse files Browse the repository at this point in the history 
## 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).
  • Loading branch information
@fedgiac
fedgiac authored Oct 30, 2024
1 parent f3a13f8 commit c15d99a
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 44 deletions.
6 changes: 3 additions & 3 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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
Expand Down
40 changes: 19 additions & 21 deletions script/TransferOwnership.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ 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";

NetworksJson internal networksJson;

struct ScriptParams {
address newOwner;
bool resetManager;
address newManager;
ERC173 authenticatorProxy;
}

Expand All @@ -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(
Expand All @@ -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) {
Expand All @@ -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 {
Expand Down
27 changes: 7 additions & 20 deletions test/script/TransferOwnership.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,40 +30,27 @@ 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 {
address notAProxy = makeAddr("not a proxy");
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), ".")));
Expand All @@ -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(
Expand All @@ -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(
Expand Down

0 comments on commit c15d99a

Please sign in to comment.