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

Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions contracts/proxy/README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ The current implementation of this security mechanism uses https://eips.ethereum

== ERC1967

{{IERC1967}}

{{ERC1967Proxy}}

{{ERC1967Upgrade}}
Expand Down
101 changes: 43 additions & 58 deletions contracts/proxy/transparent/TransparentUpgradeableProxy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ 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.
* @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy}
* does not implement this interface directly, and some of its functions are implemented by functions without arguments
* that match in function selector. The compiler is unaware that these functions are implemented by
* {TransparentUpgradeableProxy} and will not include them in the ABI so this interface must be used to interact with it.
*/
interface ITransparentUpgradeableProxy is IERC1967 {
function admin() external view returns (address);
Expand All @@ -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.
* NOTE: The real interface of this proxy is that defined in `ITransparentUpgradeableProxy`. This contract does not
* inherit from that interface, and instead you will see some admin functions are implemented by oddly named functions.
* These functions are designed to match the selectors of `ITransparentUpgradeableProxy`, but without any arguments.
* This is necessary to fully implement transparency without decoding reverts caused by selector clashes between the
* proxy and the implementation.
*/
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,77 @@ 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 must be invoked as `changeAdmin(address newAdmin)`, which has the same function selector.
*
* 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 must be invoked as `upgradeTo(address newImplementation)`, which has the same function selector.
*
* 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 must be invoked as `upgradeTo(address newImplementation, bytes data)`, which has the same function
* selector.
*
* 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 +152,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