Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TransparentUpgradeableProxy's transparency #4154

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/thirty-shrimps-mix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': patch
---

`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata.
25 changes: 25 additions & 0 deletions contracts/interfaces/IERC1967.sol
Original file line number Diff line number Diff line change
@@ -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._
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct since it's released as part of v4.8.3. Will open a PR @Amxx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI #4183

*/
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);
}
Amxx marked this conversation as resolved.
Show resolved Hide resolved
18 changes: 2 additions & 16 deletions contracts/proxy/ERC1967/ERC1967Upgrade.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -14,7 +15,7 @@ import "../../utils/StorageSlot.sol";
*
* _Available since v4.1._
*/
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;

Expand All @@ -25,11 +26,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.
*/
Expand Down Expand Up @@ -95,11 +91,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.
*/
Expand Down Expand Up @@ -131,11 +122,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.
*/
Expand Down
10 changes: 5 additions & 5 deletions contracts/proxy/transparent/ProxyAdmin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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");
Expand All @@ -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);
}

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

Expand All @@ -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 {
Expand Down
103 changes: 78 additions & 25 deletions contracts/proxy/transparent/TransparentUpgradeableProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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 {
/**
Expand All @@ -37,6 +62,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() {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
if (msg.sender == _getAdmin()) {
Expand All @@ -46,65 +74,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 "";
}

/**
Expand All @@ -114,14 +175,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 {
frangio marked this conversation as resolved.
Show resolved Hide resolved
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.
Expand Down
4 changes: 3 additions & 1 deletion test/proxy/transparent/ProxyAdmin.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 () {
Expand Down
Loading