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 3 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 # optional parameter, the manager does not change if this variable is unset
Copy link
Contributor

@mateo-mro mateo-mro Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for safety purposes, wouldn't it be better than we change manager and owner is optional? mostly because if we change the manager but not the owner, it is easy to fix, but if we change the owner and not the manager, and the owner is a SAFE... it will be hard to amend.

Copy link
Contributor Author

@fedgiac fedgiac Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is intended to be used only during the first deployment of the script, after that both the owner and the manager are expected to be a multisig and this script wouldn't be used anymore.
That is, I wouldn't expect this script to be used to just change the manager.
But this is maybe a good argument to make the manager mandatory and not offer a default behavior if there's no manager.

Copy link
Contributor Author

@fedgiac fedgiac Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the unneeded complexity in 14c8f9f.

Copy link
Contributor

@mateo-mro mateo-mro Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! the README still applies as it is?

Copy link
Contributor Author

@fedgiac fedgiac Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! The only change is in d3d1a69.

```

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
35 changes: 24 additions & 11 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,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);
Copy link
Contributor

@mateo-mro mateo-mro Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this work in order to determine there's no manager?

Copy link
Contributor Author

@fedgiac fedgiac Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a flag to internally communicate in the script that the address was actually unset. This workaround isn't needed after 14c8f9f.


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;
}

Expand All @@ -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.");
}

Expand All @@ -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) {
Expand All @@ -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 {
Expand Down
17 changes: 10 additions & 7 deletions test/script/TransferOwnership.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 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, resetManager: false});
TransferOwnership.ScriptParams({newOwner: newOwner, authenticatorProxy: proxy, newManager: noManager});

script.runWith(params);

Expand All @@ -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), ".")));
Expand All @@ -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(
Expand All @@ -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(
Expand Down
Loading