diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 6301979b50..893f9f5e7e 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -21,6 +21,7 @@ from .variables.unused_state_variables import UnusedStateVars from .variables.could_be_constant import CouldBeConstant from .variables.could_be_immutable import CouldBeImmutable +from .variables.complex_struct_getter import ComplexStructGetter from .statements.tx_origin import TxOrigin from .statements.assembly import Assembly from .operations.low_level_calls import LowLevelCalls diff --git a/slither/detectors/variables/complex_struct_getter.py b/slither/detectors/variables/complex_struct_getter.py new file mode 100644 index 0000000000..ad05a64244 --- /dev/null +++ b/slither/detectors/variables/complex_struct_getter.py @@ -0,0 +1,127 @@ +""" +Module detecting public state variables with complex struct types +where the automatic getter omits array and mapping members. +""" + +from slither.core.declarations import Structure +from slither.core.solidity_types import ArrayType, MappingType, UserDefinedType +from slither.core.solidity_types.type import Type +from slither.core.variables.structure_variable import StructureVariable +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + DETECTOR_INFO, +) +from slither.utils.output import Output + + +def _find_omitted_members( + struct: Structure, seen: set[str] | None = None +) -> list[StructureVariable]: + """Find struct members that are omitted from the automatic getter. + + Solidity's automatic getters skip array and mapping members in structs. + This function also recurses into nested structs to find deeply omitted members. + """ + if seen is None: + seen = set() + + # Prevent infinite recursion from recursive struct types + if struct.canonical_name in seen: + return [] + seen.add(struct.canonical_name) + + omitted: list[StructureVariable] = [] + for member in struct.elems_ordered: + member_type = member.type + if isinstance(member_type, (ArrayType, MappingType)): + omitted.append(member) + elif isinstance(member_type, UserDefinedType) and isinstance(member_type.type, Structure): + # Recursively check nested structs + nested_omitted = _find_omitted_members(member_type.type, seen) + if nested_omitted: + omitted.extend(nested_omitted) + + return omitted + + +class ComplexStructGetter(AbstractDetector): + """ + Detect public state variables with struct types where the automatic getter + omits array or mapping members. + """ + + ARGUMENT = "complex-struct-getter" + HELP = "Public struct getters with omitted members" + IMPACT = DetectorClassification.INFORMATIONAL + CONFIDENCE = DetectorClassification.HIGH + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#complex-struct-getter" + + WIKI_TITLE = "Complex struct getter" + WIKI_DESCRIPTION = ( + "Detect public state variables containing structs where the automatic getter " + "omits array and mapping members. Solidity's generated getters skip these types, " + "which may cause confusion about data accessibility." + ) + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +struct UserData { + string name; + uint256 balance; + uint256[] tokenIds; + mapping(address => uint256) allowances; +} + +contract Example { + UserData public userData; +} +``` +The automatic getter for `userData` will not return `tokenIds` or `allowances`, \ +potentially causing integration issues with external contracts or front-ends.""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = ( + "Create a custom getter function that returns the omitted members, " + "or separate complex members into their own state variables." + ) + + def _detect(self) -> list[Output]: + results = [] + + for contract in self.compilation_unit.contracts_derived: + if contract.is_interface or contract.is_from_dependency(): + continue + + for var in contract.state_variables_declared: + if var.visibility != "public": + continue + + if not isinstance(var.type, UserDefinedType): + continue + + if not isinstance(var.type.type, Structure): + continue + + struct = var.type.type + omitted = _find_omitted_members(struct) + + if not omitted: + continue + + omitted_names = ", ".join(f"{m.name} ({m.type})" for m in omitted) + + info: DETECTOR_INFO = [ + var, + " is a public state variable of type ", + struct, + " whose automatic getter omits: ", + omitted_names, + "\n", + ] + res = self.generate_result(info) + results.append(res) + + return results diff --git a/tests/e2e/detectors/snapshots/detectors__detector_ComplexStructGetter_0_8_0_complex_struct_getter_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_ComplexStructGetter_0_8_0_complex_struct_getter_sol__0.txt new file mode 100644 index 0000000000..a633a9cffb --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_ComplexStructGetter_0_8_0_complex_struct_getter_sol__0.txt @@ -0,0 +1,6 @@ +DirectArrayMapping.config (complex_struct_getter.sol#12) is a public state variable of type DirectArrayMapping.Config (complex_struct_getter.sol#7-11) whose automatic getter omits: limits (uint256[]), whitelist (mapping(address => bool)) + +NestedStruct.settings (complex_struct_getter.sol#23) is a public state variable of type NestedStruct.Outer (complex_struct_getter.sol#19-22) whose automatic getter omits: values (uint256[]) + +MappingOnly.permissions (complex_struct_getter.sol#31) is a public state variable of type MappingOnly.Permissions (complex_struct_getter.sol#27-30) whose automatic getter omits: allowed (mapping(address => bool)) + diff --git a/tests/e2e/detectors/test_data/complex-struct-getter/0.8.0/complex_struct_getter.sol b/tests/e2e/detectors/test_data/complex-struct-getter/0.8.0/complex_struct_getter.sol new file mode 100644 index 0000000000..c72d095d14 --- /dev/null +++ b/tests/e2e/detectors/test_data/complex-struct-getter/0.8.0/complex_struct_getter.sol @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +// --- Should be flagged --- + +contract DirectArrayMapping { + struct Config { + address owner; + uint256[] limits; + mapping(address => bool) whitelist; + } + Config public config; +} + +contract NestedStruct { + struct Inner { + uint256[] values; + } + struct Outer { + Inner data; + string name; + } + Outer public settings; +} + +contract MappingOnly { + struct Permissions { + uint256 level; + mapping(address => bool) allowed; + } + Permissions public permissions; +} + +// --- Should NOT be flagged --- + +contract SimpleStruct { + struct Info { + address owner; + uint256 balance; + string name; + } + Info public info; +} + +contract PrivateComplex { + struct Data { + uint256[] items; + mapping(address => uint256) balances; + } + Data internal data; + + function getData() external view returns (uint256[] memory) { + return data.items; + } +} + +contract NoStruct { + uint256 public value; + address public owner; +} + +contract NestedSimple { + struct InnerSimple { + uint256 x; + uint256 y; + } + struct OuterSimple { + InnerSimple point; + string label; + } + OuterSimple public shape; +} diff --git a/tests/e2e/detectors/test_data/complex-struct-getter/0.8.0/complex_struct_getter.sol-0.8.0.zip b/tests/e2e/detectors/test_data/complex-struct-getter/0.8.0/complex_struct_getter.sol-0.8.0.zip new file mode 100644 index 0000000000..b28e993042 Binary files /dev/null and b/tests/e2e/detectors/test_data/complex-struct-getter/0.8.0/complex_struct_getter.sol-0.8.0.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index 7bd6e34600..9c73b45509 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -539,6 +539,11 @@ def id_test(test_item: Test): "immut_state_variables.sol", "0.8.0", ), + Test( + all_detectors.ComplexStructGetter, + "complex_struct_getter.sol", + "0.8.0", + ), Test( all_detectors.ExternalFunction, "external_function.sol",