diff --git a/README.md b/README.md index 846f88f12..60b00d621 100644 --- a/README.md +++ b/README.md @@ -60,25 +60,27 @@ 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 | `locked-ether` | [Contracts that lock ether](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#contracts-that-lock-ether) | Medium | High -9 | `shadowing-abstract` | [State variables shadowing from abstract contracts](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variable-shadowing-from-abstract-contracts) | Medium | High -10 | `constant-function` | [Constant functions changing the state](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#constant-functions-changing-the-state) | Medium | Medium -11 | `reentrancy-no-eth` | [Reentrancy vulnerabilities (no theft of ethers)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities) | Medium | Medium -12 | `tx-origin` | [Dangerous usage of `tx.origin`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-usage-of-txorigin) | Medium | Medium -13 | `uninitialized-local` | [Uninitialized local variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-local-variables) | Medium | Medium -14 | `unused-return` | [Unused return values](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return) | Medium | Medium -15 | `calls-loop` | [Multiple calls in a loop](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description/_edit#calls-inside-a-loop) | Low | Medium -16 | `reentrancy-benign` | [Benign reentrancy vulnerabilities](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities) | Low | Medium -17 | `timestamp` | [Dangerous usage of `block.timestamp`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#block-timestamp) | Low | Medium -18 | `assembly` | [Assembly usage](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage) | Informational | High -19 | `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 -20 | `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 -21 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | Informational | High -22 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | Informational | High -23 | `pragma` | [If different pragma directives are used](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#different-pragma-directives-are-used) | Informational | High -24 | `solc-version` | [Old versions of Solidity (< 0.4.23)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#old-versions-of-solidity) | Informational | High -25 | `unused-state` | [Unused state variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables) | Informational | High - +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) | 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) | 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` | [Old versions of Solidity (< 0.4.23)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#old-versions-of-solidity) | Informational | High +28 | `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. @@ -89,14 +91,15 @@ To run a printer, use `--print` and a comma-separated list of printers. Num | Printer | Description --- | --- | --- 1 | `call-graph` | Export the call-graph of the contracts to a dot file -2 | `contract-summary` | Print a summary of the contracts -3 | `function-summary` | Print a summary of the functions -4 | `human-summary` | Print a human readable summary of the contracts -5 | `inheritance` | Print the inheritance relations between contracts -6 | `inheritance-graph` | Export the inheritance graph of each contract to a dot file -7 | `slithir` | Print the slithIR representation of the functions -8 | `vars-and-auth` | Print the state variables written and the authorization of the functions - +2 | `cfg` | Export the CFG of each functions +3 | `contract-summary` | Print a summary of the contracts +4 | `function-summary` | Print a summary of the functions +5 | `human-summary` | Print a human readable summary of the contracts +6 | `inheritance` | Print the inheritance relations between contracts +7 | `inheritance-graph` | Export the inheritance graph of each contract to a dot file +8 | `slithir` | Print the slithIR representation of the functions +9 | `slithir-ssa` | Print the slithIR representation of the functions +10 | `vars-and-auth` | Print the state variables written and the authorization of the functions ## How to install Slither requires Python 3.6+ and [solc](https://github.com/ethereum/solidity/), the Solidity compiler. diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index 18db4d8d1..1034181fa 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -40,3 +40,5 @@ generate_expected_json(){ #generate_expected_json tests/shadowing_abstract.sol "shadowing-abstract" #generate_expected_json tests/timestamp.sol "timestamp" #generate_expected_json tests/multiple_calls_in_loop.sol "calls-loop" +#generate_expected_json tests/shadowing_builtin_symbols.sol "shadowing-builtin" +#generate_expected_json tests/shadowing_local_variable.sol "shadowing-local" diff --git a/scripts/tests_generate_expected_json_5.sh b/scripts/tests_generate_expected_json_5.sh index 212f463e6..6615bd823 100755 --- a/scripts/tests_generate_expected_json_5.sh +++ b/scripts/tests_generate_expected_json_5.sh @@ -28,3 +28,4 @@ generate_expected_json(){ #generate_expected_json tests/inline_assembly_contract-0.5.1.sol "assembly" #generate_expected_json tests/inline_assembly_library-0.5.1.sol "assembly" #generate_expected_json tests/constant-0.5.1.sol "constant-function" +#generate_expected_json tests/incorrect_equality.sol "incorrect-equality" diff --git a/scripts/travis_test_4.sh b/scripts/travis_test_4.sh index b6de5f3a2..6814e2986 100755 --- a/scripts/travis_test_4.sh +++ b/scripts/travis_test_4.sh @@ -91,5 +91,6 @@ test_slither tests/shadowing_abstract.sol "shadowing-abstract" test_slither tests/shadowing_state_variable.sol "shadowing-state" test_slither tests/timestamp.sol "timestamp" test_slither tests/multiple_calls_in_loop.sol "calls-loop" - +test_slither tests/shadowing_builtin_symbols.sol "shadowing-builtin" +test_slither tests/shadowing_local_variable.sol "shadowing-local" diff --git a/scripts/travis_test_5.sh b/scripts/travis_test_5.sh index 5e24dd21e..737238ab5 100755 --- a/scripts/travis_test_5.sh +++ b/scripts/travis_test_5.sh @@ -86,7 +86,7 @@ test_slither tests/controlled_delegatecall.sol "controlled-delegatecall" test_slither tests/constant-0.5.1.sol "constant-function" test_slither tests/unused_return.sol "unused-return" test_slither tests/timestamp.sol "timestamp" - +test_slither tests/incorrect_equality.sol "incorrect-equality" ### Test scripts diff --git a/slither/__main__.py b/slither/__main__.py index e68a8059e..25b9d1aa0 100644 --- a/slither/__main__.py +++ b/slither/__main__.py @@ -132,9 +132,11 @@ def get_detectors_and_printers(): from slither.detectors.attributes.const_functions import ConstantFunctions from slither.detectors.shadowing.abstract import ShadowingAbstractDetection from slither.detectors.shadowing.state import StateShadowing + from slither.detectors.shadowing.local import LocalShadowing + from slither.detectors.shadowing.builtin_symbols import BuiltinSymbolShadowing from slither.detectors.operations.block_timestamp import Timestamp from slither.detectors.statements.calls_in_loop import MultipleCallsInLoop - + from slither.detectors.statements.incorrect_strict_equality import IncorrectStrictEquality detectors = [Backdoor, UninitializedStateVarsDetection, @@ -162,7 +164,10 @@ def get_detectors_and_printers(): ShadowingAbstractDetection, StateShadowing, Timestamp, - MultipleCallsInLoop] + MultipleCallsInLoop, + IncorrectStrictEquality, + LocalShadowing, + BuiltinSymbolShadowing] from slither.printers.summary.function import FunctionSummary from slither.printers.summary.contract import ContractSummary diff --git a/slither/detectors/reentrancy/reentrancy_benign.py b/slither/detectors/reentrancy/reentrancy_benign.py index dbccd86a1..336b19384 100644 --- a/slither/detectors/reentrancy/reentrancy_benign.py +++ b/slither/detectors/reentrancy/reentrancy_benign.py @@ -112,13 +112,7 @@ class ReentrancyBenign(AbstractDetector): node.context[self.key] = fathers_context - contains_call = False - if self._can_callback(node): - node.context[self.key]['calls'] = list(set(node.context[self.key]['calls'] + [node])) - contains_call = True - if self._can_send_eth(node): - node.context[self.key]['send_eth'] = list(set(node.context[self.key]['send_eth'] + [node])) - + state_vars_read = node.state_variables_read # All the state variables written state_vars_written = node.state_variables_written @@ -127,10 +121,18 @@ class ReentrancyBenign(AbstractDetector): # Filter to Function, as internal_call can be a solidity call if isinstance(internal_call, Function): state_vars_written += internal_call.all_state_variables_written() + state_vars_read += internal_call.all_state_variables_read() + + contains_call = False + if self._can_callback(node): + node.context[self.key]['calls'] = list(set(node.context[self.key]['calls'] + [node])) + contains_call = True + if self._can_send_eth(node): + node.context[self.key]['send_eth'] = list(set(node.context[self.key]['send_eth'] + [node])) not_read_then_written = [(v, node) for v in state_vars_written if v not in node.context[self.key]['read']] + node.context[self.key]['read'] = list(set(node.context[self.key]['read'] + state_vars_read)) - node.context[self.key]['read'] = list(set(node.context[self.key]['read'] + node.state_variables_read)) # If a state variables was read and is then written, there is a dangerous call and # ether were sent # We found a potential re-entrancy bug diff --git a/slither/detectors/reentrancy/reentrancy_eth.py b/slither/detectors/reentrancy/reentrancy_eth.py index c9b0efcc9..baf872e4c 100644 --- a/slither/detectors/reentrancy/reentrancy_eth.py +++ b/slither/detectors/reentrancy/reentrancy_eth.py @@ -88,36 +88,33 @@ class ReentrancyEth(AbstractDetector): # send_eth returns the list of calls sending value # calls returns the list of calls that can callback # read returns the variable read - fathers_context = {'send_eth':[], 'calls':[], 'read':[]} + fathers_context = {'send_eth':[], 'calls':[], 'read':[], 'read_prior_calls':[]} for father in node.fathers: if self.key in father.context: fathers_context['send_eth'] += [s for s in father.context[self.key]['send_eth'] if s!=skip_father] fathers_context['calls'] += [c for c in father.context[self.key]['calls'] if c!=skip_father] fathers_context['read'] += father.context[self.key]['read'] + fathers_context['read_prior_calls'] += father.context[self.key]['read_prior_calls'] # Exclude path that dont bring further information if node in self.visited_all_paths: if all(call in self.visited_all_paths[node]['calls'] for call in fathers_context['calls']): if all(send in self.visited_all_paths[node]['send_eth'] for send in fathers_context['send_eth']): if all(read in self.visited_all_paths[node]['read'] for read in fathers_context['read']): - return + if all(read in self.visited_all_paths[node]['read_prior_calls'] for read in fathers_context['read_prior_calls']): + return else: - self.visited_all_paths[node] = {'send_eth':[], 'calls':[], 'read':[]} + self.visited_all_paths[node] = {'send_eth':[], 'calls':[], 'read':[], 'read_prior_calls':[]} self.visited_all_paths[node]['send_eth'] = list(set(self.visited_all_paths[node]['send_eth'] + fathers_context['send_eth'])) self.visited_all_paths[node]['calls'] = list(set(self.visited_all_paths[node]['calls'] + fathers_context['calls'])) self.visited_all_paths[node]['read'] = list(set(self.visited_all_paths[node]['read'] + fathers_context['read'])) + self.visited_all_paths[node]['read_prior_calls'] = list(set(self.visited_all_paths[node]['read_prior_calls'] + fathers_context['read_prior_calls'])) node.context[self.key] = fathers_context - contains_call = False - if self._can_callback(node): - node.context[self.key]['calls'] = list(set(node.context[self.key]['calls'] + [node])) - contains_call = True - if self._can_send_eth(node): - node.context[self.key]['send_eth'] = list(set(node.context[self.key]['send_eth'] + [node])) - + state_vars_read = node.state_variables_read # All the state variables written state_vars_written = node.state_variables_written @@ -126,10 +123,21 @@ class ReentrancyEth(AbstractDetector): # Filter to Function, as internal_call can be a solidity call if isinstance(internal_call, Function): state_vars_written += internal_call.all_state_variables_written() + state_vars_read += internal_call.all_state_variables_read() + + contains_call = False + if self._can_callback(node): + node.context[self.key]['calls'] = list(set(node.context[self.key]['calls'] + [node])) + node.context[self.key]['read_prior_calls'] = list(set(node.context[self.key]['read_prior_calls'] + node.context[self.key]['read']+ state_vars_read)) + node.context[self.key]['read'] = [] + contains_call = True + if self._can_send_eth(node): + node.context[self.key]['send_eth'] = list(set(node.context[self.key]['send_eth'] + [node])) + - read_then_written = [(v, node) for v in state_vars_written if v in node.context[self.key]['read']] + read_then_written = [(v, node) for v in state_vars_written if v in node.context[self.key]['read_prior_calls']] - node.context[self.key]['read'] = list(set(node.context[self.key]['read'] + node.state_variables_read)) + node.context[self.key]['read'] = list(set(node.context[self.key]['read'] + state_vars_read)) # If a state variables was read and is then written, there is a dangerous call and # ether were sent # We found a potential re-entrancy bug diff --git a/slither/detectors/reentrancy/reentrancy_read_before_write.py b/slither/detectors/reentrancy/reentrancy_read_before_write.py index 508a34393..d0552b126 100644 --- a/slither/detectors/reentrancy/reentrancy_read_before_write.py +++ b/slither/detectors/reentrancy/reentrancy_read_before_write.py @@ -89,37 +89,33 @@ class ReentrancyReadBeforeWritten(AbstractDetector): # send_eth returns the list of calls sending value # calls returns the list of calls that can callback # read returns the variable read - fathers_context = {'send_eth':[], 'calls':[], 'read':[]} + fathers_context = {'send_eth':[], 'calls':[], 'read':[], 'read_prior_calls':[]} for father in node.fathers: if self.key in father.context: fathers_context['send_eth'] += [s for s in father.context[self.key]['send_eth'] if s!=skip_father] fathers_context['calls'] += [c for c in father.context[self.key]['calls'] if c!=skip_father] fathers_context['read'] += father.context[self.key]['read'] + fathers_context['read_prior_calls'] += father.context[self.key]['read_prior_calls'] # Exclude path that dont bring further information if node in self.visited_all_paths: if all(call in self.visited_all_paths[node]['calls'] for call in fathers_context['calls']): if all(send in self.visited_all_paths[node]['send_eth'] for send in fathers_context['send_eth']): if all(read in self.visited_all_paths[node]['read'] for read in fathers_context['read']): - return + if all(read in self.visited_all_paths[node]['read_prior_calls'] for read in fathers_context['read_prior_calls']): + return else: - self.visited_all_paths[node] = {'send_eth':[], 'calls':[], 'read':[]} + self.visited_all_paths[node] = {'send_eth':[], 'calls':[], 'read':[], 'read_prior_calls':[]} self.visited_all_paths[node]['send_eth'] = list(set(self.visited_all_paths[node]['send_eth'] + fathers_context['send_eth'])) self.visited_all_paths[node]['calls'] = list(set(self.visited_all_paths[node]['calls'] + fathers_context['calls'])) self.visited_all_paths[node]['read'] = list(set(self.visited_all_paths[node]['read'] + fathers_context['read'])) + self.visited_all_paths[node]['read_prior_calls'] = list(set(self.visited_all_paths[node]['read_prior_calls'] + fathers_context['read_prior_calls'])) node.context[self.key] = fathers_context - contains_call = False - if self._can_callback(node): - node.context[self.key]['calls'] = list(set(node.context[self.key]['calls'] + [node])) - contains_call = True - if self._can_send_eth(node): - node.context[self.key]['send_eth'] = list(set(node.context[self.key]['send_eth'] + [node])) - - + state_vars_read = node.state_variables_read # All the state variables written state_vars_written = node.state_variables_written # Add the state variables written in internal calls @@ -127,10 +123,20 @@ class ReentrancyReadBeforeWritten(AbstractDetector): # Filter to Function, as internal_call can be a solidity call if isinstance(internal_call, Function): state_vars_written += internal_call.all_state_variables_written() + state_vars_read += internal_call.all_state_variables_read() + + contains_call = False + if self._can_callback(node): + node.context[self.key]['calls'] = list(set(node.context[self.key]['calls'] + [node])) + node.context[self.key]['read_prior_calls'] = list(set(node.context[self.key]['read_prior_calls'] + node.context[self.key]['read'] + state_vars_read)) + node.context[self.key]['read'] = [] + contains_call = True + if self._can_send_eth(node): + node.context[self.key]['send_eth'] = list(set(node.context[self.key]['send_eth'] + [node])) - read_then_written = [(v, node) for v in state_vars_written if v in node.context[self.key]['read']] + read_then_written = [(v, node) for v in state_vars_written if v in node.context[self.key]['read_prior_calls']] - node.context[self.key]['read'] = list(set(node.context[self.key]['read'] + node.state_variables_read)) + node.context[self.key]['read'] = list(set(node.context[self.key]['read'] + state_vars_read)) # If a state variables was read and is then written, there is a dangerous call and # ether were sent # We found a potential re-entrancy bug diff --git a/slither/detectors/shadowing/builtin_symbols.py b/slither/detectors/shadowing/builtin_symbols.py new file mode 100644 index 000000000..927a5a810 --- /dev/null +++ b/slither/detectors/shadowing/builtin_symbols.py @@ -0,0 +1,133 @@ +""" +Module detecting reserved keyword shadowing +""" + +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification + + +class BuiltinSymbolShadowing(AbstractDetector): + """ + Built-in symbol shadowing + """ + + ARGUMENT = 'shadowing-builtin' + HELP = 'Built-in symbol shadowing' + IMPACT = DetectorClassification.LOW + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#builtin-symbol-shadowing' + + vuln_name = 'ShadowingBuiltinSymbols' + + SHADOWING_FUNCTION = "function" + SHADOWING_MODIFIER = "modifier" + SHADOWING_LOCAL_VARIABLE = "local variable" + SHADOWING_STATE_VARIABLE = "state variable" + SHADOWING_EVENT = "event" + + # Reserved keywords reference: https://solidity.readthedocs.io/en/v0.5.2/units-and-global-variables.html + BUILTIN_SYMBOLS = ["assert", "require", "revert", "block", "blockhash", + "gasleft", "msg", "now", "tx", "this", "addmod", "mulmod", + "keccak256", "sha256", "sha3", "ripemd160", "ecrecover", + "selfdestruct", "suicide", "abi"] + + # https://solidity.readthedocs.io/en/v0.5.2/miscellaneous.html#reserved-keywords + RESERVED_KEYWORDS = ['abstract', 'after', 'alias', 'apply', 'auto', 'case', 'catch', 'copyof', + 'default', 'define', 'final', 'immutable', 'implements', 'in', 'inline', + 'let', 'macro', 'match', 'mutable', 'null', 'of', 'override', 'partial', + 'promise', 'reference', 'relocatable', 'sealed', 'sizeof', 'static', + 'supports', 'switch', 'try', 'type', 'typedef', 'typeof', 'unchecked'] + + def is_builtin_symbol(self, word): + """ Detects if a given word is a built-in symbol. + + Returns: + boolean: True if the given word represents a built-in symbol.""" + + return word in self.BUILTIN_SYMBOLS or word in self.RESERVED_KEYWORDS + + def detect_builtin_shadowing_locals(self, function_or_modifier): + """ Detects if local variables in a given function/modifier are named after built-in symbols. + Any such items are returned in a list. + + Returns: + list of tuple: (type, definition, local variable parent)""" + + results = [] + for local in function_or_modifier.variables: + if self.is_builtin_symbol(local.name): + results.append((self.SHADOWING_LOCAL_VARIABLE, local, function_or_modifier)) + return results + + def detect_builtin_shadowing_definitions(self, contract): + """ Detects if functions, access modifiers, events, state variables, or local variables are named after built-in + symbols. Any such definitions are returned in a list. + + Returns: + list of tuple: (type, definition, [local variable parent])""" + + result = [] + + # Loop through all functions, modifiers, variables (state and local) to detect any built-in symbol keywords. + for function in contract.functions: + if function.contract == contract: + if self.is_builtin_symbol(function.name): + result.append((self.SHADOWING_FUNCTION, function, None)) + result += self.detect_builtin_shadowing_locals(function) + for modifier in contract.modifiers: + if modifier.contract == contract: + if self.is_builtin_symbol(modifier.name): + result.append((self.SHADOWING_MODIFIER, modifier, None)) + result += self.detect_builtin_shadowing_locals(modifier) + for variable in contract.variables: + if variable.contract == contract: + if self.is_builtin_symbol(variable.name): + result.append((self.SHADOWING_STATE_VARIABLE, variable, None)) + for event in contract.events: + if event.contract == contract: + if self.is_builtin_symbol(event.name): + result.append((self.SHADOWING_EVENT, event, None)) + + return result + + def detect(self): + """ Detect shadowing of built-in symbols + + Recursively visit the calls + Returns: + list: {'vuln', 'filename,'contract','func', 'shadow'} + + """ + + results = [] + for contract in self.contracts: + shadows = self.detect_builtin_shadowing_definitions(contract) + if shadows: + for shadow in shadows: + # Obtain components + shadow_type = shadow[0] + shadow_object = shadow[1] + local_variable_parent = shadow[2] + + # Build the path for our info string + local_variable_path = contract.name + "." + if local_variable_parent is not None: + local_variable_path += local_variable_parent.name + "." + local_variable_path += shadow_object.name + + info = '{} ({} @ {}) shadows built-in symbol \"{}"\n'.format(local_variable_path, + shadow_type, + shadow_object.source_mapping_str, + shadow_object.name) + # Print relevant information to the log + self.log(info) + + # Generate relevant JSON data for this shadowing definition. + json = self.generate_json_result(info) + if shadow_type in [self.SHADOWING_FUNCTION, self.SHADOWING_MODIFIER, self.SHADOWING_EVENT]: + self.add_function_to_json(shadow_object, json) + elif shadow_type in [self.SHADOWING_STATE_VARIABLE, self.SHADOWING_LOCAL_VARIABLE]: + self.add_variable_to_json(shadow_object, json) + results.append(json) + + return results diff --git a/slither/detectors/shadowing/local.py b/slither/detectors/shadowing/local.py new file mode 100644 index 000000000..2d9d77931 --- /dev/null +++ b/slither/detectors/shadowing/local.py @@ -0,0 +1,109 @@ +""" +Module detecting local variable shadowing +""" + +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification + + +class LocalShadowing(AbstractDetector): + """ + Local variable shadowing + """ + + ARGUMENT = 'shadowing-local' + HELP = 'Local variables shadowing' + IMPACT = DetectorClassification.LOW + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#local-variable-shadowing' + + vuln_name = 'ShadowingLocalVariable' + + OVERSHADOWED_FUNCTION = "function" + OVERSHADOWED_MODIFIER = "modifier" + OVERSHADOWED_STATE_VARIABLE = "state variable" + OVERSHADOWED_EVENT = "event" + + def detect_shadowing_definitions(self, contract): + """ Detects if functions, access modifiers, events, state variables, and local variables are named after + reserved keywords. Any such definitions are returned in a list. + + Returns: + list of tuple: (type, contract name, definition)""" + result = [] + + # 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 + + # This function was declared in this contract, we check what its local variables might shadow. + for variable in function.variables: + overshadowed = [] + for scope_contract in [contract] + contract.inheritance: + # Check functions + for scope_function in scope_contract.functions: + if variable.name == scope_function.name and scope_function.contract == scope_contract: + overshadowed.append((self.OVERSHADOWED_FUNCTION, scope_contract.name, scope_function)) + # Check modifiers + for scope_modifier in scope_contract.modifiers: + if variable.name == scope_modifier.name and scope_modifier.contract == scope_contract: + overshadowed.append((self.OVERSHADOWED_MODIFIER, scope_contract.name, scope_modifier)) + # Check events + for scope_event in scope_contract.events: + if variable.name == scope_event.name and scope_event.contract == scope_contract: + overshadowed.append((self.OVERSHADOWED_EVENT, scope_contract.name, scope_event)) + # Check state variables + for scope_state_variable in scope_contract.variables: + if variable.name == scope_state_variable.name and scope_state_variable.contract == scope_contract: + overshadowed.append((self.OVERSHADOWED_STATE_VARIABLE, scope_contract.name, scope_state_variable)) + + # If we have found any overshadowed objects, we'll want to add it to our result list. + if overshadowed: + result.append((contract.name, function.name, variable, overshadowed)) + + return result + + def detect(self): + """ Detect shadowing local variables + + Recursively visit the calls + Returns: + list: {'vuln', 'filename,'contract','func', 'shadow'} + + """ + + results = [] + for contract in self.contracts: + shadows = self.detect_shadowing_definitions(contract) + if shadows: + for shadow in shadows: + local_parent_name = shadow[1] + local_variable = shadow[2] + overshadowed = shadow[3] + info = '{}.{}.{} (local variable @ {}) shadows:\n'.format(contract.name, + local_parent_name, + local_variable.name, + local_variable.source_mapping_str) + for overshadowed_entry in overshadowed: + info += "\t- {}.{} ({} @ {})\n".format(overshadowed_entry[1], + overshadowed_entry[2], + overshadowed_entry[0], + overshadowed_entry[2].source_mapping_str) + + # Print relevant information to the log + self.log(info) + + # Generate relevant JSON data for this shadowing definition. + json = self.generate_json_result(info) + self.add_variable_to_json(local_variable, json) + for overshadowed_entry in overshadowed: + if overshadowed_entry[0] in [self.OVERSHADOWED_FUNCTION, self.OVERSHADOWED_MODIFIER, + self.OVERSHADOWED_EVENT]: + self.add_function_to_json(overshadowed_entry[2], json) + elif overshadowed_entry[0] == self.OVERSHADOWED_STATE_VARIABLE: + self.add_variable_to_json(overshadowed_entry[2], json) + results.append(json) + + return results diff --git a/slither/detectors/statements/incorrect_strict_equality.py b/slither/detectors/statements/incorrect_strict_equality.py new file mode 100644 index 000000000..65ad0974e --- /dev/null +++ b/slither/detectors/statements/incorrect_strict_equality.py @@ -0,0 +1,119 @@ +""" + Module detecting dangerous strict equality + +""" + +import itertools +from slither.analyses.data_dependency.data_dependency import is_dependent_ssa +from slither.core.declarations import Function +from slither.detectors.abstract_detector import (AbstractDetector, + DetectorClassification) +from slither.slithir.operations import (Assignment, Balance, Binary, BinaryType, + HighLevelCall) + +from slither.core.solidity_types import MappingType, ElementaryType + +from slither.core.variables.state_variable import StateVariable +from slither.core.declarations.solidity_variables import SolidityVariable, SolidityVariableComposed +from slither.slithir.variables import ReferenceVariable + +class IncorrectStrictEquality(AbstractDetector): + ARGUMENT = 'incorrect-equality' + HELP = 'Dangerous strict equalities' + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-strict-equalities' + + sources_taint = [SolidityVariable('now'), + SolidityVariableComposed('block.number'), + SolidityVariableComposed('block.timestamp')] + + @staticmethod + def is_direct_comparison(ir): + return isinstance(ir, Binary) and ir.type == BinaryType.EQUAL + + @staticmethod + def is_any_tainted(variables, taints, function): + return any([is_dependent_ssa(var, taint, function.contract) for var in variables for taint in taints]) + + def taint_balance_equalities(self, functions): + taints = [] + for func in functions: + for node in func.nodes: + for ir in node.irs_ssa: + if isinstance(ir, Balance): + taints.append(ir.lvalue) + if isinstance(ir, HighLevelCall): + #print(ir.function.full_name) + if isinstance(ir.function, Function) and\ + ir.function.full_name == 'balanceOf(address)': + taints.append(ir.lvalue) + if isinstance(ir.function, StateVariable) and\ + isinstance(ir.function.type, MappingType) and\ + ir.function.name == 'balanceOf' and\ + ir.function.type.type_from == ElementaryType('address') and\ + ir.function.type.type_to == ElementaryType('uint256'): + taints.append(ir.lvalue) + if isinstance(ir, Assignment): + if ir.rvalue in self.sources_taint: + taints.append(ir.lvalue) + + return taints + + # Retrieve all tainted (node, function) pairs + def tainted_equality_nodes(self, funcs, taints): + results = dict() + taints += self.sources_taint + + for func in funcs: + for node in func.nodes: + for ir in node.irs_ssa: + + # Filter to only tainted equality (==) comparisons + if self.is_direct_comparison(ir) and self.is_any_tainted(ir.used, taints, func): + if func not in results: + results[func] = [] + results[func].append(node) + + return results + + def detect_strict_equality(self, contract): + funcs = contract.all_functions_called + contract.modifiers + + # Taint all BALANCE accesses + taints = self.taint_balance_equalities(funcs) + + # Accumulate tainted (node,function) pairs involved in strict equality (==) comparisons + results = self.tainted_equality_nodes(funcs, taints) + + return results + + def detect(self): + results = [] + + for c in self.slither.contracts_derived: + ret = self.detect_strict_equality(c) + + info = '' + # sort ret to get deterministic results + ret = sorted(list(ret.items()), key=lambda x:x[0].name) + for f, nodes in ret: + info += "{}.{} ({}) uses a dangerous strict equality:\n".format(f.contract.name, + f.name, + f.source_mapping_str) + + # sort the nodes to get deterministic results + nodes.sort(key=lambda x: x.node_id) + for node in nodes: + info += "\t- {}\n".format(str(node.expression)) + + json = self.generate_json_result(info) + self.add_function_to_json(f, json) + self.add_nodes_to_json(nodes, json) + results.append(json) + + if info: + self.log(info) + + return results diff --git a/slither/slithir/utils/ssa.py b/slither/slithir/utils/ssa.py index 98fd990e2..84fad4dd8 100644 --- a/slither/slithir/utils/ssa.py +++ b/slither/slithir/utils/ssa.py @@ -469,7 +469,7 @@ def copy_ir(ir, local_variables_instances, state_variables_instances, temporary_ new_ir.call_value = get_variable(ir, lambda x: ir.call_value) new_ir.call_gas = get_variable(ir, lambda x: ir.call_gas) new_ir.arguments = get_arguments(ir) - new_ir.function_instance = ir.function + new_ir.function = ir.function return new_ir elif isinstance(ir, Index): lvalue = get_variable(ir, lambda x: ir.lvalue) diff --git a/tests/expected_json/incorrect_equality.incorrect-equality.json b/tests/expected_json/incorrect_equality.incorrect-equality.json new file mode 100644 index 000000000..9bd015a04 --- /dev/null +++ b/tests/expected_json/incorrect_equality.incorrect-equality.json @@ -0,0 +1 @@ +[{"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "ERC20TestBalance.bad0 (tests/incorrect_equality.sol#21-23) uses a dangerous strict equality:\n\t- require(bool)(erc.balanceOf(address(this)) == 10)\n", "elements": [{"type": "function", "name": "bad0", "source_mapping": {"start": 404, "length": 101, "filename": "tests/incorrect_equality.sol", "lines": [21, 22, 23]}, "contract": {"type": "contract", "name": "ERC20TestBalance", "source_mapping": {"start": 165, "length": 445, "filename": "tests/incorrect_equality.sol", "lines": [10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28]}}}, {"type": "expression", "expression": "require(bool)(erc.balanceOf(address(this)) == 10)", "source_mapping": {"start": 455, "length": 43, "filename": "tests/incorrect_equality.sol", "lines": [22]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "ERC20TestBalance.bad0 (tests/incorrect_equality.sol#21-23) uses a dangerous strict equality:\n\t- require(bool)(erc.balanceOf(address(this)) == 10)\nERC20TestBalance.bad1 (tests/incorrect_equality.sol#25-27) uses a dangerous strict equality:\n\t- require(bool)(erc.balanceOf(msg.sender) == 10)\n", "elements": [{"type": "function", "name": "bad1", "source_mapping": {"start": 511, "length": 97, "filename": "tests/incorrect_equality.sol", "lines": [25, 26, 27]}, "contract": {"type": "contract", "name": "ERC20TestBalance", "source_mapping": {"start": 165, "length": 445, "filename": "tests/incorrect_equality.sol", "lines": [10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28]}}}, {"type": "expression", "expression": "require(bool)(erc.balanceOf(msg.sender) == 10)", "source_mapping": {"start": 562, "length": 39, "filename": "tests/incorrect_equality.sol", "lines": [26]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestContractBalance.bad0 (tests/incorrect_equality.sol#32-35) uses a dangerous strict equality:\n\t- require(bool)(address(address(this)).balance == 10000000000000000000)\n", "elements": [{"type": "function", "name": "bad0", "source_mapping": {"start": 648, "length": 133, "filename": "tests/incorrect_equality.sol", "lines": [32, 33, 34, 35]}, "contract": {"type": "contract", "name": "TestContractBalance", "source_mapping": {"start": 612, "length": 1754, "filename": "tests/incorrect_equality.sol", "lines": [30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97]}}}, {"type": "expression", "expression": "require(bool)(address(address(this)).balance == 10000000000000000000)", "source_mapping": {"start": 683, "length": 51, "filename": "tests/incorrect_equality.sol", "lines": [33]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestContractBalance.bad0 (tests/incorrect_equality.sol#32-35) uses a dangerous strict equality:\n\t- require(bool)(address(address(this)).balance == 10000000000000000000)\nTestContractBalance.bad1 (tests/incorrect_equality.sol#37-40) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(address(this)).balance)\n", "elements": [{"type": "function", "name": "bad1", "source_mapping": {"start": 787, "length": 133, "filename": "tests/incorrect_equality.sol", "lines": [37, 38, 39, 40]}, "contract": {"type": "contract", "name": "TestContractBalance", "source_mapping": {"start": 612, "length": 1754, "filename": "tests/incorrect_equality.sol", "lines": [30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97]}}}, {"type": "expression", "expression": "require(bool)(10000000000000000000 == address(address(this)).balance)", "source_mapping": {"start": 822, "length": 51, "filename": "tests/incorrect_equality.sol", "lines": [38]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestContractBalance.bad0 (tests/incorrect_equality.sol#32-35) uses a dangerous strict equality:\n\t- require(bool)(address(address(this)).balance == 10000000000000000000)\nTestContractBalance.bad1 (tests/incorrect_equality.sol#37-40) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(address(this)).balance)\nTestContractBalance.bad2 (tests/incorrect_equality.sol#42-45) uses a dangerous strict equality:\n\t- require(bool)(address(this).balance == 10000000000000000000)\n", "elements": [{"type": "function", "name": "bad2", "source_mapping": {"start": 926, "length": 124, "filename": "tests/incorrect_equality.sol", "lines": [42, 43, 44, 45]}, "contract": {"type": "contract", "name": "TestContractBalance", "source_mapping": {"start": 612, "length": 1754, "filename": "tests/incorrect_equality.sol", "lines": [30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97]}}}, {"type": "expression", "expression": "require(bool)(address(this).balance == 10000000000000000000)", "source_mapping": {"start": 961, "length": 42, "filename": "tests/incorrect_equality.sol", "lines": [43]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestContractBalance.bad0 (tests/incorrect_equality.sol#32-35) uses a dangerous strict equality:\n\t- require(bool)(address(address(this)).balance == 10000000000000000000)\nTestContractBalance.bad1 (tests/incorrect_equality.sol#37-40) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(address(this)).balance)\nTestContractBalance.bad2 (tests/incorrect_equality.sol#42-45) uses a dangerous strict equality:\n\t- require(bool)(address(this).balance == 10000000000000000000)\nTestContractBalance.bad3 (tests/incorrect_equality.sol#47-50) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(this).balance)\n", "elements": [{"type": "function", "name": "bad3", "source_mapping": {"start": 1056, "length": 124, "filename": "tests/incorrect_equality.sol", "lines": [47, 48, 49, 50]}, "contract": {"type": "contract", "name": "TestContractBalance", "source_mapping": {"start": 612, "length": 1754, "filename": "tests/incorrect_equality.sol", "lines": [30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97]}}}, {"type": "expression", "expression": "require(bool)(10000000000000000000 == address(this).balance)", "source_mapping": {"start": 1091, "length": 42, "filename": "tests/incorrect_equality.sol", "lines": [48]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestContractBalance.bad0 (tests/incorrect_equality.sol#32-35) uses a dangerous strict equality:\n\t- require(bool)(address(address(this)).balance == 10000000000000000000)\nTestContractBalance.bad1 (tests/incorrect_equality.sol#37-40) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(address(this)).balance)\nTestContractBalance.bad2 (tests/incorrect_equality.sol#42-45) uses a dangerous strict equality:\n\t- require(bool)(address(this).balance == 10000000000000000000)\nTestContractBalance.bad3 (tests/incorrect_equality.sol#47-50) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(this).balance)\nTestContractBalance.bad4 (tests/incorrect_equality.sol#52-57) uses a dangerous strict equality:\n\t- balance == 10000000000000000000\n", "elements": [{"type": "function", "name": "bad4", "source_mapping": {"start": 1186, "length": 170, "filename": "tests/incorrect_equality.sol", "lines": [52, 53, 54, 55, 56, 57]}, "contract": {"type": "contract", "name": "TestContractBalance", "source_mapping": {"start": 612, "length": 1754, "filename": "tests/incorrect_equality.sol", "lines": [30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97]}}}, {"type": "expression", "expression": "balance == 10000000000000000000", "source_mapping": {"start": 1270, "length": 80, "filename": "tests/incorrect_equality.sol", "lines": [54, 55, 56]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestContractBalance.bad0 (tests/incorrect_equality.sol#32-35) uses a dangerous strict equality:\n\t- require(bool)(address(address(this)).balance == 10000000000000000000)\nTestContractBalance.bad1 (tests/incorrect_equality.sol#37-40) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(address(this)).balance)\nTestContractBalance.bad2 (tests/incorrect_equality.sol#42-45) uses a dangerous strict equality:\n\t- require(bool)(address(this).balance == 10000000000000000000)\nTestContractBalance.bad3 (tests/incorrect_equality.sol#47-50) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(this).balance)\nTestContractBalance.bad4 (tests/incorrect_equality.sol#52-57) uses a dangerous strict equality:\n\t- balance == 10000000000000000000\nTestContractBalance.bad5 (tests/incorrect_equality.sol#59-64) uses a dangerous strict equality:\n\t- 10000000000000000000 == balance\n", "elements": [{"type": "function", "name": "bad5", "source_mapping": {"start": 1362, "length": 170, "filename": "tests/incorrect_equality.sol", "lines": [59, 60, 61, 62, 63, 64]}, "contract": {"type": "contract", "name": "TestContractBalance", "source_mapping": {"start": 612, "length": 1754, "filename": "tests/incorrect_equality.sol", "lines": [30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97]}}}, {"type": "expression", "expression": "10000000000000000000 == balance", "source_mapping": {"start": 1446, "length": 80, "filename": "tests/incorrect_equality.sol", "lines": [61, 62, 63]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestContractBalance.bad0 (tests/incorrect_equality.sol#32-35) uses a dangerous strict equality:\n\t- require(bool)(address(address(this)).balance == 10000000000000000000)\nTestContractBalance.bad1 (tests/incorrect_equality.sol#37-40) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(address(this)).balance)\nTestContractBalance.bad2 (tests/incorrect_equality.sol#42-45) uses a dangerous strict equality:\n\t- require(bool)(address(this).balance == 10000000000000000000)\nTestContractBalance.bad3 (tests/incorrect_equality.sol#47-50) uses a dangerous strict equality:\n\t- require(bool)(10000000000000000000 == address(this).balance)\nTestContractBalance.bad4 (tests/incorrect_equality.sol#52-57) uses a dangerous strict equality:\n\t- balance == 10000000000000000000\nTestContractBalance.bad5 (tests/incorrect_equality.sol#59-64) uses a dangerous strict equality:\n\t- 10000000000000000000 == balance\nTestContractBalance.bad6 (tests/incorrect_equality.sol#66-71) uses a dangerous strict equality:\n\t- balance == 10000000000000000000\n", "elements": [{"type": "function", "name": "bad6", "source_mapping": {"start": 1538, "length": 179, "filename": "tests/incorrect_equality.sol", "lines": [66, 67, 68, 69, 70, 71]}, "contract": {"type": "contract", "name": "TestContractBalance", "source_mapping": {"start": 612, "length": 1754, "filename": "tests/incorrect_equality.sol", "lines": [30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97]}}}, {"type": "expression", "expression": "balance == 10000000000000000000", "source_mapping": {"start": 1631, "length": 80, "filename": "tests/incorrect_equality.sol", "lines": [68, 69, 70]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestSolidityKeyword.bad0 (tests/incorrect_equality.sol#123-125) uses a dangerous strict equality:\n\t- require(bool)(now == 0)\n", "elements": [{"type": "function", "name": "bad0", "source_mapping": {"start": 2935, "length": 59, "filename": "tests/incorrect_equality.sol", "lines": [123, 124, 125]}, "contract": {"type": "contract", "name": "TestSolidityKeyword", "source_mapping": {"start": 2368, "length": 774, "filename": "tests/incorrect_equality.sol", "lines": [99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135]}}}, {"type": "expression", "expression": "require(bool)(now == 0)", "source_mapping": {"start": 2969, "length": 18, "filename": "tests/incorrect_equality.sol", "lines": [124]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestSolidityKeyword.bad0 (tests/incorrect_equality.sol#123-125) uses a dangerous strict equality:\n\t- require(bool)(now == 0)\nTestSolidityKeyword.bad1 (tests/incorrect_equality.sol#127-129) uses a dangerous strict equality:\n\t- require(bool)(block.number == 0)\n", "elements": [{"type": "function", "name": "bad1", "source_mapping": {"start": 3000, "length": 66, "filename": "tests/incorrect_equality.sol", "lines": [127, 128, 129]}, "contract": {"type": "contract", "name": "TestSolidityKeyword", "source_mapping": {"start": 2368, "length": 774, "filename": "tests/incorrect_equality.sol", "lines": [99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135]}}}, {"type": "expression", "expression": "require(bool)(block.number == 0)", "source_mapping": {"start": 3034, "length": 25, "filename": "tests/incorrect_equality.sol", "lines": [128]}}]}, {"check": "incorrect-equality", "impact": "Medium", "confidence": "High", "description": "TestSolidityKeyword.bad0 (tests/incorrect_equality.sol#123-125) uses a dangerous strict equality:\n\t- require(bool)(now == 0)\nTestSolidityKeyword.bad1 (tests/incorrect_equality.sol#127-129) uses a dangerous strict equality:\n\t- require(bool)(block.number == 0)\nTestSolidityKeyword.bad2 (tests/incorrect_equality.sol#131-133) uses a dangerous strict equality:\n\t- require(bool)(block.number == 0)\n", "elements": [{"type": "function", "name": "bad2", "source_mapping": {"start": 3072, "length": 67, "filename": "tests/incorrect_equality.sol", "lines": [131, 132, 133]}, "contract": {"type": "contract", "name": "TestSolidityKeyword", "source_mapping": {"start": 2368, "length": 774, "filename": "tests/incorrect_equality.sol", "lines": [99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135]}}}, {"type": "expression", "expression": "require(bool)(block.number == 0)", "source_mapping": {"start": 3106, "length": 26, "filename": "tests/incorrect_equality.sol", "lines": [132]}}]}] \ No newline at end of file diff --git a/tests/expected_json/shadowing_builtin_symbols.shadowing-builtin.json b/tests/expected_json/shadowing_builtin_symbols.shadowing-builtin.json new file mode 100644 index 000000000..ff59e0c14 --- /dev/null +++ b/tests/expected_json/shadowing_builtin_symbols.shadowing-builtin.json @@ -0,0 +1 @@ +[{"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "BaseContract.blockhash (state variable @ tests/shadowing_builtin_symbols.sol#4) shadows built-in symbol \"blockhash\"\n", "elements": [{"type": "variable", "name": "blockhash", "source_mapping": {"start": 54, "length": 14, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [4]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "BaseContract.now (state variable @ tests/shadowing_builtin_symbols.sol#5) shadows built-in symbol \"now\"\n", "elements": [{"type": "variable", "name": "now", "source_mapping": {"start": 74, "length": 8, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [5]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "BaseContract.revert (event @ tests/shadowing_builtin_symbols.sol#7) shadows built-in symbol \"revert\"\n", "elements": [{"type": "function", "name": "revert", "source_mapping": {"start": 89, "length": 29, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [7]}, "contract": {"type": "contract", "name": "BaseContract", "source_mapping": {"start": 26, "length": 94, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [3, 4, 5, 6, 7, 8]}}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "ExtendedContract.assert (function @ tests/shadowing_builtin_symbols.sol#13-15) shadows built-in symbol \"assert\"\n", "elements": [{"type": "function", "name": "assert", "source_mapping": {"start": 195, "length": 64, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [13, 14, 15]}, "contract": {"type": "contract", "name": "ExtendedContract", "source_mapping": {"start": 122, "length": 139, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [10, 11, 12, 13, 14, 15, 16]}}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "ExtendedContract.assert.msg (local variable @ tests/shadowing_builtin_symbols.sol#14) shadows built-in symbol \"msg\"\n", "elements": [{"type": "variable", "name": "msg", "source_mapping": {"start": 244, "length": 8, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [14]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "ExtendedContract.ecrecover (state variable @ tests/shadowing_builtin_symbols.sol#11) shadows built-in symbol \"ecrecover\"\n", "elements": [{"type": "variable", "name": "ecrecover", "source_mapping": {"start": 170, "length": 18, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [11]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.require (modifier @ tests/shadowing_builtin_symbols.sol#23-28) shadows built-in symbol \"require\"\n", "elements": [{"type": "function", "name": "require", "source_mapping": {"start": 380, "length": 120, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [23, 24, 25, 26, 27, 28]}, "contract": {"type": "contract", "name": "FurtherExtendedContract", "source_mapping": {"start": 263, "length": 239, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29]}}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.require.keccak256 (local variable @ tests/shadowing_builtin_symbols.sol#25) shadows built-in symbol \"keccak256\"\n", "elements": [{"type": "variable", "name": "keccak256", "source_mapping": {"start": 449, "length": 14, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [25]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.require.sha3 (local variable @ tests/shadowing_builtin_symbols.sol#26) shadows built-in symbol \"sha3\"\n", "elements": [{"type": "variable", "name": "sha3", "source_mapping": {"start": 473, "length": 9, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [26]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.blockhash (state variable @ tests/shadowing_builtin_symbols.sol#19) shadows built-in symbol \"blockhash\"\n", "elements": [{"type": "variable", "name": "blockhash", "source_mapping": {"start": 322, "length": 18, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [19]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.this (state variable @ tests/shadowing_builtin_symbols.sol#20) shadows built-in symbol \"this\"\n", "elements": [{"type": "variable", "name": "this", "source_mapping": {"start": 346, "length": 13, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [20]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.abi (state variable @ tests/shadowing_builtin_symbols.sol#21) shadows built-in symbol \"abi\"\n", "elements": [{"type": "variable", "name": "abi", "source_mapping": {"start": 365, "length": 8, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [21]}}]}, {"check": "shadowing-builtin", "impact": "Low", "confidence": "High", "description": "Reserved.mutable (state variable @ tests/shadowing_builtin_symbols.sol#32) shadows built-in symbol \"mutable\"\n", "elements": [{"type": "variable", "name": "mutable", "source_mapping": {"start": 527, "length": 15, "filename": "tests/shadowing_builtin_symbols.sol", "lines": [32]}}]}] \ No newline at end of file diff --git a/tests/expected_json/shadowing_local_variable.shadowing-local.json b/tests/expected_json/shadowing_local_variable.shadowing-local.json new file mode 100644 index 000000000..d0cf0451f --- /dev/null +++ b/tests/expected_json/shadowing_local_variable.shadowing-local.json @@ -0,0 +1 @@ +[{"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.x (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- FurtherExtendedContract.x (state variable @ tests/shadowing_local_variable.sol#17)\n\t- ExtendedContract.x (state variable @ tests/shadowing_local_variable.sol#9)\n\t- BaseContract.x (state variable @ tests/shadowing_local_variable.sol#4)\n", "elements": [{"type": "variable", "name": "x", "source_mapping": {"start": 376, "length": 6, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "variable", "name": "x", "source_mapping": {"start": 256, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [17]}}, {"type": "variable", "name": "x", "source_mapping": {"start": 133, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [9]}}, {"type": "variable", "name": "x", "source_mapping": {"start": 54, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [4]}}]}, {"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.y (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- BaseContract.y (state variable @ tests/shadowing_local_variable.sol#5)\n", "elements": [{"type": "variable", "name": "y", "source_mapping": {"start": 398, "length": 5, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "variable", "name": "y", "source_mapping": {"start": 70, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [5]}}]}, {"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.z (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- ExtendedContract.z (function @ tests/shadowing_local_variable.sol#11)\n", "elements": [{"type": "variable", "name": "z", "source_mapping": {"start": 405, "length": 6, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "function", "name": "z", "source_mapping": {"start": 150, "length": 27, "filename": "tests/shadowing_local_variable.sol", "lines": [11]}, "contract": {"type": "contract", "name": "ExtendedContract", "source_mapping": {"start": 85, "length": 110, "filename": "tests/shadowing_local_variable.sol", "lines": [8, 9, 10, 11, 12, 13, 14]}}}]}, {"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.w (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- FurtherExtendedContract.w (modifier @ tests/shadowing_local_variable.sol#20-23)\n", "elements": [{"type": "variable", "name": "w", "source_mapping": {"start": 413, "length": 6, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "function", "name": "w", "source_mapping": {"start": 274, "length": 71, "filename": "tests/shadowing_local_variable.sol", "lines": [20, 21, 22, 23]}, "contract": {"type": "contract", "name": "FurtherExtendedContract", "source_mapping": {"start": 197, "length": 235, "filename": "tests/shadowing_local_variable.sol", "lines": [16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26]}}}]}, {"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.v (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- ExtendedContract.v (event @ tests/shadowing_local_variable.sol#13)\n", "elements": [{"type": "variable", "name": "v", "source_mapping": {"start": 421, "length": 6, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "function", "name": "v", "source_mapping": {"start": 183, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [13]}, "contract": {"type": "contract", "name": "ExtendedContract", "source_mapping": {"start": 85, "length": 110, "filename": "tests/shadowing_local_variable.sol", "lines": [8, 9, 10, 11, 12, 13, 14]}}}]}] \ No newline at end of file diff --git a/tests/incorrect_equality.sol b/tests/incorrect_equality.sol new file mode 100644 index 000000000..d37d0663d --- /dev/null +++ b/tests/incorrect_equality.sol @@ -0,0 +1,136 @@ +contract ERC20Function{ + function balanceOf(address _owner) external returns(uint); +} + +contract ERC20Variable{ + mapping(address => uint) public balanceOf; +} + + +contract ERC20TestBalance{ + + + function good0(ERC20Function erc) external{ + require(erc.balanceOf(msg.sender) > 0); + } + + function good1(ERC20Variable erc) external{ + require(erc.balanceOf(msg.sender) > 0); + } + + function bad0(ERC20Function erc) external{ + require(erc.balanceOf(address(this)) == 10); + } + + function bad1(ERC20Variable erc) external{ + require(erc.balanceOf(msg.sender) ==10); + } +} + +contract TestContractBalance { + + function bad0() external { + require(address(address(this)).balance == 10 ether); + msg.sender.transfer(0.1 ether); + } + + function bad1() external { + require(10 ether == address(address(this)).balance); + msg.sender.transfer(0.1 ether); + } + + function bad2() external { + require(address(this).balance == 10 ether); + msg.sender.transfer(0.1 ether); + } + + function bad3() external { + require(10 ether == address(this).balance); + msg.sender.transfer(0.1 ether); + } + + function bad4() external { + uint256 balance = address(this).balance; + if (balance == 10 ether) { + msg.sender.transfer(0.1 ether); + } + } + + function bad5() external { + uint256 balance = address(this).balance; + if (10 ether == balance) { + msg.sender.transfer(0.1 ether); + } + } + + function bad6() external { + uint256 balance = address(address(this)).balance; + if (balance == 10 ether) { + msg.sender.transfer(0.1 ether); + } + } + + function myfunc(uint256 balance) pure internal returns (uint256) { + return balance - balance; + } + + function good1() external { + require (address(address(this)).balance >= 10 ether); + msg.sender.transfer(0.1 ether); + } + + function good2() external { + require (10 <= address(address(this)).balance); + msg.sender.transfer(0.1 ether); + } + + function good3() external { + require (address(this).balance >= 10 ether); + msg.sender.transfer(0.1 ether); + } + + function good4() external { + require (10 <= address(this).balance); + msg.sender.transfer(0.1 ether); + } + +} + +contract TestSolidityKeyword{ + + function good0() external{ + require(now > 0); + } + + function good1() external{ + require(block.number > 0); + } + + function good2() external{ + require(block.timestamp > 0); + } + + function good3(uint param) public{ + // address(this) simulate a particular corner case + // where the SSA is better + // the naive data dependency without SSA + // will consider param and block.number to have a dep + if(param == 0){ + param = block.number; + } + } + + function bad0() external{ + require(now == 0); + } + + function bad1() external{ + require(block.number== 0); + } + + function bad2() external{ + require(block.number == 0); + } + +} + diff --git a/tests/shadowing_builtin_symbols.sol b/tests/shadowing_builtin_symbols.sol new file mode 100644 index 000000000..303f8deba --- /dev/null +++ b/tests/shadowing_builtin_symbols.sol @@ -0,0 +1,34 @@ +pragma solidity ^0.4.25; + +contract BaseContract { + uint blockhash; + uint now; + + event revert(bool condition); +} + +contract ExtendedContract is BaseContract { + uint ecrecover = 7; + + function assert(bool condition) public { + uint msg; + } +} + +contract FurtherExtendedContract is ExtendedContract { + uint blockhash = 7; + uint this = 5; + uint abi; + + modifier require { + assert(msg.sender != address(0)); + uint keccak256; + uint sha3; + _; + } +} + +contract Reserved{ + address mutable; + +} diff --git a/tests/shadowing_local_variable.sol b/tests/shadowing_local_variable.sol new file mode 100644 index 000000000..b67833f05 --- /dev/null +++ b/tests/shadowing_local_variable.sol @@ -0,0 +1,26 @@ +pragma solidity ^0.4.24; + +contract BaseContract { + uint x = 5; + uint y = 5; +} + +contract ExtendedContract is BaseContract { + uint x = 7; + + function z() public pure {} + + event v(); +} + +contract FurtherExtendedContract is ExtendedContract { + uint x = 7; + + + modifier w { + assert(msg.sender != address(0)); + _; + } + + function shadowingParent(uint x) public pure { int y; uint z; uint w; uint v; } +}