NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix TransparentUpgradeableProxy's transparency (#4154)

Browse Source 
Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Ernesto García <ernestognw@gmail.com>
(cherry picked from commit 5523c1482b)
This commit is contained in:
Hadrien Croubois
2023-04-05 16:57:08 +02:00
committed by Francisco Giordano
parent 8dfeb5d79e
commit c01ea99123
8 changed files with 132 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/thirty-shrimps-mix.md Normal file
View File

@ -0,0 +1,5 @@
---
'openzeppelin-solidity': patch
---
`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata.

25
contracts/interfaces/IERC1967.sol Normal file
View File

@ -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);
}

18
contracts/proxy/ERC1967/ERC1967Upgrade.sol
View File

@ -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.
*/

10
contracts/proxy/transparent/ProxyAdmin.sol
View File

@ -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 {

103
contracts/proxy/transparent/TransparentUpgradeableProxy.sol
View File

@ -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.

4
test/proxy/transparent/ProxyAdmin.test.js
View File

@ -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 () {

22
test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js
View File

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

4
test/proxy/transparent/TransparentUpgradeableProxy.test.js
View File

@ -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);