From 072cb0b769b9938c62395131cba6f1a3113aa78f Mon Sep 17 00:00:00 2001 From: webthethird Date: Wed, 29 Mar 2023 16:40:51 -0500 Subject: [PATCH 1/4] Find tainted functions/variables from external calls Given a list of functions from one contract, finds tainted functions/variables in other contracts --- slither/utils/upgradeability.py | 91 ++++++++++++++++++++++++++++++--- 1 file changed, 85 insertions(+), 6 deletions(-) diff --git a/slither/utils/upgradeability.py b/slither/utils/upgradeability.py index 7b4e8493a..947c4652a 100644 --- a/slither/utils/upgradeability.py +++ b/slither/utils/upgradeability.py @@ -1,4 +1,4 @@ -from typing import Optional, Tuple, List, Union +from typing import Optional, Tuple, List, Union, TypedDict from slither.core.declarations import ( Contract, Structure, @@ -19,10 +19,12 @@ from slither.core.variables.local_variable_init_from_tuple import LocalVariableI from slither.core.variables.state_variable import StateVariable from slither.analyses.data_dependency.data_dependency import get_dependencies from slither.core.variables.variable import Variable -from slither.core.expressions.literal import Literal -from slither.core.expressions.identifier import Identifier -from slither.core.expressions.call_expression import CallExpression -from slither.core.expressions.assignment_operation import AssignmentOperation +from slither.core.expressions import ( + Literal, + Identifier, + CallExpression, + AssignmentOperation, +) from slither.core.cfg.node import Node, NodeType from slither.slithir.operations import ( Operation, @@ -61,11 +63,23 @@ from slither.slithir.variables import ( from slither.tools.read_storage.read_storage import SlotInfo, SlitherReadStorage +class TaintedExternalContract(TypedDict): + contract: Contract + functions: List[Function] + variables: List[Variable] + + # pylint: disable=too-many-locals def compare( v1: Contract, v2: Contract ) -> Tuple[ - List[Variable], List[Variable], List[Variable], List[Function], List[Function], List[Function] + List[Variable], + List[Variable], + List[Variable], + List[Function], + List[Function], + List[Function], + List[TaintedExternalContract], ]: """ Compares two versions of a contract. Most useful for upgradeable (logic) contracts, @@ -159,6 +173,11 @@ def compare( ): tainted_variables.append(var) + # Find all external contracts and functions called by new/modified/tainted functions + tainted_contracts = tainted_external_contracts( + new_functions + modified_functions + tainted_functions + ) + return ( missing_vars_in_v2, new_variables, @@ -166,9 +185,69 @@ def compare( new_functions, modified_functions, tainted_functions, + tainted_contracts, ) +def tainted_external_contracts(funcs: List[Function]) -> List[TaintedExternalContract]: + """ + Takes a list of functions from one contract, finds any calls in these to functions in external contracts, + and determines which variables and functions in the external contracts are tainted by these external calls. + Args: + funcs: a list of Function objects to search for external calls. + + Returns: + TaintedExternalContract(TypedDict) ( + contract: Contract, + functions: List[Function], + variables: List[Variable] + ) + """ + tainted_contracts = {} + + for func in funcs: + for contract, target in func.all_high_level_calls(): + if contract.name not in tainted_contracts: + tainted_contracts[contract.name] = TaintedExternalContract( + contract=contract, functions=[], variables=[] + ) + if ( + isinstance(target, Function) + and target not in funcs + and target not in tainted_contracts[contract.name]["functions"] + and not (target.is_constructor or target.is_fallback or target.is_receive) + ): + tainted_contracts[contract.name]["functions"].append(target) + for var in target.all_state_variables_written(): + if var not in tainted_contracts[contract.name]["variables"]: + tainted_contracts[contract.name]["variables"].append(var) + elif ( + isinstance(target, Variable) + and target not in tainted_contracts[contract.name]["variables"] + and not (target.is_constant or target.is_immutable) + ): + tainted_contracts[contract.name]["variables"].append(target) + tainted_contracts = { + item + for item in tainted_contracts.items() + if len(item[1]["variables"]) > 0 and len(item[1]["functions"]) > 0 + } + for c in tainted_contracts.items(): + contract = c[1]["contract"] + variables = c[1]["variables"] + for var in variables: + read_write = set( + contract.get_functions_reading_from_variable(var) + + contract.get_functions_writing_to_variable(var) + ) + for f in read_write: + if f not in tainted_contracts[contract.name]["functions"] and not ( + f.is_constructor or f.is_fallback or f.is_receive + ): + tainted_contracts[contract.name]["functions"].append(f) + return list(tainted_contracts.values()) + + def get_missing_vars(v1: Contract, v2: Contract) -> List[StateVariable]: """ Gets all non-constant/immutable StateVariables that appear in v1 but not v2 From c1e3743c041ca23263c218a8b25bb9430f1b1998 Mon Sep 17 00:00:00 2001 From: webthethird Date: Fri, 31 Mar 2023 14:19:05 -0500 Subject: [PATCH 2/4] Undo changes from new branch re: cross-contract taint --- slither/utils/upgradeability.py | 66 --------------------------------- 1 file changed, 66 deletions(-) diff --git a/slither/utils/upgradeability.py b/slither/utils/upgradeability.py index 947c4652a..04fd31f54 100644 --- a/slither/utils/upgradeability.py +++ b/slither/utils/upgradeability.py @@ -79,7 +79,6 @@ def compare( List[Function], List[Function], List[Function], - List[TaintedExternalContract], ]: """ Compares two versions of a contract. Most useful for upgradeable (logic) contracts, @@ -173,11 +172,6 @@ def compare( ): tainted_variables.append(var) - # Find all external contracts and functions called by new/modified/tainted functions - tainted_contracts = tainted_external_contracts( - new_functions + modified_functions + tainted_functions - ) - return ( missing_vars_in_v2, new_variables, @@ -185,69 +179,9 @@ def compare( new_functions, modified_functions, tainted_functions, - tainted_contracts, ) -def tainted_external_contracts(funcs: List[Function]) -> List[TaintedExternalContract]: - """ - Takes a list of functions from one contract, finds any calls in these to functions in external contracts, - and determines which variables and functions in the external contracts are tainted by these external calls. - Args: - funcs: a list of Function objects to search for external calls. - - Returns: - TaintedExternalContract(TypedDict) ( - contract: Contract, - functions: List[Function], - variables: List[Variable] - ) - """ - tainted_contracts = {} - - for func in funcs: - for contract, target in func.all_high_level_calls(): - if contract.name not in tainted_contracts: - tainted_contracts[contract.name] = TaintedExternalContract( - contract=contract, functions=[], variables=[] - ) - if ( - isinstance(target, Function) - and target not in funcs - and target not in tainted_contracts[contract.name]["functions"] - and not (target.is_constructor or target.is_fallback or target.is_receive) - ): - tainted_contracts[contract.name]["functions"].append(target) - for var in target.all_state_variables_written(): - if var not in tainted_contracts[contract.name]["variables"]: - tainted_contracts[contract.name]["variables"].append(var) - elif ( - isinstance(target, Variable) - and target not in tainted_contracts[contract.name]["variables"] - and not (target.is_constant or target.is_immutable) - ): - tainted_contracts[contract.name]["variables"].append(target) - tainted_contracts = { - item - for item in tainted_contracts.items() - if len(item[1]["variables"]) > 0 and len(item[1]["functions"]) > 0 - } - for c in tainted_contracts.items(): - contract = c[1]["contract"] - variables = c[1]["variables"] - for var in variables: - read_write = set( - contract.get_functions_reading_from_variable(var) - + contract.get_functions_writing_to_variable(var) - ) - for f in read_write: - if f not in tainted_contracts[contract.name]["functions"] and not ( - f.is_constructor or f.is_fallback or f.is_receive - ): - tainted_contracts[contract.name]["functions"].append(f) - return list(tainted_contracts.values()) - - def get_missing_vars(v1: Contract, v2: Contract) -> List[StateVariable]: """ Gets all non-constant/immutable StateVariables that appear in v1 but not v2 From 4b07282c7276a2ab77997a6a22b65e814776f057 Mon Sep 17 00:00:00 2001 From: webthethird Date: Fri, 31 Mar 2023 14:28:42 -0500 Subject: [PATCH 3/4] Add failing test of `get_implementation_var` due to get_dependencies missing dependencies --- .../TestUpgrades-0.8.2.sol | 1 + .../src/TransparentUpgradeableProxy.sol | 224 ++++++++++++++++++ tests/unit/utils/test_upgradeability_util.py | 6 + 3 files changed, 231 insertions(+) create mode 100644 tests/unit/utils/test_data/upgradeability_util/src/TransparentUpgradeableProxy.sol diff --git a/tests/unit/utils/test_data/upgradeability_util/TestUpgrades-0.8.2.sol b/tests/unit/utils/test_data/upgradeability_util/TestUpgrades-0.8.2.sol index d3371d3c6..676dd892a 100644 --- a/tests/unit/utils/test_data/upgradeability_util/TestUpgrades-0.8.2.sol +++ b/tests/unit/utils/test_data/upgradeability_util/TestUpgrades-0.8.2.sol @@ -4,3 +4,4 @@ import "./src/ContractV1.sol"; import "./src/ContractV2.sol"; import "./src/InheritedStorageProxy.sol"; import "./src/ERC1967Proxy.sol"; +import "./src/TransparentUpgradeableProxy.sol"; diff --git a/tests/unit/utils/test_data/upgradeability_util/src/TransparentUpgradeableProxy.sol b/tests/unit/utils/test_data/upgradeability_util/src/TransparentUpgradeableProxy.sol new file mode 100644 index 000000000..a78d4f152 --- /dev/null +++ b/tests/unit/utils/test_data/upgradeability_util/src/TransparentUpgradeableProxy.sol @@ -0,0 +1,224 @@ +pragma solidity ^0.8.0; + +import "./Proxy.sol"; +import "./Address.sol"; + +/** + * @dev This contract implements an upgradeable proxy. It is upgradeable because calls are delegated to an + * implementation address that can be changed. This address is stored in storage in the location specified by + * https://eips.ethereum.org/EIPS/eip-1967[EIP1967], so that it doesn't conflict with the storage layout of the + * implementation behind the proxy. + * + * Upgradeability is only provided internally through {_upgradeTo}. For an externally upgradeable proxy see + * {TransparentUpgradeableProxy}. + */ +contract UpgradeableProxy is Proxy { + /** + * @dev Initializes the upgradeable proxy with an initial implementation specified by `_logic`. + * + * If `_data` is nonempty, it's used as data in a delegate call to `_logic`. This will typically be an encoded + * function call, and allows initializating the storage of the proxy like a Solidity constructor. + */ + constructor(address _logic, bytes memory _data) payable { + assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1)); + _setImplementation(_logic); + if(_data.length > 0) { + Address.functionDelegateCall(_logic, _data); + } + } + + /** + * @dev Emitted when the implementation is upgraded. + */ + event Upgraded(address indexed implementation); + + /** + * @dev Storage slot with the address of the current implementation. + * This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1, and is + * validated in the constructor. + */ + bytes32 private constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; + + /** + * @dev Returns the current implementation address. + */ + function _implementation() internal view virtual override returns (address impl) { + bytes32 slot = _IMPLEMENTATION_SLOT; + // solhint-disable-next-line no-inline-assembly + assembly { + impl := sload(slot) + } + } + + /** + * @dev Upgrades the proxy to a new implementation. + * + * Emits an {Upgraded} event. + */ + function _upgradeTo(address newImplementation) internal virtual { + _setImplementation(newImplementation); + emit Upgraded(newImplementation); + } + + /** + * @dev Stores a new address in the EIP1967 implementation slot. + */ + function _setImplementation(address newImplementation) private { + require(Address.isContract(newImplementation), "UpgradeableProxy: new implementation is not a contract"); + + bytes32 slot = _IMPLEMENTATION_SLOT; + + // solhint-disable-next-line no-inline-assembly + assembly { + sstore(slot, newImplementation) + } + } +} + + + +/** + * @dev This contract implements a proxy that is upgradeable by an admin. + * + * To avoid https://medium.com/nomic-labs-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357[proxy selector + * clashing], which can potentially be used in an attack, this contract uses the + * https://blog.openzeppelin.com/the-transparent-proxy-pattern/[transparent proxy pattern]. This pattern implies two + * things that go hand in hand: + * + * 1. If any account other than the admin calls the proxy, the call will be forwarded to the implementation, even if + * that call matches one of the admin functions exposed by the proxy itself. + * 2. If the admin calls the proxy, it can access the admin functions, but its calls will never be forwarded to the + * implementation. If the admin tries to call a function on the implementation it will fail with an error that says + * "admin cannot fallback to proxy target". + * + * These properties mean that the admin account can only be used for admin actions like upgrading the proxy or changing + * the admin, so it's best if it's a dedicated account that is not used for anything else. This will avoid headaches due + * to sudden errors when trying to call a function from the proxy implementation. + * + * 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. + */ +contract TransparentUpgradeableProxy is UpgradeableProxy { + /** + * @dev Initializes an upgradeable proxy managed by `_admin`, backed by the implementation at `_logic`, and + * optionally initialized with `_data` as explained in {UpgradeableProxy-constructor}. + */ + constructor(address _logic, address admin_, bytes memory _data) payable UpgradeableProxy(_logic, _data) { + assert(_ADMIN_SLOT == bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1)); + _setAdmin(admin_); + } + + /** + * @dev Emitted when the admin account has changed. + */ + event AdminChanged(address previousAdmin, address newAdmin); + + /** + * @dev Storage slot with the admin of the contract. + * This is the keccak-256 hash of "eip1967.proxy.admin" subtracted by 1, and is + * validated in the constructor. + */ + bytes32 private constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; + + /** + * @dev Modifier used internally that will delegate the call to the implementation unless the sender is the admin. + */ + modifier ifAdmin() { + if (msg.sender == _admin()) { + _; + } else { + _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 ifAdmin returns (address admin_) { + admin_ = _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 ifAdmin returns (address implementation_) { + implementation_ = _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 virtual ifAdmin { + require(newAdmin != address(0), "TransparentUpgradeableProxy: new admin is the zero address"); + emit AdminChanged(_admin(), newAdmin); + _setAdmin(newAdmin); + } + + /** + * @dev Upgrade the implementation of the proxy. + * + * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}. + */ + function upgradeTo(address newImplementation) external virtual ifAdmin { + _upgradeTo(newImplementation); + } + + /** + * @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 virtual ifAdmin { + _upgradeTo(newImplementation); + Address.functionDelegateCall(newImplementation, data); + } + + /** + * @dev Returns the current admin. + */ + function _admin() internal view virtual returns (address adm) { + bytes32 slot = _ADMIN_SLOT; + // solhint-disable-next-line no-inline-assembly + assembly { + adm := sload(slot) + } + } + + /** + * @dev Stores a new address in the EIP1967 admin slot. + */ + function _setAdmin(address newAdmin) private { + bytes32 slot = _ADMIN_SLOT; + + // solhint-disable-next-line no-inline-assembly + assembly { + sstore(slot, newAdmin) + } + } + + /** + * @dev Makes sure the admin cannot access the fallback function. See {Proxy-_beforeFallback}. + */ + function _beforeFallback() internal virtual override { + require(msg.sender != _admin(), "TransparentUpgradeableProxy: admin cannot fallback to proxy target"); + super._beforeFallback(); + } +} diff --git a/tests/unit/utils/test_upgradeability_util.py b/tests/unit/utils/test_upgradeability_util.py index 7d6fb82da..f119c2ba3 100644 --- a/tests/unit/utils/test_upgradeability_util.py +++ b/tests/unit/utils/test_upgradeability_util.py @@ -43,6 +43,8 @@ def test_upgrades_implementation_var() -> None: erc_1967_proxy = sl.get_contract_from_name("ERC1967Proxy")[0] storage_proxy = sl.get_contract_from_name("InheritedStorageProxy")[0] + upgradeable_proxy = sl.get_contract_from_name("UpgradeableProxy")[0] + transparent_proxy = sl.get_contract_from_name("TransparentUpgradeableProxy")[0] target = get_proxy_implementation_var(erc_1967_proxy) slot = get_proxy_implementation_slot(erc_1967_proxy) @@ -52,6 +54,10 @@ def test_upgrades_implementation_var() -> None: slot = get_proxy_implementation_slot(storage_proxy) assert target == storage_proxy.get_state_variable_from_name("implementation") assert slot.slot == 1 + target = get_proxy_implementation_var(transparent_proxy) + slot = get_proxy_implementation_slot(transparent_proxy) + assert target == upgradeable_proxy.get_state_variable_from_name("_IMPLEMENTATION_SLOT") + assert slot.slot == 0x360894A13BA1A3210667C828492DB98DCA3E2076CC3735A920A3CA505D382BBC solc_select.switch_global_version("0.5.0", always_install=True) sl = Slither(os.path.join(TEST_DATA_DIR, "TestUpgrades-0.5.0.sol")) From 1e40d19b5d89b9d3b325e18bc28b2e6cfe206244 Mon Sep 17 00:00:00 2001 From: webthethird Date: Fri, 31 Mar 2023 14:39:57 -0500 Subject: [PATCH 4/4] Add TODO --- slither/utils/upgradeability.py | 1 + 1 file changed, 1 insertion(+) diff --git a/slither/utils/upgradeability.py b/slither/utils/upgradeability.py index 04fd31f54..7633e87a1 100644 --- a/slither/utils/upgradeability.py +++ b/slither/utils/upgradeability.py @@ -411,6 +411,7 @@ def get_proxy_implementation_var(proxy: Contract) -> Optional[Variable]: try: delegate = next(var for var in dependencies if isinstance(var, StateVariable)) except StopIteration: + # TODO: Handle case where get_dependencies does not return any state variables. return delegate return delegate