From c522ef9c14acc493948c546b3c50ba261ad64d47 Mon Sep 17 00:00:00 2001 From: Richie Date: Mon, 21 Nov 2022 14:13:34 -0800 Subject: [PATCH 01/12] refactor: add VULNERABLE_SOLC_VERSIONS and logic --- slither/detectors/abstract_detector.py | 15 ++++++++++++ .../compiler_bugs/enum_conversion.py | 23 ++++--------------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/slither/detectors/abstract_detector.py b/slither/detectors/abstract_detector.py index 4ebead96a..56989abcf 100644 --- a/slither/detectors/abstract_detector.py +++ b/slither/detectors/abstract_detector.py @@ -61,6 +61,11 @@ class AbstractDetector(metaclass=abc.ABCMeta): STANDARD_JSON = True + # list of vulnerable solc versions as strings (e.g. ["0.4.25", "0.5.0"]) + # if this list is not empty then the detector will not run unless the solc version is on the list + # an empty list means that the detector will run on any solc version + VULNERABLE_SOLC_VERSIONS = [] + def __init__( self, compilation_unit: SlitherCompilationUnit, slither: "Slither", logger: Logger ): @@ -139,6 +144,11 @@ class AbstractDetector(metaclass=abc.ABCMeta): if self.logger: self.logger.info(self.color(info)) + def _uses_vulnerable_solc_version(self) -> bool: + if self.VULNERABLE_SOLC_VERSIONS: + return self.compilation_unit.solc_version in self.VULNERABLE_SOLC_VERSIONS + return True + @abc.abstractmethod def _detect(self) -> List[Output]: """TODO Documentation""" @@ -147,6 +157,11 @@ class AbstractDetector(metaclass=abc.ABCMeta): # pylint: disable=too-many-branches def detect(self) -> List[Dict]: results: List[Dict] = [] + + # check solc version + if not self._uses_vulnerable_solc_version(): + return results + # only keep valid result, and remove duplicate # Keep only dictionaries for r in [output.data for output in self._detect()]: diff --git a/slither/detectors/compiler_bugs/enum_conversion.py b/slither/detectors/compiler_bugs/enum_conversion.py index 1db166ac2..60a012f10 100644 --- a/slither/detectors/compiler_bugs/enum_conversion.py +++ b/slither/detectors/compiler_bugs/enum_conversion.py @@ -7,18 +7,6 @@ from slither.slithir.operations import TypeConversion from slither.core.declarations.enum import Enum -def _uses_vulnerable_solc_version(version): - """Detect if used compiler version is 0.4.[0|1|2|3|4] - Args: - version (solc version used) - Returns: - Bool - """ - if version in ["0.4.0", "0.4.1", "0.4.2", "0.4.3", "0.4.4"]: - return True - return False - - def _detect_dangerous_enum_conversions(contract): """Detect dangerous conversion to enum by checking IR Args: @@ -54,11 +42,11 @@ class EnumConversion(AbstractDetector): ```solidity pragma solidity 0.4.2; contract Test{ - + enum E{a} - + function bug(uint a) public returns(E){ - return E(a); + return E(a); } } ``` @@ -67,12 +55,11 @@ Attackers can trigger unexpected behaviour by calling `bug(1)`.""" WIKI_RECOMMENDATION = "Use a recent compiler version. If `solc` <`0.4.5` is required, check the `enum` conversion range." + VULNERABLE_SOLC_VERSIONS = ["0.4.0", "0.4.1", "0.4.2", "0.4.3", "0.4.4"] + def _detect(self): """Detect dangerous conversion to enum""" results = [] - # If solc version >= 0.4.5 then return - if not _uses_vulnerable_solc_version(self.compilation_unit.solc_version): - return results for c in self.compilation_unit.contracts: ret = _detect_dangerous_enum_conversions(c) From 2412f834f09636ae33e7308164142ea61c5183bd Mon Sep 17 00:00:00 2001 From: Richie Date: Mon, 28 Nov 2022 09:25:28 -0800 Subject: [PATCH 02/12] fix: add type --- slither/detectors/abstract_detector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/detectors/abstract_detector.py b/slither/detectors/abstract_detector.py index 56989abcf..d00a9a780 100644 --- a/slither/detectors/abstract_detector.py +++ b/slither/detectors/abstract_detector.py @@ -64,7 +64,7 @@ class AbstractDetector(metaclass=abc.ABCMeta): # list of vulnerable solc versions as strings (e.g. ["0.4.25", "0.5.0"]) # if this list is not empty then the detector will not run unless the solc version is on the list # an empty list means that the detector will run on any solc version - VULNERABLE_SOLC_VERSIONS = [] + VULNERABLE_SOLC_VERSIONS: List[str] = [] def __init__( self, compilation_unit: SlitherCompilationUnit, slither: "Slither", logger: Logger From 04217fda53e1fe105fd56ab93c7c047708edde6e Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Wed, 30 Nov 2022 10:17:18 +0100 Subject: [PATCH 03/12] - Create helpers for VULNERABLE_SOLC_VERSIONS - Use VULNERABLE_SOLC_VERSIONS for all relevant detectors - Fix incorrect solc version in storage-signed-integer-array --- slither/detectors/abstract_detector.py | 24 +- .../attributes/const_functions_asm.py | 10 +- .../attributes/const_functions_state.py | 10 +- .../compiler_bugs/enum_conversion.py | 8 +- .../compiler_bugs/public_mapping_nested.py | 16 +- .../compiler_bugs/reused_base_constructor.py | 12 +- .../storage_ABIEncoderV2_array.py | 44 +- .../storage_signed_integer_array.py | 45 +- ...initialized_function_ptr_in_constructor.py | 45 +- .../detectors/functions/external_function.py | 21 +- .../statements/array_length_assignment.py | 12 +- ....sol.0.5.10.StorageSignedIntegerArray.json | 505 +----------------- 12 files changed, 97 insertions(+), 655 deletions(-) diff --git a/slither/detectors/abstract_detector.py b/slither/detectors/abstract_detector.py index d00a9a780..778695d85 100644 --- a/slither/detectors/abstract_detector.py +++ b/slither/detectors/abstract_detector.py @@ -46,6 +46,20 @@ classification_txt = { } +def make_solc_versions(minor: int, patch_min: int, patch_max: int) -> List[str]: + """ + Create a list of solc version: [0.minor.patch_min .... 0.minor.patch_max] + """ + return [f"0.{minor}.{x}" for x in range(patch_min, patch_max + 1)] + + +ALL_SOLC_VERSIONS_04 = make_solc_versions(4, 0, 26) +ALL_SOLC_VERSIONS_05 = make_solc_versions(5, 0, 17) +ALL_SOLC_VERSIONS_06 = make_solc_versions(6, 0, 12) +ALL_SOLC_VERSIONS_07 = make_solc_versions(7, 0, 6) +# No VERSIONS_08 as it is still in dev + + class AbstractDetector(metaclass=abc.ABCMeta): ARGUMENT = "" # run the detector with slither.py --ARGUMENT HELP = "" # help information @@ -62,9 +76,8 @@ class AbstractDetector(metaclass=abc.ABCMeta): STANDARD_JSON = True # list of vulnerable solc versions as strings (e.g. ["0.4.25", "0.5.0"]) - # if this list is not empty then the detector will not run unless the solc version is on the list - # an empty list means that the detector will run on any solc version - VULNERABLE_SOLC_VERSIONS: List[str] = [] + # If the detector is meant to run on all versions, use None + VULNERABLE_SOLC_VERSIONS: Optional[List[str]] = None def __init__( self, compilation_unit: SlitherCompilationUnit, slither: "Slither", logger: Logger @@ -113,6 +126,11 @@ class AbstractDetector(metaclass=abc.ABCMeta): f"WIKI_RECOMMENDATION is not initialized {self.__class__.__name__}" ) + if self.VULNERABLE_SOLC_VERSIONS is not None and not self.VULNERABLE_SOLC_VERSIONS: + raise IncorrectDetectorInitialization( + f"VULNERABLE_SOLC_VERSIONS should not be an empty list {self.__class__.__name__}" + ) + if re.match("^[a-zA-Z0-9_-]*$", self.ARGUMENT) is None: raise IncorrectDetectorInitialization( f"ARGUMENT has illegal character {self.__class__.__name__}" diff --git a/slither/detectors/attributes/const_functions_asm.py b/slither/detectors/attributes/const_functions_asm.py index cb805afb7..33853c9f4 100644 --- a/slither/detectors/attributes/const_functions_asm.py +++ b/slither/detectors/attributes/const_functions_asm.py @@ -2,7 +2,11 @@ Module detecting constant functions Recursively check the called functions """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + ALL_SOLC_VERSIONS_04, +) from slither.formatters.attributes.const_functions import custom_format @@ -49,6 +53,8 @@ All the calls to `get` revert, breaking Bob's smart contract execution.""" "Ensure the attributes of contracts compiled prior to Solidity 0.5.0 are correct." ) + VULNERABLE_SOLC_VERSIONS = ALL_SOLC_VERSIONS_04 + def _detect(self): """Detect the constant function using assembly code @@ -57,8 +63,6 @@ All the calls to `get` revert, breaking Bob's smart contract execution.""" list: {'vuln', 'filename,'contract','func','#varsWritten'} """ results = [] - if self.compilation_unit.solc_version and self.compilation_unit.solc_version >= "0.5.0": - return results for c in self.contracts: for f in c.functions: if f.contract_declarer != c: diff --git a/slither/detectors/attributes/const_functions_state.py b/slither/detectors/attributes/const_functions_state.py index 78618c523..a351727cf 100644 --- a/slither/detectors/attributes/const_functions_state.py +++ b/slither/detectors/attributes/const_functions_state.py @@ -2,7 +2,11 @@ Module detecting constant functions Recursively check the called functions """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + ALL_SOLC_VERSIONS_04, +) from slither.formatters.attributes.const_functions import custom_format @@ -49,6 +53,8 @@ All the calls to `get` revert, breaking Bob's smart contract execution.""" "Ensure that attributes of contracts compiled prior to Solidity 0.5.0 are correct." ) + VULNERABLE_SOLC_VERSIONS = ALL_SOLC_VERSIONS_04 + def _detect(self): """Detect the constant function changing the state @@ -57,8 +63,6 @@ All the calls to `get` revert, breaking Bob's smart contract execution.""" list: {'vuln', 'filename,'contract','func','#varsWritten'} """ results = [] - if self.compilation_unit.solc_version and self.compilation_unit.solc_version >= "0.5.0": - return results for c in self.contracts: for f in c.functions: if f.contract_declarer != c: diff --git a/slither/detectors/compiler_bugs/enum_conversion.py b/slither/detectors/compiler_bugs/enum_conversion.py index 60a012f10..477188fe0 100644 --- a/slither/detectors/compiler_bugs/enum_conversion.py +++ b/slither/detectors/compiler_bugs/enum_conversion.py @@ -2,7 +2,11 @@ Module detecting dangerous conversion to enum """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + make_solc_versions, +) from slither.slithir.operations import TypeConversion from slither.core.declarations.enum import Enum @@ -55,7 +59,7 @@ Attackers can trigger unexpected behaviour by calling `bug(1)`.""" WIKI_RECOMMENDATION = "Use a recent compiler version. If `solc` <`0.4.5` is required, check the `enum` conversion range." - VULNERABLE_SOLC_VERSIONS = ["0.4.0", "0.4.1", "0.4.2", "0.4.3", "0.4.4"] + VULNERABLE_SOLC_VERSIONS = make_solc_versions(4, 0, 4) def _detect(self): """Detect dangerous conversion to enum""" diff --git a/slither/detectors/compiler_bugs/public_mapping_nested.py b/slither/detectors/compiler_bugs/public_mapping_nested.py index 5d557ddcc..8e6b6f4a8 100644 --- a/slither/detectors/compiler_bugs/public_mapping_nested.py +++ b/slither/detectors/compiler_bugs/public_mapping_nested.py @@ -2,7 +2,11 @@ Module detecting public mappings with nested variables (returns incorrect values prior to 0.5.x) """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + ALL_SOLC_VERSIONS_04, +) from slither.core.solidity_types.mapping_type import MappingType from slither.core.solidity_types.user_defined_type import UserDefinedType from slither.core.declarations.structure import Structure @@ -62,6 +66,8 @@ class PublicMappingNested(AbstractDetector): WIKI_EXPLOIT_SCENARIO = """Bob interacts with a contract that has a public mapping with nested structures. The values returned by the mapping are incorrect, breaking Bob's usage""" WIKI_RECOMMENDATION = "Do not use public mapping with nested structures." + VULNERABLE_SOLC_VERSIONS = ALL_SOLC_VERSIONS_04 + def _detect(self): """ Detect public mappings with nested variables (returns incorrect values prior to 0.5.x) @@ -72,14 +78,6 @@ class PublicMappingNested(AbstractDetector): """ results = [] - if self.compilation_unit.solc_version >= "0.5.0": - return [] - - if self.compilation_unit.solc_version and self.compilation_unit.solc_version.startswith( - "0.5." - ): - return [] - for contract in self.contracts: public_nested_mappings = detect_public_nested_mappings(contract) if public_nested_mappings: diff --git a/slither/detectors/compiler_bugs/reused_base_constructor.py b/slither/detectors/compiler_bugs/reused_base_constructor.py index 2ad0b0a6a..9d0b91448 100644 --- a/slither/detectors/compiler_bugs/reused_base_constructor.py +++ b/slither/detectors/compiler_bugs/reused_base_constructor.py @@ -2,7 +2,11 @@ Module detecting re-used base constructors in inheritance hierarchy. """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + ALL_SOLC_VERSIONS_04, +) # Helper: adds explicitly called constructors with arguments to the results lookup. @@ -71,6 +75,8 @@ The constructor of `A` is called multiple times in `D` and `E`: WIKI_RECOMMENDATION = "Remove the duplicate constructor call." + VULNERABLE_SOLC_VERSIONS = ALL_SOLC_VERSIONS_04 + def _detect_explicitly_called_base_constructors(self, contract): """ Detects explicitly calls to base constructors with arguments in the inheritance hierarchy. @@ -126,10 +132,6 @@ The constructor of `A` is called multiple times in `D` and `E`: results = [] - # The bug is not possible with solc >= 0.5.0 - if not self.compilation_unit.solc_version.startswith("0.4."): - return [] - # Loop for each contract for contract in self.contracts: diff --git a/slither/detectors/compiler_bugs/storage_ABIEncoderV2_array.py b/slither/detectors/compiler_bugs/storage_ABIEncoderV2_array.py index 5045a0e68..59d52760e 100644 --- a/slither/detectors/compiler_bugs/storage_ABIEncoderV2_array.py +++ b/slither/detectors/compiler_bugs/storage_ABIEncoderV2_array.py @@ -2,7 +2,11 @@ Module detecting ABIEncoderV2 array bug """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + make_solc_versions, +) from slither.core.solidity_types import ArrayType from slither.core.solidity_types import UserDefinedType from slither.core.variables.local_variable import LocalVariable @@ -13,38 +17,6 @@ from slither.slithir.operations import EventCall from slither.slithir.operations import HighLevelCall from slither.utils.utils import unroll -vulnerable_solc_versions = [ - "0.4.7", - "0.4.8", - "0.4.9", - "0.4.10", - "0.4.11", - "0.4.12", - "0.4.13", - "0.4.14", - "0.4.15", - "0.4.16", - "0.4.17", - "0.4.18", - "0.4.19", - "0.4.20", - "0.4.21", - "0.4.22", - "0.4.23", - "0.4.24", - "0.4.25", - "0.5.0", - "0.5.1", - "0.5.2", - "0.5.3", - "0.5.4", - "0.5.5", - "0.5.6", - "0.5.7", - "0.5.8", - "0.5.9", -] - class ABIEncoderV2Array(AbstractDetector): """ @@ -80,6 +52,8 @@ contract A { WIKI_RECOMMENDATION = "Use a compiler >= `0.5.10`." + VULNERABLE_SOLC_VERSIONS = make_solc_versions(4, 7, 25) + make_solc_versions(5, 0, 9) + @staticmethod def _detect_storage_abiencoderv2_arrays(contract): """ @@ -130,10 +104,6 @@ contract A { """ results = [] - # Check if vulnerable solc versions are used - if self.compilation_unit.solc_version not in vulnerable_solc_versions: - return results - # Check if pragma experimental ABIEncoderV2 is used if not any( (p.directive[0] == "experimental" and p.directive[1] == "ABIEncoderV2") diff --git a/slither/detectors/compiler_bugs/storage_signed_integer_array.py b/slither/detectors/compiler_bugs/storage_signed_integer_array.py index 408b49905..419c71c87 100644 --- a/slither/detectors/compiler_bugs/storage_signed_integer_array.py +++ b/slither/detectors/compiler_bugs/storage_signed_integer_array.py @@ -2,7 +2,11 @@ Module detecting storage signed integer array bug """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + make_solc_versions, +) from slither.core.cfg.node import NodeType from slither.core.solidity_types import ArrayType from slither.core.solidity_types.elementary_type import Int, ElementaryType @@ -11,39 +15,6 @@ from slither.core.variables.state_variable import StateVariable from slither.slithir.operations.assignment import Assignment from slither.slithir.operations.init_array import InitArray -vulnerable_solc_versions = [ - "0.4.7", - "0.4.8", - "0.4.9", - "0.4.10", - "0.4.11", - "0.4.12", - "0.4.13", - "0.4.14", - "0.4.15", - "0.4.16", - "0.4.17", - "0.4.18", - "0.4.19", - "0.4.20", - "0.4.21", - "0.4.22", - "0.4.23", - "0.4.24", - "0.4.25", - "0.5.0", - "0.5.1", - "0.5.2", - "0.5.3", - "0.5.4", - "0.5.5", - "0.5.6", - "0.5.7", - "0.5.8", - "0.5.9", - "0.5.10", -] - class StorageSignedIntegerArray(AbstractDetector): """ @@ -61,7 +32,7 @@ class StorageSignedIntegerArray(AbstractDetector): WIKI_TITLE = "Storage Signed Integer Array" # region wiki_description - WIKI_DESCRIPTION = """`solc` versions `0.4.7`-`0.5.10` contain [a compiler bug](https://blog.ethereum.org/2019/06/25/solidity-storage-array-bugs) + WIKI_DESCRIPTION = """`solc` versions `0.4.7`-`0.5.9` contain [a compiler bug](https://blog.ethereum.org/2019/06/25/solidity-storage-array-bugs) leading to incorrect values in signed integer arrays.""" # endregion wiki_description @@ -84,6 +55,8 @@ contract A { WIKI_RECOMMENDATION = "Use a compiler version >= `0.5.10`." + VULNERABLE_SOLC_VERSIONS = make_solc_versions(4, 7, 25) + make_solc_versions(5, 0, 9) + @staticmethod def _is_vulnerable_type(ir): """ @@ -140,8 +113,6 @@ contract A { Detect storage signed integer array init/assignment """ results = [] - if self.compilation_unit.solc_version not in vulnerable_solc_versions: - return results for contract in self.contracts: storage_signed_integer_arrays = self.detect_storage_signed_integer_arrays(contract) for function, node in storage_signed_integer_arrays: diff --git a/slither/detectors/compiler_bugs/uninitialized_function_ptr_in_constructor.py b/slither/detectors/compiler_bugs/uninitialized_function_ptr_in_constructor.py index 69fd45cae..a4d3cb8f2 100644 --- a/slither/detectors/compiler_bugs/uninitialized_function_ptr_in_constructor.py +++ b/slither/detectors/compiler_bugs/uninitialized_function_ptr_in_constructor.py @@ -2,44 +2,15 @@ Module detecting uninitialized function pointer calls in constructors """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + make_solc_versions, +) from slither.slithir.operations import InternalDynamicCall, OperationWithLValue from slither.slithir.variables import ReferenceVariable from slither.slithir.variables.variable import SlithIRVariable -vulnerable_solc_versions = [ - "0.4.5", - "0.4.6", - "0.4.7", - "0.4.8", - "0.4.9", - "0.4.10", - "0.4.11", - "0.4.12", - "0.4.13", - "0.4.14", - "0.4.15", - "0.4.16", - "0.4.17", - "0.4.18", - "0.4.19", - "0.4.20", - "0.4.21", - "0.4.22", - "0.4.23", - "0.4.24", - "0.4.25", - "0.5.0", - "0.5.1", - "0.5.2", - "0.5.3", - "0.5.4", - "0.5.5", - "0.5.6", - "0.5.7", - "0.5.8", -] - def _get_variables_entrance(function): """ @@ -110,6 +81,8 @@ The call to `a(10)` will lead to unexpected behavior because function pointer `a "Initialize function pointers before calling. Avoid function pointers if possible." ) + VULNERABLE_SOLC_VERSIONS = make_solc_versions(4, 5, 25) + make_solc_versions(5, 0, 8) + @staticmethod def _detect_uninitialized_function_ptr_in_constructor(contract): """ @@ -134,10 +107,6 @@ The call to `a(10)` will lead to unexpected behavior because function pointer `a """ results = [] - # Check if vulnerable solc versions are used - if self.compilation_unit.solc_version not in vulnerable_solc_versions: - return results - for contract in self.compilation_unit.contracts: contract_info = ["Contract ", contract, " \n"] nodes = self._detect_uninitialized_function_ptr_in_constructor(contract) diff --git a/slither/detectors/functions/external_function.py b/slither/detectors/functions/external_function.py index 631e5ffc1..5858c2baf 100644 --- a/slither/detectors/functions/external_function.py +++ b/slither/detectors/functions/external_function.py @@ -5,7 +5,13 @@ from slither.core.declarations.structure import Structure from slither.core.solidity_types.array_type import ArrayType from slither.core.solidity_types.user_defined_type import UserDefinedType from slither.core.variables.variable import Variable -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + ALL_SOLC_VERSIONS_04, + ALL_SOLC_VERSIONS_05, + make_solc_versions, +) from slither.formatters.functions.external_function import custom_format from slither.slithir.operations import InternalCall, InternalDynamicCall from slither.slithir.operations import SolidityCall @@ -31,6 +37,10 @@ class ExternalFunction(AbstractDetector): WIKI_DESCRIPTION = "`public` functions that are never called by the contract should be declared `external`, and its immutable parameters should be located in `calldata` to save gas." WIKI_RECOMMENDATION = "Use the `external` attribute for functions never called from the contract, and change the location of immutable parameters to `calldata` to save gas." + VULNERABLE_SOLC_VERSIONS = ( + ALL_SOLC_VERSIONS_04 + ALL_SOLC_VERSIONS_05 + make_solc_versions(6, 0, 8) + ) + @staticmethod def detect_functions_called(contract: Contract) -> List[Function]: """Returns a list of InternallCall, SolidityCall @@ -134,15 +144,6 @@ class ExternalFunction(AbstractDetector): def _detect(self) -> List[Output]: # pylint: disable=too-many-locals,too-many-branches results: List[Output] = [] - # After solc 0.6.9, calldata arguments are allowed in public functions - if self.compilation_unit.solc_version >= "0.7." or self.compilation_unit.solc_version in [ - "0.6.9", - "0.6.10", - "0.6.11", - "0.6.12", - ]: - return results - # Create a set to track contracts with dynamic calls. All contracts with dynamic calls could potentially be # calling functions internally, and thus we can't assume any function in such contracts isn't called by them. dynamic_call_contracts: Set[Contract] = set() diff --git a/slither/detectors/statements/array_length_assignment.py b/slither/detectors/statements/array_length_assignment.py index 93ba36da3..7f875fa9e 100644 --- a/slither/detectors/statements/array_length_assignment.py +++ b/slither/detectors/statements/array_length_assignment.py @@ -2,7 +2,12 @@ Module detecting assignment of array length """ -from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + ALL_SOLC_VERSIONS_04, + ALL_SOLC_VERSIONS_05, +) from slither.core.cfg.node import NodeType from slither.slithir.operations import Assignment, Length from slither.slithir.variables.reference import ReferenceVariable @@ -103,14 +108,13 @@ Note that storage slots here are indexed via a hash of the indexers; nonetheless Otherwise, thoroughly review the contract to ensure a user-controlled variable cannot reach an array length assignment.""" # endregion wiki_recommendation + VULNERABLE_SOLC_VERSIONS = ALL_SOLC_VERSIONS_04 + ALL_SOLC_VERSIONS_05 + def _detect(self): """ Detect array length assignments """ results = [] - # Starting from 0.6 .length is read only - if self.compilation_unit.solc_version >= "0.6.": - return results for contract in self.contracts: array_length_assignments = detect_array_length_assignment(contract) if array_length_assignments: diff --git a/tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol.0.5.10.StorageSignedIntegerArray.json b/tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol.0.5.10.StorageSignedIntegerArray.json index 139c92e55..5825bcacc 100644 --- a/tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol.0.5.10.StorageSignedIntegerArray.json +++ b/tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol.0.5.10.StorageSignedIntegerArray.json @@ -1,506 +1,3 @@ [ - [ - { - "elements": [ - { - "type": "contract", - "name": "A", - "source_mapping": { - "start": 25, - "length": 2256, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44, - 45 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - { - "type": "function", - "name": "bad1", - "source_mapping": { - "start": 601, - "length": 170, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 15, - 16, - 17 - ], - "starting_column": 3, - "ending_column": 4 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "A", - "source_mapping": { - "start": 25, - "length": 2256, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44, - 45 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "bad1(int128[3])" - } - }, - { - "type": "node", - "name": "intArray = userArray", - "source_mapping": { - "start": 746, - "length": 20, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 16 - ], - "starting_column": 5, - "ending_column": 25 - }, - "type_specific_fields": { - "parent": { - "type": "function", - "name": "bad1", - "source_mapping": { - "start": 601, - "length": 170, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 15, - 16, - 17 - ], - "starting_column": 3, - "ending_column": 4 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "A", - "source_mapping": { - "start": 25, - "length": 2256, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44, - 45 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "bad1(int128[3])" - } - } - } - } - ], - "description": "Contract A (tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#3-45) \n\t- Function A.bad1(int128[3]) (tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#15-17)\n\t\t- intArray = userArray (tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#16) has a storage signed integer array assignment\n", - "markdown": "Contract [A](tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#L3-L45) \n\t- Function [A.bad1(int128[3])](tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#L15-L17)\n\t\t- [intArray = userArray](tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#L16) has a storage signed integer array assignment\n", - "first_markdown_element": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#L3-L45", - "id": "7ba5efbfb61ba63a7ac01d376a0cede2fda18c2a2d8604c4a82cccec92ae2bdb", - "check": "storage-array", - "impact": "High", - "confidence": "Medium" - }, - { - "elements": [ - { - "type": "contract", - "name": "A", - "source_mapping": { - "start": 25, - "length": 2256, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44, - 45 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - { - "type": "function", - "name": "bad0", - "source_mapping": { - "start": 355, - "length": 132, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 10, - 11, - 12 - ], - "starting_column": 3, - "ending_column": 4 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "A", - "source_mapping": { - "start": 25, - "length": 2256, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44, - 45 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "bad0()" - } - }, - { - "type": "node", - "name": "intArray = (- 1,- 2,- 3)", - "source_mapping": { - "start": 384, - "length": 23, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 11 - ], - "starting_column": 5, - "ending_column": 28 - }, - "type_specific_fields": { - "parent": { - "type": "function", - "name": "bad0", - "source_mapping": { - "start": 355, - "length": 132, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 10, - 11, - 12 - ], - "starting_column": 3, - "ending_column": 4 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "A", - "source_mapping": { - "start": 25, - "length": 2256, - "filename_relative": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol", - "is_dependency": false, - "lines": [ - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44, - 45 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "bad0()" - } - } - } - } - ], - "description": "Contract A (tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#3-45) \n\t- Function A.bad0() (tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#10-12)\n\t\t- intArray = (- 1,- 2,- 3) (tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#11) has a storage signed integer array assignment\n", - "markdown": "Contract [A](tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#L3-L45) \n\t- Function [A.bad0()](tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#L10-L12)\n\t\t- [intArray = (- 1,- 2,- 3)](tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#L11) has a storage signed integer array assignment\n", - "first_markdown_element": "tests/detectors/storage-array/0.5.10/storage_signed_integer_array.sol#L3-L45", - "id": "da870be9a396bc52d2f6f8caeb00e6b8809ad1b6fb4c24a019568257b3404a2f", - "check": "storage-array", - "impact": "High", - "confidence": "Medium" - } - ] + [] ] \ No newline at end of file From 7648a871919ea5e8641dcbc8297070cd5a10ae13 Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Fri, 2 Dec 2022 10:38:14 -0600 Subject: [PATCH 04/12] update missing events wiki close https://github.com/crytic/slither/issues/1355 --- slither/detectors/operations/missing_events_arithmetic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/detectors/operations/missing_events_arithmetic.py b/slither/detectors/operations/missing_events_arithmetic.py index 04291d127..340f471a5 100644 --- a/slither/detectors/operations/missing_events_arithmetic.py +++ b/slither/detectors/operations/missing_events_arithmetic.py @@ -42,7 +42,7 @@ contract C { } } ``` -`updateOwner()` has no event, so it is difficult to track off-chain changes in the buy price. +`setBuyPrice()` does not emit an event, so it is difficult to track changes in the value of `buyPrice` off-chain. """ # endregion wiki_exploit_scenario From 8ad3ea7555edc9b038c3b098ab3bf616c0f6bb0f Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Fri, 2 Dec 2022 12:54:18 -0600 Subject: [PATCH 05/12] copy event arguments during ssa conversion --- slither/slithir/utils/ssa.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/slither/slithir/utils/ssa.py b/slither/slithir/utils/ssa.py index 827be8e18..a16047c19 100644 --- a/slither/slithir/utils/ssa.py +++ b/slither/slithir/utils/ssa.py @@ -690,7 +690,9 @@ def copy_ir(ir, *instances): return Delete(lvalue, variable) if isinstance(ir, EventCall): name = ir.name - return EventCall(name) + new_ir = EventCall(name) + new_ir.arguments = get_arguments(ir, *instances) + return new_ir if isinstance(ir, HighLevelCall): # include LibraryCall destination = get_variable(ir, lambda x: x.destination, *instances) function_name = ir.function_name From c80c3ba3b62c5e8b7783c3abfa70dadb8db49142 Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Tue, 6 Dec 2022 10:44:24 +0100 Subject: [PATCH 06/12] Fix dapp Based on @elopez work from https://github.com/crytic/crytic-compile/pull/320 --- scripts/ci_test_dapp.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/scripts/ci_test_dapp.sh b/scripts/ci_test_dapp.sh index b5051b131..b11ea46a5 100755 --- a/scripts/ci_test_dapp.sh +++ b/scripts/ci_test_dapp.sh @@ -2,6 +2,11 @@ ### Test Dapp integration +# work around having two python versions loading libraries from each other in CI +OLD_LD_LIBRARY_PATH="$LD_LIBRARY_PATH" +alias crytic-compile='LD_LIBRARY_PATH=$OLD_LD_LIBRARY_PATH crytic-compile' +unset LD_LIBRARY_PATH + mkdir test_dapp cd test_dapp || exit 255 # The dapp init process makes a temporary local git repo and needs certain values to be set From d22abb1f45e2c8e60df4439e1c1c065a5b2e5487 Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Tue, 6 Dec 2022 13:04:23 +0100 Subject: [PATCH 07/12] Improve protected variable detector Have the detector look through all the state variables written --- .../detectors/functions/protected_variable.py | 2 +- .../protected-vars/0.8.2/comment.sol | 19 ++ .../comment.sol.0.8.2.ProtectedVariables.json | 217 ------------------ 3 files changed, 20 insertions(+), 218 deletions(-) delete mode 100644 tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json diff --git a/slither/detectors/functions/protected_variable.py b/slither/detectors/functions/protected_variable.py index 0c1341d3c..cbd640e18 100644 --- a/slither/detectors/functions/protected_variable.py +++ b/slither/detectors/functions/protected_variable.py @@ -48,7 +48,7 @@ contract Buggy{ def _analyze_function(self, function: Function, contract: Contract) -> List[Output]: results = [] - for state_variable_written in function.state_variables_written: + for state_variable_written in function.all_state_variables_written(): if state_variable_written.write_protection: for function_sig in state_variable_written.write_protection: function_protection = contract.get_function_from_signature(function_sig) diff --git a/tests/detectors/protected-vars/0.8.2/comment.sol b/tests/detectors/protected-vars/0.8.2/comment.sol index e06ff3902..a49558b61 100644 --- a/tests/detectors/protected-vars/0.8.2/comment.sol +++ b/tests/detectors/protected-vars/0.8.2/comment.sol @@ -33,3 +33,22 @@ contract ReentrancyAndWrite{ } } +contract Internal { + /// @custom:security write-protection="onlyOwner()" + address owner; + + + + modifier onlyOwner(){ + // lets assume there is an access control + _; + } + + function buggy() public { + internal_write(); + } + + function internal_write() internal { + owner = msg.sender; + } +} \ No newline at end of file diff --git a/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json b/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json deleted file mode 100644 index cb1ead621..000000000 --- a/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json +++ /dev/null @@ -1,217 +0,0 @@ -[ - [ - { - "elements": [ - { - "type": "function", - "name": "set_not_protected", - "source_mapping": { - "start": 653, - "length": 85, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 31, - 32, - 33 - ], - "starting_column": 5, - "ending_column": 6 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "ReentrancyAndWrite", - "source_mapping": { - "start": 55, - "length": 685, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "set_not_protected()" - } - }, - { - "type": "function", - "name": "onlyOwner", - "source_mapping": { - "start": 210, - "length": 88, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 11, - 12, - 13, - 14 - ], - "starting_column": 5, - "ending_column": 6 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "ReentrancyAndWrite", - "source_mapping": { - "start": 55, - "length": 685, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "onlyOwner()" - } - }, - { - "type": "variable", - "name": "external_contract", - "source_mapping": { - "start": 184, - "length": 19, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 9 - ], - "starting_column": 5, - "ending_column": 24 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "ReentrancyAndWrite", - "source_mapping": { - "start": 55, - "length": 685, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34 - ], - "starting_column": 1, - "ending_column": 2 - } - } - } - } - ], - "description": "ReentrancyAndWrite.set_not_protected() (tests/detectors/protected-vars/0.8.2/comment.sol#31-33) should have ReentrancyAndWrite.onlyOwner() (tests/detectors/protected-vars/0.8.2/comment.sol#11-14) to protect ReentrancyAndWrite.external_contract (tests/detectors/protected-vars/0.8.2/comment.sol#9)\n", - "markdown": "[ReentrancyAndWrite.set_not_protected()](tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33) should have [ReentrancyAndWrite.onlyOwner()](tests/detectors/protected-vars/0.8.2/comment.sol#L11-L14) to protect [ReentrancyAndWrite.external_contract](tests/detectors/protected-vars/0.8.2/comment.sol#L9)\n", - "first_markdown_element": "tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33", - "id": "3f3bc8c8a9b3e23482f47f1133aceaed81c2c781c6aaf25656a8e578c9f6cb0e", - "check": "protected-vars", - "impact": "High", - "confidence": "High" - } - ] -] \ No newline at end of file From 3825b1ac496cdcffac4d1a0b5f2c12f95d02ccbd Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Tue, 6 Dec 2022 13:07:59 +0100 Subject: [PATCH 08/12] Add missing json --- .../comment.sol.0.8.2.ProtectedVariables.json | 400 ++++++++++++++++++ 1 file changed, 400 insertions(+) create mode 100644 tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json diff --git a/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json b/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json new file mode 100644 index 000000000..2c1a11fa4 --- /dev/null +++ b/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json @@ -0,0 +1,400 @@ +[ + [ + { + "elements": [ + { + "type": "function", + "name": "buggy", + "source_mapping": { + "start": 938, + "length": 57, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 47, + 48, + 49 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Internal", + "source_mapping": { + "start": 742, + "length": 331, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55 + ], + "starting_column": 1, + "ending_column": 0 + } + }, + "signature": "buggy()" + } + }, + { + "type": "function", + "name": "onlyOwner", + "source_mapping": { + "start": 844, + "length": 88, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 42, + 43, + 44, + 45 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Internal", + "source_mapping": { + "start": 742, + "length": 331, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55 + ], + "starting_column": 1, + "ending_column": 0 + } + }, + "signature": "onlyOwner()" + } + }, + { + "type": "variable", + "name": "owner", + "source_mapping": { + "start": 822, + "length": 13, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 38 + ], + "starting_column": 5, + "ending_column": 18 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Internal", + "source_mapping": { + "start": 742, + "length": 331, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55 + ], + "starting_column": 1, + "ending_column": 0 + } + } + } + } + ], + "description": "Internal.buggy() (tests/detectors/protected-vars/0.8.2/comment.sol#47-49) should have Internal.onlyOwner() (tests/detectors/protected-vars/0.8.2/comment.sol#42-45) to protect Internal.owner (tests/detectors/protected-vars/0.8.2/comment.sol#38)\n", + "markdown": "[Internal.buggy()](tests/detectors/protected-vars/0.8.2/comment.sol#L47-L49) should have [Internal.onlyOwner()](tests/detectors/protected-vars/0.8.2/comment.sol#L42-L45) to protect [Internal.owner](tests/detectors/protected-vars/0.8.2/comment.sol#L38)\n", + "first_markdown_element": "tests/detectors/protected-vars/0.8.2/comment.sol#L47-L49", + "id": "347d5dbdb03710066bc29d7772156fe5ff3d3371fa4eee4839ee221a1d0de0a4", + "check": "protected-vars", + "impact": "High", + "confidence": "High" + }, + { + "elements": [ + { + "type": "function", + "name": "set_not_protected", + "source_mapping": { + "start": 653, + "length": 85, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 31, + 32, + 33 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "set_not_protected()" + } + }, + { + "type": "function", + "name": "onlyOwner", + "source_mapping": { + "start": 210, + "length": 88, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 11, + 12, + 13, + 14 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "onlyOwner()" + } + }, + { + "type": "variable", + "name": "external_contract", + "source_mapping": { + "start": 184, + "length": 19, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 9 + ], + "starting_column": 5, + "ending_column": 24 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + } + } + } + ], + "description": "ReentrancyAndWrite.set_not_protected() (tests/detectors/protected-vars/0.8.2/comment.sol#31-33) should have ReentrancyAndWrite.onlyOwner() (tests/detectors/protected-vars/0.8.2/comment.sol#11-14) to protect ReentrancyAndWrite.external_contract (tests/detectors/protected-vars/0.8.2/comment.sol#9)\n", + "markdown": "[ReentrancyAndWrite.set_not_protected()](tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33) should have [ReentrancyAndWrite.onlyOwner()](tests/detectors/protected-vars/0.8.2/comment.sol#L11-L14) to protect [ReentrancyAndWrite.external_contract](tests/detectors/protected-vars/0.8.2/comment.sol#L9)\n", + "first_markdown_element": "tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33", + "id": "3f3bc8c8a9b3e23482f47f1133aceaed81c2c781c6aaf25656a8e578c9f6cb0e", + "check": "protected-vars", + "impact": "High", + "confidence": "High" + } + ] +] \ No newline at end of file From c02231ff1c42261ac573f06e85e78042ecda9e66 Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Tue, 6 Dec 2022 13:39:51 +0100 Subject: [PATCH 09/12] Add Codex vuln detector The detector requires: - The user to use the flag `--codex` (meaning that codex is not ran by default) - `openai` must be installed, and `OPENAI_API_KEY` set The detector works at the contract level, and send the whole contract body to codex --- setup.py | 1 + slither/__main__.py | 4 ++ slither/detectors/all_detectors.py | 1 + slither/detectors/functions/codex.py | 80 ++++++++++++++++++++++++++++ slither/slither.py | 3 ++ 5 files changed, 89 insertions(+) create mode 100644 slither/detectors/functions/codex.py diff --git a/setup.py b/setup.py index 510efd097..b29244713 100644 --- a/setup.py +++ b/setup.py @@ -26,6 +26,7 @@ setup( "deepdiff", "numpy", "solc-select>=v1.0.0b1", + "openai" ] }, license="AGPL-3.0", diff --git a/slither/__main__.py b/slither/__main__.py index 70357586e..00ea308d2 100644 --- a/slither/__main__.py +++ b/slither/__main__.py @@ -301,6 +301,10 @@ def parse_args( parser.add_argument("filename", help=argparse.SUPPRESS) + parser.add_argument( + "--codex", help="Enable codex (require an OpenAI API Key)", action="store_true", default=False + ) + cryticparser.init(parser) parser.add_argument( diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 2c8d24428..e8c8334b7 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -85,3 +85,4 @@ from .statements.msg_value_in_loop import MsgValueInLoop from .statements.delegatecall_in_loop import DelegatecallInLoop from .functions.protected_variable import ProtectedVariables from .functions.permit_domain_signature_collision import DomainSeparatorCollision +from .functions.codex import Codex diff --git a/slither/detectors/functions/codex.py b/slither/detectors/functions/codex.py new file mode 100644 index 000000000..daed06425 --- /dev/null +++ b/slither/detectors/functions/codex.py @@ -0,0 +1,80 @@ +import logging +import os +from typing import List + +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.utils.output import Output + +logger = logging.getLogger("Slither") + + +class Codex(AbstractDetector): + """ + Use codex to detect vulnerability + """ + + ARGUMENT = "codex" + HELP = "Use Codex to find vulnerabilities." + IMPACT = DetectorClassification.HIGH + CONFIDENCE = DetectorClassification.LOW + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#codex" + + WIKI_TITLE = "Codex" + WIKI_DESCRIPTION = "Use [codex](https://openai.com/blog/openai-codex/) to find vulnerabilities" + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """N/A""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = "Review codex's message." + + def _detect(self) -> List[Output]: + results: List[Output] = [] + + if not self.slither.codex_enabled: + return [] + + try: + # pylint: disable=import-outside-toplevel + import openai + except ImportError: + logging.info("OpenAI was not installed") + logging.info('run "pip install openai"') + return [] + + api_key = os.getenv("OPENAI_API_KEY") + if api_key is None: + logging.info( + "Please provide an Open API Key in OPENAI_API_KEY (https://beta.openai.com/account/api-keys)" + ) + return [] + openai.api_key = api_key + + for contract in self.compilation_unit.contracts: + prompt = "Is there a vulnerability in this solidity contracts?\n" + src_mapping = contract.source_mapping + content = contract.compilation_unit.core.source_code[src_mapping.filename.absolute] + start = src_mapping.start + end = src_mapping.start + src_mapping.length + prompt += content[start:end] + answer = openai.Completion.create( # type: ignore + model="text-davinci-003", prompt=prompt, temperature=0, max_tokens=200 + ) + + if "choices" in answer: + if answer["choices"]: + if "text" in answer["choices"][0]: + if "Yes," in answer["choices"][0]["text"]: + info = [ + "Codex detected a potential bug in ", + contract, + "\n", + answer["choices"][0]["text"], + "\n", + ] + + res = self.generate_result(info) + results.append(res) + + return results diff --git a/slither/slither.py b/slither/slither.py index dcfc0ad7e..a61e8255f 100644 --- a/slither/slither.py +++ b/slither/slither.py @@ -83,6 +83,9 @@ class Slither(SlitherCore): # pylint: disable=too-many-instance-attributes self.line_prefix = kwargs.get("change_line_prefix", "#") + # Indicate if codex-related features should be used + self.codex_enabled = kwargs.get("codex", False) + self._parsers: List[SlitherCompilationUnitSolc] = [] try: if isinstance(target, CryticCompile): From 5763c7431bcf4190b3b59f9b5c6de07777b4d31f Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Tue, 6 Dec 2022 13:58:16 +0100 Subject: [PATCH 10/12] black --- setup.py | 2 +- slither/__main__.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/setup.py b/setup.py index b29244713..03fe64c42 100644 --- a/setup.py +++ b/setup.py @@ -26,7 +26,7 @@ setup( "deepdiff", "numpy", "solc-select>=v1.0.0b1", - "openai" + "openai", ] }, license="AGPL-3.0", diff --git a/slither/__main__.py b/slither/__main__.py index 00ea308d2..8e32a1c43 100644 --- a/slither/__main__.py +++ b/slither/__main__.py @@ -302,7 +302,10 @@ def parse_args( parser.add_argument("filename", help=argparse.SUPPRESS) parser.add_argument( - "--codex", help="Enable codex (require an OpenAI API Key)", action="store_true", default=False + "--codex", + help="Enable codex (require an OpenAI API Key)", + action="store_true", + default=False, ) cryticparser.init(parser) From 00d33c6e781f05e6f279a4c07fe5754a457c03a0 Mon Sep 17 00:00:00 2001 From: Richie Date: Tue, 6 Dec 2022 13:56:38 -0800 Subject: [PATCH 11/12] feat: parameterize OpenAI inputs --- slither/__main__.py | 30 ++++++++++- slither/detectors/functions/codex.py | 75 ++++++++++++++++++++-------- slither/slither.py | 6 ++- slither/utils/command_line.py | 5 ++ 4 files changed, 94 insertions(+), 22 deletions(-) diff --git a/slither/__main__.py b/slither/__main__.py index 8e32a1c43..9426a6de9 100644 --- a/slither/__main__.py +++ b/slither/__main__.py @@ -305,7 +305,35 @@ def parse_args( "--codex", help="Enable codex (require an OpenAI API Key)", action="store_true", - default=False, + default=defaults_flag_in_config["codex"], + ) + + parser.add_argument( + "--codex-contracts", + help="Comma separated list of contracts to submit to OpenAI Codex", + action="store", + default=defaults_flag_in_config["codex_contracts"], + ) + + parser.add_argument( + "--codex-model", + help="Name of the Codex model to use (affects pricing). Defaults to 'text-davinci-003'", + action="store", + default=defaults_flag_in_config["codex_model"], + ) + + parser.add_argument( + "--codex-temperature", + help="Temperature to use with Codex. Lower number indicates a more precise answer while higher numbers return more creative answers. Defaults to 0", + action="store", + default=defaults_flag_in_config["codex_temperature"], + ) + + parser.add_argument( + "--codex-max-tokens", + help="Maximum amount of tokens to use on the response. This number plus the size of the prompt can be no larger than the limit (4097 for text-davinci-003)", + action="store", + default=defaults_flag_in_config["codex_max_tokens"], ) cryticparser.init(parser) diff --git a/slither/detectors/functions/codex.py b/slither/detectors/functions/codex.py index daed06425..4dca44775 100644 --- a/slither/detectors/functions/codex.py +++ b/slither/detectors/functions/codex.py @@ -7,6 +7,7 @@ from slither.utils.output import Output logger = logging.getLogger("Slither") +VULN_FOUND = "VULN_FOUND" class Codex(AbstractDetector): """ @@ -52,29 +53,63 @@ class Codex(AbstractDetector): openai.api_key = api_key for contract in self.compilation_unit.contracts: - prompt = "Is there a vulnerability in this solidity contracts?\n" + if self.slither.codex_contracts != "all" and contract.name not in self.slither.codex_contracts.split(","): + continue + prompt = "Analyze this Solidity contract and find the vulnerabilities. If you find any vulnerabilities, begin the response with {}".format(VULN_FOUND) src_mapping = contract.source_mapping content = contract.compilation_unit.core.source_code[src_mapping.filename.absolute] start = src_mapping.start end = src_mapping.start + src_mapping.length prompt += content[start:end] - answer = openai.Completion.create( # type: ignore - model="text-davinci-003", prompt=prompt, temperature=0, max_tokens=200 - ) - - if "choices" in answer: - if answer["choices"]: - if "text" in answer["choices"][0]: - if "Yes," in answer["choices"][0]["text"]: - info = [ - "Codex detected a potential bug in ", - contract, - "\n", - answer["choices"][0]["text"], - "\n", - ] - - res = self.generate_result(info) - results.append(res) - + logging.info("Querying OpenAI") + print("Querying OpenAI") + answer = "" + res = {} + try: + res = openai.Completion.create( # type: ignore + prompt=prompt, + model=self.slither.codex_model, + temperature=self.slither.codex_temperature, + max_tokens=self.slither.codex_max_tokens, + ) + except Exception as e: + print("OpenAI request failed: " + str(e)) + logging.info("OpenAI request failed: " + str(e)) + + """ OpenAI completion response shape example: + { + "choices": [ + { + "finish_reason": "stop", + "index": 0, + "logprobs": null, + "text": "VULNERABILITIES:. The withdraw() function does not check..." + } + ], + "created": 1670357537, + "id": "cmpl-6KYaXdA6QIisHlTMM7RCJ1nR5wTKx", + "model": "text-davinci-003", + "object": "text_completion", + "usage": { + "completion_tokens": 80, + "prompt_tokens": 249, + "total_tokens": 329 + } + } """ + + if len(res.get("choices", [])) and VULN_FOUND in res["choices"][0].get("text", ""): + # remove VULN_FOUND keyword and cleanup + answer = res["choices"][0]["text"].replace(VULN_FOUND, "").replace("\n", "").replace(": ", "") + + if len(answer): + info = [ + "Codex detected a potential bug in ", + contract, + "\n", + answer, + "\n", + ] + + res = self.generate_result(info) + results.append(res) return results diff --git a/slither/slither.py b/slither/slither.py index a61e8255f..0b1f57a37 100644 --- a/slither/slither.py +++ b/slither/slither.py @@ -83,8 +83,12 @@ class Slither(SlitherCore): # pylint: disable=too-many-instance-attributes self.line_prefix = kwargs.get("change_line_prefix", "#") - # Indicate if codex-related features should be used + # Indicate if Codex related features should be used self.codex_enabled = kwargs.get("codex", False) + self.codex_contracts = kwargs.get("codex_contracts") + self.codex_model = kwargs.get("codex_model") + self.codex_temperature = kwargs.get("codex_temperature") + self.codex_max_tokens = kwargs.get("codex_max_tokens") self._parsers: List[SlitherCompilationUnitSolc] = [] try: diff --git a/slither/utils/command_line.py b/slither/utils/command_line.py index c2fef5eca..f774437a1 100644 --- a/slither/utils/command_line.py +++ b/slither/utils/command_line.py @@ -29,6 +29,11 @@ JSON_OUTPUT_TYPES = [ # Those are the flags shared by the command line and the config file defaults_flag_in_config = { + "codex": False, + "codex_contracts": "all", + "codex_model": "text-davinci-003", + "codex_temperature": 0, + "codex_max_tokens": 300, "detectors_to_run": "all", "printers_to_run": None, "detectors_to_exclude": None, From f62433bd50fbc8d95a0a328b67a051cc305b6498 Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Wed, 7 Dec 2022 11:07:43 +0100 Subject: [PATCH 12/12] Create utils.codex Refactor functions/codex Minor improvements --- slither/__main__.py | 80 +++++++------- slither/detectors/functions/codex.py | 149 +++++++++++++++------------ slither/slither.py | 9 +- slither/utils/codex.py | 53 ++++++++++ slither/utils/command_line.py | 1 + 5 files changed, 187 insertions(+), 105 deletions(-) create mode 100644 slither/utils/codex.py diff --git a/slither/__main__.py b/slither/__main__.py index 9426a6de9..75707af06 100644 --- a/slither/__main__.py +++ b/slither/__main__.py @@ -166,7 +166,6 @@ def process_from_asts( def get_detectors_and_printers() -> Tuple[ List[Type[AbstractDetector]], List[Type[AbstractPrinter]] ]: - detectors_ = [getattr(all_detectors, name) for name in dir(all_detectors)] detectors = [d for d in detectors_ if inspect.isclass(d) and issubclass(d, AbstractDetector)] @@ -286,7 +285,6 @@ def parse_filter_paths(args: argparse.Namespace) -> List[str]: def parse_args( detector_classes: List[Type[AbstractDetector]], printer_classes: List[Type[AbstractPrinter]] ) -> argparse.Namespace: - usage = "slither target [flag]\n" usage += "\ntarget can be:\n" usage += "\t- file.sol // a Solidity file\n" @@ -301,41 +299,6 @@ def parse_args( parser.add_argument("filename", help=argparse.SUPPRESS) - parser.add_argument( - "--codex", - help="Enable codex (require an OpenAI API Key)", - action="store_true", - default=defaults_flag_in_config["codex"], - ) - - parser.add_argument( - "--codex-contracts", - help="Comma separated list of contracts to submit to OpenAI Codex", - action="store", - default=defaults_flag_in_config["codex_contracts"], - ) - - parser.add_argument( - "--codex-model", - help="Name of the Codex model to use (affects pricing). Defaults to 'text-davinci-003'", - action="store", - default=defaults_flag_in_config["codex_model"], - ) - - parser.add_argument( - "--codex-temperature", - help="Temperature to use with Codex. Lower number indicates a more precise answer while higher numbers return more creative answers. Defaults to 0", - action="store", - default=defaults_flag_in_config["codex_temperature"], - ) - - parser.add_argument( - "--codex-max-tokens", - help="Maximum amount of tokens to use on the response. This number plus the size of the prompt can be no larger than the limit (4097 for text-davinci-003)", - action="store", - default=defaults_flag_in_config["codex_max_tokens"], - ) - cryticparser.init(parser) parser.add_argument( @@ -351,6 +314,7 @@ def parse_args( "Checklist (consider using https://github.com/crytic/slither-action)" ) group_misc = parser.add_argument_group("Additional options") + group_codex = parser.add_argument_group("Codex (https://beta.openai.com/docs/guides/code)") group_detector.add_argument( "--detect", @@ -591,6 +555,48 @@ def parse_args( default=False, ) + group_codex.add_argument( + "--codex", + help="Enable codex (require an OpenAI API Key)", + action="store_true", + default=defaults_flag_in_config["codex"], + ) + + group_codex.add_argument( + "--codex-log", + help="Log codex queries (in crytic_export/codex/)", + action="store_true", + default=False, + ) + + group_codex.add_argument( + "--codex-contracts", + help="Comma separated list of contracts to submit to OpenAI Codex", + action="store", + default=defaults_flag_in_config["codex_contracts"], + ) + + group_codex.add_argument( + "--codex-model", + help="Name of the Codex model to use (affects pricing). Defaults to 'text-davinci-003'", + action="store", + default=defaults_flag_in_config["codex_model"], + ) + + group_codex.add_argument( + "--codex-temperature", + help="Temperature to use with Codex. Lower number indicates a more precise answer while higher numbers return more creative answers. Defaults to 0", + action="store", + default=defaults_flag_in_config["codex_temperature"], + ) + + group_codex.add_argument( + "--codex-max-tokens", + help="Maximum amount of tokens to use on the response. This number plus the size of the prompt can be no larger than the limit (4097 for text-davinci-003)", + action="store", + default=defaults_flag_in_config["codex_max_tokens"], + ) + # debugger command parser.add_argument("--debug", help=argparse.SUPPRESS, action="store_true", default=False) diff --git a/slither/detectors/functions/codex.py b/slither/detectors/functions/codex.py index 4dca44775..fb00f64c0 100644 --- a/slither/detectors/functions/codex.py +++ b/slither/detectors/functions/codex.py @@ -1,14 +1,16 @@ import logging -import os -from typing import List +import uuid +from typing import List, Union from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification -from slither.utils.output import Output +from slither.utils import codex +from slither.utils.output import Output, SupportedOutput logger = logging.getLogger("Slither") VULN_FOUND = "VULN_FOUND" + class Codex(AbstractDetector): """ Use codex to detect vulnerability @@ -30,79 +32,98 @@ class Codex(AbstractDetector): WIKI_RECOMMENDATION = "Review codex's message." + def _run_codex(self, logging_file: str, prompt: str) -> str: + """ + Handle the codex logic + + Args: + logging_file (str): file where to log the queries + prompt (str): prompt to send to codex + + Returns: + codex answer (str) + """ + openai_module = codex.openai_module() # type: ignore + if openai_module is None: + return "" + + if self.slither.codex_log: + codex.log_codex(logging_file, "Q: " + prompt) + + answer = "" + res = {} + try: + res = openai_module.Completion.create( + prompt=prompt, + model=self.slither.codex_model, + temperature=self.slither.codex_temperature, + max_tokens=self.slither.codex_max_tokens, + ) + except Exception as e: # pylint: disable=broad-except + logger.info("OpenAI request failed: " + str(e)) + + # """ OpenAI completion response shape example: + # { + # "choices": [ + # { + # "finish_reason": "stop", + # "index": 0, + # "logprobs": null, + # "text": "VULNERABILITIES:. The withdraw() function does not check..." + # } + # ], + # "created": 1670357537, + # "id": "cmpl-6KYaXdA6QIisHlTMM7RCJ1nR5wTKx", + # "model": "text-davinci-003", + # "object": "text_completion", + # "usage": { + # "completion_tokens": 80, + # "prompt_tokens": 249, + # "total_tokens": 329 + # } + # } """ + + if res: + if self.slither.codex_log: + codex.log_codex(logging_file, "A: " + str(res)) + else: + codex.log_codex(logging_file, "A: Codex failed") + + if res.get("choices", []) and VULN_FOUND in res["choices"][0].get("text", ""): + # remove VULN_FOUND keyword and cleanup + answer = ( + res["choices"][0]["text"] + .replace(VULN_FOUND, "") + .replace("\n", "") + .replace(": ", "") + ) + return answer + def _detect(self) -> List[Output]: results: List[Output] = [] if not self.slither.codex_enabled: return [] - try: - # pylint: disable=import-outside-toplevel - import openai - except ImportError: - logging.info("OpenAI was not installed") - logging.info('run "pip install openai"') - return [] - - api_key = os.getenv("OPENAI_API_KEY") - if api_key is None: - logging.info( - "Please provide an Open API Key in OPENAI_API_KEY (https://beta.openai.com/account/api-keys)" - ) - return [] - openai.api_key = api_key + logging_file = str(uuid.uuid4()) for contract in self.compilation_unit.contracts: - if self.slither.codex_contracts != "all" and contract.name not in self.slither.codex_contracts.split(","): + if ( + self.slither.codex_contracts != "all" + and contract.name not in self.slither.codex_contracts.split(",") + ): continue - prompt = "Analyze this Solidity contract and find the vulnerabilities. If you find any vulnerabilities, begin the response with {}".format(VULN_FOUND) + prompt = f"Analyze this Solidity contract and find the vulnerabilities. If you find any vulnerabilities, begin the response with {VULN_FOUND}\n" src_mapping = contract.source_mapping content = contract.compilation_unit.core.source_code[src_mapping.filename.absolute] start = src_mapping.start end = src_mapping.start + src_mapping.length prompt += content[start:end] - logging.info("Querying OpenAI") - print("Querying OpenAI") - answer = "" - res = {} - try: - res = openai.Completion.create( # type: ignore - prompt=prompt, - model=self.slither.codex_model, - temperature=self.slither.codex_temperature, - max_tokens=self.slither.codex_max_tokens, - ) - except Exception as e: - print("OpenAI request failed: " + str(e)) - logging.info("OpenAI request failed: " + str(e)) - - """ OpenAI completion response shape example: - { - "choices": [ - { - "finish_reason": "stop", - "index": 0, - "logprobs": null, - "text": "VULNERABILITIES:. The withdraw() function does not check..." - } - ], - "created": 1670357537, - "id": "cmpl-6KYaXdA6QIisHlTMM7RCJ1nR5wTKx", - "model": "text-davinci-003", - "object": "text_completion", - "usage": { - "completion_tokens": 80, - "prompt_tokens": 249, - "total_tokens": 329 - } - } """ - - if len(res.get("choices", [])) and VULN_FOUND in res["choices"][0].get("text", ""): - # remove VULN_FOUND keyword and cleanup - answer = res["choices"][0]["text"].replace(VULN_FOUND, "").replace("\n", "").replace(": ", "") - - if len(answer): - info = [ + + answer = self._run_codex(logging_file, prompt) + + if answer: + info: List[Union[str, SupportedOutput]] = [ "Codex detected a potential bug in ", contract, "\n", @@ -110,6 +131,6 @@ class Codex(AbstractDetector): "\n", ] - res = self.generate_result(info) - results.append(res) + new_result = self.generate_result(info) + results.append(new_result) return results diff --git a/slither/slither.py b/slither/slither.py index 0b1f57a37..81e920d01 100644 --- a/slither/slither.py +++ b/slither/slither.py @@ -85,10 +85,11 @@ class Slither(SlitherCore): # pylint: disable=too-many-instance-attributes # Indicate if Codex related features should be used self.codex_enabled = kwargs.get("codex", False) - self.codex_contracts = kwargs.get("codex_contracts") - self.codex_model = kwargs.get("codex_model") - self.codex_temperature = kwargs.get("codex_temperature") - self.codex_max_tokens = kwargs.get("codex_max_tokens") + self.codex_contracts = kwargs.get("codex_contracts", "all") + self.codex_model = kwargs.get("codex_model", "text-davinci-003") + self.codex_temperature = kwargs.get("codex_temperature", 0) + self.codex_max_tokens = kwargs.get("codex_max_tokens", 300) + self.codex_log = kwargs.get("codex_log", False) self._parsers: List[SlitherCompilationUnitSolc] = [] try: diff --git a/slither/utils/codex.py b/slither/utils/codex.py new file mode 100644 index 000000000..0040fb03c --- /dev/null +++ b/slither/utils/codex.py @@ -0,0 +1,53 @@ +import logging +import os +from pathlib import Path + +logger = logging.getLogger("Slither") + + +# TODO: investigate how to set the correct return type +# So that the other modules can work with openai +def openai_module(): # type: ignore + """ + Return the openai module + Consider checking the usage of open (slither.codex_enabled) before using this function + + Returns: + Optional[the openai module] + """ + try: + # pylint: disable=import-outside-toplevel + import openai + + api_key = os.getenv("OPENAI_API_KEY") + if api_key is None: + logger.info( + "Please provide an Open API Key in OPENAI_API_KEY (https://beta.openai.com/account/api-keys)" + ) + return None + openai.api_key = api_key + except ImportError: + logger.info("OpenAI was not installed") # type: ignore + logger.info('run "pip install openai"') + return None + return openai + + +def log_codex(filename: str, prompt: str) -> None: + """ + Log the prompt in crytic/export/codex/filename + Append to the file + + Args: + filename: filename to write to + prompt: prompt to write + + Returns: + None + """ + + Path("crytic_export/codex").mkdir(parents=True, exist_ok=True) + + with open(Path("crytic_export/codex", filename), "a", encoding="utf8") as file: + file.write(prompt) + file.write("\n") diff --git a/slither/utils/command_line.py b/slither/utils/command_line.py index f774437a1..71305c56e 100644 --- a/slither/utils/command_line.py +++ b/slither/utils/command_line.py @@ -34,6 +34,7 @@ defaults_flag_in_config = { "codex_model": "text-davinci-003", "codex_temperature": 0, "codex_max_tokens": 300, + "codex_log": False, "detectors_to_run": "all", "printers_to_run": None, "detectors_to_exclude": None,