From b3b718101d3946079beb029e32d9b5764414c726 Mon Sep 17 00:00:00 2001 From: rajeevgopalakrishna Date: Mon, 18 Feb 2019 15:21:43 +0530 Subject: [PATCH 1/4] Fixed minor typos in comments --- slither/detectors/attributes/const_functions.py | 4 ++-- slither/detectors/attributes/incorrect_solc.py | 2 +- slither/detectors/attributes/locked_ether.py | 4 ++-- slither/detectors/functions/suicidal.py | 4 ++-- slither/detectors/operations/block_timestamp.py | 9 +-------- slither/detectors/operations/unused_return_values.py | 6 +++--- 6 files changed, 11 insertions(+), 18 deletions(-) diff --git a/slither/detectors/attributes/const_functions.py b/slither/detectors/attributes/const_functions.py index 6d69ca207..954d11fa6 100644 --- a/slither/detectors/attributes/const_functions.py +++ b/slither/detectors/attributes/const_functions.py @@ -37,9 +37,9 @@ contract Constant{ } ``` `Constant` was deployed with Solidity 0.4.25. Bob writes a smart contract interacting with `Constant` in Solidity 0.5.0. -All the calls to `get` reverts, breaking Bob's smart contract execution.''' +All the calls to `get` revert, breaking Bob's smart contract execution.''' - WIKI_RECOMMENDATION = 'Ensure that the attributes of contracts compiled prior Solidity 0.5.0 are correct.' + WIKI_RECOMMENDATION = 'Ensure that the attributes of contracts compiled prior to Solidity 0.5.0 are correct.' def _detect(self): """ Detect the constant function changing the state diff --git a/slither/detectors/attributes/incorrect_solc.py b/slither/detectors/attributes/incorrect_solc.py index 03a84db77..c5d26ed9f 100644 --- a/slither/detectors/attributes/incorrect_solc.py +++ b/slither/detectors/attributes/incorrect_solc.py @@ -27,7 +27,7 @@ class IncorrectSolc(AbstractDetector): WIKI_TITLE = 'Incorrect versions of Solidity' WIKI_DESCRIPTION = ''' -Solc frequently releases new compiler versions. Using an old version prevent access to new Solidity security checks. +Solc frequently releases new compiler versions. Using an old version prevents access to new Solidity security checks. We recommend avoiding complex pragma statement.''' WIKI_RECOMMENDATION = 'Use Solidity 0.4.25 or 0.5.2.' diff --git a/slither/detectors/attributes/locked_ether.py b/slither/detectors/attributes/locked_ether.py index b34d39328..963b47c04 100644 --- a/slither/detectors/attributes/locked_ether.py +++ b/slither/detectors/attributes/locked_ether.py @@ -1,5 +1,5 @@ """ - Check if ether are locked in the contract + Check if ethers are locked in the contract """ from slither.detectors.abstract_detector import (AbstractDetector, @@ -30,7 +30,7 @@ contract Locked{ } } ``` -Every ethers send to `Locked` will be lost.''' +Every ether sent to `Locked` will be lost.''' WIKI_RECOMMENDATION = 'Remove the payable attribute or add a withdraw function.' diff --git a/slither/detectors/functions/suicidal.py b/slither/detectors/functions/suicidal.py index b1a6aebed..6381d119b 100644 --- a/slither/detectors/functions/suicidal.py +++ b/slither/detectors/functions/suicidal.py @@ -25,11 +25,11 @@ class Suicidal(AbstractDetector): ```solidity contract Suicidal{ function kill() public{ - selfdestruct(msg.value); + selfdestruct(msg.sender); } } ``` -Bob calls `kill` and destruct the contract.''' +Bob calls `kill` and destructs the contract.''' WIKI_RECOMMENDATION = 'Protect access to all sensitive functions.' diff --git a/slither/detectors/operations/block_timestamp.py b/slither/detectors/operations/block_timestamp.py index e31fe8ce2..86d3a2d92 100644 --- a/slither/detectors/operations/block_timestamp.py +++ b/slither/detectors/operations/block_timestamp.py @@ -1,13 +1,6 @@ """ - Module detecting send to arbitrary address + Module detecting dangerous use of block.timestamp - To avoid FP, it does not report: - - If msg.sender is used as index (withdraw situation) - - If the function is protected - - If the value sent is msg.value (repay situation) - - If there is a call to transferFrom - - TODO: dont report if the value is tainted by msg.value """ from slither.core.declarations import Function from slither.analyses.data_dependency.data_dependency import is_tainted, is_dependent diff --git a/slither/detectors/operations/unused_return_values.py b/slither/detectors/operations/unused_return_values.py index eee1d39c6..8203fabe6 100644 --- a/slither/detectors/operations/unused_return_values.py +++ b/slither/detectors/operations/unused_return_values.py @@ -31,9 +31,9 @@ contract MyConc{ } } ``` -`MyConc` call `add` of safemath, but does not store the result in `a`. As a result, the computation has no effect.''' +`MyConc` calls `add` of SafeMath, but does not store the result in `a`. As a result, the computation has no effect.''' - WIKI_RECOMMENDATION = 'Ensure that all the return value of the function call are stored in a local or state variable.' + WIKI_RECOMMENDATION = 'Ensure that all the return values of the function calls are stored in a local or state variable.' def detect_unused_return_values(self, f): """ @@ -59,7 +59,7 @@ contract MyConc{ return [nodes_origin[value].node for value in values_returned] def _detect(self): - """ Detect unused high level calls that return a value but are never used + """ Detect high level calls which return a value that are never used """ results = [] for c in self.slither.contracts: From 8344c4edf371a87e50e821e1b25e419a7d4fc09c Mon Sep 17 00:00:00 2001 From: rajeevgopalakrishna Date: Mon, 18 Feb 2019 15:24:23 +0530 Subject: [PATCH 2/4] Fixed a typo in detector where *callcode* was specified as *codecall*. Added a test to verify the fix. --- slither/detectors/statements/controlled_delegatecall.py | 4 ++-- tests/controlled_delegatecall.sol | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/slither/detectors/statements/controlled_delegatecall.py b/slither/detectors/statements/controlled_delegatecall.py index 276214121..60b891932 100644 --- a/slither/detectors/statements/controlled_delegatecall.py +++ b/slither/detectors/statements/controlled_delegatecall.py @@ -24,7 +24,7 @@ contract Delegatecall{ } } ``` -Bob calls `delegate` and delegate the execution to its malicious contract. As a result, Bob withdraws the funds of the contract and destruct it.''' +Bob calls `delegate` and delegates the execution to its malicious contract. As a result, Bob withdraws the funds of the contract and destructs it.''' WIKI_RECOMMENDATION = 'Avoid using `delegatecall`. Use only trusted destinations.' @@ -32,7 +32,7 @@ Bob calls `delegate` and delegate the execution to its malicious contract. As a ret = [] for node in function.nodes: for ir in node.irs: - if isinstance(ir, LowLevelCall) and ir.function_name in ['delegatecall', 'codecall']: + if isinstance(ir, LowLevelCall) and ir.function_name in ['delegatecall', 'callcode']: if is_tainted(ir.destination, function.contract): ret.append(node) return ret diff --git a/tests/controlled_delegatecall.sol b/tests/controlled_delegatecall.sol index d3d3a78dd..411fa50cb 100644 --- a/tests/controlled_delegatecall.sol +++ b/tests/controlled_delegatecall.sol @@ -5,6 +5,10 @@ contract C{ bytes4 func_id; + function bad_callcode_call(bytes memory data) public{ + addr_bad.callcode(data); + } + function bad_delegate_call(bytes memory data) public{ addr_good.delegatecall(data); addr_bad.delegatecall(data); From fe997e703013bb7292def05a83e94a2903608fdf Mon Sep 17 00:00:00 2001 From: rajeevgopalakrishna Date: Tue, 19 Feb 2019 11:09:37 +0530 Subject: [PATCH 3/4] Fixed minor typos in comments --- slither/detectors/erc20/incorrect_interface.py | 3 +-- slither/detectors/erc20/unindexed_event_parameters.py | 2 +- slither/detectors/statements/deprecated_calls.py | 4 ++-- slither/detectors/statements/incorrect_strict_equality.py | 2 +- slither/detectors/statements/tx_origin.py | 6 +++--- .../detectors/variables/uninitialized_local_variables.py | 8 ++++---- .../variables/uninitialized_storage_variables.py | 8 ++++---- 7 files changed, 16 insertions(+), 17 deletions(-) diff --git a/slither/detectors/erc20/incorrect_interface.py b/slither/detectors/erc20/incorrect_interface.py index 0da0df1db..9cba6b21b 100644 --- a/slither/detectors/erc20/incorrect_interface.py +++ b/slither/detectors/erc20/incorrect_interface.py @@ -1,7 +1,6 @@ """ 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) +Some contracts do not return a bool on transfer/transferFrom/approve, which may lead to preventing the contract to be used with contracts compiled with recent solc (>0.4.22) """ from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification diff --git a/slither/detectors/erc20/unindexed_event_parameters.py b/slither/detectors/erc20/unindexed_event_parameters.py index 55f2916ba..3c9d87895 100644 --- a/slither/detectors/erc20/unindexed_event_parameters.py +++ b/slither/detectors/erc20/unindexed_event_parameters.py @@ -17,7 +17,7 @@ class UnindexedERC20EventParameters(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Detectors-Documentation#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_DESCRIPTION = 'Detects that events defined by the ERC20 specification which are meant to have some parameters as `indexed`, are missing the `indexed` keyword.' WIKI_EXPLOIT_SCENARIO = ''' ```solidity contract ERC20Bad { diff --git a/slither/detectors/statements/deprecated_calls.py b/slither/detectors/statements/deprecated_calls.py index 8138e69d0..56522f307 100644 --- a/slither/detectors/statements/deprecated_calls.py +++ b/slither/detectors/statements/deprecated_calls.py @@ -144,7 +144,7 @@ contract ContractWithDeprecatedReferences { return results def _detect(self): - """ Detect shadowing local variables + """ Detects if an expression makes use of any deprecated standards. Recursively visit the calls Returns: @@ -165,7 +165,7 @@ contract ContractWithDeprecatedReferences { recommended_disc) - # Generate relevant JSON data for this shadowing definition. + # Generate relevant JSON data for this deprecated standard. json = self.generate_json_result(info) if isinstance(source_object, StateVariableSolc) or isinstance(source_object, StateVariable): self.add_variable_to_json(source_object, json) diff --git a/slither/detectors/statements/incorrect_strict_equality.py b/slither/detectors/statements/incorrect_strict_equality.py index 18fafc00f..232b0b86b 100644 --- a/slither/detectors/statements/incorrect_strict_equality.py +++ b/slither/detectors/statements/incorrect_strict_equality.py @@ -26,7 +26,7 @@ class IncorrectStrictEquality(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Detectors-Documentation#dangerous-strict-equalities' WIKI_TITLE = 'Dangerous strict equalities' - WIKI_DESCRIPTION = 'Use of strick equalities that can be easily manipulated by an attacker.' + WIKI_DESCRIPTION = 'Use of strict equalities that can be easily manipulated by an attacker.' WIKI_EXPLOIT_SCENARIO = ''' ```solidity contract Crowdsale{ diff --git a/slither/detectors/statements/tx_origin.py b/slither/detectors/statements/tx_origin.py index eeb482e5d..8fd6684a6 100644 --- a/slither/detectors/statements/tx_origin.py +++ b/slither/detectors/statements/tx_origin.py @@ -27,14 +27,14 @@ contract TxOrigin { require(tx.origin == owner); } ``` -Bob is the owner of `TxOrigin`. Bob calls Eve's contract. Eve's contact calls `TxOrigin` and bypass the `tx.origin` protection.''' +Bob is the owner of `TxOrigin`. Bob calls Eve's contract. Eve's contract calls `TxOrigin` and bypasses the `tx.origin` protection.''' - WIKI_RECOMMENDATION = 'Do not use `tx.origin` for authentification.' + WIKI_RECOMMENDATION = 'Do not use `tx.origin` for authorization.' @staticmethod def _contains_incorrect_tx_origin_use(node): """ - Check if the node read tx.origin and dont read msg.sender + Check if the node reads tx.origin and doesn't read msg.sender Avoid the FP due to (msg.sender == tx.origin) Returns: (bool) diff --git a/slither/detectors/variables/uninitialized_local_variables.py b/slither/detectors/variables/uninitialized_local_variables.py index f731567fb..03da48215 100644 --- a/slither/detectors/variables/uninitialized_local_variables.py +++ b/slither/detectors/variables/uninitialized_local_variables.py @@ -1,8 +1,8 @@ """ - Module detecting state uninitialized local variables + Module detecting uninitialized local variables Recursively explore the CFG to only report uninitialized local variables that are - written before being read + read before being written """ from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification @@ -77,11 +77,11 @@ Bob calls `transfer`. As a result, the ethers are sent to the address 0x0 and ar def _detect(self): - """ Detect uninitialized state variables + """ Detect uninitialized local variables Recursively visit the calls Returns: - dict: [contract name] = set(state variable uninitialized) + dict: [contract name] = set(local variable uninitialized) """ results = [] diff --git a/slither/detectors/variables/uninitialized_storage_variables.py b/slither/detectors/variables/uninitialized_storage_variables.py index 1a36ca403..d4b85b9a3 100644 --- a/slither/detectors/variables/uninitialized_storage_variables.py +++ b/slither/detectors/variables/uninitialized_storage_variables.py @@ -1,5 +1,5 @@ """ - Module detecting state uninitialized storage variables + Module detecting uninitialized storage variables Recursively explore the CFG to only report uninitialized storage variables that are written before being read @@ -58,7 +58,7 @@ Bob calls `func`. As a result, `owner` is override to 0. if self.key in father.context: fathers_context += father.context[self.key] - # Exclude path that dont bring further information + # Exclude paths that dont bring further information if node in self.visited_all_paths: if all(f_c in self.visited_all_paths[node] for f_c in fathers_context): return @@ -84,11 +84,11 @@ Bob calls `func`. As a result, `owner` is override to 0. def _detect(self): - """ Detect uninitialized state variables + """ Detect uninitialized storage variables Recursively visit the calls Returns: - dict: [contract name] = set(state variable uninitialized) + dict: [contract name] = set(storage variable uninitialized) """ results = [] From 39500c092e512c14c462b7e541cd887548e7b59e Mon Sep 17 00:00:00 2001 From: rajeevgopalakrishna Date: Thu, 21 Feb 2019 09:55:52 +0530 Subject: [PATCH 4/4] Iterate over contracts_derived instead of contracts to prevent duplicate results --- slither/detectors/statements/calls_in_loop.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/detectors/statements/calls_in_loop.py b/slither/detectors/statements/calls_in_loop.py index 82d9cdd71..4a15691b1 100644 --- a/slither/detectors/statements/calls_in_loop.py +++ b/slither/detectors/statements/calls_in_loop.py @@ -82,7 +82,7 @@ If one of the destinations has a fallback function which reverts, `bad` will alw """ """ results = [] - for c in self.contracts: + for c in self.slither.contracts_derived: values = self.detect_call_in_loop(c) for node in values: func = node.function