From ec27576a770294995aa229be63f1dd4f984e1b7e Mon Sep 17 00:00:00 2001 From: Josselin Date: Thu, 31 Jan 2019 10:52:08 +0000 Subject: [PATCH] Add wiki information to all detectors --- slither/detectors/abstract_detector.py | 18 +++++++++++ .../detectors/attributes/const_functions.py | 24 ++++++++++++++ .../detectors/attributes/constant_pragma.py | 5 +++ .../detectors/attributes/incorrect_solc.py | 6 ++++ slither/detectors/attributes/locked_ether.py | 15 +++++++++ slither/detectors/examples/backdoor.py | 6 ++++ slither/detectors/functions/arbitrary_send.py | 19 ++++++++++++ .../detectors/functions/external_function.py | 5 +++ slither/detectors/functions/suicidal.py | 15 +++++++++ .../naming_convention/naming_convention.py | 10 ++++++ .../detectors/operations/block_timestamp.py | 6 ++++ .../detectors/operations/low_level_calls.py | 4 +++ .../operations/unused_return_values.py | 16 ++++++++++ .../detectors/reentrancy/reentrancy_benign.py | 18 +++++++++++ .../detectors/reentrancy/reentrancy_eth.py | 22 +++++++++++++ .../reentrancy_read_before_write.py | 18 +++++++++++ slither/detectors/shadowing/abstract.py | 18 ++++++++++- .../detectors/shadowing/builtin_symbols.py | 23 +++++++++++++- slither/detectors/shadowing/local.py | 26 +++++++++++++++- slither/detectors/shadowing/state.py | 31 ++++++++++++++++++- slither/detectors/statements/assembly.py | 5 +++ slither/detectors/statements/calls_in_loop.py | 25 +++++++++++++++ .../statements/controlled_delegatecall.py | 15 +++++++++ .../statements/incorrect_strict_equality.py | 13 ++++++++ slither/detectors/statements/tx_origin.py | 15 +++++++++ .../possible_const_state_variables.py | 5 +++ .../uninitialized_local_variables.py | 16 ++++++++++ .../uninitialized_state_variables.py | 18 +++++++++++ .../uninitialized_storage_variables.py | 22 +++++++++++++ .../variables/unused_state_variables.py | 6 ++++ 30 files changed, 441 insertions(+), 4 deletions(-) diff --git a/slither/detectors/abstract_detector.py b/slither/detectors/abstract_detector.py index 91a117401..8c07ab8fc 100644 --- a/slither/detectors/abstract_detector.py +++ b/slither/detectors/abstract_detector.py @@ -38,6 +38,12 @@ class AbstractDetector(metaclass=abc.ABCMeta): WIKI = '' + WIKI_TITLE = '' + WIKI_DESCRIPTION = '' + WIKI_EXPLOIT_SCENARIO = '' + WIKI_RECOMMENDATION = '' + + def __init__(self, slither, logger): self.slither = slither self.contracts = slither.contracts @@ -50,6 +56,18 @@ class AbstractDetector(metaclass=abc.ABCMeta): if not self.ARGUMENT: raise IncorrectDetectorInitialization('ARGUMENT is not initialized {}'.format(self.__class__.__name__)) + if not self.WIKI_TITLE: + raise IncorrectDetectorInitialization('WIKI_TITLE is not initialized {}'.format(self.__class__.__name__)) + + if not self.WIKI_DESCRIPTION: + raise IncorrectDetectorInitialization('WIKI_DESCRIPTION is not initialized {}'.format(self.__class__.__name__)) + + if not self.WIKI_EXPLOIT_SCENARIO and self.IMPACT != DetectorClassification.INFORMATIONAL: + raise IncorrectDetectorInitialization('WIKI_EXPLOIT_SCENARIO is not initialized {}'.format(self.__class__.__name__)) + + if not self.WIKI_RECOMMENDATION: + raise IncorrectDetectorInitialization('WIKI_RECOMMENDATION is not initialized {}'.format(self.__class__.__name__)) + if re.match('^[a-zA-Z0-9_-]*$', self.ARGUMENT) is None: raise IncorrectDetectorInitialization('ARGUMENT has illegal character {}'.format(self.__class__.__name__)) diff --git a/slither/detectors/attributes/const_functions.py b/slither/detectors/attributes/const_functions.py index 4e9968a17..26ac42a20 100644 --- a/slither/detectors/attributes/const_functions.py +++ b/slither/detectors/attributes/const_functions.py @@ -17,6 +17,30 @@ class ConstantFunctions(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#constant-functions-changing-the-state' + WIKI_TITLE = 'Constant functions changing the state' + WIKI_DESCRIPTION = ''' +Functions declared as `constant`/`pure`/`view` changing the state or using assembly code. + +`constant`/`pure`/`view` was not enforced prior Solidity 0.5. +Starting from Solidity 0.5, a call to a `constant`/`pure`/`view` function uses the `STATICCALL` opcode, which reverts in case of state modification. + +As a result, a call to an [incorrectly labeled function may trap a contract compiled with Solidity 0.5](https://solidity.readthedocs.io/en/develop/050-breaking-changes.html#interoperability-with-older-contracts).''' + + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Constant{ + uint counter; + function get() public view returns(uint){ + counter = counter +1; + return counter + } +} +``` +`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.''' + + WIKI_RECOMMENDATION = 'Ensure that the attributes of contracts compiled prior Solidity 0.5.0 are correct.' + def detect(self): """ Detect the constant function changing the state diff --git a/slither/detectors/attributes/constant_pragma.py b/slither/detectors/attributes/constant_pragma.py index f41dcdc7b..eaa62e83d 100644 --- a/slither/detectors/attributes/constant_pragma.py +++ b/slither/detectors/attributes/constant_pragma.py @@ -17,6 +17,11 @@ class ConstantPragma(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#different-pragma-directives-are-used' + + WIKI_TITLE = 'Different pragma directives are used' + WIKI_DESCRIPTION = 'Detect if different Solidity versions are used.' + WIKI_RECOMMENDATION = 'Use one Solidity version.' + def detect(self): results = [] pragma = self.slither.pragma_directives diff --git a/slither/detectors/attributes/incorrect_solc.py b/slither/detectors/attributes/incorrect_solc.py index 9226a16cc..f10929958 100644 --- a/slither/detectors/attributes/incorrect_solc.py +++ b/slither/detectors/attributes/incorrect_solc.py @@ -25,6 +25,12 @@ class IncorrectSolc(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#incorrect-version-of-solidity' + 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. +We recommend avoiding complex pragma statement.''' + WIKI_RECOMMENDATION = 'Use Solidity 0.4.25 or 0.5.2.' + COMPLEX_PRAGMA = "is has a complex pragma" OLD_VERSION = "it allows old versions" LESS_THAN = "it uses lesser than" diff --git a/slither/detectors/attributes/locked_ether.py b/slither/detectors/attributes/locked_ether.py index a2fe6a1c3..affa05fc1 100644 --- a/slither/detectors/attributes/locked_ether.py +++ b/slither/detectors/attributes/locked_ether.py @@ -19,6 +19,21 @@ class LockedEther(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#contracts-that-lock-ether' + + WIKI_TITLE = 'Contracts that lock ether' + WIKI_DESCRIPTION = 'Contract with a `payable` function, but without a withdraw capacity.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +pragma solidity 0.4.24; +contract Locked{ + function receive() payable public{ + } +} +``` +Every ethers send to `Locked` will be lost.''' + + WIKI_RECOMMENDATION = 'Remove the payable attribute or add a withdraw function.' + @staticmethod def do_no_send_ether(contract): functions = contract.all_functions_called diff --git a/slither/detectors/examples/backdoor.py b/slither/detectors/examples/backdoor.py index c3070ef95..b6ed7323a 100644 --- a/slither/detectors/examples/backdoor.py +++ b/slither/detectors/examples/backdoor.py @@ -11,6 +11,12 @@ class Backdoor(AbstractDetector): IMPACT = DetectorClassification.HIGH CONFIDENCE = DetectorClassification.HIGH + + WIKI_TITLE = 'Backdoor example' + WIKI_DESCRIPTION = 'Plugin example' + WIKI_EXPLOIT_SCENARIO = '..' + WIKI_RECOMMENDATION = '..' + def detect(self): results = [] diff --git a/slither/detectors/functions/arbitrary_send.py b/slither/detectors/functions/arbitrary_send.py index 67e28fe82..ab1f5e68d 100644 --- a/slither/detectors/functions/arbitrary_send.py +++ b/slither/detectors/functions/arbitrary_send.py @@ -30,6 +30,25 @@ class ArbitrarySend(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations' + WIKI_TITLE = 'Functions that send ether to arbitrary destinations' + WIKI_DESCRIPTION = 'Unprotected call to a function executing sending ethers to an arbitrary address.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract ArbitrarySend{ + address destination; + function setDestination(){ + destination = msg.sender; + } + + function withdraw() public{ + destination.transfer(this.balance); + } +} +``` +Bob calls `setDestination` and `withdraw`. As a result he withdraws the contract's balance.''' + + WIKI_RECOMMENDATION = 'Ensure that an arbitrary user cannot withdraw unauthorize funds.' + def arbitrary_send(self, func): """ """ diff --git a/slither/detectors/functions/external_function.py b/slither/detectors/functions/external_function.py index 3c83f811b..11cafdfad 100644 --- a/slither/detectors/functions/external_function.py +++ b/slither/detectors/functions/external_function.py @@ -18,6 +18,11 @@ class ExternalFunction(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#public-function-that-could-be-declared-as-external' + + WIKI_TITLE = 'Public function that could be declared as external' + WIKI_DESCRIPTION = '`public` functions that are never called by the contract should be declared `external` to save gas.' + WIKI_RECOMMENDATION = 'Use the `external` attribute for functions never called from the contract.' + @staticmethod def detect_functions_called(contract): """ Returns a list of InternallCall, SolidityCall diff --git a/slither/detectors/functions/suicidal.py b/slither/detectors/functions/suicidal.py index f0796e4ea..466cc535c 100644 --- a/slither/detectors/functions/suicidal.py +++ b/slither/detectors/functions/suicidal.py @@ -18,6 +18,21 @@ class Suicidal(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#suicidal' + + WIKI_TITLE = 'Suicidal' + WIKI_DESCRIPTION = 'Unprotected call to a function executing `selfdestruct`/`suicide`.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Suicidal{ + function kill() public{ + selfdestruct(msg.value); + } +} +``` +Bob calls `kill` and destruct the contract.''' + + WIKI_RECOMMENDATION = 'Protect access to all sensitive functions.' + @staticmethod def detect_suicidal_func(func): """ Detect if the function is suicidal diff --git a/slither/detectors/naming_convention/naming_convention.py b/slither/detectors/naming_convention/naming_convention.py index 9abfbcd56..e560b5c20 100644 --- a/slither/detectors/naming_convention/naming_convention.py +++ b/slither/detectors/naming_convention/naming_convention.py @@ -19,6 +19,16 @@ class NamingConvention(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions' + WIKI_TITLE = 'Conformance to Solidity naming conventions' + WIKI_DESCRIPTION = ''' +Solidity defines a [naming convention](https://solidity.readthedocs.io/en/v0.4.25/style-guide.html#naming-conventions) that should be followed. +### Rules exceptions +- Allow constant variables name/symbol/decimals to be lowercase (ERC20) +- Allow `_` at the beginning of the mixed_case match for private variables and unused parameters.''' + + WIKI_RECOMMENDATION = 'Follow the Solidity [naming convention](https://solidity.readthedocs.io/en/v0.4.25/style-guide.html#naming-conventions).' + + @staticmethod def is_cap_words(name): return re.search('^[A-Z]([A-Za-z0-9]+)?_?$', name) is not None diff --git a/slither/detectors/operations/block_timestamp.py b/slither/detectors/operations/block_timestamp.py index 87cee8642..fcb7d7b50 100644 --- a/slither/detectors/operations/block_timestamp.py +++ b/slither/detectors/operations/block_timestamp.py @@ -29,6 +29,12 @@ class Timestamp(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#block-timestamp' + + WIKI_TITLE = 'Block timestamp' + WIKI_DESCRIPTION = 'Dangerous usage of `block.timestamp`. `block.timestamp` can be manipulated by miners.' + WIKI_EXPLOIT_SCENARIO = '''"Bob's contract relies on `block.timestamp` for its randomness. Eve is a miner and manipulates `block.timestamp` to exploit Bob's contract.''' + WIKI_RECOMMENDATION = 'Avoid relying on `block.timestamp`.' + def timestamp(self, func): """ """ diff --git a/slither/detectors/operations/low_level_calls.py b/slither/detectors/operations/low_level_calls.py index 932b0e6d4..57451d46b 100644 --- a/slither/detectors/operations/low_level_calls.py +++ b/slither/detectors/operations/low_level_calls.py @@ -18,6 +18,10 @@ class LowLevelCalls(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls' + WIKI_TITLE = 'Low level calls' + WIKI_DESCRIPTION = 'The use of low-level calls is error-prone. Low-level calls do not check for [code existence](https://solidity.readthedocs.io/en/v0.4.25/control-structures.html#error-handling-assert-require-revert-and-exceptions) or call success.' + WIKI_RECOMMENDATION = 'Avoid low-level calls. Check the call success. If the call is meant for a contract, check for code existence.' + @staticmethod def _contains_low_level_calls(node): """ diff --git a/slither/detectors/operations/unused_return_values.py b/slither/detectors/operations/unused_return_values.py index d9871b7d6..b48921b9e 100644 --- a/slither/detectors/operations/unused_return_values.py +++ b/slither/detectors/operations/unused_return_values.py @@ -19,6 +19,22 @@ class UnusedReturnValues(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return' + + WIKI_TITLE = 'Unused return' + WIKI_DESCRIPTION = 'The return value of an external call is not stored in a local or state variable.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract MyConc{ + using SafeMath for uint; + function my_func(uint a, uint b) public{ + a.add(b); + } +} +``` +`MyConc` call `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.' + def detect_unused_return_values(self, f): """ Return the nodes where the return value of a call is unused diff --git a/slither/detectors/reentrancy/reentrancy_benign.py b/slither/detectors/reentrancy/reentrancy_benign.py index 9744c8991..719512a9f 100644 --- a/slither/detectors/reentrancy/reentrancy_benign.py +++ b/slither/detectors/reentrancy/reentrancy_benign.py @@ -24,6 +24,24 @@ class ReentrancyBenign(Reentrancy): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities-2' + WIKI_TITLE = 'Reentrancy vulnerabilities' + WIKI_DESCRIPTION = ''' +Detection of the [re-entrancy bug](https://github.com/trailofbits/not-so-smart-contracts/tree/master/reentrancy). +Only report reentrancy that acts as a double call (see `reentrancy-eth`, `reentrancy-no-eth`).''' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity + function callme(){ + if( ! (msg.sender.call()() ) ){ + throw; + } + counter += 1 + } +``` + +`callme` contains a reentrancy. The reentrancy is benign because it's exploitation would have the same effect as two consecutive calls.''' + + WIKI_RECOMMENDATION = 'Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy).' + def find_reentrancies(self): result = {} for contract in self.contracts: diff --git a/slither/detectors/reentrancy/reentrancy_eth.py b/slither/detectors/reentrancy/reentrancy_eth.py index 896893fb8..515fb3c46 100644 --- a/slither/detectors/reentrancy/reentrancy_eth.py +++ b/slither/detectors/reentrancy/reentrancy_eth.py @@ -22,6 +22,28 @@ class ReentrancyEth(Reentrancy): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities' + WIKI_TITLE = 'Reentrancy vulnerabilities' + WIKI_DESCRIPTION = ''' +Detection of the [re-entrancy bug](https://github.com/trailofbits/not-so-smart-contracts/tree/master/reentrancy). +Do not report reentrancies that don't involve ethers (see `reentrancy-no-eth`)''' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity + function withdrawBalance(){ + // send userBalance[msg.sender] ethers to msg.sender + // if mgs.sender is a contract, it will call its fallback function + if( ! (msg.sender.call.value(userBalance[msg.sender])() ) ){ + throw; + } + userBalance[msg.sender] = 0; + } +``` + +Bob uses the re-entrancy bug to call `withdrawBalance` two times, and withdraw more than its initial deposit to the contract.''' + + + WIKI_RECOMMENDATION = 'Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy).' + + def find_reentrancies(self): result = {} for contract in self.contracts: diff --git a/slither/detectors/reentrancy/reentrancy_read_before_write.py b/slither/detectors/reentrancy/reentrancy_read_before_write.py index 4879d85eb..92c1fb147 100644 --- a/slither/detectors/reentrancy/reentrancy_read_before_write.py +++ b/slither/detectors/reentrancy/reentrancy_read_before_write.py @@ -25,6 +25,24 @@ class ReentrancyReadBeforeWritten(Reentrancy): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities-1' + WIKI_TITLE = 'Reentrancy vulnerabilities' + WIKI_DESCRIPTION = ''' +Detection of the [re-entrancy bug](https://github.com/trailofbits/not-so-smart-contracts/tree/master/reentrancy). +Do not report reentrancies that involve ethers (see `reentrancy-eth`)''' + + WIKI_EXPLOIT_SCENARIO = ''' +```solidity + function bug(){ + require(not_called); + if( ! (msg.sender.call() ) ){ + throw; + } + not_called = False; + } +``` +''' + WIKI_RECOMMENDATION = 'Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy).' + def find_reentrancies(self): result = {} for contract in self.contracts: diff --git a/slither/detectors/shadowing/abstract.py b/slither/detectors/shadowing/abstract.py index 17ace6249..5115a3f3f 100644 --- a/slither/detectors/shadowing/abstract.py +++ b/slither/detectors/shadowing/abstract.py @@ -18,7 +18,23 @@ class ShadowingAbstractDetection(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variable-shadowing-from-abstract-contracts' - vuln_name = "ShadowingAbstractContract" + + WIKI_TITLE = 'State variable shadowing from abstract contracts' + WIKI_DESCRIPTION = 'Detection of state variables shadowed from abstract contracts.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract BaseContract{ + address owner; +} + +contract DerivedContract is BaseContract{ + address owner; +} +``` +`owner` of `BaseContract` is shadowed in `DerivedContract`.''' + + WIKI_RECOMMENDATION = 'Remove the state variable shadowing.' + def detect_shadowing(self, contract): ret = [] diff --git a/slither/detectors/shadowing/builtin_symbols.py b/slither/detectors/shadowing/builtin_symbols.py index 927a5a810..2cb4c4471 100644 --- a/slither/detectors/shadowing/builtin_symbols.py +++ b/slither/detectors/shadowing/builtin_symbols.py @@ -17,7 +17,28 @@ class BuiltinSymbolShadowing(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#builtin-symbol-shadowing' - vuln_name = 'ShadowingBuiltinSymbols' + + WIKI_TITLE = 'Builtin Symbol Shadowing' + WIKI_DESCRIPTION = 'Detection of shadowing built-in symbols using local variables/state variables/functions/modifiers/events.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +pragma solidity ^0.4.24; + +contract Bug { + uint now; // Overshadows current time stamp. + + function assert(bool condition) public { + // Overshadows built-in symbol for providing assertions. + } + + function get_next_expiration(uint earlier_time) private returns (uint) { + return now + 259200; // References overshadowed timestamp. + } +} +``` +`now` is defined as a state variable, and shadows with the built-in symbol `now`. The function `assert` overshadows the built-in `assert` function. Any use of either of these built-in symbols may lead to unexpected results.''' + + WIKI_RECOMMENDATION = 'Rename the local variable/state variable/function/modifier/event, so as not to mistakenly overshadow any built-in symbol definitions.' SHADOWING_FUNCTION = "function" SHADOWING_MODIFIER = "modifier" diff --git a/slither/detectors/shadowing/local.py b/slither/detectors/shadowing/local.py index 2d9d77931..4dfbb3443 100644 --- a/slither/detectors/shadowing/local.py +++ b/slither/detectors/shadowing/local.py @@ -17,7 +17,31 @@ class LocalShadowing(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#local-variable-shadowing' - vuln_name = 'ShadowingLocalVariable' + WIKI_TITLE = 'Local Variable Shadowing' + WIKI_DESCRIPTION = 'Detection of shadowing using local variables.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +pragma solidity ^0.4.24; + +contract Bug { + uint owner; + + function sensitive_function(address owner) public { + // ... + require(owner == msg.sender); + } + + function alternate_sensitive_function() public { + address owner = msg.sender; + // ... + require(owner == msg.sender); + } +} +``` +`sensitive_function.owner` shadows `Bug.owner`. As a result, the use of `owner` inside `sensitive_function` might be incorrect.''' + + WIKI_RECOMMENDATION = 'Rename the local variable so as not to mistakenly overshadow any state variable/function/modifier/event definitions.' + OVERSHADOWED_FUNCTION = "function" OVERSHADOWED_MODIFIER = "modifier" diff --git a/slither/detectors/shadowing/state.py b/slither/detectors/shadowing/state.py index 8551262e9..096d5107f 100644 --- a/slither/detectors/shadowing/state.py +++ b/slither/detectors/shadowing/state.py @@ -17,7 +17,36 @@ class StateShadowing(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variable-shadowing' - vuln_name = 'Shadowing' + WIKI_TITLE = 'State variable shadowing' + WIKI_DESCRIPTION = 'Detection of state variables shadowed.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract BaseContract{ + address owner; + + modifier isOwner(){ + require(owner == msg.sender); + _; + } + +} + +contract DerivedContract is BaseContract{ + address owner; + + constructor(){ + owner = msg.sender; + } + + function withdraw() isOwner() external{ + msg.sender.transfer(this.balance); + } +} +``` +`owner` of `BaseContract` is never assigned and the modifier `isOwner` does not work.''' + + WIKI_RECOMMENDATION = 'Remove the state variable shadowing.' + def detect_shadowing(self, contract): ret = [] diff --git a/slither/detectors/statements/assembly.py b/slither/detectors/statements/assembly.py index 5f4ad3cc8..b770456bc 100644 --- a/slither/detectors/statements/assembly.py +++ b/slither/detectors/statements/assembly.py @@ -18,6 +18,11 @@ class Assembly(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage' + + WIKI_TITLE = 'Assembly usage' + WIKI_DESCRIPTION = 'The use of assembly is error-prone and should be avoided.' + WIKI_RECOMMENDATION = 'Do not use evm assembly.' + @staticmethod def _contains_inline_assembly_use(node): """ diff --git a/slither/detectors/statements/calls_in_loop.py b/slither/detectors/statements/calls_in_loop.py index c38f2bb8a..2e5dbf9c7 100644 --- a/slither/detectors/statements/calls_in_loop.py +++ b/slither/detectors/statements/calls_in_loop.py @@ -18,6 +18,31 @@ class MultipleCallsInLoop(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description/_edit#calls-inside-a-loop' + + WIKI_TITLE = 'Calls inside a loop' + WIKI_DESCRIPTION = 'Calls inside a loop might lead to denial of service attack.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract CallsInLoop{ + + address[] destinations; + + constructor(address[] newDestinations) public{ + destinations = newDestinations; + } + + function bad() external{ + for (uint i=0; i < destinations.length; i++){ + destinations[i].transfer(i); + } + } + +} +``` +If one of the destinations has a fallback function which reverts, `bad` will always revert.''' + + WIKI_RECOMMENDATION = 'Favor [pull over push](https://github.com/ethereum/wiki/wiki/Safety#favor-pull-over-push-for-external-calls) strategy for external calls.' + @staticmethod def call_in_loop(node, in_loop, visited, ret): if node in visited: diff --git a/slither/detectors/statements/controlled_delegatecall.py b/slither/detectors/statements/controlled_delegatecall.py index 8300a6609..698a749eb 100644 --- a/slither/detectors/statements/controlled_delegatecall.py +++ b/slither/detectors/statements/controlled_delegatecall.py @@ -13,6 +13,21 @@ class ControlledDelegateCall(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#controlled-delegatecall' + + WIKI_TITLE = 'Controlled Delegatecall' + WIKI_DESCRIPTION = 'Delegatecall or callcode to an address controlled by the user.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Delegatecall{ + function delegate(address to, bytes data){ + to.delegatecall(data); + } +} +``` +Bob calls `delegate` and delegate the execution to its malicious contract. As a result, Bob withdraws the funds of the contract and destruct it.''' + + WIKI_RECOMMENDATION = 'Avoid using `delegatecall`. Use only trusted destinations.' + def controlled_delegatecall(self, function): ret = [] for node in function.nodes: diff --git a/slither/detectors/statements/incorrect_strict_equality.py b/slither/detectors/statements/incorrect_strict_equality.py index 65ad0974e..786f7d650 100644 --- a/slither/detectors/statements/incorrect_strict_equality.py +++ b/slither/detectors/statements/incorrect_strict_equality.py @@ -25,6 +25,19 @@ class IncorrectStrictEquality(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-strict-equalities' + WIKI_TITLE = 'Dangerous strict equalities' + WIKI_DESCRIPTION = 'Use of strick equalities that can be easily manipulated by an attacker.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Crowdsale{ + function fund_reached() public returns(bool){ + return this.balance == 100 ether; + } +``` +`Crowdsale` relies on `fund_reached` to know when to stop the sale of tokens. `Crowdsale` reaches 100 ether. Bob sends 0.1 ether. As a result, `fund_reached` is always false and the crowdsale never ends.''' + + WIKI_RECOMMENDATION = '''Don't use strict equality to determine if an account has enough ethers or tokens.''' + sources_taint = [SolidityVariable('now'), SolidityVariableComposed('block.number'), SolidityVariableComposed('block.timestamp')] diff --git a/slither/detectors/statements/tx_origin.py b/slither/detectors/statements/tx_origin.py index e4d7b063b..e4f01e764 100644 --- a/slither/detectors/statements/tx_origin.py +++ b/slither/detectors/statements/tx_origin.py @@ -16,6 +16,21 @@ class TxOrigin(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-usage-of-txorigin' + WIKI_TITLE = 'Dangerous usage of `tx.origin`' + WIKI_DESCRIPTION = '`tx.origin`-based protection can be abused by malicious contract if a legitimate user interacts with the malicious contract.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract TxOrigin { + address owner = msg.sender; + + function bug() { + 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.''' + + WIKI_RECOMMENDATION = 'Do not use `tx.origin` for authentification.' + @staticmethod def _contains_incorrect_tx_origin_use(node): """ diff --git a/slither/detectors/variables/possible_const_state_variables.py b/slither/detectors/variables/possible_const_state_variables.py index 49214f437..d46912374 100644 --- a/slither/detectors/variables/possible_const_state_variables.py +++ b/slither/detectors/variables/possible_const_state_variables.py @@ -23,6 +23,11 @@ class ConstCandidateStateVars(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant' + + WIKI_TITLE = 'State variables that could be declared constant' + WIKI_DESCRIPTION = 'Constant state variable should be declared constant to save gas.' + WIKI_RECOMMENDATION = 'Add the `constant` attributes to the state variables that never change.' + @staticmethod def _valid_candidate(v): return isinstance(v.type, ElementaryType) and not v.is_constant diff --git a/slither/detectors/variables/uninitialized_local_variables.py b/slither/detectors/variables/uninitialized_local_variables.py index 2aa08e1ab..36d9e2bf7 100644 --- a/slither/detectors/variables/uninitialized_local_variables.py +++ b/slither/detectors/variables/uninitialized_local_variables.py @@ -21,6 +21,22 @@ class UninitializedLocalVars(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-local-variables' + + WIKI_TITLE = 'Uninitialized local variables' + WIKI_DESCRIPTION = 'Uninitialized local variables.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Uninitialized is Owner{ + function withdraw() payable public onlyOwner{ + address to; + to.transfer(this.balance) + } +} +``` +Bob calls `transfer`. As a result, the ethers are sent to the address 0x0 and are lost.''' + + WIKI_RECOMMENDATION = 'Initialize all the variables. If a variable is meant to be initialized to zero, explicitly set it to zero.' + key = "UNINITIALIZEDLOCAL" def _detect_uninitialized(self, function, node, visited): diff --git a/slither/detectors/variables/uninitialized_state_variables.py b/slither/detectors/variables/uninitialized_state_variables.py index c362bc3ff..35a8bd552 100644 --- a/slither/detectors/variables/uninitialized_state_variables.py +++ b/slither/detectors/variables/uninitialized_state_variables.py @@ -30,6 +30,24 @@ class UninitializedStateVarsDetection(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-state-variables' + WIKI_TITLE = 'Uninitialized state variables' + WIKI_DESCRIPTION = 'Uninitialized state variables.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Uninitialized{ + address destination; + + function transfer() payable public{ + destination.transfer(msg.value); + } +} +``` +Bob calls `transfer`. As a result, the ethers are sent to the address 0x0 and are lost. +''' + WIKI_RECOMMENDATION = ''' +Initialize all the variables. If a variable is meant to be initialized to zero, explicitly set it to zero. +''' + @staticmethod def written_variables(contract): ret = [] diff --git a/slither/detectors/variables/uninitialized_storage_variables.py b/slither/detectors/variables/uninitialized_storage_variables.py index 731106ed4..b0d8930d2 100644 --- a/slither/detectors/variables/uninitialized_storage_variables.py +++ b/slither/detectors/variables/uninitialized_storage_variables.py @@ -21,6 +21,28 @@ class UninitializedStorageVars(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-storage-variables' + WIKI_TITLE = 'Uninitialized storage variables' + WIKI_DESCRIPTION = 'An uinitialized storage variable will act as a reference to the first state variable, and can override a critical variable.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Uninitialized{ + address owner = msg.sender; + + struct St{ + uint a; + } + + function func() { + St st; + st.a = 0x0; + } +} +``` +Bob calls `func`. As a result, `owner` is override to 0. +''' + + WIKI_RECOMMENDATION = 'Initialize all the storage variables.' + # node.context[self.key] contains the uninitialized storage variables key = "UNINITIALIZEDSTORAGE" diff --git a/slither/detectors/variables/unused_state_variables.py b/slither/detectors/variables/unused_state_variables.py index 8a593cf09..e23ca6777 100644 --- a/slither/detectors/variables/unused_state_variables.py +++ b/slither/detectors/variables/unused_state_variables.py @@ -16,6 +16,12 @@ class UnusedStateVars(AbstractDetector): WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables' + + WIKI_TITLE = 'Unused state variables' + WIKI_DESCRIPTION = 'Unused state variable.' + WIKI_EXPLOIT_SCENARIO = '' + WIKI_RECOMMENDATION = 'Remove unused state variables.' + def detect_unused(self, contract): if contract.is_signature_only(): return None