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.

Sign up for GitHub

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

Jump to bottom

Alternative fix for the TransparentUpgradeableProxy "decoding" bug #4168

Closed
wants to merge 7 commits into from
Closed

Alternative fix for the TransparentUpgradeableProxy "decoding" bug #4168

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0047674
revert changs to TransparentUpgradeableProxy
Amxx Apr 6, 2023
9679865
Different fix for the TransparentUpgradeableProxy decoding bug.
Amxx Apr 6, 2023
51615f4
lint
frangio Apr 6, 2023
9c5de8e
udpate docs
frangio Apr 6, 2023
13a46bc
update docs
frangio Apr 6, 2023
c61b7cd
update docs
frangio Apr 6, 2023
fae9f5a
Update TransparentUpgradeableProxy.sol
frangio Apr 7, 2023
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
95 changes: 41 additions & 54 deletions contracts/proxy/transparent/TransparentUpgradeableProxy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ interface ITransparentUpgradeableProxy is IERC1967 {
function upgradeToAndCall(address, bytes memory) external payable;
}

// solhint-disable func-name-mixedcase

/**
* @dev This contract implements a proxy that is upgradeable by an admin.
*
Expand All @@ -44,12 +46,11 @@ interface ITransparentUpgradeableProxy is IERC1967 {
* 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.
* WARNING: This contract does not inherit from {ITransparentUpgradeableProxy}, and the admin functions are
* implemented by functions that have signature designed to match the selector of the {ITransparentUpgradeableProxy}
* interface, but without any arguments. This allows use to decode the argument manually after the `ifAdmin` modifier
* has had a change to forward the call. This is done so that the argument decoding does not fail here in case the
* proxy and the implementation have "selector clash".
frangio marked this conversation as resolved.
Show resolved Hide resolved
*/
contract TransparentUpgradeableProxy is ERC1967Proxy {
/**
Expand All @@ -62,9 +63,6 @@ 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()) {
Expand All @@ -74,98 +72,79 @@ 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 _dispatchAdmin() private returns (bytes memory) {
function admin() external payable ifAdmin returns (address admin_) {
_requireZeroValue();

address admin = _getAdmin();
return abi.encode(admin);
admin_ = _getAdmin();
}

/**
* @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 _dispatchImplementation() private returns (bytes memory) {
function implementation() external payable ifAdmin returns (address implementation_) {
_requireZeroValue();

address implementation = _implementation();
return abi.encode(implementation);
implementation_ = _implementation();
}

/**
* @dev Changes the admin of the proxy.
*
* This function's name is designed to have the same selector as {ITransparentUpgradeableProxy-changeAdmin}. This
* is so the argument decoding is done manually after the ifAdmin modifier has had a change to proxy the call.
*
* Emits an {AdminChanged} event.
*
* NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}.
*/
function _dispatchChangeAdmin() private returns (bytes memory) {
function changeAdmin_277BB5030() external payable virtual ifAdmin {
_requireZeroValue();

address newAdmin = abi.decode(msg.data[4:], (address));
_changeAdmin(newAdmin);

return "";
}

/**
* @dev Upgrade the implementation of the proxy.
*
* This function's name is designed to have the same selector as {ITransparentUpgradeableProxy-upgradeTo}. This
* is so the argument decoding is done manually after the ifAdmin modifier has had a change to proxy the call.
*
* NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}.
*/
function _dispatchUpgradeTo() private returns (bytes memory) {
function upgradeTo_790AA3D() external payable ifAdmin {
_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.
*
* This function's name is designed to have the same selector as {ITransparentUpgradeableProxy-upgradeToAndCall}.
* This is so the argument decoding is done manually after the ifAdmin modifier has had a change to proxy the call.
*
* NOTE: Only the admin can call this function. See {ProxyAdmin-upgradeAndCall}.
*/
function _dispatchUpgradeToAndCall() private returns (bytes memory) {
function upgradeToAndCall_23573451() external payable ifAdmin {
(address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes));
_upgradeToAndCall(newImplementation, data, true);

return "";
}

/**
Expand All @@ -175,6 +154,14 @@ 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();
}
ernestognw marked this conversation as resolved.
Show resolved Hide resolved

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