diff --git a/CHANGELOG.md b/CHANGELOG.md index 678f75d671f..43f53611183 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * `ERC20Permit`: added an implementation of the ERC20 permit extension for gasless token approvals. ([#2237](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2237)) * Presets: added token presets with preminted fixed supply `ERC20PresetFixedSupply` and `ERC777PresetFixedSupply`. ([#2399](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2399)) * `Address`: added `functionDelegateCall`, similar to the existing `functionCall`. ([#2333](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2333)) + * `UpgradeableProxy`: bubble revert reasons from initialization calls. ([#2454](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2454)) ## 3.3.0 (2020-11-26) diff --git a/contracts/proxy/TransparentUpgradeableProxy.sol b/contracts/proxy/TransparentUpgradeableProxy.sol index 7908a6497c7..a387943e37c 100644 --- a/contracts/proxy/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/TransparentUpgradeableProxy.sol @@ -6,22 +6,22 @@ import "./UpgradeableProxy.sol"; /** * @dev This contract implements a proxy that is upgradeable by an admin. - * + * * 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 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". - * + * * 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. - * + * * 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. */ @@ -60,9 +60,9 @@ contract TransparentUpgradeableProxy is UpgradeableProxy { /** * @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` @@ -73,9 +73,9 @@ contract TransparentUpgradeableProxy is UpgradeableProxy { /** * @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` @@ -86,9 +86,9 @@ contract TransparentUpgradeableProxy is UpgradeableProxy { /** * @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 ifAdmin { @@ -99,7 +99,7 @@ contract TransparentUpgradeableProxy is UpgradeableProxy { /** * @dev Upgrade the implementation of the proxy. - * + * * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}. */ function upgradeTo(address newImplementation) external ifAdmin { @@ -110,14 +110,12 @@ contract TransparentUpgradeableProxy is UpgradeableProxy { * @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 { _upgradeTo(newImplementation); - // solhint-disable-next-line avoid-low-level-calls - (bool success,) = newImplementation.delegatecall(data); - require(success); + Address.functionDelegateCall(newImplementation, data); } /** diff --git a/contracts/proxy/UpgradeableProxy.sol b/contracts/proxy/UpgradeableProxy.sol index 1889351a3cd..e9a58db8c18 100644 --- a/contracts/proxy/UpgradeableProxy.sol +++ b/contracts/proxy/UpgradeableProxy.sol @@ -10,14 +10,14 @@ import "../utils/Address.sol"; * 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. - * + * * Upgradeability is only provided internally through {_upgradeTo}. For an externally upgradeable proxy see * {TransparentUpgradeableProxy}. */ contract UpgradeableProxy is Proxy { /** * @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 initializating the storage of the proxy like a Solidity constructor. */ @@ -25,9 +25,7 @@ contract UpgradeableProxy is Proxy { assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1)); _setImplementation(_logic); if(_data.length > 0) { - // solhint-disable-next-line avoid-low-level-calls - (bool success,) = _logic.delegatecall(_data); - require(success); + Address.functionDelegateCall(_logic, _data); } } @@ -56,7 +54,7 @@ contract UpgradeableProxy is Proxy { /** * @dev Upgrades the proxy to a new implementation. - * + * * Emits an {Upgraded} event. */ function _upgradeTo(address newImplementation) internal { diff --git a/test/proxy/UpgradeableProxy.behaviour.js b/test/proxy/UpgradeableProxy.behaviour.js index 047451508ad..52e679c8fc8 100644 --- a/test/proxy/UpgradeableProxy.behaviour.js +++ b/test/proxy/UpgradeableProxy.behaviour.js @@ -210,5 +210,17 @@ module.exports = function shouldBehaveLikeUpgradeableProxy (createProxy, proxyAd }); }); }); + + describe('reverting initialization', function () { + const initializeData = new DummyImplementation('').contract + .methods.reverts().encodeABI(); + + it('reverts', async function () { + await expectRevert( + createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator }), + 'DummyImplementation reverted', + ); + }); + }); }); };