Skip to content

feat: ownership transfer script can set arbitrary manager #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 30, 2024
Merged

feat: ownership transfer script can set arbitrary manager #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f957940
feat: ownership transfer script can set arbitrary manager
fedgiac Oct 30, 2024
4fa9311
fix: linting issue, case
fedgiac Oct 30, 2024
b8caeb0
Merge branch 'main' into specify-new-manager
fedgiac Oct 30, 2024
14c8f9f
chore: remove unnecessary optional manager feature
fedgiac Oct 30, 2024
d3d1a69
chore: update readme regarding now-mandatory parameter
fedgiac Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading