diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d21be5f3df..61e6d6c23f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,9 @@ ## Unreleased (Breaking) * `TimelockController`: Changed the role architecture to use `DEFAULT_ADMIN_ROLE` as the admin for all roles, instead of the bespoke `TIMELOCK_ADMIN_ROLE` that was used previously. This aligns with the general recommendation for `AccessControl` and makes the addition of new roles easier. Accordingly, the `admin` parameter and timelock will now be granted `DEFAULT_ADMIN_ROLE` instead of `TIMELOCK_ADMIN_ROLE`. ([#3799](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3799)) -* Removed presets in favor of [OpenZeppelin Contracts Wizard](https://wizard.openzeppelin.com/). + * Removed presets in favor of [OpenZeppelin Contracts Wizard](https://wizard.openzeppelin.com/). + * `TransparentUpgradeableProxy`: Removed `admin` and `implementation` getters, which were only callable by the proxy owner and thus not very useful. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820)) + * `ProxyAdmin`: Removed `getProxyAdmin` and `getProxyImplementation` getters. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820)) ## Unreleased diff --git a/contracts/proxy/ERC1967/ERC1967Proxy.sol b/contracts/proxy/ERC1967/ERC1967Proxy.sol index a04d701ce19..642d811ccae 100644 --- a/contracts/proxy/ERC1967/ERC1967Proxy.sol +++ b/contracts/proxy/ERC1967/ERC1967Proxy.sol @@ -25,6 +25,10 @@ contract ERC1967Proxy is Proxy, ERC1967Upgrade { /** * @dev Returns the current implementation address. + * + * 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 _implementation() internal view virtual override returns (address impl) { return ERC1967Upgrade._getImplementation(); diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index 77fbdd16565..07dc048625a 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Upgrade.sol @@ -112,6 +112,10 @@ abstract contract ERC1967Upgrade { /** * @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. + * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103` */ function _getAdmin() internal view returns (address) { return StorageSlot.getAddressSlot(_ADMIN_SLOT).value; diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index 839534298b9..7f340f62935 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -11,36 +11,6 @@ import "../../access/Ownable.sol"; * explanation of why you would want to use this see the documentation for {TransparentUpgradeableProxy}. */ 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`. * diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 4de85075ac1..57417296d6e 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -50,32 +50,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { } } - /** - * @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 ifAdmin returns (address admin_) { - 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` - */ - function implementation() external ifAdmin returns (address implementation_) { - implementation_ = _implementation(); - } - /** * @dev Changes the admin of the proxy. * diff --git a/test/helpers/erc1967.js b/test/helpers/erc1967.js index aab0e2288a3..184f1cd0f8e 100644 --- a/test/helpers/erc1967.js +++ b/test/helpers/erc1967.js @@ -13,6 +13,11 @@ function getSlot (address, slot) { ); } +async function getAddressInSlot (address, slot) { + const slotValue = await getSlot(address, slot); + return web3.utils.toChecksumAddress(slotValue.substr(-40)); +} + module.exports = { ImplementationLabel, AdminLabel, @@ -21,4 +26,5 @@ module.exports = { AdminSlot: labelToSlot(AdminLabel), BeaconSlot: labelToSlot(BeaconLabel), getSlot, + getAddressInSlot, }; diff --git a/test/proxy/transparent/ProxyAdmin.test.js b/test/proxy/transparent/ProxyAdmin.test.js index 07adba6ad69..b1f5500296d 100644 --- a/test/proxy/transparent/ProxyAdmin.test.js +++ b/test/proxy/transparent/ProxyAdmin.test.js @@ -1,7 +1,6 @@ const { expectRevert } = require('@openzeppelin/test-helpers'); - +const { getAddressInSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967'); const { expect } = require('chai'); - const ImplV1 = artifacts.require('DummyImplementation'); const ImplV2 = artifacts.require('DummyImplementationV2'); const ProxyAdmin = artifacts.require('ProxyAdmin'); @@ -30,17 +29,6 @@ contract('ProxyAdmin', function (accounts) { expect(await this.proxyAdmin.owner()).to.equal(proxyAdminOwner); }); - describe('#getProxyAdmin', function () { - it('returns proxyAdmin as admin of the proxy', async function () { - const admin = await this.proxyAdmin.getProxyAdmin(this.proxy.address); - expect(admin).to.be.equal(this.proxyAdmin.address); - }); - - it('call to invalid proxy', async function () { - await expectRevert.unspecified(this.proxyAdmin.getProxyAdmin(this.implementationV1.address)); - }); - }); - describe('#changeProxyAdmin', function () { it('fails to change proxy admin if its not the proxy owner', async function () { await expectRevert( @@ -51,18 +39,9 @@ contract('ProxyAdmin', function (accounts) { it('changes proxy admin', async function () { await this.proxyAdmin.changeProxyAdmin(this.proxy.address, newAdmin, { from: proxyAdminOwner }); - expect(await this.proxy.admin.call({ from: newAdmin })).to.eq(newAdmin); - }); - }); - describe('#getProxyImplementation', function () { - it('returns proxy implementation address', async function () { - const implementationAddress = await this.proxyAdmin.getProxyImplementation(this.proxy.address); - expect(implementationAddress).to.be.equal(this.implementationV1.address); - }); - - it('call to invalid proxy', async function () { - await expectRevert.unspecified(this.proxyAdmin.getProxyImplementation(this.implementationV1.address)); + const newProxyAdmin = await getAddressInSlot(this.proxy, AdminSlot); + expect(newProxyAdmin).to.be.eq(newAdmin); }); }); @@ -79,8 +58,9 @@ contract('ProxyAdmin', function (accounts) { context('with authorized account', function () { it('upgrades implementation', async function () { await this.proxyAdmin.upgrade(this.proxy.address, this.implementationV2.address, { from: proxyAdminOwner }); - const implementationAddress = await this.proxyAdmin.getProxyImplementation(this.proxy.address); - expect(implementationAddress).to.be.equal(this.implementationV2.address); + + const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); + expect(implementationAddress).to.be.eq(this.implementationV2.address); }); }); }); @@ -116,8 +96,8 @@ contract('ProxyAdmin', function (accounts) { await this.proxyAdmin.upgradeAndCall(this.proxy.address, this.implementationV2.address, callData, { from: proxyAdminOwner }, ); - const implementationAddress = await this.proxyAdmin.getProxyImplementation(this.proxy.address); - expect(implementationAddress).to.be.equal(this.implementationV2.address); + const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); + expect(implementationAddress).to.be.eq(this.implementationV2.address); }); }); }); diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 33fef6f4119..478bc1c299b 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 { getSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967'); +const { getAddressInSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967'); const { expect } = require('chai'); @@ -34,9 +34,8 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro describe('implementation', function () { it('returns the current implementation address', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); - - expect(implementation).to.be.equal(this.implementationV0); + const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); + expect(implementationAddress).to.be.equal(this.implementationV0); }); it('delegates to the implementation', async function () { @@ -55,8 +54,8 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro it('upgrades to the requested implementation', async function () { await this.proxy.upgradeTo(this.implementationV1, { from }); - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); - expect(implementation).to.be.equal(this.implementationV1); + const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); + expect(implementationAddress).to.be.equal(this.implementationV1); }); it('emits an event', async function () { @@ -108,8 +107,8 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro }); it('upgrades to the requested implementation', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); - expect(implementation).to.be.equal(this.behavior.address); + const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); + expect(implementationAddress).to.be.equal(this.behavior.address); }); it('emits an event', function () { @@ -173,7 +172,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await getAddressInSlot(this.proxy, ImplementationSlot); expect(implementation).to.be.equal(this.behaviorV1.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV1.address }); }); @@ -199,7 +198,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await getAddressInSlot(this.proxy, ImplementationSlot); expect(implementation).to.be.equal(this.behaviorV2.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV2.address }); }); @@ -228,7 +227,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await getAddressInSlot(this.proxy, ImplementationSlot); expect(implementation).to.be.equal(this.behaviorV3.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV3.address }); }); @@ -274,7 +273,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro }); it('assigns new proxy admin', async function () { - const newProxyAdmin = await this.proxy.admin.call({ from: newAdmin }); + const newProxyAdmin = await getAddressInSlot(this.proxy, AdminSlot); expect(newProxyAdmin).to.be.equal(anotherAccount); }); @@ -303,20 +302,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro }); }); - describe('storage', function () { - it('should store the implementation address in specified location', async function () { - const implementationSlot = await getSlot(this.proxy, ImplementationSlot); - const implementationAddress = web3.utils.toChecksumAddress(implementationSlot.substr(-40)); - expect(implementationAddress).to.be.equal(this.implementationV0); - }); - - it('should store the admin proxy in specified location', async function () { - const proxyAdminSlot = await getSlot(this.proxy, AdminSlot); - const proxyAdminAddress = web3.utils.toChecksumAddress(proxyAdminSlot.substr(-40)); - expect(proxyAdminAddress).to.be.equal(proxyAdminAddress); - }); - }); - describe('transparent proxy', function () { beforeEach('creating proxy', async function () { const initializeData = Buffer.from(''); @@ -332,18 +317,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro 'TransparentUpgradeableProxy: admin cannot fallback to proxy target', ); }); - - context('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 }); - 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 }); - expect(value).to.be.equal('0x0000000000000000000000000000000011111142'); - }); - }); }); describe('regression', () => {