diff --git a/README.md b/README.md index 8d186569b..efc2b74f7 100644 --- a/README.md +++ b/README.md @@ -39,37 +39,38 @@ By default, all the detectors are run. Num | Detector | What it Detects | Impact | Confidence --- | --- | --- | --- | --- -1 | `shadowing-state` | [State variables shadowing](https://github.com/crytic/slither/wiki/Detector-Documentation#state-variable-shadowing) | High | High -2 | `suicidal` | [Functions allowing anyone to destruct the contract](https://github.com/crytic/slither/wiki/Detector-Documentation#suicidal) | High | High -3 | `uninitialized-state` | [Uninitialized state variables](https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-state-variables) | High | High -4 | `uninitialized-storage` | [Uninitialized storage variables](https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-storage-variables) | High | High -5 | `arbitrary-send` | [Functions that send ether to arbitrary destinations](https://github.com/crytic/slither/wiki/Detector-Documentation#functions-that-send-ether-to-arbitrary-destinations) | High | Medium -6 | `controlled-delegatecall` | [Controlled delegatecall destination](https://github.com/crytic/slither/wiki/Detector-Documentation#controlled-delegatecall) | High | Medium -7 | `reentrancy-eth` | [Reentrancy vulnerabilities (theft of ethers)](https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities) | High | Medium -8 | `erc20-interface` | [Incorrect ERC20 interfaces](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-erc20-interface) | Medium | High -9 | `incorrect-equality` | [Dangerous strict equalities](https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities) | Medium | High -10 | `locked-ether` | [Contracts that lock ether](https://github.com/crytic/slither/wiki/Detector-Documentation#contracts-that-lock-ether) | Medium | High -11 | `shadowing-abstract` | [State variables shadowing from abstract contracts](https://github.com/crytic/slither/wiki/Detector-Documentation#state-variable-shadowing-from-abstract-contracts) | Medium | High -12 | `constant-function` | [Constant functions changing the state](https://github.com/crytic/slither/wiki/Detector-Documentation#constant-functions-changing-the-state) | Medium | Medium -13 | `reentrancy-no-eth` | [Reentrancy vulnerabilities (no theft of ethers)](https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-1) | Medium | Medium -14 | `tx-origin` | [Dangerous usage of `tx.origin`](https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-usage-of-txorigin) | Medium | Medium -15 | `uninitialized-local` | [Uninitialized local variables](https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-local-variables) | Medium | Medium -16 | `unused-return` | [Unused return values](https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return) | Medium | Medium -17 | `shadowing-builtin` | [Built-in symbol shadowing](https://github.com/crytic/slither/wiki/Detector-Documentation#builtin-symbol-shadowing) | Low | High -18 | `shadowing-local` | [Local variables shadowing](https://github.com/crytic/slither/wiki/Detector-Documentation#local-variable-shadowing) | Low | High -19 | `calls-loop` | [Multiple calls in a loop](https://github.com/crytic/slither/wiki/Detector-Documentation/_edit#calls-inside-a-loop) | Low | Medium -20 | `reentrancy-benign` | [Benign reentrancy vulnerabilities](https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-2) | Low | Medium -21 | `timestamp` | [Dangerous usage of `block.timestamp`](https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp) | Low | Medium -22 | `assembly` | [Assembly usage](https://github.com/crytic/slither/wiki/Detector-Documentation#assembly-usage) | Informational | High -23 | `constable-states` | [State variables that could be declared constant](https://github.com/crytic/slither/wiki/Detector-Documentation#state-variables-that-could-be-declared-constant) | Informational | High -24 | `deprecated-standards` | [Deprecated Solidity Standards](https://github.com/crytic/slither/wiki/Detector-Documentation#deprecated-standards) | Informational | High -25 | `erc20-indexed` | [Un-indexed ERC20 event parameters](https://github.com/crytic/slither/wiki/Detector-Documentation#unindexed-erc20-event-parameters) | Informational | High -26 | `external-function` | [Public function that could be declared as external](https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-as-external) | Informational | High -27 | `low-level-calls` | [Low level calls](https://github.com/crytic/slither/wiki/Detector-Documentation#low-level-calls) | Informational | High -28 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions) | Informational | High -29 | `pragma` | [If different pragma directives are used](https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used) | Informational | High -30 | `solc-version` | [Incorrect Solidity version (< 0.4.24 or complex pragma)](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-version-of-solidity) | Informational | High -31 | `unused-state` | [Unused state variables](https://github.com/crytic/slither/wiki/Detector-Documentation#unused-state-variables) | Informational | High +1 | `rtlo` | [Right-To-Left-Override control character is used](https://github.com/crytic/slither/wiki/Detector-Documentation#right-to-left-override-character) | High | High +2 | `shadowing-state` | [State variables shadowing](https://github.com/crytic/slither/wiki/Detector-Documentation#state-variable-shadowing) | High | High +3 | `suicidal` | [Functions allowing anyone to destruct the contract](https://github.com/crytic/slither/wiki/Detector-Documentation#suicidal) | High | High +4 | `uninitialized-state` | [Uninitialized state variables](https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-state-variables) | High | High +5 | `uninitialized-storage` | [Uninitialized storage variables](https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-storage-variables) | High | High +6 | `arbitrary-send` | [Functions that send ether to arbitrary destinations](https://github.com/crytic/slither/wiki/Detector-Documentation#functions-that-send-ether-to-arbitrary-destinations) | High | Medium +7 | `controlled-delegatecall` | [Controlled delegatecall destination](https://github.com/crytic/slither/wiki/Detector-Documentation#controlled-delegatecall) | High | Medium +8 | `reentrancy-eth` | [Reentrancy vulnerabilities (theft of ethers)](https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities) | High | Medium +9 | `erc20-interface` | [Incorrect ERC20 interfaces](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-erc20-interface) | Medium | High +10 | `incorrect-equality` | [Dangerous strict equalities](https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities) | Medium | High +11 | `locked-ether` | [Contracts that lock ether](https://github.com/crytic/slither/wiki/Detector-Documentation#contracts-that-lock-ether) | Medium | High +12 | `shadowing-abstract` | [State variables shadowing from abstract contracts](https://github.com/crytic/slither/wiki/Detector-Documentation#state-variable-shadowing-from-abstract-contracts) | Medium | High +13 | `constant-function` | [Constant functions changing the state](https://github.com/crytic/slither/wiki/Detector-Documentation#constant-functions-changing-the-state) | Medium | Medium +14 | `reentrancy-no-eth` | [Reentrancy vulnerabilities (no theft of ethers)](https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-1) | Medium | Medium +15 | `tx-origin` | [Dangerous usage of `tx.origin`](https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-usage-of-txorigin) | Medium | Medium +16 | `uninitialized-local` | [Uninitialized local variables](https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-local-variables) | Medium | Medium +17 | `unused-return` | [Unused return values](https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return) | Medium | Medium +18 | `shadowing-builtin` | [Built-in symbol shadowing](https://github.com/crytic/slither/wiki/Detector-Documentation#builtin-symbol-shadowing) | Low | High +19 | `shadowing-local` | [Local variables shadowing](https://github.com/crytic/slither/wiki/Detector-Documentation#local-variable-shadowing) | Low | High +20 | `calls-loop` | [Multiple calls in a loop](https://github.com/crytic/slither/wiki/Detector-Documentation/_edit#calls-inside-a-loop) | Low | Medium +21 | `reentrancy-benign` | [Benign reentrancy vulnerabilities](https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-2) | Low | Medium +22 | `timestamp` | [Dangerous usage of `block.timestamp`](https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp) | Low | Medium +23 | `assembly` | [Assembly usage](https://github.com/crytic/slither/wiki/Detector-Documentation#assembly-usage) | Informational | High +24 | `constable-states` | [State variables that could be declared constant](https://github.com/crytic/slither/wiki/Detector-Documentation#state-variables-that-could-be-declared-constant) | Informational | High +25 | `deprecated-standards` | [Deprecated Solidity Standards](https://github.com/crytic/slither/wiki/Detector-Documentation#deprecated-standards) | Informational | High +26 | `erc20-indexed` | [Un-indexed ERC20 event parameters](https://github.com/crytic/slither/wiki/Detector-Documentation#unindexed-erc20-event-parameters) | Informational | High +27 | `external-function` | [Public function that could be declared as external](https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-as-external) | Informational | High +28 | `low-level-calls` | [Low level calls](https://github.com/crytic/slither/wiki/Detector-Documentation#low-level-calls) | Informational | High +29 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions) | Informational | High +30 | `pragma` | [If different pragma directives are used](https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used) | Informational | High +31 | `solc-version` | [Incorrect Solidity version (< 0.4.24 or complex pragma)](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-version-of-solidity) | Informational | High +32 | `unused-state` | [Unused state variables](https://github.com/crytic/slither/wiki/Detector-Documentation#unused-state-variables) | Informational | High [Contact us](https://www.trailofbits.com/contact/) to get access to additional detectors. diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index 0db3b07ac..a3fdf99c4 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -47,3 +47,4 @@ generate_expected_json(){ #generate_expected_json tests/shadowing_builtin_symbols.sol "shadowing-builtin" #generate_expected_json tests/shadowing_local_variable.sol "shadowing-local" #generate_expected_json tests/solc_version_incorrect.sol "solc-version" +generate_expected_json tests/right_to_left_override.sol "rtlo" diff --git a/scripts/travis_test_4.sh b/scripts/travis_test_4.sh index fb095fa90..37270e4d2 100755 --- a/scripts/travis_test_4.sh +++ b/scripts/travis_test_4.sh @@ -52,7 +52,7 @@ test_slither(){ fi result=$(python "$DIR/json_diff.py" "$expected" "$DIR/tmp-test.json") - + rm "$DIR/tmp-test.json" if [ "$result" != "{}" ]; then echo "" @@ -97,3 +97,4 @@ 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" test_slither tests/solc_version_incorrect.sol "solc-version" +test_slither tests/right_to_left_override.sol "rtlo" diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 8f1f5cb48..36c35496c 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -31,5 +31,6 @@ from .statements.incorrect_strict_equality import IncorrectStrictEquality from .erc20.incorrect_interface import IncorrectERC20InterfaceDetection from .erc20.unindexed_event_parameters import UnindexedERC20EventParameters from .statements.deprecated_calls import DeprecatedStandards +from .source.rtlo import RightToLeftOverride # # diff --git a/slither/detectors/source/__init__.py b/slither/detectors/source/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/slither/detectors/source/rtlo.py b/slither/detectors/source/rtlo.py new file mode 100644 index 000000000..86690f271 --- /dev/null +++ b/slither/detectors/source/rtlo.py @@ -0,0 +1,63 @@ +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +import re + +class RightToLeftOverride(AbstractDetector): + """ + Detect the usage of a Right-To-Left-Override (U+202E) character + """ + + ARGUMENT = 'rtlo' + HELP = 'Right-To-Left-Override control character is used' + IMPACT = DetectorClassification.HIGH + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#right-to-left-override-character' + WIKI_TITLE = 'Right-To-Left-Override character' + WIKI_DESCRIPTION = 'An attacker can manipulate the logic of the contract by using a right-to-left-override character (U+202E)' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Token +{ + + address payable o; // owner + mapping(address => uint) tokens; + + function withdraw() external returns(uint) + { + uint amount = tokens[msg.sender]; + address payable d = msg.sender; + tokens[msg.sender] = 0; + _withdraw(/*owner‮/*noitanitsed*/ d, o/*‭ + /*value */, amount); + } + + function _withdraw(address payable fee_receiver, address payable destination, uint value) internal + { + fee_receiver.transfer(1); + destination.transfer(value); + } +} +``` + +`Token` uses the right-to-left-override character when calling `_withdraw`. As a result, the fee is incorrectly sent to `msg.sender`, and the token balance is sent to the owner. + +''' + WIKI_RECOMMENDATION = 'Special control characters must not be allowed.' + + def _detect(self): + results = [] + + pattern = re.compile(".*\u202e.*") + for filename, source in self.slither.source_code.items(): + info = "{} contains a unicode right-to-left-override character:\n".format(filename) + found = False + for match in pattern.finditer(source): + match_line = match.group(0) + info += "\t- {}\n".format(match_line) + found = True + + if found: + json = self.generate_json_result(info) + results.append(json) + + return results diff --git a/tests/expected_json/right_to_left_override.rtlo.json b/tests/expected_json/right_to_left_override.rtlo.json new file mode 100644 index 000000000..203035b01 --- /dev/null +++ b/tests/expected_json/right_to_left_override.rtlo.json @@ -0,0 +1 @@ +[{"check": "rtlo", "impact": "High", "confidence": "High", "description": "tests/right_to_left_override.sol contains a unicode right-to-left-override character:\n\t- return test1(/*A\u202e/*B*/2 , 1/*\u202d\n", "elements": []}] \ No newline at end of file diff --git a/tests/right_to_left_override.sol b/tests/right_to_left_override.sol new file mode 100644 index 000000000..ce5216b55 --- /dev/null +++ b/tests/right_to_left_override.sol @@ -0,0 +1,15 @@ +//pragma solidity ^0.4.24; + +contract A +{ + function test() public pure + { + test1(/*A‮/*B*/2 , 1/*‭ + /*C */,3); + } + + function test1(uint a, uint b, uint c) internal pure + { + a = b + c; + } +}