Bubble revert reasons in proxy initialization (#2454)

Co-authored-by: Hadrien Croubois <hadrien@openzeppelin.com>

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)
 

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);
     }
 
     /**

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 {

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',
+        );
+      });
+    });
   });
 };