From 079f54eaf1e65f04eb167541b7944c2699fe9eba Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Mar 2023 18:19:51 +0100 Subject: [PATCH 01/20] manualy root ifAdmin function in TransparentUpgradeableProxy --- contracts/proxy/transparent/ProxyAdmin.sol | 10 +- .../TransparentUpgradeableProxy.sol | 122 ++++++++---------- .../TransparentUpgradeableProxy.test.js | 4 +- 3 files changed, 64 insertions(+), 72 deletions(-) diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index 839534298b9..1a969819647 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -18,7 +18,7 @@ contract ProxyAdmin is Ownable { * * - This contract must be the admin of `proxy`. */ - function getProxyImplementation(TransparentUpgradeableProxy proxy) public view virtual returns (address) { + function getProxyImplementation(ITransparentUpgradeableProxy 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"); @@ -33,7 +33,7 @@ contract ProxyAdmin is Ownable { * * - This contract must be the admin of `proxy`. */ - function getProxyAdmin(TransparentUpgradeableProxy proxy) public view virtual returns (address) { + function getProxyAdmin(ITransparentUpgradeableProxy 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"); @@ -48,7 +48,7 @@ contract ProxyAdmin is Ownable { * * - This contract must be the current admin of `proxy`. */ - function changeProxyAdmin(TransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner { + function changeProxyAdmin(ITransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner { proxy.changeAdmin(newAdmin); } @@ -59,7 +59,7 @@ contract ProxyAdmin is Ownable { * * - This contract must be the admin of `proxy`. */ - function upgrade(TransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner { + function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner { proxy.upgradeTo(implementation); } @@ -72,7 +72,7 @@ contract ProxyAdmin is Ownable { * - This contract must be the admin of `proxy`. */ function upgradeAndCall( - TransparentUpgradeableProxy proxy, + ITransparentUpgradeableProxy proxy, address implementation, bytes memory data ) public payable virtual onlyOwner { diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 155a22e01f7..ccb436211ec 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -5,6 +5,16 @@ pragma solidity ^0.8.0; import "../ERC1967/ERC1967Proxy.sol"; +interface ITransparentUpgradeableProxy { + event Upgraded(address indexed implementation); + event AdminChanged(address previousAdmin, address newAdmin); + function admin() external returns (address); + function implementation() external returns (address); + function changeAdmin(address) external; + function upgradeTo(address) external; + function upgradeToAndCall(address, bytes memory) payable external; +} + /** * @dev This contract implements a proxy that is upgradeable by an admin. * @@ -42,69 +52,57 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { if (msg.sender == _getAdmin()) { _; } else { - _fallback(); + super._fallback(); } } /** - * @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` - */ - function admin() external payable ifAdmin returns (address admin_) { - _requireZeroValue(); - admin_ = _getAdmin(); - } - - /** - * @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 TODO */ - function implementation() external payable ifAdmin returns (address implementation_) { - _requireZeroValue(); - implementation_ = _implementation(); - } - - /** - * @dev Changes the admin of the proxy. - * - * Emits an {AdminChanged} event. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}. - */ - function changeAdmin(address newAdmin) external payable virtual ifAdmin { - _requireZeroValue(); - _changeAdmin(newAdmin); - } - - /** - * @dev Upgrade the implementation of the proxy. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}. - */ - function upgradeTo(address newImplementation) external payable ifAdmin { - _requireZeroValue(); - _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. - * - * 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); + function _fallback() internal virtual override ifAdmin { + bytes4 selector = msg.sig; + if (selector == ITransparentUpgradeableProxy.admin.selector) { + // Returns the current admin. + // + // 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` + _requireZeroValue(); + address admin = _getAdmin(); + assembly { + mstore(0x00, admin) + return(0, 0x20) + } + } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { + // Returns the current implementation. + // 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` + _requireZeroValue(); + address implementation = _implementation(); + assembly { + mstore(0x00, implementation) + return(0, 0x20) + } + } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) { + // Changes the admin of the proxy. + _requireZeroValue(); + address newAdmin = abi.decode(msg.data[4:], (address)); + _changeAdmin(newAdmin); + } else if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { + // Upgrade the implementation of the proxy. + _requireZeroValue(); + address newImplementation = abi.decode(msg.data[4:], (address)); + _upgradeToAndCall(newImplementation, bytes(""), false); + } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { + // 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. + (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); + _upgradeToAndCall(newImplementation, data, true); + } else { + revert('TransparentUpgradeableProxy: admin cannot fallback to proxy target'); + } } /** @@ -114,14 +112,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { return _getAdmin(); } - /** - * @dev Makes sure the admin cannot access the fallback function. See {Proxy-_beforeFallback}. - */ - function _beforeFallback() internal virtual override { - require(msg.sender != _getAdmin(), "TransparentUpgradeableProxy: admin cannot fallback to proxy target"); - super._beforeFallback(); - } - /** * @dev To keep this contract fully transparent, all `ifAdmin` functions must be payable. This helper is here to * emulate some proxy functions being non-payable while still allowing value to pass through. diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.test.js b/test/proxy/transparent/TransparentUpgradeableProxy.test.js index 86dd55d32c1..d60a31a21da 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.test.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.test.js @@ -2,12 +2,14 @@ const shouldBehaveLikeProxy = require('../Proxy.behaviour'); const shouldBehaveLikeTransparentUpgradeableProxy = require('./TransparentUpgradeableProxy.behaviour'); const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy'); +const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy'); contract('TransparentUpgradeableProxy', function (accounts) { const [proxyAdminAddress, proxyAdminOwner] = accounts; const createProxy = async function (logic, admin, initData, opts) { - return TransparentUpgradeableProxy.new(logic, admin, initData, opts); + const { address } = await TransparentUpgradeableProxy.new(logic, admin, initData, opts); + return ITransparentUpgradeableProxy.at(address); }; shouldBehaveLikeProxy(createProxy, proxyAdminAddress, proxyAdminOwner); From 48ea529a336786251169155367bfae03900a7245 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Mar 2023 18:23:40 +0100 Subject: [PATCH 02/20] minor improvement --- .../TransparentUpgradeableProxy.sol | 97 +++++++++---------- 1 file changed, 45 insertions(+), 52 deletions(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index ccb436211ec..ed71dd4f3d4 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -46,62 +46,55 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { } /** - * @dev Modifier used internally that will delegate the call to the implementation unless the sender is the admin. + * @dev If caller is the admin process the call internally, otherwize transparently fallback to the proxy behavior */ - modifier ifAdmin() { + function _fallback() internal virtual override { if (msg.sender == _getAdmin()) { - _; - } else { - super._fallback(); - } - } - - /** - * @dev TODO - */ - function _fallback() internal virtual override ifAdmin { - bytes4 selector = msg.sig; - if (selector == ITransparentUpgradeableProxy.admin.selector) { - // Returns the current admin. - // - // 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` - _requireZeroValue(); - address admin = _getAdmin(); - assembly { - mstore(0x00, admin) - return(0, 0x20) + bytes4 selector = msg.sig; + if (selector == ITransparentUpgradeableProxy.admin.selector) { + // Returns the current admin. + // + // 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` + _requireZeroValue(); + address admin = _getAdmin(); + assembly { + mstore(0x00, admin) + return(0, 0x20) + } + } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { + // Returns the current implementation. + // 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` + _requireZeroValue(); + address implementation = _implementation(); + assembly { + mstore(0x00, implementation) + return(0, 0x20) + } + } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) { + // Changes the admin of the proxy. + _requireZeroValue(); + address newAdmin = abi.decode(msg.data[4:], (address)); + _changeAdmin(newAdmin); + } else if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { + // Upgrade the implementation of the proxy. + _requireZeroValue(); + address newImplementation = abi.decode(msg.data[4:], (address)); + _upgradeToAndCall(newImplementation, bytes(""), false); + } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { + // 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. + (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); + _upgradeToAndCall(newImplementation, data, true); + } else { + revert('TransparentUpgradeableProxy: admin cannot fallback to proxy target'); } - } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { - // Returns the current implementation. - // 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` - _requireZeroValue(); - address implementation = _implementation(); - assembly { - mstore(0x00, implementation) - return(0, 0x20) - } - } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) { - // Changes the admin of the proxy. - _requireZeroValue(); - address newAdmin = abi.decode(msg.data[4:], (address)); - _changeAdmin(newAdmin); - } else if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { - // Upgrade the implementation of the proxy. - _requireZeroValue(); - address newImplementation = abi.decode(msg.data[4:], (address)); - _upgradeToAndCall(newImplementation, bytes(""), false); - } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { - // 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. - (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); - _upgradeToAndCall(newImplementation, data, true); } else { - revert('TransparentUpgradeableProxy: admin cannot fallback to proxy target'); + super._fallback(); } } From 81826a53a3782a4fbe07680b07349ad1f38fc36e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Mar 2023 18:31:41 +0100 Subject: [PATCH 03/20] split fallback routing --- .../TransparentUpgradeableProxy.sol | 102 ++++++++++++------ 1 file changed, 68 insertions(+), 34 deletions(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index ed71dd4f3d4..0403bc18ace 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -52,44 +52,15 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { if (msg.sender == _getAdmin()) { bytes4 selector = msg.sig; if (selector == ITransparentUpgradeableProxy.admin.selector) { - // Returns the current admin. - // - // 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` - _requireZeroValue(); - address admin = _getAdmin(); - assembly { - mstore(0x00, admin) - return(0, 0x20) - } + _internalAdmin(); } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { - // Returns the current implementation. - // 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` - _requireZeroValue(); - address implementation = _implementation(); - assembly { - mstore(0x00, implementation) - return(0, 0x20) - } + _internalImplementation(); } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) { - // Changes the admin of the proxy. - _requireZeroValue(); - address newAdmin = abi.decode(msg.data[4:], (address)); - _changeAdmin(newAdmin); + _internalChangeAdmin(); } else if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { - // Upgrade the implementation of the proxy. - _requireZeroValue(); - address newImplementation = abi.decode(msg.data[4:], (address)); - _upgradeToAndCall(newImplementation, bytes(""), false); + _internalUpgradeTo(); } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { - // 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. - (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); - _upgradeToAndCall(newImplementation, data, true); + _internalUpgradeToAndCall(); } else { revert('TransparentUpgradeableProxy: admin cannot fallback to proxy target'); } @@ -98,6 +69,69 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { } } + /** + * Returns the current admin. + * + * 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` + */ + function _internalAdmin() private { + _requireZeroValue(); + address admin = _getAdmin(); + assembly { + mstore(0x00, admin) + return(0, 0x20) + } + } + + /** + * @dev Returns the current implementation. + * + * 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` + */ + function _internalImplementation() private { + _requireZeroValue(); + + address implementation = _implementation(); + assembly { + mstore(0x00, implementation) + return(0, 0x20) + } + } + + /** + * @dev Changes the admin of the proxy. + */ + function _internalChangeAdmin() private { + _requireZeroValue(); + + address newAdmin = abi.decode(msg.data[4:], (address)); + _changeAdmin(newAdmin); + } + + /** + * @dev Upgrade the implementation of the proxy. + */ + function _internalUpgradeTo() private { + _requireZeroValue(); + + address newImplementation = abi.decode(msg.data[4:], (address)); + _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. + */ + function _internalUpgradeToAndCall() private { + (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); + _upgradeToAndCall(newImplementation, data, true); + } + /** * @dev Returns the current admin. */ From d7fbc2d540cfcb39d8c2de3ff9cd43c196527780 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Mar 2023 18:32:33 +0100 Subject: [PATCH 04/20] cleanup --- contracts/proxy/transparent/TransparentUpgradeableProxy.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 0403bc18ace..6b3005a9b35 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -70,7 +70,7 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { } /** - * Returns the current admin. + * @dev Returns the current admin. * * 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. From a7cec2fb5acd186ef83d079d3311de61b9546f89 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Mar 2023 20:00:46 +0100 Subject: [PATCH 05/20] =?UTF-8?q?rename=20=5FinternalX=20=E2=86=92=20=5Fdi?= =?UTF-8?q?spatchX=20&=20returns=20bytes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../TransparentUpgradeableProxy.sol | 41 +++++++++++-------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 6b3005a9b35..cf6da7b08c9 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -50,20 +50,24 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { */ function _fallback() internal virtual override { if (msg.sender == _getAdmin()) { + bytes memory ret; bytes4 selector = msg.sig; if (selector == ITransparentUpgradeableProxy.admin.selector) { - _internalAdmin(); + ret = _dispatchAdmin(); } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { - _internalImplementation(); + ret = _dispatchImplementation(); } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) { - _internalChangeAdmin(); + ret = _dispatchChangeAdmin(); } else if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { - _internalUpgradeTo(); + ret = _dispatchUpgradeTo(); } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { - _internalUpgradeToAndCall(); + ret = _dispatchUpgradeToAndCall(); } else { revert('TransparentUpgradeableProxy: admin cannot fallback to proxy target'); } + assembly { + return(add(ret, 0x20), mload(ret)) + } } else { super._fallback(); } @@ -76,13 +80,11 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103` */ - function _internalAdmin() private { + function _dispatchAdmin() private returns (bytes memory) { _requireZeroValue(); + address admin = _getAdmin(); - assembly { - mstore(0x00, admin) - return(0, 0x20) - } + return abi.encode(admin); } /** @@ -92,34 +94,35 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc` */ - function _internalImplementation() private { + function _dispatchImplementation() private returns (bytes memory) { _requireZeroValue(); address implementation = _implementation(); - assembly { - mstore(0x00, implementation) - return(0, 0x20) - } + return abi.encode(implementation); } /** * @dev Changes the admin of the proxy. */ - function _internalChangeAdmin() private { + function _dispatchChangeAdmin() private returns (bytes memory) { _requireZeroValue(); address newAdmin = abi.decode(msg.data[4:], (address)); _changeAdmin(newAdmin); + + return ""; } /** * @dev Upgrade the implementation of the proxy. */ - function _internalUpgradeTo() private { + function _dispatchUpgradeTo() private returns (bytes memory) { _requireZeroValue(); address newImplementation = abi.decode(msg.data[4:], (address)); _upgradeToAndCall(newImplementation, bytes(""), false); + + return ""; } /** @@ -127,9 +130,11 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { * by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the * proxied contract. */ - function _internalUpgradeToAndCall() private { + function _dispatchUpgradeToAndCall() private returns (bytes memory) { (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); _upgradeToAndCall(newImplementation, data, true); + + return ""; } /** From 6d5e72f17e0f26d8435b495e8af82750e7bbe934 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Mar 2023 20:04:30 +0100 Subject: [PATCH 06/20] view functions in the interface --- .../TransparentUpgradeableProxy.sol | 4 ++-- .../TransparentUpgradeableProxy.behaviour.js | 22 +++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index cf6da7b08c9..33b05eec5ad 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -8,8 +8,8 @@ import "../ERC1967/ERC1967Proxy.sol"; interface ITransparentUpgradeableProxy { event Upgraded(address indexed implementation); event AdminChanged(address previousAdmin, address newAdmin); - function admin() external returns (address); - function implementation() external returns (address); + function admin() external view returns (address); + function implementation() external view returns (address); function changeAdmin(address) external; function upgradeTo(address) external; function upgradeToAndCall(address, bytes memory) payable external; diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 3a10357a905..66c0a406cac 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -34,7 +34,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('implementation', function () { it('returns the current implementation address', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.implementationV0); }); @@ -55,7 +55,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('upgrades to the requested implementation', async function () { await this.proxy.upgradeTo(this.implementationV1, { from }); - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.implementationV1); }); @@ -103,7 +103,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('upgrades to the requested implementation', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behavior.address); }); @@ -168,7 +168,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV1.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV1.address }); }); @@ -196,7 +196,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV2.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV2.address }); }); @@ -227,7 +227,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV3.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV3.address }); }); @@ -271,7 +271,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('assigns new proxy admin', async function () { - const newProxyAdmin = await this.proxy.admin.call({ from: newAdmin }); + const newProxyAdmin = await this.proxy.admin({ from: newAdmin }); expect(newProxyAdmin).to.be.equal(anotherAccount); }); @@ -332,21 +332,21 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('when function names clash', function () { it('when sender is proxy admin should run the proxy function', async function () { - const value = await this.proxy.admin.call({ from: proxyAdminAddress, value: 0 }); + const value = await this.proxy.admin({ from: proxyAdminAddress, value: 0 }); expect(value).to.be.equal(proxyAdminAddress); }); it('when sender is other should delegate to implementation', async function () { - const value = await this.proxy.admin.call({ from: anotherAccount, value: 0 }); + const value = await this.proxy.admin({ from: anotherAccount, value: 0 }); expect(value).to.be.equal('0x0000000000000000000000000000000011111142'); }); it('when sender is proxy admin value should not be accepted', async function () { - await expectRevert.unspecified(this.proxy.admin.call({ from: proxyAdminAddress, value: 1 })); + await expectRevert.unspecified(this.proxy.admin({ from: proxyAdminAddress, value: 1 })); }); it('when sender is other value should be accepted', async function () { - const value = await this.proxy.admin.call({ from: anotherAccount, value: 1 }); + const value = await this.proxy.admin({ from: anotherAccount, value: 1 }); expect(value).to.be.equal('0x0000000000000000000000000000000011111142'); }); }); From 15297153c8d93066f33940231cd2a27aff1327e1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Mar 2023 20:05:19 +0100 Subject: [PATCH 07/20] reorder dispatch for gas optimisation --- .../transparent/TransparentUpgradeableProxy.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 33b05eec5ad..a6978c3e10b 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -52,16 +52,16 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { if (msg.sender == _getAdmin()) { bytes memory ret; bytes4 selector = msg.sig; - if (selector == ITransparentUpgradeableProxy.admin.selector) { - ret = _dispatchAdmin(); - } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { - ret = _dispatchImplementation(); - } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) { - ret = _dispatchChangeAdmin(); - } else if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { + if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { ret = _dispatchUpgradeTo(); } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { ret = _dispatchUpgradeToAndCall(); + } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) { + ret = _dispatchChangeAdmin(); + } else if (selector == ITransparentUpgradeableProxy.admin.selector) { + ret = _dispatchAdmin(); + } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { + ret = _dispatchImplementation(); } else { revert('TransparentUpgradeableProxy: admin cannot fallback to proxy target'); } From e8a44a77d2e4a373b91b60807d028646384a10d2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 3 Apr 2023 19:07:42 +0200 Subject: [PATCH 08/20] add changeset --- .changeset/thirty-shrimps-mix.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/thirty-shrimps-mix.md diff --git a/.changeset/thirty-shrimps-mix.md b/.changeset/thirty-shrimps-mix.md new file mode 100644 index 00000000000..cd3cc01ba24 --- /dev/null +++ b/.changeset/thirty-shrimps-mix.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`TransparentUpgradeableProxy`: fix transparency in case of selector clash with non-decodable calldata From 5a1df2c8a6ae9831d6825eb4060738b97ac1a7b6 Mon Sep 17 00:00:00 2001 From: Francisco Date: Mon, 3 Apr 2023 17:15:05 -0300 Subject: [PATCH 09/20] typo --- contracts/proxy/transparent/TransparentUpgradeableProxy.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index a6978c3e10b..cec67536f1d 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -46,7 +46,7 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { } /** - * @dev If caller is the admin process the call internally, otherwize transparently fallback to the proxy behavior + * @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 == _getAdmin()) { From f6f8a48077eaf398f1cc2d4d4024a4685cd1b944 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 4 Apr 2023 14:26:20 +0200 Subject: [PATCH 10/20] add IERC1967 & reenable ifAdmin modifier --- contracts/interfaces/IERC1967.sol | 20 ++++++++++++++++ contracts/proxy/ERC1967/ERC1967Upgrade.sol | 13 ++-------- .../TransparentUpgradeableProxy.sol | 24 ++++++++++++++++--- 3 files changed, 43 insertions(+), 14 deletions(-) create mode 100644 contracts/interfaces/IERC1967.sol diff --git a/contracts/interfaces/IERC1967.sol b/contracts/interfaces/IERC1967.sol new file mode 100644 index 00000000000..9181e59daad --- /dev/null +++ b/contracts/interfaces/IERC1967.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +/** + * @dev ERC-1967: Proxy Storage Slots. + * + * _Available since v4.9._ + */ +interface IERC1967 { + /** + * @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); +} \ No newline at end of file diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index 0680f35499b..be6929e8162 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Upgrade.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.2; import "../beacon/IBeacon.sol"; +import "../../interfaces/IERC1967.sol"; import "../../interfaces/draft-IERC1822.sol"; import "../../utils/Address.sol"; import "../../utils/StorageSlot.sol"; @@ -14,7 +15,7 @@ import "../../utils/StorageSlot.sol"; * * _Available since v4.1._ */ -abstract contract ERC1967Upgrade { +abstract contract ERC1967Upgrade is IERC1967 { // This is the keccak-256 hash of "eip1967.proxy.rollback" subtracted by 1 bytes32 private constant _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143; @@ -25,11 +26,6 @@ abstract contract ERC1967Upgrade { */ bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; - /** - * @dev Emitted when the implementation is upgraded. - */ - event Upgraded(address indexed implementation); - /** * @dev Returns the current implementation address. */ @@ -95,11 +91,6 @@ abstract contract ERC1967Upgrade { */ bytes32 internal constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; - /** - * @dev Emitted when the admin account has changed. - */ - event AdminChanged(address previousAdmin, address newAdmin); - /** * @dev Returns the current admin. */ diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index a6978c3e10b..2472e5994f4 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -5,9 +5,13 @@ pragma solidity ^0.8.0; import "../ERC1967/ERC1967Proxy.sol"; -interface ITransparentUpgradeableProxy { - event Upgraded(address indexed implementation); - event AdminChanged(address previousAdmin, address newAdmin); +/** + * @dev Interface for the {TransparentUpgradeableProxy}. This is usefull because {TransparentUpgradeableProxy} uses a + * custom call-routing mechanism, the compiler is unaware of the functions being exposed, and cannot list them. Also + * {TransparentUpgradeableProxy} does not inherit from this interface because its implemented in a way that the + * compiler doesn't understand and cannot verify. + */ +interface ITransparentUpgradeableProxy is IERC1967 { function admin() external view returns (address); function implementation() external view returns (address); function changeAdmin(address) external; @@ -45,6 +49,20 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { _changeAdmin(admin_); } + /** + * @dev Modifier used internally that will delegate the call to the implementation unless the sender is the admin. + * + * CAUTION: this modifier is deprecated, as it could cause issues if the modified function as arguments, and the + * implementation provide a function with a similar selector. + */ + modifier ifAdmin() { + if (msg.sender == _getAdmin()) { + _; + } else { + _fallback(); + } + } + /** * @dev If caller is the admin process the call internally, otherwize transparently fallback to the proxy behavior */ From 4583b59e2e9e05ed5a62440920efc7cf0ae790fc Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 4 Apr 2023 14:35:51 +0200 Subject: [PATCH 11/20] spelling --- contracts/proxy/transparent/TransparentUpgradeableProxy.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 2e724b52271..1aaf3808c62 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -6,7 +6,7 @@ pragma solidity ^0.8.0; import "../ERC1967/ERC1967Proxy.sol"; /** - * @dev Interface for the {TransparentUpgradeableProxy}. This is usefull because {TransparentUpgradeableProxy} uses a + * @dev Interface for the {TransparentUpgradeableProxy}. This is useful because {TransparentUpgradeableProxy} uses a * custom call-routing mechanism, the compiler is unaware of the functions being exposed, and cannot list them. Also * {TransparentUpgradeableProxy} does not inherit from this interface because its implemented in a way that the * compiler doesn't understand and cannot verify. From 9041109412563f4e37e06e3d843f6239ffe54d94 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 4 Apr 2023 14:48:43 +0200 Subject: [PATCH 12/20] Update IERC1967.sol --- contracts/interfaces/IERC1967.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/interfaces/IERC1967.sol b/contracts/interfaces/IERC1967.sol index 9181e59daad..6070bd43970 100644 --- a/contracts/interfaces/IERC1967.sol +++ b/contracts/interfaces/IERC1967.sol @@ -17,4 +17,4 @@ interface IERC1967 { * @dev Emitted when the admin account has changed. */ event AdminChanged(address previousAdmin, address newAdmin); -} \ No newline at end of file +} From 0b144e66b5b26b766c75f7420dd123e76c6f8976 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 4 Apr 2023 14:50:44 +0200 Subject: [PATCH 13/20] re-add comment --- contracts/proxy/transparent/TransparentUpgradeableProxy.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 1aaf3808c62..f51a374c024 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -121,6 +121,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { /** * @dev Changes the admin of the proxy. + * + * Emits an {AdminChanged} event. */ function _dispatchChangeAdmin() private returns (bytes memory) { _requireZeroValue(); From 707cde88ee410e62a0eb4a9a284105e4ba7b04e4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 4 Apr 2023 17:09:11 +0200 Subject: [PATCH 14/20] add a warning about missing ABI and possible selector clash in TransparentUpgradeableProxy --- .../proxy/transparent/TransparentUpgradeableProxy.sol | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index f51a374c024..9a8abf3537a 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -39,6 +39,13 @@ interface ITransparentUpgradeableProxy is IERC1967 { * * 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. + * + * WARNING: this contract does not inherit from {ITransparentUpgradeableProxy}, and the admin function implicitly + * implemented using a custom call-routing mechanism in `_fallback`. Consequently, the compiler will not produce an + * ABI for this contract. Also, if you inherit from this contract and add additional functions, the compiler will not + * check that there are no selector conflicts. 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 admin operation + * inaccessible, with could prevent upgradeability. */ contract TransparentUpgradeableProxy is ERC1967Proxy { /** From 7750b9cde35b0a1fe6fc15d08bc4bb3473b9dc73 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 4 Apr 2023 12:49:14 -0300 Subject: [PATCH 15/20] grammar --- .../proxy/transparent/TransparentUpgradeableProxy.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 9a8abf3537a..3fbf1895cdb 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -40,12 +40,12 @@ interface ITransparentUpgradeableProxy is IERC1967 { * 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. * - * WARNING: this contract does not inherit from {ITransparentUpgradeableProxy}, and the admin function implicitly + * WARNING: This contract does not inherit from {ITransparentUpgradeableProxy}, and the admin function is implicitly * implemented using a custom call-routing mechanism in `_fallback`. Consequently, the compiler will not produce an * ABI for this contract. Also, if you inherit from this contract and add additional functions, the compiler will not - * check that there are no selector conflicts. 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 admin operation - * inaccessible, with could prevent upgradeability. + * check that there are no selector conflicts. 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 admin operations + * inaccessible, which could prevent upgradeability. */ contract TransparentUpgradeableProxy is ERC1967Proxy { /** From de042ada9ddbc6bbf57a1fded11ff8e955eb0efa Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 4 Apr 2023 13:15:16 -0300 Subject: [PATCH 16/20] grammar --- contracts/proxy/transparent/TransparentUpgradeableProxy.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 3fbf1895cdb..eae79c508e0 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -59,8 +59,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { /** * @dev Modifier used internally that will delegate the call to the implementation unless the sender is the admin. * - * CAUTION: this modifier is deprecated, as it could cause issues if the modified function as arguments, and the - * implementation provide a function with a similar selector. + * CAUTION: This modifier is deprecated, as it could cause issues if the modified function has arguments, and the + * implementation provides a function with the same selector. */ modifier ifAdmin() { if (msg.sender == _getAdmin()) { From 5a029ce32664cd99b0f2dfde4cf7088001e956de Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 4 Apr 2023 13:53:15 -0300 Subject: [PATCH 17/20] fix ProxyAdmin test --- test/proxy/transparent/ProxyAdmin.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/proxy/transparent/ProxyAdmin.test.js b/test/proxy/transparent/ProxyAdmin.test.js index 811dc5671f0..6d473d89379 100644 --- a/test/proxy/transparent/ProxyAdmin.test.js +++ b/test/proxy/transparent/ProxyAdmin.test.js @@ -6,6 +6,7 @@ const ImplV1 = artifacts.require('DummyImplementation'); const ImplV2 = artifacts.require('DummyImplementationV2'); const ProxyAdmin = artifacts.require('ProxyAdmin'); const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy'); +const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy'); contract('ProxyAdmin', function (accounts) { const [proxyAdminOwner, newAdmin, anotherAccount] = accounts; @@ -18,12 +19,13 @@ contract('ProxyAdmin', function (accounts) { beforeEach(async function () { const initializeData = Buffer.from(''); this.proxyAdmin = await ProxyAdmin.new({ from: proxyAdminOwner }); - this.proxy = await TransparentUpgradeableProxy.new( + const proxy = await TransparentUpgradeableProxy.new( this.implementationV1.address, this.proxyAdmin.address, initializeData, { from: proxyAdminOwner }, ); + this.proxy = await ITransparentUpgradeableProxy.at(proxy.address); }); it('has an owner', async function () { From f7595a69af61af20e125dbe5861a6c3b849a1903 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 4 Apr 2023 13:54:52 -0300 Subject: [PATCH 18/20] lint --- .../proxy/transparent/TransparentUpgradeableProxy.sol | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index eae79c508e0..651f74448a9 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -13,10 +13,14 @@ import "../ERC1967/ERC1967Proxy.sol"; */ interface ITransparentUpgradeableProxy is IERC1967 { function admin() external view returns (address); + function implementation() external view returns (address); + function changeAdmin(address) external; + function upgradeTo(address) external; - function upgradeToAndCall(address, bytes memory) payable external; + + function upgradeToAndCall(address, bytes memory) external payable; } /** @@ -88,7 +92,7 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { ret = _dispatchImplementation(); } else { - revert('TransparentUpgradeableProxy: admin cannot fallback to proxy target'); + revert("TransparentUpgradeableProxy: admin cannot fallback to proxy target"); } assembly { return(add(ret, 0x20), mload(ret)) From 4b61bfe653167d7a1ba076c1645b7c1ffc7fc324 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 5 Apr 2023 10:30:26 +0200 Subject: [PATCH 19/20] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto GarcĂ­a Co-authored-by: Francisco --- .changeset/thirty-shrimps-mix.md | 2 +- contracts/interfaces/IERC1967.sol | 7 ++++++- .../proxy/transparent/TransparentUpgradeableProxy.sol | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/.changeset/thirty-shrimps-mix.md b/.changeset/thirty-shrimps-mix.md index cd3cc01ba24..54256d52cbf 100644 --- a/.changeset/thirty-shrimps-mix.md +++ b/.changeset/thirty-shrimps-mix.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': patch --- -`TransparentUpgradeableProxy`: fix transparency in case of selector clash with non-decodable calldata +`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata. diff --git a/contracts/interfaces/IERC1967.sol b/contracts/interfaces/IERC1967.sol index 6070bd43970..e5deebee9c2 100644 --- a/contracts/interfaces/IERC1967.sol +++ b/contracts/interfaces/IERC1967.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; /** - * @dev ERC-1967: Proxy Storage Slots. + * @dev ERC-1967: Proxy Storage Slots. This interface contains the events defined in the ERC. * * _Available since v4.9._ */ @@ -17,4 +17,9 @@ interface IERC1967 { * @dev Emitted when the admin account has changed. */ event AdminChanged(address previousAdmin, address newAdmin); + + /** + * @dev Emitted when the beacon is changed. + */ + event BeaconUpgraded(address indexed beacon); } diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 651f74448a9..a89b233f235 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -8,7 +8,7 @@ import "../ERC1967/ERC1967Proxy.sol"; /** * @dev Interface for the {TransparentUpgradeableProxy}. This is useful because {TransparentUpgradeableProxy} uses a * custom call-routing mechanism, the compiler is unaware of the functions being exposed, and cannot list them. Also - * {TransparentUpgradeableProxy} does not inherit from this interface because its implemented in a way that the + * {TransparentUpgradeableProxy} does not inherit from this interface because it's implemented in a way that the * compiler doesn't understand and cannot verify. */ interface ITransparentUpgradeableProxy is IERC1967 { From b082da8228dfcc42665fb4d609efb1ef0b87370f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 5 Apr 2023 10:31:12 +0200 Subject: [PATCH 20/20] Update ERC1967Upgrade.sol --- contracts/proxy/ERC1967/ERC1967Upgrade.sol | 5 ----- 1 file changed, 5 deletions(-) diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index be6929e8162..a79c10502c0 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Upgrade.sol @@ -122,11 +122,6 @@ abstract contract ERC1967Upgrade is IERC1967 { */ bytes32 internal constant _BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50; - /** - * @dev Emitted when the beacon is upgraded. - */ - event BeaconUpgraded(address indexed beacon); - /** * @dev Returns the current beacon. */