Skip to content
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.

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

About proxyAdmin for TransparentUpgradeableProxy.sol #5149

Open
idyllsss opened this issue Aug 15, 2024 · 2 comments
Open

About proxyAdmin for TransparentUpgradeableProxy.sol #5149

idyllsss opened this issue Aug 15, 2024 · 2 comments

Comments

@idyllsss
Copy link

🧐 Motivation
My concern is whether it is reasonable to forcibly generate a new ProxyAdmin in the constructor of TransparentUpgradeableProxy.sol:

📝 Details
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/transparent/TransparentUpgradeableProxy.sol

constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
    _admin = address(new ProxyAdmin(initialOwner));
    // Set the storage value and emit an event for ERC-1967 compatibility
    ERC1967Utils.changeAdmin(_proxyAdmin());
}

If I want to use a TimelockController as the ProxyAdmin, it becomes very difficult. The contract's extensibility seems to be reduced.

@ernestognw
Copy link
Member

Hi @idyllsss,

We had a long discussion around this and concluded that both contracts are tightly coupled since the ProxyAdmin can't do other than upgrade its corresponding proxy and the proxy doesn't allow the admin to interact in any other way.

The case where a TimelockController is the upgrader, then the ProxyAdmin can be owned by a TimelockController instead. Making the TimelockController the ProxyAdmin itself will prevent it from performing any other action (e.g. if your proxy is a token and the TimelockController has balance, it won't be able to transfer if it's the admin).

Let me know if that works!

@sakulstra
Copy link

sakulstra commented Nov 12, 2024

@ernestognw Hey, also just stumbled over this.

For us the usecase is slightly different.
Currently, on aave there are multiple "Executors(I guess similar to timelockControllers)", from which each owns a ProxyAdmin and these proxy admins control the TransparentUpgradeableProxy instances.
So there is 1 ProxyAdmin per executor and when the executor ever migrates, the ProxyAdmin owner will be replaced.

With the new method of having a forced ProxyAdmin per TransparentUpgradeableProxy this pattern is no longer possible.
Therefore it would be great if we could pass our existing admin instead of creating a new one per contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants