From 2a62fb4a2d9b9248aa15bba9c89d75a7ddc8cfc6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 19 Jan 2023 22:34:15 +0100 Subject: [PATCH] Improve TransparentUpgradeableProxy's transparency (#3977) Co-authored-by: Francisco --- .changeset/many-panthers-hide.md | 5 +++++ .../mocks/proxy/ClashingImplementation.sol | 7 +++---- .../TransparentUpgradeableProxy.sol | 20 +++++++++++++++---- .../TransparentUpgradeableProxy.behaviour.js | 15 +++++++++++--- 4 files changed, 36 insertions(+), 11 deletions(-) create mode 100644 .changeset/many-panthers-hide.md diff --git a/.changeset/many-panthers-hide.md b/.changeset/many-panthers-hide.md new file mode 100644 index 00000000000..5f04c99df18 --- /dev/null +++ b/.changeset/many-panthers-hide.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`TransparentUpgradeableProxy`: support value passthrough for all ifAdmin function. diff --git a/contracts/mocks/proxy/ClashingImplementation.sol b/contracts/mocks/proxy/ClashingImplementation.sol index 80aca0c298f..5c272a89d24 100644 --- a/contracts/mocks/proxy/ClashingImplementation.sol +++ b/contracts/mocks/proxy/ClashingImplementation.sol @@ -3,12 +3,11 @@ pragma solidity ^0.8.0; /** - * @dev Implementation contract with an admin() function made to clash with - * @dev TransparentUpgradeableProxy's to test correct functioning of the - * @dev Transparent Proxy feature. + * @dev Implementation contract with a payable admin() function made to clash with TransparentUpgradeableProxy's to + * test correct functioning of the Transparent Proxy feature. */ contract ClashingImplementation { - function admin() external pure returns (address) { + function admin() external payable returns (address) { return 0x0000000000000000000000000000000011111142; } diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 3685360e76e..155a22e01f7 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -55,7 +55,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103` */ - function admin() external ifAdmin returns (address admin_) { + function admin() external payable ifAdmin returns (address admin_) { + _requireZeroValue(); admin_ = _getAdmin(); } @@ -68,7 +69,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc` */ - function implementation() external ifAdmin returns (address implementation_) { + function implementation() external payable ifAdmin returns (address implementation_) { + _requireZeroValue(); implementation_ = _implementation(); } @@ -79,7 +81,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { * * NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}. */ - function changeAdmin(address newAdmin) external virtual ifAdmin { + function changeAdmin(address newAdmin) external payable virtual ifAdmin { + _requireZeroValue(); _changeAdmin(newAdmin); } @@ -88,7 +91,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { * * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}. */ - function upgradeTo(address newImplementation) external ifAdmin { + function upgradeTo(address newImplementation) external payable ifAdmin { + _requireZeroValue(); _upgradeToAndCall(newImplementation, bytes(""), false); } @@ -117,4 +121,12 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { 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. + */ + function _requireZeroValue() private { + require(msg.value == 0); + } } diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index da8f21bee18..3a10357a905 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -330,14 +330,23 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx ); }); - context('when function names clash', function () { + 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 }); + const value = await this.proxy.admin.call({ 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 }); + const value = await this.proxy.admin.call({ 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 })); + }); + + it('when sender is other value should be accepted', async function () { + const value = await this.proxy.admin.call({ from: anotherAccount, value: 1 }); expect(value).to.be.equal('0x0000000000000000000000000000000011111142'); }); });