From 045c5dba868865368ca29a1d53d6ffec0d5c3283 Mon Sep 17 00:00:00 2001 From: Lukas Date: Thu, 2 Jan 2025 20:15:37 +0100 Subject: [PATCH] feat: remove custom proxy admin --- .../transparent-proxy/ProxyAdmin.sol | 45 ------- .../TransparentProxyFactoryBase.sol | 24 ++-- .../TransparentUpgradeableProxy.sol | 125 ------------------ .../interfaces/IProxyAdminOzV4.sol | 15 +++ .../interfaces/ITransparentProxyFactory.sol | 8 +- test/PermissionlessRescuable.t.sol | 2 +- test/TransparentProxyFactory.t.sol | 12 +- test/UpgradeableOwnableWithGuardian.t.sol | 2 +- .../test/TransparentProxyFactoryZkSync.t.sol | 15 +-- 9 files changed, 47 insertions(+), 201 deletions(-) delete mode 100644 src/contracts/transparent-proxy/ProxyAdmin.sol delete mode 100644 src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol create mode 100644 src/contracts/transparent-proxy/interfaces/IProxyAdminOzV4.sol diff --git a/src/contracts/transparent-proxy/ProxyAdmin.sol b/src/contracts/transparent-proxy/ProxyAdmin.sol deleted file mode 100644 index 161ca6e..0000000 --- a/src/contracts/transparent-proxy/ProxyAdmin.sol +++ /dev/null @@ -1,45 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v5.0.0) (proxy/transparent/ProxyAdmin.sol) - -pragma solidity ^0.8.20; - -import {ITransparentUpgradeableProxy} from './TransparentUpgradeableProxy.sol'; -import {Ownable} from 'openzeppelin-contracts/contracts/access/Ownable.sol'; - -/** - * @dev This is an auxiliary contract meant to be assigned as the admin of a {TransparentUpgradeableProxy}. For an - * explanation of why you would want to use this see the documentation for {TransparentUpgradeableProxy}. - */ -contract ProxyAdmin is Ownable { - /** - * @dev The version of the upgrade interface of the contract. If this getter is missing, both `upgrade(address)` - * and `upgradeAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called, - * while `upgradeAndCall` will invoke the `receive` function if the second argument is the empty byte string. - * If the getter returns `"5.0.0"`, only `upgradeAndCall(address,bytes)` is present, and the second argument must - * be the empty byte string if no function should be called, making it impossible to invoke the `receive` function - * during an upgrade. - */ - string public constant UPGRADE_INTERFACE_VERSION = '5.0.0'; - - /** - * @dev Sets the initial owner who can perform upgrades. - */ - constructor(address initialOwner) Ownable(initialOwner) {} - - /** - * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. - * See {TransparentUpgradeableProxy-_dispatchUpgradeToAndCall}. - * - * Requirements: - * - * - This contract must be the admin of `proxy`. - * - If `data` is empty, `msg.value` must be zero. - */ - function upgradeAndCall( - ITransparentUpgradeableProxy proxy, - address implementation, - bytes memory data - ) public payable virtual onlyOwner { - proxy.upgradeToAndCall{value: msg.value}(implementation, data); - } -} diff --git a/src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol b/src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol index 68ec22d..db3f5ae 100644 --- a/src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol +++ b/src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol @@ -1,9 +1,9 @@ // SPDX-License-Identifier: MIT pragma solidity >=0.8.0; +import {TransparentUpgradeableProxy} from 'openzeppelin-contracts/contracts/proxy/transparent/TransparentUpgradeableProxy.sol'; +import {ProxyAdmin} from 'openzeppelin-contracts/contracts/proxy/transparent/ProxyAdmin.sol'; import {ITransparentProxyFactory} from './interfaces/ITransparentProxyFactory.sol'; -import {TransparentUpgradeableProxy} from './TransparentUpgradeableProxy.sol'; -import {ProxyAdmin} from './ProxyAdmin.sol'; /** * @title TransparentProxyFactory @@ -15,10 +15,14 @@ import {ProxyAdmin} from './ProxyAdmin.sol'; **/ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory { /// @inheritdoc ITransparentProxyFactory - function create(address logic, ProxyAdmin admin, bytes calldata data) external returns (address) { - address proxy = address(new TransparentUpgradeableProxy(logic, admin, data)); + function create( + address logic, + address adminOwner, + bytes calldata data + ) external returns (address) { + address proxy = address(new TransparentUpgradeableProxy(logic, adminOwner, data)); - emit ProxyCreated(proxy, logic, address(admin)); + emit ProxyCreated(proxy, logic, address(adminOwner)); return proxy; } @@ -33,13 +37,13 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory { /// @inheritdoc ITransparentProxyFactory function createDeterministic( address logic, - ProxyAdmin admin, + address adminOwner, bytes calldata data, bytes32 salt ) external returns (address) { - address proxy = address(new TransparentUpgradeableProxy{salt: salt}(logic, admin, data)); + address proxy = address(new TransparentUpgradeableProxy{salt: salt}(logic, adminOwner, data)); - emit ProxyDeterministicCreated(proxy, logic, address(admin), salt); + emit ProxyDeterministicCreated(proxy, logic, address(adminOwner), salt); return proxy; } @@ -57,7 +61,7 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory { /// @inheritdoc ITransparentProxyFactory function predictCreateDeterministic( address logic, - ProxyAdmin admin, + address admin, bytes calldata data, bytes32 salt ) public view returns (address) { @@ -66,7 +70,7 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory { address(this), salt, type(TransparentUpgradeableProxy).creationCode, - abi.encode(logic, address(admin), data) + abi.encode(logic, admin, data) ); } diff --git a/src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol b/src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol deleted file mode 100644 index a7c97dc..0000000 --- a/src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol +++ /dev/null @@ -1,125 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v5.0.0) (proxy/transparent/TransparentUpgradeableProxy.sol) - -/** - * Adaptation of OpenZeppelin's TransparentUpgradeableProxy contract by BGD Labs. - * The original contract creates a new proxy admin per contract. - * In the context of AAVE this is suboptimal, as it is more efficient to have a single proxy admin for all proxies. - * This way, if an executor is ever migrated to a new address only the ownership of that single proxy admin has to ever change. - */ -pragma solidity ^0.8.20; - -import {ERC1967Utils} from 'openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Utils.sol'; -import {ERC1967Proxy} from 'openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol'; -import {IERC1967} from 'openzeppelin-contracts/contracts/interfaces/IERC1967.sol'; -import {ProxyAdmin} from './ProxyAdmin.sol'; - -/** - * @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy} - * does not implement this interface directly, and its upgradeability mechanism is implemented by an internal dispatch - * mechanism. The compiler is unaware that these functions are implemented by {TransparentUpgradeableProxy} and will not - * include them in the ABI so this interface must be used to interact with it. - */ -interface ITransparentUpgradeableProxy is IERC1967 { - function upgradeToAndCall(address, bytes calldata) external payable; -} - -/** - * @dev This contract implements a proxy that is upgradeable through an associated {ProxyAdmin} instance. - * - * To avoid https://medium.com/nomic-labs-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357[proxy selector - * clashing], which can potentially be used in an attack, this contract uses the - * https://blog.openzeppelin.com/the-transparent-proxy-pattern/[transparent proxy pattern]. This pattern implies two - * things that go hand in hand: - * - * 1. If any account other than the admin calls the proxy, the call will be forwarded to the implementation, even if - * that call matches the {ITransparentUpgradeableProxy-upgradeToAndCall} function exposed by the proxy itself. - * 2. If the admin calls the proxy, it can call the `upgradeToAndCall` function but any other call won't be forwarded to - * the implementation. If the admin tries to call a function on the implementation it will fail with an error indicating - * the proxy admin cannot fallback to the target implementation. - * - * These properties mean that the admin account can only be used for upgrading the proxy, so it's best if it's a - * dedicated account that is not used for anything else. This will avoid headaches due to sudden errors when trying to - * call a function from the proxy implementation. You should think of the `ProxyAdmin` instance as the administrative - * interface of the proxy, including the ability to change who can trigger upgrades by transferring ownership. - * - * NOTE: The real interface of this proxy is that defined in `ITransparentUpgradeableProxy`. This contract does not - * inherit from that interface, and instead `upgradeToAndCall` is implicitly implemented using a custom dispatch - * mechanism in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to - * fully implement transparency without decoding reverts caused by selector clashes between the proxy and the - * implementation. - * - * NOTE: This proxy does not inherit from {Context} deliberately. The {ProxyAdmin} of this contract won't send a - * meta-transaction in any way, and any other meta-transaction setup should be made in the implementation contract. - * - * IMPORTANT: This contract avoids unnecessary storage reads by setting the admin only during construction as an - * immutable variable, preventing any changes thereafter. However, the admin slot defined in ERC-1967 can still be - * overwritten by the implementation logic pointed to by this proxy. In such cases, the contract may end up in an - * undesirable state where the admin slot is different from the actual admin. - * - * WARNING: It is not recommended to extend this contract to add additional external functions. If you do so, the - * compiler will not check that there are no selector conflicts, due to the note above. A selector clash between any new - * function and the functions declared in {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This - * could render the `upgradeToAndCall` function inaccessible, preventing upgradeability and compromising transparency. - */ -contract TransparentUpgradeableProxy is ERC1967Proxy { - // An immutable address for the admin to avoid unnecessary SLOADs before each call - // at the expense of removing the ability to change the admin once it's set. - // This is acceptable if the admin is always a ProxyAdmin instance or similar contract - // with its own ability to transfer the permissions to another account. - address private immutable _admin; - - /** - * @dev The proxy caller is the current admin, and can't fallback to the proxy target. - */ - error ProxyDeniedAdminAccess(); - - /** - * @dev Initializes an upgradeable proxy managed by an instance of a {ProxyAdmin} with an `initialOwner`, - * backed by the implementation at `_logic`, and optionally initialized with `_data` as explained in - * {ERC1967Proxy-constructor}. - */ - constructor( - address _logic, - ProxyAdmin initialOwner, - bytes memory _data - ) payable ERC1967Proxy(_logic, _data) { - _admin = address(initialOwner); - // Set the storage value and emit an event for ERC-1967 compatibility - ERC1967Utils.changeAdmin(_proxyAdmin()); - } - - /** - * @dev Returns the admin of this proxy. - */ - function _proxyAdmin() internal virtual returns (address) { - return _admin; - } - - /** - * @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior. - */ - function _fallback() internal virtual override { - if (msg.sender == _proxyAdmin()) { - if (msg.sig != ITransparentUpgradeableProxy.upgradeToAndCall.selector) { - revert ProxyDeniedAdminAccess(); - } else { - _dispatchUpgradeToAndCall(); - } - } else { - super._fallback(); - } - } - - /** - * @dev Upgrade the implementation of the proxy. See {ERC1967Utils-upgradeToAndCall}. - * - * Requirements: - * - * - If `data` is empty, `msg.value` must be zero. - */ - function _dispatchUpgradeToAndCall() private { - (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); - ERC1967Utils.upgradeToAndCall(newImplementation, data); - } -} diff --git a/src/contracts/transparent-proxy/interfaces/IProxyAdminOzV4.sol b/src/contracts/transparent-proxy/interfaces/IProxyAdminOzV4.sol new file mode 100644 index 0000000..b9e092b --- /dev/null +++ b/src/contracts/transparent-proxy/interfaces/IProxyAdminOzV4.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.10; + +/** + * The package relies on OZ-v5, some legacy contracts rely on the oz v4 versions of `TransparentUpgradeableProxy` and `ProxyAdmin`. + * While we no longer recommend deploying new instances of these, we expose the interface to allow interacting with existing contracts. + */ + +interface IProxyAdminOzV4 { + function changeProxyAdmin(address proxy, address newAdmin) external; + + function upgrade(address proxy, address implementation) external; + + function upgradeAndCall(address proxy, address implementation, bytes memory data) external; +} diff --git a/src/contracts/transparent-proxy/interfaces/ITransparentProxyFactory.sol b/src/contracts/transparent-proxy/interfaces/ITransparentProxyFactory.sol index 52fe49c..d3151e3 100644 --- a/src/contracts/transparent-proxy/interfaces/ITransparentProxyFactory.sol +++ b/src/contracts/transparent-proxy/interfaces/ITransparentProxyFactory.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity >=0.8.0; -import {ProxyAdmin} from '../ProxyAdmin.sol'; +import {ProxyAdmin} from 'openzeppelin-contracts/contracts/proxy/transparent/ProxyAdmin.sol'; interface ITransparentProxyFactory { event ProxyCreated(address proxy, address indexed logic, address indexed proxyAdmin); @@ -28,7 +28,7 @@ interface ITransparentProxyFactory { * for an `initialize` function being `function initialize(uint256 foo) external initializer;` * @return address The address of the proxy deployed **/ - function create(address logic, ProxyAdmin admin, bytes memory data) external returns (address); + function create(address logic, address admin, bytes memory data) external returns (address); /** * @notice Creates a proxyAdmin instance, and transfers ownership to provided owner @@ -51,7 +51,7 @@ interface ITransparentProxyFactory { **/ function createDeterministic( address logic, - ProxyAdmin admin, + address admin, bytes memory data, bytes32 salt ) external returns (address); @@ -80,7 +80,7 @@ interface ITransparentProxyFactory { **/ function predictCreateDeterministic( address logic, - ProxyAdmin admin, + address admin, bytes calldata data, bytes32 salt ) external view returns (address); diff --git a/test/PermissionlessRescuable.t.sol b/test/PermissionlessRescuable.t.sol index 6ee4821..1791c3a 100644 --- a/test/PermissionlessRescuable.t.sol +++ b/test/PermissionlessRescuable.t.sol @@ -60,7 +60,7 @@ contract PermissionlessRescuableTest is Test { rescuable = new PermissionlessRescuable(fundsReceiver, address(restrictedMockToken)); } - function test_whoShouldReceiveFunds() public { + function test_whoShouldReceiveFunds() public view { assertEq(rescuable.whoShouldReceiveFunds(), fundsReceiver); } diff --git a/test/TransparentProxyFactory.t.sol b/test/TransparentProxyFactory.t.sol index 5108103..5738727 100644 --- a/test/TransparentProxyFactory.t.sol +++ b/test/TransparentProxyFactory.t.sol @@ -3,9 +3,9 @@ pragma solidity ^0.8.0; import 'forge-std/Test.sol'; import {Ownable} from 'openzeppelin-contracts/contracts/access/Ownable.sol'; -import {ProxyAdmin} from '../src/contracts/transparent-proxy/ProxyAdmin.sol'; +import {TransparentUpgradeableProxy} from 'openzeppelin-contracts/contracts/proxy/transparent/TransparentUpgradeableProxy.sol'; +import {ProxyAdmin} from 'openzeppelin-contracts/contracts/proxy/transparent/ProxyAdmin.sol'; import {TransparentProxyFactory} from '../src/contracts/transparent-proxy/TransparentProxyFactory.sol'; -import {TransparentUpgradeableProxy} from '../src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol'; import {MockImpl} from '../src/mocks/MockImpl.sol'; contract TestTransparentProxyFactory is Test { @@ -26,12 +26,12 @@ contract TestTransparentProxyFactory is Test { address predictedAddress1 = factory.predictCreateDeterministic( address(mockImpl), - ProxyAdmin(admin), + admin, data, salt ); - address proxy1 = factory.createDeterministic(address(mockImpl), ProxyAdmin(admin), data, salt); + address proxy1 = factory.createDeterministic(address(mockImpl), admin, data, salt); assertEq(predictedAddress1, proxy1); assertEq(MockImpl(proxy1).getFoo(), FOO); @@ -53,14 +53,14 @@ contract TestTransparentProxyFactory is Test { address predictedAddress1 = factory.predictCreateDeterministic( address(mockImpl), - ProxyAdmin(deterministicProxyAdmin), + deterministicProxyAdmin, data, proxySalt ); address proxy1 = factory.createDeterministic( address(mockImpl), - ProxyAdmin(deterministicProxyAdmin), + deterministicProxyAdmin, data, proxySalt ); diff --git a/test/UpgradeableOwnableWithGuardian.t.sol b/test/UpgradeableOwnableWithGuardian.t.sol index 0c666b8..61b0915 100644 --- a/test/UpgradeableOwnableWithGuardian.t.sol +++ b/test/UpgradeableOwnableWithGuardian.t.sol @@ -26,7 +26,7 @@ contract TestOfUpgradableOwnableWithGuardian is Test { ImplOwnableWithGuardian(address(withGuardian)).initialize(owner, guardian); } - function test_initializer() external { + function test_initializer() external view { assertEq(withGuardian.owner(), owner); assertEq(withGuardian.guardian(), guardian); } diff --git a/zksync/test/TransparentProxyFactoryZkSync.t.sol b/zksync/test/TransparentProxyFactoryZkSync.t.sol index b16d39c..ed08916 100644 --- a/zksync/test/TransparentProxyFactoryZkSync.t.sol +++ b/zksync/test/TransparentProxyFactoryZkSync.t.sol @@ -3,28 +3,25 @@ pragma solidity ^0.8.24; import {Test} from 'forge-std/Test.sol'; import {TransparentProxyFactoryZkSync} from '../src/contracts/transparent-proxy/TransparentProxyFactoryZkSync.sol'; -import {TransparentUpgradeableProxy} from '../../src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol'; +import {TransparentUpgradeableProxy} from 'openzeppelin-contracts/contracts/proxy/transparent/TransparentUpgradeableProxy.sol'; import {Ownable} from 'openzeppelin-contracts/contracts/access/Ownable.sol'; -import {ProxyAdmin} from '../../src/contracts/transparent-proxy/ProxyAdmin.sol'; import {MockImpl} from '../../src/mocks/MockImpl.sol'; contract TestTransparentProxyFactoryZkSync is Test { TransparentProxyFactoryZkSync internal factory; MockImpl internal mockImpl; - ProxyAdmin internal proxyAdmin; address internal owner = makeAddr('owner'); function setUp() public { factory = new TransparentProxyFactoryZkSync(); mockImpl = new MockImpl(); - proxyAdmin = new ProxyAdmin(owner); } function testCreate() public { uint256 FOO = 2; bytes memory data = abi.encodeWithSelector(mockImpl.initialize.selector, FOO); - address proxy = factory.create(address(mockImpl), proxyAdmin, data); + address proxy = factory.create(address(mockImpl), owner, data); assertTrue(proxy.code.length != 0); } @@ -34,12 +31,12 @@ contract TestTransparentProxyFactoryZkSync is Test { address predictedAddress1 = factory.predictCreateDeterministic( address(mockImpl), - proxyAdmin, + owner, data, salt ); - address proxy1 = factory.createDeterministic(address(mockImpl), proxyAdmin, data, salt); + address proxy1 = factory.createDeterministic(address(mockImpl), owner, data, salt); assertEq(predictedAddress1, proxy1); assertTrue(proxy1.code.length != 0); @@ -61,14 +58,14 @@ contract TestTransparentProxyFactoryZkSync is Test { address predictedAddress1 = factory.predictCreateDeterministic( address(mockImpl), - ProxyAdmin(deterministicProxyAdmin), + deterministicProxyAdmin, data, proxySalt ); address proxy1 = factory.createDeterministic( address(mockImpl), - ProxyAdmin(deterministicProxyAdmin), + deterministicProxyAdmin, data, proxySalt );