From c01ea99123a9be7494557b6b2ca5942c6be487f9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 5 Apr 2023 16:57:08 +0200 Subject: [PATCH] Fix TransparentUpgradeableProxy's transparency (#4154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Francisco Co-authored-by: Ernesto García (cherry picked from commit 5523c1482bd0a503e4c4dfe821bd2f6b523f3c86) --- .changeset/thirty-shrimps-mix.md | 5 + contracts/interfaces/IERC1967.sol | 25 +++++ contracts/proxy/ERC1967/ERC1967Upgrade.sol | 18 +-- contracts/proxy/transparent/ProxyAdmin.sol | 10 +- .../TransparentUpgradeableProxy.sol | 103 +++++++++++++----- test/proxy/transparent/ProxyAdmin.test.js | 4 +- .../TransparentUpgradeableProxy.behaviour.js | 22 ++-- .../TransparentUpgradeableProxy.test.js | 4 +- 8 files changed, 132 insertions(+), 59 deletions(-) create mode 100644 .changeset/thirty-shrimps-mix.md create mode 100644 contracts/interfaces/IERC1967.sol diff --git a/.changeset/thirty-shrimps-mix.md b/.changeset/thirty-shrimps-mix.md new file mode 100644 index 00000000000..54256d52cbf --- /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. diff --git a/contracts/interfaces/IERC1967.sol b/contracts/interfaces/IERC1967.sol new file mode 100644 index 00000000000..e5deebee9c2 --- /dev/null +++ b/contracts/interfaces/IERC1967.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +/** + * @dev ERC-1967: Proxy Storage Slots. This interface contains the events defined in the ERC. + * + * _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); + + /** + * @dev Emitted when the beacon is changed. + */ + event BeaconUpgraded(address indexed beacon); +} diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index 77fbdd16565..fdd532a3e47 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"; @@ -16,7 +17,7 @@ import "../../utils/StorageSlot.sol"; * * @custom:oz-upgrades-unsafe-allow delegatecall */ -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; @@ -27,11 +28,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. */ @@ -105,11 +101,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. */ @@ -141,11 +132,6 @@ abstract contract ERC1967Upgrade { */ bytes32 internal constant _BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50; - /** - * @dev Emitted when the beacon is upgraded. - */ - event BeaconUpgraded(address indexed beacon); - /** * @dev Returns the current beacon. */ 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 589b1c66e87..909ef64056f 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -5,6 +5,24 @@ pragma solidity ^0.8.0; 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 it's 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; + + function upgradeTo(address) external; + + function upgradeToAndCall(address, bytes memory) external payable; +} + /** * @dev This contract implements a proxy that is upgradeable by an admin. * @@ -25,6 +43,13 @@ import "../ERC1967/ERC1967Proxy.sol"; * * 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 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. 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 { /** @@ -41,6 +66,9 @@ 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 has arguments, and the + * implementation provides a function with the same selector. */ modifier ifAdmin() { if (msg.sender == _getAdmin()) { @@ -50,65 +78,98 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { } } + /** + * @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()) { + bytes memory ret; + bytes4 selector = msg.sig; + 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"); + } + assembly { + return(add(ret, 0x20), mload(ret)) + } + } else { + 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_) { + function _dispatchAdmin() private returns (bytes memory) { _requireZeroValue(); - admin_ = _getAdmin(); + + address admin = _getAdmin(); + return abi.encode(admin); } /** * @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 payable ifAdmin returns (address implementation_) { + function _dispatchImplementation() private returns (bytes memory) { _requireZeroValue(); - implementation_ = _implementation(); + + address implementation = _implementation(); + return abi.encode(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 { + 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. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}. */ - function upgradeTo(address newImplementation) external payable ifAdmin { + function _dispatchUpgradeTo() private returns (bytes memory) { _requireZeroValue(); + + address newImplementation = abi.decode(msg.data[4:], (address)); _upgradeToAndCall(newImplementation, bytes(""), false); + + return ""; } /** * @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 { + function _dispatchUpgradeToAndCall() private returns (bytes memory) { + (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); _upgradeToAndCall(newImplementation, data, true); + + return ""; } /** @@ -118,14 +179,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/ProxyAdmin.test.js b/test/proxy/transparent/ProxyAdmin.test.js index 07adba6ad69..5187fb2a2aa 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 () { diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 2c44efaf0b8..114b9388713 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -34,7 +34,7 @@ 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 }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.implementationV0); }); @@ -55,7 +55,7 @@ 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 }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.implementationV1); }); @@ -108,7 +108,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro }); 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); }); @@ -173,7 +173,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 this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV1.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV1.address }); }); @@ -199,7 +199,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 this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV2.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV2.address }); }); @@ -228,7 +228,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 this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV3.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV3.address }); }); @@ -274,7 +274,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 this.proxy.admin({ from: newAdmin }); expect(newProxyAdmin).to.be.equal(anotherAccount); }); @@ -335,21 +335,21 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro 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'); }); }); 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);