From 190286ff398299ca67c2da1fac1ea61cc7e1ae58 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 30 Dec 2021 11:10:32 +0100 Subject: [PATCH] address comments from PR --- .../draft-IERC1822.sol} | 2 +- contracts/proxy/ERC1967/ERC1967Upgrade.sol | 7 ++++-- contracts/proxy/utils/UUPSUpgradeable.sol | 4 ++-- test/helpers/erc1967.js | 24 +++++++++++++++++++ test/proxy/Proxy.behaviour.js | 18 +++++--------- test/proxy/beacon/BeaconProxy.test.js | 16 ++++--------- .../TransparentUpgradeableProxy.behaviour.js | 21 ++++++---------- test/proxy/utils/UUPSUpgradeable.test.js | 9 +++++-- 8 files changed, 57 insertions(+), 44 deletions(-) rename contracts/{proxy/ERC1822/IProxiable.sol => interfaces/draft-IERC1822.sol} (86%) create mode 100644 test/helpers/erc1967.js diff --git a/contracts/proxy/ERC1822/IProxiable.sol b/contracts/interfaces/draft-IERC1822.sol similarity index 86% rename from contracts/proxy/ERC1822/IProxiable.sol rename to contracts/interfaces/draft-IERC1822.sol index 265f09e10aa..32d14e89915 100644 --- a/contracts/proxy/ERC1822/IProxiable.sol +++ b/contracts/interfaces/draft-IERC1822.sol @@ -3,6 +3,6 @@ pragma solidity ^0.8.0; -interface IProxiable { +interface IERC1822Proxiable { function proxiableUUID() external view returns (bytes32); } diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index adc8254bdbe..256336cde11 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Upgrade.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.2; import "../beacon/IBeacon.sol"; -import "../ERC1822/IProxiable.sol"; +import "../../interfaces/draft-IERC1822.sol"; import "../../utils/Address.sol"; import "../../utils/StorageSlot.sol"; @@ -83,11 +83,14 @@ abstract contract ERC1967Upgrade { bytes memory data, bool forceCall ) internal { + // Upgrades from old implementations will perform a rollback test. This test requires this + // new implementation to upgrade to an old, non-ERC1822 compliant, implementation. Removing + // this special case will break upgrade paths from old UUPS implementation to new ones. if (StorageSlot.getBooleanSlot(_ROLLBACK_SLOT).value) { _setImplementation(newImplementation); } else { require( - IProxiable(newImplementation).proxiableUUID() == _IMPLEMENTATION_SLOT, + IERC1822Proxiable(newImplementation).proxiableUUID() == _IMPLEMENTATION_SLOT, "Invalid proxiableUUID value" ); _upgradeToAndCall(newImplementation, data, forceCall); diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index a26df02edb0..9ef067a3975 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; -import "../ERC1822/IProxiable.sol"; +import "../../interfaces/draft-IERC1822.sol"; import "../ERC1967/ERC1967Upgrade.sol"; /** @@ -18,7 +18,7 @@ import "../ERC1967/ERC1967Upgrade.sol"; * * _Available since v4.1._ */ -abstract contract UUPSUpgradeable is IProxiable, ERC1967Upgrade { +abstract contract UUPSUpgradeable is IERC1822Proxiable, ERC1967Upgrade { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment address private immutable __self = address(this); diff --git a/test/helpers/erc1967.js b/test/helpers/erc1967.js new file mode 100644 index 00000000000..592ad7234ef --- /dev/null +++ b/test/helpers/erc1967.js @@ -0,0 +1,24 @@ +const { BN } = require('@openzeppelin/test-helpers'); +const ethereumjsUtil = require('ethereumjs-util'); + +const ImplementationLabel = 'eip1967.proxy.implementation'; +const AdminLabel = 'eip1967.proxy.admin'; +const BeaconLabel = 'eip1967.proxy.beacon'; + +function labelToSlot (label) { + return '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(label))).subn(1).toString(16); +} + +function toChecksumAddress (address) { + return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0')); +} + +module.exports = { + ImplementationLabel, + AdminLabel, + BeaconLabel, + ImplementationSlot: labelToSlot(ImplementationLabel), + AdminSlot: labelToSlot(AdminLabel), + BeaconSlot: labelToSlot(BeaconLabel), + toChecksumAddress, +}; diff --git a/test/proxy/Proxy.behaviour.js b/test/proxy/Proxy.behaviour.js index 8cb457b06c6..01ae8315c6e 100644 --- a/test/proxy/Proxy.behaviour.js +++ b/test/proxy/Proxy.behaviour.js @@ -1,16 +1,10 @@ -const { BN, expectRevert } = require('@openzeppelin/test-helpers'); -const ethereumjsUtil = require('ethereumjs-util'); +const { expectRevert } = require('@openzeppelin/test-helpers'); +const { ImplementationSlot, toChecksumAddress } = require('../helpers/erc1967'); const { expect } = require('chai'); const DummyImplementation = artifacts.require('DummyImplementation'); -const IMPLEMENTATION_LABEL = 'eip1967.proxy.implementation'; - -function toChecksumAddress (address) { - return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0')); -} - module.exports = function shouldBehaveLikeProxy (createProxy, proxyAdminAddress, proxyCreator) { it('cannot be initialized with a non-contract address', async function () { const nonContractAddress = proxyCreator; @@ -23,14 +17,14 @@ module.exports = function shouldBehaveLikeProxy (createProxy, proxyAdminAddress, }); before('deploy implementation', async function () { - this.implementation = web3.utils.toChecksumAddress((await DummyImplementation.new()).address); + this.implementation = toChecksumAddress((await DummyImplementation.new()).address); }); const assertProxyInitialization = function ({ value, balance }) { it('sets the implementation address', async function () { - const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(IMPLEMENTATION_LABEL))).subn(1).toString(16); - const implementation = toChecksumAddress((await web3.eth.getStorageAt(this.proxy, slot)).substr(-40)); - expect(implementation).to.be.equal(this.implementation); + const implementationSlot = await web3.eth.getStorageAt(this.proxy, ImplementationSlot); + const implementationAddress = toChecksumAddress(implementationSlot.substr(-40)); + expect(implementationAddress).to.be.equal(this.implementation); }); it('initializes the proxy', async function () { diff --git a/test/proxy/beacon/BeaconProxy.test.js b/test/proxy/beacon/BeaconProxy.test.js index e02bb3b297a..e5f7b07ffca 100644 --- a/test/proxy/beacon/BeaconProxy.test.js +++ b/test/proxy/beacon/BeaconProxy.test.js @@ -1,6 +1,6 @@ -const { BN, expectRevert } = require('@openzeppelin/test-helpers'); -const ethereumjsUtil = require('ethereumjs-util'); -const { keccak256 } = ethereumjsUtil; +const { expectRevert } = require('@openzeppelin/test-helpers'); +const { keccak256 } = require('ethereumjs-util'); +const { BeaconSlot, toChecksumAddress } = require('../../helpers/erc1967'); const { expect } = require('chai'); @@ -11,13 +11,6 @@ const DummyImplementationV2 = artifacts.require('DummyImplementationV2'); const BadBeaconNoImpl = artifacts.require('BadBeaconNoImpl'); const BadBeaconNotContract = artifacts.require('BadBeaconNotContract'); -function toChecksumAddress (address) { - return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0').substr(-40)); -} - -const BEACON_LABEL = 'eip1967.proxy.beacon'; -const BEACON_SLOT = '0x' + new BN(keccak256(Buffer.from(BEACON_LABEL))).subn(1).toString(16); - contract('BeaconProxy', function (accounts) { const [anotherAccount] = accounts; @@ -53,7 +46,8 @@ contract('BeaconProxy', function (accounts) { describe('initialization', function () { before(function () { this.assertInitialized = async ({ value, balance }) => { - const beaconAddress = toChecksumAddress(await web3.eth.getStorageAt(this.proxy.address, BEACON_SLOT)); + const beaconSlot = await web3.eth.getStorageAt(this.proxy.address, BeaconSlot); + const beaconAddress = toChecksumAddress(beaconSlot.substr(-40)); expect(beaconAddress).to.equal(this.beacon.address); const dummy = new DummyImplementation(this.proxy.address); diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 54b78e06420..d2b96166947 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -1,6 +1,6 @@ const { BN, expectRevert, expectEvent, constants } = require('@openzeppelin/test-helpers'); const { ZERO_ADDRESS } = constants; -const ethereumjsUtil = require('ethereumjs-util'); +const { ImplementationSlot, AdminSlot, toChecksumAddress } = require('../../helpers/erc1967'); const { expect } = require('chai'); @@ -16,13 +16,6 @@ const InitializableMock = artifacts.require('InitializableMock'); const DummyImplementation = artifacts.require('DummyImplementation'); const ClashingImplementation = artifacts.require('ClashingImplementation'); -const IMPLEMENTATION_LABEL = 'eip1967.proxy.implementation'; -const ADMIN_LABEL = 'eip1967.proxy.admin'; - -function toChecksumAddress (address) { - return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0').substr(-40)); -} - module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createProxy, accounts) { const [proxyAdminAddress, proxyAdminOwner, anotherAccount] = accounts; @@ -312,15 +305,15 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro describe('storage', function () { it('should store the implementation address in specified location', async function () { - const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(IMPLEMENTATION_LABEL))).subn(1).toString(16); - const implementation = toChecksumAddress(await web3.eth.getStorageAt(this.proxyAddress, slot)); - expect(implementation).to.be.equal(this.implementationV0); + const implementationSlot = await web3.eth.getStorageAt(this.proxyAddress, ImplementationSlot); + const implementationAddress = toChecksumAddress(implementationSlot.substr(-40)); + expect(implementationAddress).to.be.equal(this.implementationV0); }); it('should store the admin proxy in specified location', async function () { - const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(ADMIN_LABEL))).subn(1).toString(16); - const proxyAdmin = toChecksumAddress(await web3.eth.getStorageAt(this.proxyAddress, slot)); - expect(proxyAdmin).to.be.equal(proxyAdminAddress); + const proxyAdminSlot = await web3.eth.getStorageAt(this.proxyAddress, AdminSlot); + const proxyAdminAddress = toChecksumAddress(proxyAdminSlot.substr(-40)); + expect(proxyAdminAddress).to.be.equal(proxyAdminAddress); }); }); diff --git a/test/proxy/utils/UUPSUpgradeable.test.js b/test/proxy/utils/UUPSUpgradeable.test.js index 8885e0b8330..5d4115d655f 100644 --- a/test/proxy/utils/UUPSUpgradeable.test.js +++ b/test/proxy/utils/UUPSUpgradeable.test.js @@ -1,4 +1,6 @@ const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { web3 } = require('@openzeppelin/test-helpers/src/setup'); +const { ImplementationSlot, toChecksumAddress } = require('../../helpers/erc1967'); const ERC1967Proxy = artifacts.require('ERC1967Proxy'); const UUPSUpgradeableMock = artifacts.require('UUPSUpgradeableMock'); @@ -55,9 +57,8 @@ contract('UUPSUpgradeable', function (accounts) { // delegate to a non existing upgradeTo function causes a low level revert it('reject upgrade to non uups implementation', async function () { - await expectRevert( + await expectRevert.unspecified( this.instance.upgradeTo(this.implUpgradeNonUUPS.address), - 'Transaction reverted: function selector was not recognized and there\'s no fallback function', ); }); @@ -88,6 +89,10 @@ contract('UUPSUpgradeable', function (accounts) { expect(UpgradedEvents.length).to.be.equal(1); expectEvent(receipt, 'Upgraded', { implementation: this.implInitial.address }); + + const implementationSlot = await web3.eth.getStorageAt(legacyInstance.address, ImplementationSlot); + const implementationAddress = toChecksumAddress(implementationSlot.substr(-40)); + expect(implementationAddress).to.be.equal(this.implInitial.address); }); }; });