diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a85ce8e..299cd9e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -30,9 +30,11 @@ jobs: ZKSYNC: 'true' - name: Run Forge tests + id: test uses: bgd-labs/github-workflows/.github/actions/foundry-test@main - name: Run ZK tests + id: zktest uses: bgd-labs/github-workflows/.github/actions/foundry-test@main with: ZKSYNC: true @@ -43,23 +45,12 @@ jobs: - name: Run Lcov report uses: bgd-labs/github-workflows/.github/actions/foundry-lcov-report@main - - name: Save PR number - if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target' - env: - PR_NUMBER: ${{ github.event.number }} - run: | - mkdir -p ./pr - echo $PR_NUMBER > /tmp/content/pr_number.txt - - - uses: actions/upload-artifact@v4 - if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target' - with: - name: content - path: /tmp/content + - name: Run Forge tests + uses: bgd-labs/github-workflows/.github/actions/comment-artifact@main # we let failing tests pass so we can log them in the comment, still we want the ci to fail - name: Post test - if: ${{ env.testStatus != 0 }} + if: ${{ steps.test.outputs.testStatus != 0 || steps.zktest.outputs.testStatus != 0 }} run: | echo "tests failed" exit 1 diff --git a/src/contracts/transparent-proxy/ERC1967Proxy.sol b/src/contracts/transparent-proxy/ERC1967Proxy.sol deleted file mode 100644 index 409395c..0000000 --- a/src/contracts/transparent-proxy/ERC1967Proxy.sol +++ /dev/null @@ -1,38 +0,0 @@ -// SPDX-License-Identifier: MIT - -/** @dev OpenZeppelin Contracts (last updated v4.7.0) (proxy/ERC1967/ERC1967Proxy.sol) - * From https://github.com/OpenZeppelin/openzeppelin-contracts/tree/8b778fa20d6d76340c5fac1ed66c80273f05b95a - * - * BGD Labs adaptations: - * - Same exact version as OZ, only linting changes - */ - -pragma solidity ^0.8.0; - -import './Proxy.sol'; -import './ERC1967Upgrade.sol'; - -/** - * @dev This contract implements an upgradeable proxy. It is upgradeable because calls are delegated to an - * implementation address that can be changed. This address is stored in storage in the location specified by - * https://eips.ethereum.org/EIPS/eip-1967[EIP1967], so that it doesn't conflict with the storage layout of the - * implementation behind the proxy. - */ -contract ERC1967Proxy is Proxy, ERC1967Upgrade { - /** - * @dev Initializes the upgradeable proxy with an initial implementation specified by `_logic`. - * - * If `_data` is nonempty, it's used as data in a delegate call to `_logic`. This will typically be an encoded - * function call, and allows initializing the storage of the proxy like a Solidity constructor. - */ - constructor(address _logic, bytes memory _data) payable { - _upgradeToAndCall(_logic, _data, false); - } - - /** - * @dev Returns the current implementation address. - */ - function _implementation() internal view virtual override returns (address impl) { - return ERC1967Upgrade._getImplementation(); - } -} diff --git a/src/contracts/transparent-proxy/ERC1967Upgrade.sol b/src/contracts/transparent-proxy/ERC1967Upgrade.sol deleted file mode 100644 index f662f92..0000000 --- a/src/contracts/transparent-proxy/ERC1967Upgrade.sol +++ /dev/null @@ -1,119 +0,0 @@ -// SPDX-License-Identifier: MIT - -/** @dev OpenZeppelin Contracts (last updated v4.5.0) (proxy/ERC1967/ERC1967Upgrade.sol) - * From https://github.com/OpenZeppelin/openzeppelin-contracts/tree/8b778fa20d6d76340c5fac1ed66c80273f05b95a - * - * BGD Labs adaptations: - * - This is an opinionated version, to be used on "classic" transparent upgradeable proxies (non UUPS/Beacon) - * - For the sake of simplification and gas savings on deployment, the functions/constants related with UUPS/Beacon have been removed - * - Moved declaration of `_ADMIN_SLOT` constant and `AdminChanged` event to the top - * - Linting - * - Removed imports not used anymore due to not have UUPS/Beacon logic - */ - -pragma solidity ^0.8.2; - -import '../oz-common/Address.sol'; -import '../oz-common/StorageSlot.sol'; - -/** - * @dev This abstract contract provides getters and event emitting update functions for - * https://eips.ethereum.org/EIPS/eip-1967[EIP1967] slots. - * - * _Available since v4.1._ - * - * @custom:oz-upgrades-unsafe-allow delegatecall - */ -abstract contract ERC1967Upgrade { - /** - * @dev Storage slot with the address of the current implementation. - * This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1, and is - * validated in the constructor. - */ - bytes32 internal constant _IMPLEMENTATION_SLOT = - 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; - - /** - * @dev Storage slot with the admin of the contract. - * This is the keccak-256 hash of "eip1967.proxy.admin" subtracted by 1, and is - * validated in the constructor. - */ - bytes32 internal constant _ADMIN_SLOT = - 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; - - /** - * @dev Emitted when the implementation is upgraded. - */ - event Upgraded(address indexed implementation); - - /** - * @dev Emitted when the admin account has changed. - */ - event AdminChanged(address previousAdmin, address newAdmin); - - /** - * @dev Returns the current implementation address. - */ - function _getImplementation() internal view returns (address) { - return StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value; - } - - /** - * @dev Stores a new address in the EIP1967 implementation slot. - */ - function _setImplementation(address newImplementation) private { - require(Address.isContract(newImplementation), 'ERC1967: new implementation is not a contract'); - StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation; - } - - /** - * @dev Perform implementation upgrade - * - * Emits an {Upgraded} event. - */ - function _upgradeTo(address newImplementation) internal { - _setImplementation(newImplementation); - emit Upgraded(newImplementation); - } - - /** - * @dev Perform implementation upgrade with additional setup call. - * - * Emits an {Upgraded} event. - */ - function _upgradeToAndCall( - address newImplementation, - bytes memory data, - bool forceCall - ) internal { - _upgradeTo(newImplementation); - if (data.length > 0 || forceCall) { - Address.functionDelegateCall(newImplementation, data); - } - } - - /** - * @dev Returns the current admin. - */ - function _getAdmin() internal view returns (address) { - return StorageSlot.getAddressSlot(_ADMIN_SLOT).value; - } - - /** - * @dev Stores a new address in the EIP1967 admin slot. - */ - function _setAdmin(address newAdmin) private { - require(newAdmin != address(0), 'ERC1967: new admin is the zero address'); - StorageSlot.getAddressSlot(_ADMIN_SLOT).value = newAdmin; - } - - /** - * @dev Changes the admin of the proxy. - * - * Emits an {AdminChanged} event. - */ - function _changeAdmin(address newAdmin) internal { - emit AdminChanged(_getAdmin(), newAdmin); - _setAdmin(newAdmin); - } -} diff --git a/src/contracts/transparent-proxy/ProxyAdmin.sol b/src/contracts/transparent-proxy/ProxyAdmin.sol index d1b2034..161ca6e 100644 --- a/src/contracts/transparent-proxy/ProxyAdmin.sol +++ b/src/contracts/transparent-proxy/ProxyAdmin.sol @@ -1,17 +1,10 @@ // SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v5.0.0) (proxy/transparent/ProxyAdmin.sol) -/** - * @dev OpenZeppelin Contracts v4.4.1 (proxy/transparent/ProxyAdmin.sol) - * From https://github.com/OpenZeppelin/openzeppelin-contracts/tree/8b778fa20d6d76340c5fac1ed66c80273f05b95a - * - * BGD Labs adaptations: - * - Linting - */ +pragma solidity ^0.8.20; -pragma solidity ^0.8.0; - -import './TransparentUpgradeableProxy.sol'; -import '../oz-common/Ownable.sol'; +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 @@ -19,75 +12,31 @@ import '../oz-common/Ownable.sol'; */ contract ProxyAdmin is Ownable { /** - * @dev Returns the current implementation of `proxy`. - * - * Requirements: - * - * - This contract must be the admin of `proxy`. - */ - function getProxyImplementation( - TransparentUpgradeableProxy proxy - ) public view virtual returns (address) { - // We need to manually run the static call since the getter cannot be flagged as view - // bytes4(keccak256("implementation()")) == 0x5c60da1b - (bool success, bytes memory returndata) = address(proxy).staticcall(hex'5c60da1b'); - require(success); - return abi.decode(returndata, (address)); - } - - /** - * @dev Returns the current admin of `proxy`. - * - * Requirements: - * - * - This contract must be the admin of `proxy`. - */ - function getProxyAdmin(TransparentUpgradeableProxy proxy) public view virtual returns (address) { - // We need to manually run the static call since the getter cannot be flagged as view - // bytes4(keccak256("admin()")) == 0xf851a440 - (bool success, bytes memory returndata) = address(proxy).staticcall(hex'f851a440'); - require(success); - return abi.decode(returndata, (address)); - } - - /** - * @dev Changes the admin of `proxy` to `newAdmin`. - * - * Requirements: - * - * - This contract must be the current admin of `proxy`. + * @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. */ - function changeProxyAdmin( - TransparentUpgradeableProxy proxy, - address newAdmin - ) public virtual onlyOwner { - proxy.changeAdmin(newAdmin); - } + string public constant UPGRADE_INTERFACE_VERSION = '5.0.0'; /** - * @dev Upgrades `proxy` to `implementation`. See {TransparentUpgradeableProxy-upgradeTo}. - * - * Requirements: - * - * - This contract must be the admin of `proxy`. + * @dev Sets the initial owner who can perform upgrades. */ - function upgrade( - TransparentUpgradeableProxy proxy, - address implementation - ) public virtual onlyOwner { - proxy.upgradeTo(implementation); - } + constructor(address initialOwner) Ownable(initialOwner) {} /** - * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. See - * {TransparentUpgradeableProxy-upgradeToAndCall}. + * @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( - TransparentUpgradeableProxy proxy, + ITransparentUpgradeableProxy proxy, address implementation, bytes memory data ) public payable virtual onlyOwner { diff --git a/src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol b/src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol index 41c8b06..68ec22d 100644 --- a/src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol +++ b/src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity >=0.8.0; -import {IOwnable} from './interfaces/IOwnable.sol'; import {ITransparentProxyFactory} from './interfaces/ITransparentProxyFactory.sol'; import {TransparentUpgradeableProxy} from './TransparentUpgradeableProxy.sol'; import {ProxyAdmin} from './ProxyAdmin.sol'; @@ -16,21 +15,16 @@ import {ProxyAdmin} from './ProxyAdmin.sol'; **/ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory { /// @inheritdoc ITransparentProxyFactory - function create( - address logic, - address admin, - bytes calldata data - ) external returns (address) { + function create(address logic, ProxyAdmin admin, bytes calldata data) external returns (address) { address proxy = address(new TransparentUpgradeableProxy(logic, admin, data)); - emit ProxyCreated(proxy, logic, admin); + emit ProxyCreated(proxy, logic, address(admin)); return proxy; } /// @inheritdoc ITransparentProxyFactory function createProxyAdmin(address adminOwner) external returns (address) { - address proxyAdmin = address(new ProxyAdmin()); - IOwnable(proxyAdmin).transferOwnership(adminOwner); + address proxyAdmin = address(new ProxyAdmin(adminOwner)); emit ProxyAdminCreated(proxyAdmin, adminOwner); return proxyAdmin; @@ -39,23 +33,22 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory { /// @inheritdoc ITransparentProxyFactory function createDeterministic( address logic, - address admin, + ProxyAdmin admin, bytes calldata data, bytes32 salt ) external returns (address) { address proxy = address(new TransparentUpgradeableProxy{salt: salt}(logic, admin, data)); - emit ProxyDeterministicCreated(proxy, logic, admin, salt); + emit ProxyDeterministicCreated(proxy, logic, address(admin), salt); return proxy; } /// @inheritdoc ITransparentProxyFactory - function createDeterministicProxyAdmin(address adminOwner, bytes32 salt) - external - returns (address) - { - address proxyAdmin = address(new ProxyAdmin{salt: salt}()); - IOwnable(proxyAdmin).transferOwnership(adminOwner); + function createDeterministicProxyAdmin( + address adminOwner, + bytes32 salt + ) external returns (address) { + address proxyAdmin = address(new ProxyAdmin{salt: salt}(adminOwner)); emit ProxyAdminDeterministicCreated(proxyAdmin, adminOwner, salt); return proxyAdmin; @@ -64,7 +57,7 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory { /// @inheritdoc ITransparentProxyFactory function predictCreateDeterministic( address logic, - address admin, + ProxyAdmin admin, bytes calldata data, bytes32 salt ) public view returns (address) { @@ -73,13 +66,22 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory { address(this), salt, type(TransparentUpgradeableProxy).creationCode, - abi.encode(logic, admin, data) + abi.encode(logic, address(admin), data) ); } /// @inheritdoc ITransparentProxyFactory - function predictCreateDeterministicProxyAdmin(bytes32 salt) public view returns (address) { - return _predictCreate2Address(address(this), salt, type(ProxyAdmin).creationCode, abi.encode()); + function predictCreateDeterministicProxyAdmin( + bytes32 salt, + address initialOwner + ) public view returns (address) { + return + _predictCreate2Address( + address(this), + salt, + type(ProxyAdmin).creationCode, + abi.encode(initialOwner) + ); } function _predictCreate2Address( diff --git a/src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol b/src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol index 424c74e..a7c97dc 100644 --- a/src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol +++ b/src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol @@ -1,19 +1,31 @@ // SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v5.0.0) (proxy/transparent/TransparentUpgradeableProxy.sol) /** - * @dev OpenZeppelin Contracts (last updated v4.7.0) (proxy/utils/Initializable.sol) - * From https://github.com/OpenZeppelin/openzeppelin-contracts/tree/8b778fa20d6d76340c5fac1ed66c80273f05b95a - * - * BGD Labs adaptations: - * - Linting + * 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; -pragma solidity ^0.8.0; +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'; -import './ERC1967Proxy.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 by an admin. + * @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 @@ -21,117 +33,93 @@ import './ERC1967Proxy.sol'; * 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 one of the admin functions exposed by the proxy itself. - * 2. If the admin calls the proxy, it can access the admin functions, but its calls will never be forwarded to the - * implementation. If the admin tries to call a function on the implementation it will fail with an error that says - * "admin cannot fallback to proxy target". + * 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. * - * These properties mean that the admin account can only be used for admin actions like upgrading the proxy or changing - * the admin, 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. + * 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. * - * Our recommendation is for the dedicated account to be an instance of the {ProxyAdmin} contract. If set up this way, - * you should think of the `ProxyAdmin` instance as the real administrative interface of your proxy. + * 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 { - /** - * @dev Initializes an upgradeable proxy managed by `_admin`, backed by the implementation at `_logic`, and - * optionally initialized with `_data` as explained in {ERC1967Proxy-constructor}. - */ - constructor( - address _logic, - address admin_, - bytes memory _data - ) payable ERC1967Proxy(_logic, _data) { - _changeAdmin(admin_); - } + // 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 Modifier used internally that will delegate the call to the implementation unless the sender is the admin. + * @dev The proxy caller is the current admin, and can't fallback to the proxy target. */ - modifier ifAdmin() { - if (msg.sender == _getAdmin()) { - _; - } else { - _fallback(); - } - } + error ProxyDeniedAdminAccess(); /** - * @dev Returns the current admin. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyAdmin}. - * - * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the - * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. - * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103` + * @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}. */ - function admin() external ifAdmin returns (address admin_) { - admin_ = _getAdmin(); + 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 current implementation. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyImplementation}. - * - * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the - * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. - * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc` + * @dev Returns the admin of this proxy. */ - function implementation() external ifAdmin returns (address implementation_) { - implementation_ = _implementation(); + function _proxyAdmin() internal virtual returns (address) { + return _admin; } /** - * @dev Changes the admin of the proxy. - * - * Emits an {AdminChanged} event. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}. + * @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior. */ - function changeAdmin(address newAdmin) external virtual ifAdmin { - _changeAdmin(newAdmin); + 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. + * @dev Upgrade the implementation of the proxy. See {ERC1967Utils-upgradeToAndCall}. * - * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}. - */ - function upgradeTo(address newImplementation) external ifAdmin { - _upgradeToAndCall(newImplementation, bytes(''), false); - } - - /** - * @dev Upgrade the implementation of the proxy, and then call a function from the new implementation as specified - * by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the - * proxied contract. + * Requirements: * - * NOTE: Only the admin can call this function. See {ProxyAdmin-upgradeAndCall}. - */ - function upgradeToAndCall( - address newImplementation, - bytes calldata data - ) external payable ifAdmin { - _upgradeToAndCall(newImplementation, data, true); - } - - /** - * @dev Returns the current admin. - */ - function _admin() internal view virtual returns (address) { - return _getAdmin(); - } - - /** - * @dev Makes sure the admin cannot access the fallback function. See {Proxy-_beforeFallback}. + * - If `data` is empty, `msg.value` must be zero. */ - function _beforeFallback() internal virtual override { - require( - msg.sender != _getAdmin(), - 'TransparentUpgradeableProxy: admin cannot fallback to proxy target' - ); - super._beforeFallback(); + 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/IOwnable.sol b/src/contracts/transparent-proxy/interfaces/IOwnable.sol deleted file mode 100644 index dc69e40..0000000 --- a/src/contracts/transparent-proxy/interfaces/IOwnable.sol +++ /dev/null @@ -1,17 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.0; - -interface IOwnable { - /** - * @dev Transfers ownership of the contract to a new account (`newOwner`). - * @param newOwner address of the new owner. - * Can only be called by the current owner. - **/ - function transferOwnership(address newOwner) external; - - /** - * @dev Returns the address of the current owner. - **/ - function owner() external view returns (address); -} diff --git a/src/contracts/transparent-proxy/interfaces/ITransparentProxyFactory.sol b/src/contracts/transparent-proxy/interfaces/ITransparentProxyFactory.sol index 89be9c7..52fe49c 100644 --- a/src/contracts/transparent-proxy/interfaces/ITransparentProxyFactory.sol +++ b/src/contracts/transparent-proxy/interfaces/ITransparentProxyFactory.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity >=0.8.0; +import {ProxyAdmin} from '../ProxyAdmin.sol'; + interface ITransparentProxyFactory { event ProxyCreated(address proxy, address indexed logic, address indexed proxyAdmin); event ProxyAdminCreated(address proxyAdmin, address indexed adminOwner); @@ -26,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, address admin, bytes memory data) external returns (address); + function create(address logic, ProxyAdmin admin, bytes memory data) external returns (address); /** * @notice Creates a proxyAdmin instance, and transfers ownership to provided owner @@ -49,7 +51,7 @@ interface ITransparentProxyFactory { **/ function createDeterministic( address logic, - address admin, + ProxyAdmin admin, bytes memory data, bytes32 salt ) external returns (address); @@ -78,7 +80,7 @@ interface ITransparentProxyFactory { **/ function predictCreateDeterministic( address logic, - address admin, + ProxyAdmin admin, bytes calldata data, bytes32 salt ) external view returns (address); @@ -88,5 +90,8 @@ interface ITransparentProxyFactory { * @param salt Value to be used in the address calculation, to be chosen by the account calling this function * @return address The pre-calculated address **/ - function predictCreateDeterministicProxyAdmin(bytes32 salt) external view returns (address); + function predictCreateDeterministicProxyAdmin( + bytes32 salt, + address initialOwner + ) external view returns (address); } diff --git a/test/TransparentProxyFactory.t.sol b/test/TransparentProxyFactory.t.sol index 42be765..5108103 100644 --- a/test/TransparentProxyFactory.t.sol +++ b/test/TransparentProxyFactory.t.sol @@ -2,9 +2,10 @@ 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 {TransparentProxyFactory} from '../src/contracts/transparent-proxy/TransparentProxyFactory.sol'; import {TransparentUpgradeableProxy} from '../src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol'; -import {IOwnable} from '../src/contracts/transparent-proxy/interfaces/IOwnable.sol'; import {MockImpl} from '../src/mocks/MockImpl.sol'; contract TestTransparentProxyFactory is Test { @@ -25,12 +26,12 @@ contract TestTransparentProxyFactory is Test { address predictedAddress1 = factory.predictCreateDeterministic( address(mockImpl), - admin, + ProxyAdmin(admin), data, salt ); - address proxy1 = factory.createDeterministic(address(mockImpl), admin, data, salt); + address proxy1 = factory.createDeterministic(address(mockImpl), ProxyAdmin(admin), data, salt); assertEq(predictedAddress1, proxy1); assertEq(MockImpl(proxy1).getFoo(), FOO); @@ -40,7 +41,11 @@ contract TestTransparentProxyFactory is Test { bytes32 proxyAdminSalt, bytes32 proxySalt ) public { - address deterministicProxyAdmin = factory.predictCreateDeterministicProxyAdmin(proxyAdminSalt); + address owner = makeAddr('owner'); + address deterministicProxyAdmin = factory.predictCreateDeterministicProxyAdmin( + proxyAdminSalt, + owner + ); uint256 FOO = 2; @@ -48,14 +53,14 @@ contract TestTransparentProxyFactory is Test { address predictedAddress1 = factory.predictCreateDeterministic( address(mockImpl), - deterministicProxyAdmin, + ProxyAdmin(deterministicProxyAdmin), data, proxySalt ); address proxy1 = factory.createDeterministic( address(mockImpl), - deterministicProxyAdmin, + ProxyAdmin(deterministicProxyAdmin), data, proxySalt ); @@ -73,9 +78,12 @@ contract TestTransparentProxyFactory is Test { address proxyAdmin = factory.createDeterministicProxyAdmin(proxyAdminOwner, proxyAdminSalt); - address predictedProxyAdmin = factory.predictCreateDeterministicProxyAdmin(proxyAdminSalt); + address predictedProxyAdmin = factory.predictCreateDeterministicProxyAdmin( + proxyAdminSalt, + proxyAdminOwner + ); - address proxyOwner = IOwnable(proxyAdmin).owner(); + address proxyOwner = Ownable(proxyAdmin).owner(); assertEq(predictedProxyAdmin, proxyAdmin); assertEq(proxyOwner, proxyAdminOwner); @@ -86,6 +94,6 @@ contract TestTransparentProxyFactory is Test { vm.assume(proxyAdminOwner != address(0)); address proxyAdmin = factory.createDeterministicProxyAdmin(proxyAdminOwner, proxyAdminSalt); - assertEq(IOwnable(proxyAdmin).owner(), proxyAdminOwner); + assertEq(Ownable(proxyAdmin).owner(), proxyAdminOwner); } } diff --git a/zksync/test/TransparentProxyFactoryZkSync.t.sol b/zksync/test/TransparentProxyFactoryZkSync.t.sol index 7359036..b16d39c 100644 --- a/zksync/test/TransparentProxyFactoryZkSync.t.sol +++ b/zksync/test/TransparentProxyFactoryZkSync.t.sol @@ -4,43 +4,42 @@ 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 {IOwnable} from '../../src/contracts/transparent-proxy/interfaces/IOwnable.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(address admin) public { - vm.assume(admin != address(0)); - + function testCreate() public { uint256 FOO = 2; bytes memory data = abi.encodeWithSelector(mockImpl.initialize.selector, FOO); - address proxy = factory.create(address(mockImpl), admin, data); + address proxy = factory.create(address(mockImpl), proxyAdmin, data); assertTrue(proxy.code.length != 0); } - function testCreateDeterministic(address admin, bytes32 salt) public { - // we know that this is covered at the ERC1967Upgrade - vm.assume(admin != address(0) && admin != address(this)); - + function testCreateDeterministic(bytes32 salt) public { uint256 FOO = 2; bytes memory data = abi.encodeWithSelector(mockImpl.initialize.selector, FOO); address predictedAddress1 = factory.predictCreateDeterministic( address(mockImpl), - admin, + proxyAdmin, data, salt ); - address proxy1 = factory.createDeterministic(address(mockImpl), admin, data, salt); + address proxy1 = factory.createDeterministic(address(mockImpl), proxyAdmin, data, salt); assertEq(predictedAddress1, proxy1); assertTrue(proxy1.code.length != 0); @@ -51,7 +50,10 @@ contract TestTransparentProxyFactoryZkSync is Test { bytes32 proxyAdminSalt, bytes32 proxySalt ) public { - address deterministicProxyAdmin = factory.predictCreateDeterministicProxyAdmin(proxyAdminSalt); + address deterministicProxyAdmin = factory.predictCreateDeterministicProxyAdmin( + proxyAdminSalt, + address(owner) + ); uint256 FOO = 2; @@ -59,14 +61,14 @@ contract TestTransparentProxyFactoryZkSync is Test { address predictedAddress1 = factory.predictCreateDeterministic( address(mockImpl), - deterministicProxyAdmin, + ProxyAdmin(deterministicProxyAdmin), data, proxySalt ); address proxy1 = factory.createDeterministic( address(mockImpl), - deterministicProxyAdmin, + ProxyAdmin(deterministicProxyAdmin), data, proxySalt ); @@ -85,9 +87,12 @@ contract TestTransparentProxyFactoryZkSync is Test { address proxyAdmin = factory.createDeterministicProxyAdmin(proxyAdminOwner, proxyAdminSalt); - address predictedProxyAdmin = factory.predictCreateDeterministicProxyAdmin(proxyAdminSalt); + address predictedProxyAdmin = factory.predictCreateDeterministicProxyAdmin( + proxyAdminSalt, + proxyAdminOwner + ); - address proxyOwner = IOwnable(proxyAdmin).owner(); + address proxyOwner = Ownable(proxyAdmin).owner(); assertEq(predictedProxyAdmin, proxyAdmin); assertEq(proxyOwner, proxyAdminOwner); @@ -100,6 +105,6 @@ contract TestTransparentProxyFactoryZkSync is Test { address proxyAdmin = factory.createDeterministicProxyAdmin(proxyAdminOwner, proxyAdminSalt); assertTrue(proxyAdmin.code.length != 0); - assertEq(IOwnable(proxyAdmin).owner(), proxyAdminOwner); + assertEq(Ownable(proxyAdmin).owner(), proxyAdminOwner); } }