diff --git a/README.md b/README.md index 12d6d59ff..7e7f6487a 100644 --- a/README.md +++ b/README.md @@ -44,27 +44,30 @@ Num | Detector | What it Detects | Impact | Confidence 5 | `arbitrary-send` | [Functions that send ether to arbitrary destinations](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations) | High | Medium 6 | `controlled-delegatecall` | [Controlled delegatecall destination](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#controlled-delegatecall) | High | Medium 7 | `reentrancy-eth` | [Reentrancy vulnerabilities (theft of ethers)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities) | High | Medium -8 | `incorrect-equality` | [Dangerous strict equalities](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-strict-equalities) | Medium | High -9 | `locked-ether` | [Contracts that lock ether](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#contracts-that-lock-ether) | Medium | High -10 | `shadowing-abstract` | [State variables shadowing from abstract contracts](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variable-shadowing-from-abstract-contracts) | Medium | High -11 | `constant-function` | [Constant functions changing the state](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#constant-functions-changing-the-state) | Medium | Medium -12 | `reentrancy-no-eth` | [Reentrancy vulnerabilities (no theft of ethers)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities-1) | Medium | Medium -13 | `tx-origin` | [Dangerous usage of `tx.origin`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-usage-of-txorigin) | Medium | Medium -14 | `uninitialized-local` | [Uninitialized local variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-local-variables) | Medium | Medium -15 | `unused-return` | [Unused return values](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return) | Medium | Medium -16 | `shadowing-builtin` | [Built-in symbol shadowing](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#builtin-symbol-shadowing) | Low | High -17 | `shadowing-local` | [Local variables shadowing](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#local-variable-shadowing) | Low | High -18 | `calls-loop` | [Multiple calls in a loop](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description/_edit#calls-inside-a-loop) | Low | Medium -19 | `reentrancy-benign` | [Benign reentrancy vulnerabilities](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities-2) | Low | Medium -20 | `timestamp` | [Dangerous usage of `block.timestamp`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#block-timestamp) | Low | Medium -21 | `assembly` | [Assembly usage](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage) | Informational | High -22 | `constable-states` | [State variables that could be declared constant](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant) | Informational | High -23 | `external-function` | [Public function that could be declared as external](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#public-function-that-could-be-declared-as-external) | Informational | High -24 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | Informational | High -25 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | Informational | High -26 | `pragma` | [If different pragma directives are used](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#different-pragma-directives-are-used) | Informational | High -27 | `solc-version` | [Incorrect Solidity version (< 0.4.24 or complex pragma)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#incorrect-version-of-solidity) | Informational | High -28 | `unused-state` | [Unused state variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables) | Informational | High +8 | `erc20-interface` | [Incorrect ERC20 interfaces](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#incorrect-erc20-interface) | Medium | High +9 | `incorrect-equality` | [Dangerous strict equalities](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-strict-equalities) | Medium | High +10 | `locked-ether` | [Contracts that lock ether](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#contracts-that-lock-ether) | Medium | High +11 | `shadowing-abstract` | [State variables shadowing from abstract contracts](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variable-shadowing-from-abstract-contracts) | Medium | High +12 | `constant-function` | [Constant functions changing the state](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#constant-functions-changing-the-state) | Medium | Medium +13 | `reentrancy-no-eth` | [Reentrancy vulnerabilities (no theft of ethers)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities-1) | Medium | Medium +14 | `tx-origin` | [Dangerous usage of `tx.origin`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-usage-of-txorigin) | Medium | Medium +15 | `uninitialized-local` | [Uninitialized local variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-local-variables) | Medium | Medium +16 | `unused-return` | [Unused return values](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return) | Medium | Medium +17 | `shadowing-builtin` | [Built-in symbol shadowing](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#builtin-symbol-shadowing) | Low | High +18 | `shadowing-local` | [Local variables shadowing](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#local-variable-shadowing) | Low | High +19 | `calls-loop` | [Multiple calls in a loop](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description/_edit#calls-inside-a-loop) | Low | Medium +20 | `reentrancy-benign` | [Benign reentrancy vulnerabilities](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities-2) | Low | Medium +21 | `timestamp` | [Dangerous usage of `block.timestamp`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#block-timestamp) | Low | Medium +22 | `assembly` | [Assembly usage](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage) | Informational | High +23 | `constable-states` | [State variables that could be declared constant](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant) | Informational | High +24 | `deprecated-standards` | [Deprecated Solidity Standards](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#deprecated-standards) | Informational | High +25 | `erc20-indexed` | [Un-indexed ERC20 event parameters](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unindexed-erc20-event-parameters) | Informational | High +26 | `external-function` | [Public function that could be declared as external](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#public-function-that-could-be-declared-as-external) | Informational | High +27 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | Informational | High +28 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | Informational | High +29 | `pragma` | [If different pragma directives are used](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#different-pragma-directives-are-used) | Informational | High +30 | `solc-version` | [Incorrect Solidity version (< 0.4.24 or complex pragma)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#incorrect-version-of-solidity) | Informational | High +31 | `unused-state` | [Unused state variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables) | Informational | High [Contact us](https://www.trailofbits.com/contact/) to get access to additional detectors. @@ -83,10 +86,12 @@ Num | Printer | Description 7 | `human-summary` | [Print a human-readable summary of the contracts](https://github.com/trailofbits/slither/wiki/Printer-documentation#human-summary) 8 | `inheritance` | [Print the inheritance relations between contracts](https://github.com/trailofbits/slither/wiki/Printer-documentation#inheritance) 9 | `inheritance-graph` | [Export the inheritance graph of each contract to a dot file](https://github.com/trailofbits/slither/wiki/Printer-documentation#inheritance-graph) -10 | `slithir` | [Print the slithIR representation of the functions](https://github.com/trailofbits/slither/wiki/Printer-documentation#slithir) -11 | `slithir-ssa` | [Print the slithIR representation of the functions](https://github.com/trailofbits/slither/wiki/Printer-documentation#slithir-ssa) -12 | `variables-order` | [Print the storage order of the state variables](https://github.com/trailofbits/slither/wiki/Printer-documentation#variables-written-and-authorization) -13 | `vars-and-auth` | [Print the state variables written and the authorization of the functions](https://github.com/trailofbits/slither/wiki/Printer-documentation#variables-written-and-authorization) +10 | `modifiers` | [Print the modifiers called by each function](https://github.com/trailofbits/slither/wiki/Printer-documentation#modifiers) +11 | `require` | [Print the require and assert calls of each function](https://github.com/trailofbits/slither/wiki/Printer-documentation#require) +12 | `slithir` | [Print the slithIR representation of the functions](https://github.com/trailofbits/slither/wiki/Printer-documentation#slithir) +13 | `slithir-ssa` | [Print the slithIR representation of the functions](https://github.com/trailofbits/slither/wiki/Printer-documentation#slithir-ssa) +14 | `variables-order` | [Print the storage order of the state variables](https://github.com/trailofbits/slither/wiki/Printer-documentation#variables-written-and-authorization) +15 | `vars-and-auth` | [Print the state variables written and the authorization of the functions](https://github.com/trailofbits/slither/wiki/Printer-documentation#variables-written-and-authorization) ## How to install diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index d0109f21d..cc9f6fa8a 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -14,6 +14,10 @@ generate_expected_json(){ } + +#generate_expected_json tests/deprecated_calls.sol "deprecated-standards" +#generate_expected_json tests/erc20_indexed.sol "erc20-indexed" +#generate_expected_json tests/incorrect_erc20_interface.sol "erc20-interface" #generate_expected_json tests/uninitialized.sol "uninitialized-state" #generate_expected_json tests/backdoor.sol "backdoor" #generate_expected_json tests/backdoor.sol "suicidal" diff --git a/scripts/travis_test_4.sh b/scripts/travis_test_4.sh index 0312c2558..8313aa444 100755 --- a/scripts/travis_test_4.sh +++ b/scripts/travis_test_4.sh @@ -65,6 +65,9 @@ test_slither(){ } +test_slither tests/deprecated_calls.sol "deprecated-standards" +test_slither tests/erc20_indexed.sol "erc20-indexed" +test_slither tests/incorrect_erc20_interface.sol "erc20-interface" test_slither tests/uninitialized.sol "uninitialized-state" test_slither tests/backdoor.sol "backdoor" test_slither tests/backdoor.sol "suicidal" diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 8134d7059..8f1f5cb48 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -28,5 +28,8 @@ from .shadowing.builtin_symbols import BuiltinSymbolShadowing from .operations.block_timestamp import Timestamp from .statements.calls_in_loop import MultipleCallsInLoop from .statements.incorrect_strict_equality import IncorrectStrictEquality +from .erc20.incorrect_interface import IncorrectERC20InterfaceDetection +from .erc20.unindexed_event_parameters import UnindexedERC20EventParameters +from .statements.deprecated_calls import DeprecatedStandards # # diff --git a/slither/detectors/erc20/__init__.py b/slither/detectors/erc20/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/slither/detectors/erc20/incorrect_interface.py b/slither/detectors/erc20/incorrect_interface.py new file mode 100644 index 000000000..4a0161e57 --- /dev/null +++ b/slither/detectors/erc20/incorrect_interface.py @@ -0,0 +1,79 @@ +""" +Detect incorrect erc20 interface. +Some contracts do not return a bool on transfer/transferFrom/approve, which may lead to prevent +the contract to be used with contracts compiled with recent solc (>0.4.22) +""" +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification + + +class IncorrectERC20InterfaceDetection(AbstractDetector): + """ + Incorrect ERC20 Interface + """ + + ARGUMENT = 'erc20-interface' + HELP = 'Incorrect ERC20 interfaces' + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#incorrect-erc20-interface' + + WIKI_TITLE = 'Incorrect erc20 interface' + WIKI_DESCRIPTION = 'Lack of return value for the ERC20 `approve`/`transfer`/`transferFrom` functions. A contract compiled with solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Token{ + function transfer(address to, uint value) external; + //... +} +``` +`Token.transfer` does not return a boolean. Bob deploys the token. Alice creates a contract that interacts with it but assumes a correct ERC20 interface implementation. Alice's contract is unable to interact with Bob's contract.''' + + WIKI_RECOMMENDATION = 'Return a boolean for the `approve`/`transfer`/`transferFrom` functions.' + + @staticmethod + def incorrect_erc20_interface(signature): + (name, parameters, returnVars) = signature + + if name == 'transfer' and parameters == ['address', 'uint256'] and returnVars != ['bool']: + return True + + if name == 'transferFrom' and parameters == ['address', 'address', 'uint256'] and returnVars != ['bool']: + return True + + if name == 'approve' and parameters == ['address', 'uint256'] and returnVars != ['bool']: + return True + + return False + + @staticmethod + def detect_incorrect_erc20_interface(contract): + """ Detect incorrect ERC20 interface + + Returns: + list(str) : list of incorrect function signatures + """ + functions = [f for f in contract.functions if f.contract == contract and \ + IncorrectERC20InterfaceDetection.incorrect_erc20_interface(f.signature)] + return functions + + def _detect(self): + """ Detect incorrect erc20 interface + + Returns: + dict: [contrat name] = set(str) events + """ + results = [] + for c in self.contracts: + functions = IncorrectERC20InterfaceDetection.detect_incorrect_erc20_interface(c) + if functions: + info = "{} ({}) has incorrect ERC20 function interface(s):\n" + info = info.format(c.name, + c.source_mapping_str) + for function in functions: + info += "\t-{} ({})\n".format(function.name, function.source_mapping_str) + json = self.generate_json_result(info) + self.add_functions_to_json(functions, json) + results.append(json) + + return results diff --git a/slither/detectors/erc20/unindexed_event_parameters.py b/slither/detectors/erc20/unindexed_event_parameters.py new file mode 100644 index 000000000..4b989824e --- /dev/null +++ b/slither/detectors/erc20/unindexed_event_parameters.py @@ -0,0 +1,85 @@ +""" +Detect mistakenly un-indexed ERC20 event parameters +""" +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification + + +class UnindexedERC20EventParameters(AbstractDetector): + """ + Un-indexed ERC20 event parameters + """ + + ARGUMENT = 'erc20-indexed' + HELP = 'Un-indexed ERC20 event parameters' + IMPACT = DetectorClassification.INFORMATIONAL + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unindexed-erc20-event-parameters' + + WIKI_TITLE = 'Unindexed ERC20 Event Parameters' + WIKI_DESCRIPTION = 'Detects that events defined by the ERC20 specification which are meant to have some parameters as `indexed`, are not missing the `indexed` keyword.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract ERC20Bad { + // ... + event Transfer(address from, address to, uint value); + event Approval(address owner, address spender, uint value); + + // ... +} +``` +In this case, Transfer and Approval events should have the 'indexed' keyword on their two first parameters, as defined by the ERC20 specification. Failure to include these keywords will not include the parameter data in the transaction/block's bloom filter. This may cause external tooling searching for these parameters to overlook them, and fail to index logs from this token contract.''' + + WIKI_RECOMMENDATION = 'Add the `indexed` keyword to event parameters which should include it, according to the ERC20 specification.' + + @staticmethod + def detect_erc20_unindexed_event_params(contract): + """ + Detect un-indexed ERC20 event parameters in a given contract. + :param contract: The contract to check ERC20 events for un-indexed parameters in. + :return: A list of tuple(event, parameter) of parameters which should be indexed. + """ + # Create our result array + results = [] + + # If this contract isn't an ERC20 token, we return our empty results. + if not contract.is_erc20(): + return results + + # Loop through all events to look for poor form. + for event in contract.events: + + # Only handle events which are declared in this contract. + if event.contract != contract: + continue + + # If this is transfer/approval events, expect the first two parameters to be indexed. + if event.full_name in ["Transfer(address,address,uint256)", + "Approval(address,address,uint256)"]: + if not event.elems[0].indexed: + results.append((event, event.elems[0])) + if not event.elems[1].indexed: + results.append((event, event.elems[1])) + + # Return the results. + return results + + def _detect(self): + """ + Detect un-indexed ERC20 event parameters in all contracts. + """ + results = [] + for c in self.contracts: + unindexed_params = self.detect_erc20_unindexed_event_params(c) + if unindexed_params: + info = "{} ({}) does not mark important ERC20 parameters as 'indexed':\n" + info = info.format(c.name, c.source_mapping_str) + for (event, parameter) in unindexed_params: + info += "\t-{} ({}) does not index parameter '{}'\n".format(event.name, event.source_mapping_str, parameter.name) + + # Add the events to the JSON (note: we do not add the params/vars as they have no source mapping). + json = self.generate_json_result(info) + self.add_functions_to_json([event for event, _ in unindexed_params], json) + results.append(json) + + return results diff --git a/slither/detectors/statements/deprecated_calls.py b/slither/detectors/statements/deprecated_calls.py new file mode 100644 index 000000000..58c9eaf60 --- /dev/null +++ b/slither/detectors/statements/deprecated_calls.py @@ -0,0 +1,177 @@ +""" +Module detecting deprecated standards. +""" + +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.visitors.expression.export_values import ExportValues +from slither.core.declarations.solidity_variables import SolidityVariableComposed, SolidityFunction +from slither.core.cfg.node import NodeType +from slither.slithir.operations import LowLevelCall +from slither.solc_parsing.variables.state_variable import StateVariableSolc, StateVariable + +# Reference: https://smartcontractsecurity.github.io/SWC-registry/docs/SWC-111 +class DeprecatedStandards(AbstractDetector): + """ + Use of Deprecated Standards + """ + + ARGUMENT = 'deprecated-standards' + HELP = 'Deprecated Solidity Standards' + IMPACT = DetectorClassification.INFORMATIONAL + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#deprecated-standards' + + WIKI_TITLE = 'Deprecated Standards' + WIKI_DESCRIPTION = 'Detect the usage of deprecated standards (as defined by SWC-111), excluding only `constant` keyword detection on functions.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract ContractWithDeprecatedReferences { + // Deprecated: Change block.blockhash() -> blockhash() + bytes32 globalBlockHash = block.blockhash(0); + + // Deprecated: Change constant -> view + function functionWithDeprecatedThrow() public constant { + // Deprecated: Change msg.gas -> gasleft() + if(msg.gas == msg.value) { + // Deprecated: Change throw -> revert() + throw; + } + } + + // Deprecated: Change constant -> view + function functionWithDeprecatedReferences() public constant { + // Deprecated: Change sha3() -> keccak256() + bytes32 sha3Result = sha3("test deprecated sha3 usage"); + + // Deprecated: Change callcode() -> delegatecall() + address(this).callcode(); + + // Deprecated: Change suicide() -> selfdestruct() + suicide(address(0)); + } +} +```''' + + WIKI_RECOMMENDATION = 'Replace all uses of deprecated symbols.' + + # The format for the following deprecated lists is [(detecting_signature, original_text, recommended_text)] + DEPRECATED_SOLIDITY_VARIABLE = [("block.blockhash", "block.blockhash()", "blockhash()"), + ("msg.gas", "msg.gas", "gasleft()")] + DEPRECATED_SOLIDITY_FUNCTIONS = [("suicide(address)", "suicide()", "selfdestruct()"), + ("sha3()", "sha3()", "keccak256()")] + DEPRECATED_NODE_TYPES = [(NodeType.THROW, "throw", "revert()")] + DEPRECATED_LOW_LEVEL_CALLS = [("callcode", "callcode", "delegatecall")] + + def detect_deprecation_in_expression(self, expression): + """ Detects if an expression makes use of any deprecated standards. + + Returns: + list of tuple: (detecting_signature, original_text, recommended_text)""" + # Perform analysis on this expression + export = ExportValues(expression) + export_values = export.result() + + # Define our results list + results = [] + + # Check if there is usage of any deprecated solidity variables or functions + for dep_var in self.DEPRECATED_SOLIDITY_VARIABLE: + if SolidityVariableComposed(dep_var[0]) in export_values: + results.append(dep_var) + for dep_func in self.DEPRECATED_SOLIDITY_FUNCTIONS: + if SolidityFunction(dep_func[0]) in export_values: + results.append(dep_func) + + return results + + def detect_deprecated_references_in_node(self, node): + """ Detects if a node makes use of any deprecated standards. + + Returns: + list of tuple: (detecting_signature, original_text, recommended_text)""" + # Define our results list + results = [] + + # If this node has an expression, we check the underlying expression. + if node.expression: + results += self.detect_deprecation_in_expression(node.expression) + + # Check if there is usage of any deprecated solidity variables or functions + for dep_node in self.DEPRECATED_NODE_TYPES: + if node.type == dep_node[0]: + results.append(dep_node) + + return results + + def detect_deprecated_references_in_contract(self, contract): + """ Detects the usage of any deprecated built-in symbols. + + Returns: + list of tuple: (state_variable | node, (detecting_signature, original_text, recommended_text))""" + results = [] + + for state_variable in contract.variables: + if state_variable.contract != contract: + continue + if state_variable.expression: + deprecated_results = self.detect_deprecation_in_expression(state_variable.expression) + if deprecated_results: + results.append((state_variable, deprecated_results)) + + # Loop through all functions + modifiers in this contract. + for function in contract.functions + contract.modifiers: + # We should only look for functions declared directly in this contract (not in a base contract). + if function.contract != contract: + continue + + # Loop through each node in this function. + for node in function.nodes: + # Detect deprecated references in the node. + deprecated_results = self.detect_deprecated_references_in_node(node) + + # Detect additional deprecated low-level-calls. + for ir in node.irs: + if isinstance(ir, LowLevelCall): + for dep_llc in self.DEPRECATED_LOW_LEVEL_CALLS: + if ir.function_name == dep_llc[0]: + deprecated_results.append(dep_llc) + + # If we have any results from this iteration, add them to our results list. + if deprecated_results: + results.append((node, deprecated_results)) + + return results + + def _detect(self): + """ Detect shadowing local variables + + Recursively visit the calls + Returns: + list: {'vuln', 'filename,'contract','func', 'deprecated_references'} + + """ + results = [] + for contract in self.contracts: + deprecated_references = self.detect_deprecated_references_in_contract(contract) + if deprecated_references: + for deprecated_reference in deprecated_references: + source_object = deprecated_reference[0] + deprecated_entries = deprecated_reference[1] + info = 'Deprecated standard detected @ {}:\n'.format(source_object.source_mapping_str) + + for (dep_id, original_desc, recommended_disc) in deprecated_entries: + info += "\t- Usage of \"{}\" should be replaced with \"{}\"\n".format(original_desc, + recommended_disc) + + + # Generate relevant JSON data for this shadowing definition. + json = self.generate_json_result(info) + if isinstance(source_object, StateVariableSolc) or isinstance(source_object, StateVariable): + self.add_variable_to_json(source_object, json) + else: + self.add_nodes_to_json([source_object], json) + + results.append(json) + + return results diff --git a/tests/deprecated_calls.sol b/tests/deprecated_calls.sol new file mode 100644 index 000000000..6321e4e76 --- /dev/null +++ b/tests/deprecated_calls.sol @@ -0,0 +1,27 @@ +contract ContractWithDeprecatedReferences { + bytes32 globalBlockHash = block.blockhash(0); + + // Deprecated: Change constant -> view + function functionWithDeprecatedThrow() public constant { + // Deprecated: Change msg.gas -> gasleft() + if(msg.gas == msg.value) { + // Deprecated: Change throw -> revert() + throw; + } + } + + // Deprecated: Change constant -> view + function functionWithDeprecatedReferences() public constant { + // Deprecated: Change sha3() -> keccak256() + bytes32 sha3Result = sha3("test deprecated sha3 usage"); + + // Deprecated: Change block.blockhash() -> blockhash() + bytes32 blockHashResult = block.blockhash(0); + + // Deprecated: Change callcode() -> delegatecall() + address(this).callcode(); + + // Deprecated: Change suicide() -> selfdestruct() + suicide(address(0)); + } +} \ No newline at end of file diff --git a/tests/erc20_indexed.sol b/tests/erc20_indexed.sol new file mode 100644 index 000000000..0381d65d4 --- /dev/null +++ b/tests/erc20_indexed.sol @@ -0,0 +1,25 @@ +contract IERC20Good { + function transfer(address to, uint value) external returns (bool); + function approve(address spender, uint value) external returns (bool); + function transferFrom(address from, address to, uint value) external returns (bool); + function totalSupply() external view returns (uint); + function balanceOf(address who) external view returns (uint); + function allowance(address owner, address spender) external view returns (uint); + event Transfer(address indexed from, address indexed to, uint value); + event Approval(address indexed owner, address indexed spender, uint value); +} + +contract IERC20Bad { + function transfer(address to, uint value) external returns (bool); + function approve(address spender, uint value) external returns (bool); + function transferFrom(address from, address to, uint value) external returns (bool); + function totalSupply() external view returns (uint); + function balanceOf(address who) external view returns (uint); + function allowance(address owner, address spender) external view returns (uint); + event Transfer(address from, address to, uint value); + event Approval(address owner, address spender, uint value); +} + +contract ERC20BadDerived is IERC20Bad { + +} \ No newline at end of file diff --git a/tests/expected_json/deprecated_calls.deprecated-standards.json b/tests/expected_json/deprecated_calls.deprecated-standards.json new file mode 100644 index 000000000..8873478a5 --- /dev/null +++ b/tests/expected_json/deprecated_calls.deprecated-standards.json @@ -0,0 +1 @@ +[{"check": "deprecated-standards", "impact": "Informational", "confidence": "High", "description": "Deprecated standard detected @ tests/deprecated_calls.sol#2:\n\t- Usage of \"block.blockhash()\" should be replaced with \"blockhash()\"\n", "elements": [{"type": "variable", "name": "globalBlockHash", "source_mapping": {"start": 48, "length": 44, "filename": "tests/deprecated_calls.sol", "lines": [2]}}]}, {"check": "deprecated-standards", "impact": "Informational", "confidence": "High", "description": "Deprecated standard detected @ tests/deprecated_calls.sol#7-10:\n\t- Usage of \"msg.gas\" should be replaced with \"gasleft()\"\n", "elements": [{"type": "expression", "expression": "msg.gas == msg.value", "source_mapping": {"start": 258, "length": 107, "filename": "tests/deprecated_calls.sol", "lines": [7, 8, 9, 10]}}]}, {"check": "deprecated-standards", "impact": "Informational", "confidence": "High", "description": "Deprecated standard detected @ tests/deprecated_calls.sol#9:\n\t- Usage of \"throw\" should be replaced with \"revert()\"\n", "elements": [{"type": "expression", "expression": "None", "source_mapping": {"start": 349, "length": 5, "filename": "tests/deprecated_calls.sol", "lines": [9]}}]}, {"check": "deprecated-standards", "impact": "Informational", "confidence": "High", "description": "Deprecated standard detected @ tests/deprecated_calls.sol#16:\n\t- Usage of \"sha3()\" should be replaced with \"keccak256()\"\n", "elements": [{"type": "expression", "expression": "sha3Result = sha3()(test deprecated sha3 usage)", "source_mapping": {"start": 542, "length": 55, "filename": "tests/deprecated_calls.sol", "lines": [16]}}]}, {"check": "deprecated-standards", "impact": "Informational", "confidence": "High", "description": "Deprecated standard detected @ tests/deprecated_calls.sol#19:\n\t- Usage of \"block.blockhash()\" should be replaced with \"blockhash()\"\n", "elements": [{"type": "expression", "expression": "blockHashResult = block.blockhash(0)", "source_mapping": {"start": 671, "length": 44, "filename": "tests/deprecated_calls.sol", "lines": [19]}}]}, {"check": "deprecated-standards", "impact": "Informational", "confidence": "High", "description": "Deprecated standard detected @ tests/deprecated_calls.sol#22:\n\t- Usage of \"callcode\" should be replaced with \"delegatecall\"\n", "elements": [{"type": "expression", "expression": "address(this).callcode()", "source_mapping": {"start": 785, "length": 24, "filename": "tests/deprecated_calls.sol", "lines": [22]}}]}, {"check": "deprecated-standards", "impact": "Informational", "confidence": "High", "description": "Deprecated standard detected @ tests/deprecated_calls.sol#25:\n\t- Usage of \"suicide()\" should be replaced with \"selfdestruct()\"\n", "elements": [{"type": "expression", "expression": "suicide(address)(address(0))", "source_mapping": {"start": 878, "length": 19, "filename": "tests/deprecated_calls.sol", "lines": [25]}}]}] \ No newline at end of file diff --git a/tests/expected_json/erc20_indexed.erc20-indexed.json b/tests/expected_json/erc20_indexed.erc20-indexed.json new file mode 100644 index 000000000..35d3751ce --- /dev/null +++ b/tests/expected_json/erc20_indexed.erc20-indexed.json @@ -0,0 +1 @@ +[{"check": "erc20-indexed", "impact": "Informational", "confidence": "High", "description": "IERC20Bad (tests/erc20_indexed.sol#12-21) does not mark important ERC20 parameters as 'indexed':\n\t-Transfer (tests/erc20_indexed.sol#19) does not index parameter 'from'\n\t-Transfer (tests/erc20_indexed.sol#19) does not index parameter 'to'\n\t-Approval (tests/erc20_indexed.sol#20) does not index parameter 'owner'\n\t-Approval (tests/erc20_indexed.sol#20) does not index parameter 'spender'\n", "elements": [{"type": "function", "name": "Approval", "source_mapping": {"start": 1148, "length": 59, "filename": "tests/erc20_indexed.sol", "lines": [20]}, "contract": {"type": "contract", "name": "IERC20Bad", "source_mapping": {"start": 622, "length": 587, "filename": "tests/erc20_indexed.sol", "lines": [12, 13, 14, 15, 16, 17, 18, 19, 20, 21]}}}, {"type": "function", "name": "Approval", "source_mapping": {"start": 1148, "length": 59, "filename": "tests/erc20_indexed.sol", "lines": [20]}, "contract": {"type": "contract", "name": "IERC20Bad", "source_mapping": {"start": 622, "length": 587, "filename": "tests/erc20_indexed.sol", "lines": [12, 13, 14, 15, 16, 17, 18, 19, 20, 21]}}}, {"type": "function", "name": "Transfer", "source_mapping": {"start": 1090, "length": 53, "filename": "tests/erc20_indexed.sol", "lines": [19]}, "contract": {"type": "contract", "name": "IERC20Bad", "source_mapping": {"start": 622, "length": 587, "filename": "tests/erc20_indexed.sol", "lines": [12, 13, 14, 15, 16, 17, 18, 19, 20, 21]}}}, {"type": "function", "name": "Transfer", "source_mapping": {"start": 1090, "length": 53, "filename": "tests/erc20_indexed.sol", "lines": [19]}, "contract": {"type": "contract", "name": "IERC20Bad", "source_mapping": {"start": 622, "length": 587, "filename": "tests/erc20_indexed.sol", "lines": [12, 13, 14, 15, 16, 17, 18, 19, 20, 21]}}}]}] \ No newline at end of file diff --git a/tests/expected_json/incorrect_erc20_interface.erc20-interface.json b/tests/expected_json/incorrect_erc20_interface.erc20-interface.json new file mode 100644 index 000000000..78146275e --- /dev/null +++ b/tests/expected_json/incorrect_erc20_interface.erc20-interface.json @@ -0,0 +1 @@ +[{"check": "erc20-interface", "impact": "Medium", "confidence": "High", "description": "Token (tests/incorrect_erc20_interface.sol#3-7) has incorrect ERC20 function interface(s):\n\t-transfer (tests/incorrect_erc20_interface.sol#5)\n", "elements": [{"type": "function", "name": "transfer", "source_mapping": {"start": 47, "length": 51, "filename": "tests/incorrect_erc20_interface.sol", "lines": [5]}, "contract": {"type": "contract", "name": "Token", "source_mapping": {"start": 26, "length": 75, "filename": "tests/incorrect_erc20_interface.sol", "lines": [3, 4, 5, 6, 7]}}}]}] \ No newline at end of file diff --git a/tests/incorrect_erc20_interface.sol b/tests/incorrect_erc20_interface.sol new file mode 100644 index 000000000..e082b77ef --- /dev/null +++ b/tests/incorrect_erc20_interface.sol @@ -0,0 +1,7 @@ +pragma solidity ^0.4.24; + +contract Token{ + + function transfer(address to, uint value) external; + +}