Skip to content

Commit c01ea99

Browse files
Amxxfrangioernestognw
committed
Fix TransparentUpgradeableProxy's transparency (#4154)
Co-authored-by: Francisco <[email protected]> Co-authored-by: Ernesto García <[email protected]> (cherry picked from commit 5523c14)
1 parent 8dfeb5d commit c01ea99

File tree

8 files changed

+132
-59
lines changed

8 files changed

+132
-59
lines changed

.changeset/thirty-shrimps-mix.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'openzeppelin-solidity': patch
3+
---
4+
5+
`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata.

contracts/interfaces/IERC1967.sol

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// SPDX-License-Identifier: MIT
2+
3+
pragma solidity ^0.8.0;
4+
5+
/**
6+
* @dev ERC-1967: Proxy Storage Slots. This interface contains the events defined in the ERC.
7+
*
8+
* _Available since v4.9._
9+
*/
10+
interface IERC1967 {
11+
/**
12+
* @dev Emitted when the implementation is upgraded.
13+
*/
14+
event Upgraded(address indexed implementation);
15+
16+
/**
17+
* @dev Emitted when the admin account has changed.
18+
*/
19+
event AdminChanged(address previousAdmin, address newAdmin);
20+
21+
/**
22+
* @dev Emitted when the beacon is changed.
23+
*/
24+
event BeaconUpgraded(address indexed beacon);
25+
}

contracts/proxy/ERC1967/ERC1967Upgrade.sol

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
pragma solidity ^0.8.2;
55

66
import "../beacon/IBeacon.sol";
7+
import "../../interfaces/IERC1967.sol";
78
import "../../interfaces/draft-IERC1822.sol";
89
import "../../utils/Address.sol";
910
import "../../utils/StorageSlot.sol";
@@ -16,7 +17,7 @@ import "../../utils/StorageSlot.sol";
1617
*
1718
* @custom:oz-upgrades-unsafe-allow delegatecall
1819
*/
19-
abstract contract ERC1967Upgrade {
20+
abstract contract ERC1967Upgrade is IERC1967 {
2021
// This is the keccak-256 hash of "eip1967.proxy.rollback" subtracted by 1
2122
bytes32 private constant _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143;
2223

@@ -27,11 +28,6 @@ abstract contract ERC1967Upgrade {
2728
*/
2829
bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
2930

30-
/**
31-
* @dev Emitted when the implementation is upgraded.
32-
*/
33-
event Upgraded(address indexed implementation);
34-
3531
/**
3632
* @dev Returns the current implementation address.
3733
*/
@@ -105,11 +101,6 @@ abstract contract ERC1967Upgrade {
105101
*/
106102
bytes32 internal constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
107103

108-
/**
109-
* @dev Emitted when the admin account has changed.
110-
*/
111-
event AdminChanged(address previousAdmin, address newAdmin);
112-
113104
/**
114105
* @dev Returns the current admin.
115106
*/
@@ -141,11 +132,6 @@ abstract contract ERC1967Upgrade {
141132
*/
142133
bytes32 internal constant _BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50;
143134

144-
/**
145-
* @dev Emitted when the beacon is upgraded.
146-
*/
147-
event BeaconUpgraded(address indexed beacon);
148-
149135
/**
150136
* @dev Returns the current beacon.
151137
*/

contracts/proxy/transparent/ProxyAdmin.sol

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ contract ProxyAdmin is Ownable {
1818
*
1919
* - This contract must be the admin of `proxy`.
2020
*/
21-
function getProxyImplementation(TransparentUpgradeableProxy proxy) public view virtual returns (address) {
21+
function getProxyImplementation(ITransparentUpgradeableProxy proxy) public view virtual returns (address) {
2222
// We need to manually run the static call since the getter cannot be flagged as view
2323
// bytes4(keccak256("implementation()")) == 0x5c60da1b
2424
(bool success, bytes memory returndata) = address(proxy).staticcall(hex"5c60da1b");
@@ -33,7 +33,7 @@ contract ProxyAdmin is Ownable {
3333
*
3434
* - This contract must be the admin of `proxy`.
3535
*/
36-
function getProxyAdmin(TransparentUpgradeableProxy proxy) public view virtual returns (address) {
36+
function getProxyAdmin(ITransparentUpgradeableProxy proxy) public view virtual returns (address) {
3737
// We need to manually run the static call since the getter cannot be flagged as view
3838
// bytes4(keccak256("admin()")) == 0xf851a440
3939
(bool success, bytes memory returndata) = address(proxy).staticcall(hex"f851a440");
@@ -48,7 +48,7 @@ contract ProxyAdmin is Ownable {
4848
*
4949
* - This contract must be the current admin of `proxy`.
5050
*/
51-
function changeProxyAdmin(TransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner {
51+
function changeProxyAdmin(ITransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner {
5252
proxy.changeAdmin(newAdmin);
5353
}
5454

@@ -59,7 +59,7 @@ contract ProxyAdmin is Ownable {
5959
*
6060
* - This contract must be the admin of `proxy`.
6161
*/
62-
function upgrade(TransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner {
62+
function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner {
6363
proxy.upgradeTo(implementation);
6464
}
6565

@@ -72,7 +72,7 @@ contract ProxyAdmin is Ownable {
7272
* - This contract must be the admin of `proxy`.
7373
*/
7474
function upgradeAndCall(
75-
TransparentUpgradeableProxy proxy,
75+
ITransparentUpgradeableProxy proxy,
7676
address implementation,
7777
bytes memory data
7878
) public payable virtual onlyOwner {

contracts/proxy/transparent/TransparentUpgradeableProxy.sol

Lines changed: 78 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,24 @@ pragma solidity ^0.8.0;
55

66
import "../ERC1967/ERC1967Proxy.sol";
77

8+
/**
9+
* @dev Interface for the {TransparentUpgradeableProxy}. This is useful because {TransparentUpgradeableProxy} uses a
10+
* custom call-routing mechanism, the compiler is unaware of the functions being exposed, and cannot list them. Also
11+
* {TransparentUpgradeableProxy} does not inherit from this interface because it's implemented in a way that the
12+
* compiler doesn't understand and cannot verify.
13+
*/
14+
interface ITransparentUpgradeableProxy is IERC1967 {
15+
function admin() external view returns (address);
16+
17+
function implementation() external view returns (address);
18+
19+
function changeAdmin(address) external;
20+
21+
function upgradeTo(address) external;
22+
23+
function upgradeToAndCall(address, bytes memory) external payable;
24+
}
25+
826
/**
927
* @dev This contract implements a proxy that is upgradeable by an admin.
1028
*
@@ -25,6 +43,13 @@ import "../ERC1967/ERC1967Proxy.sol";
2543
*
2644
* Our recommendation is for the dedicated account to be an instance of the {ProxyAdmin} contract. If set up this way,
2745
* you should think of the `ProxyAdmin` instance as the real administrative interface of your proxy.
46+
*
47+
* WARNING: This contract does not inherit from {ITransparentUpgradeableProxy}, and the admin function is implicitly
48+
* implemented using a custom call-routing mechanism in `_fallback`. Consequently, the compiler will not produce an
49+
* ABI for this contract. Also, if you inherit from this contract and add additional functions, the compiler will not
50+
* check that there are no selector conflicts. A selector clash between any new function and the functions declared in
51+
* {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could render the admin operations
52+
* inaccessible, which could prevent upgradeability.
2853
*/
2954
contract TransparentUpgradeableProxy is ERC1967Proxy {
3055
/**
@@ -41,6 +66,9 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
4166

4267
/**
4368
* @dev Modifier used internally that will delegate the call to the implementation unless the sender is the admin.
69+
*
70+
* CAUTION: This modifier is deprecated, as it could cause issues if the modified function has arguments, and the
71+
* implementation provides a function with the same selector.
4472
*/
4573
modifier ifAdmin() {
4674
if (msg.sender == _getAdmin()) {
@@ -50,65 +78,98 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
5078
}
5179
}
5280

81+
/**
82+
* @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior
83+
*/
84+
function _fallback() internal virtual override {
85+
if (msg.sender == _getAdmin()) {
86+
bytes memory ret;
87+
bytes4 selector = msg.sig;
88+
if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) {
89+
ret = _dispatchUpgradeTo();
90+
} else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
91+
ret = _dispatchUpgradeToAndCall();
92+
} else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) {
93+
ret = _dispatchChangeAdmin();
94+
} else if (selector == ITransparentUpgradeableProxy.admin.selector) {
95+
ret = _dispatchAdmin();
96+
} else if (selector == ITransparentUpgradeableProxy.implementation.selector) {
97+
ret = _dispatchImplementation();
98+
} else {
99+
revert("TransparentUpgradeableProxy: admin cannot fallback to proxy target");
100+
}
101+
assembly {
102+
return(add(ret, 0x20), mload(ret))
103+
}
104+
} else {
105+
super._fallback();
106+
}
107+
}
108+
53109
/**
54110
* @dev Returns the current admin.
55111
*
56-
* NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyAdmin}.
57-
*
58112
* TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
59113
* https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
60114
* `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103`
61115
*/
62-
function admin() external payable ifAdmin returns (address admin_) {
116+
function _dispatchAdmin() private returns (bytes memory) {
63117
_requireZeroValue();
64-
admin_ = _getAdmin();
118+
119+
address admin = _getAdmin();
120+
return abi.encode(admin);
65121
}
66122

67123
/**
68124
* @dev Returns the current implementation.
69125
*
70-
* NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyImplementation}.
71-
*
72126
* TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
73127
* https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
74128
* `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc`
75129
*/
76-
function implementation() external payable ifAdmin returns (address implementation_) {
130+
function _dispatchImplementation() private returns (bytes memory) {
77131
_requireZeroValue();
78-
implementation_ = _implementation();
132+
133+
address implementation = _implementation();
134+
return abi.encode(implementation);
79135
}
80136

81137
/**
82138
* @dev Changes the admin of the proxy.
83139
*
84140
* Emits an {AdminChanged} event.
85-
*
86-
* NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}.
87141
*/
88-
function changeAdmin(address newAdmin) external payable virtual ifAdmin {
142+
function _dispatchChangeAdmin() private returns (bytes memory) {
89143
_requireZeroValue();
144+
145+
address newAdmin = abi.decode(msg.data[4:], (address));
90146
_changeAdmin(newAdmin);
147+
148+
return "";
91149
}
92150

93151
/**
94152
* @dev Upgrade the implementation of the proxy.
95-
*
96-
* NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}.
97153
*/
98-
function upgradeTo(address newImplementation) external payable ifAdmin {
154+
function _dispatchUpgradeTo() private returns (bytes memory) {
99155
_requireZeroValue();
156+
157+
address newImplementation = abi.decode(msg.data[4:], (address));
100158
_upgradeToAndCall(newImplementation, bytes(""), false);
159+
160+
return "";
101161
}
102162

103163
/**
104164
* @dev Upgrade the implementation of the proxy, and then call a function from the new implementation as specified
105165
* by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the
106166
* proxied contract.
107-
*
108-
* NOTE: Only the admin can call this function. See {ProxyAdmin-upgradeAndCall}.
109167
*/
110-
function upgradeToAndCall(address newImplementation, bytes calldata data) external payable ifAdmin {
168+
function _dispatchUpgradeToAndCall() private returns (bytes memory) {
169+
(address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes));
111170
_upgradeToAndCall(newImplementation, data, true);
171+
172+
return "";
112173
}
113174

114175
/**
@@ -118,14 +179,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
118179
return _getAdmin();
119180
}
120181

121-
/**
122-
* @dev Makes sure the admin cannot access the fallback function. See {Proxy-_beforeFallback}.
123-
*/
124-
function _beforeFallback() internal virtual override {
125-
require(msg.sender != _getAdmin(), "TransparentUpgradeableProxy: admin cannot fallback to proxy target");
126-
super._beforeFallback();
127-
}
128-
129182
/**
130183
* @dev To keep this contract fully transparent, all `ifAdmin` functions must be payable. This helper is here to
131184
* emulate some proxy functions being non-payable while still allowing value to pass through.

test/proxy/transparent/ProxyAdmin.test.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const ImplV1 = artifacts.require('DummyImplementation');
66
const ImplV2 = artifacts.require('DummyImplementationV2');
77
const ProxyAdmin = artifacts.require('ProxyAdmin');
88
const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy');
9+
const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy');
910

1011
contract('ProxyAdmin', function (accounts) {
1112
const [proxyAdminOwner, newAdmin, anotherAccount] = accounts;
@@ -18,12 +19,13 @@ contract('ProxyAdmin', function (accounts) {
1819
beforeEach(async function () {
1920
const initializeData = Buffer.from('');
2021
this.proxyAdmin = await ProxyAdmin.new({ from: proxyAdminOwner });
21-
this.proxy = await TransparentUpgradeableProxy.new(
22+
const proxy = await TransparentUpgradeableProxy.new(
2223
this.implementationV1.address,
2324
this.proxyAdmin.address,
2425
initializeData,
2526
{ from: proxyAdminOwner },
2627
);
28+
this.proxy = await ITransparentUpgradeableProxy.at(proxy.address);
2729
});
2830

2931
it('has an owner', async function () {

0 commit comments

Comments
 (0)