diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 6301979b50..6186205e81 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -81,6 +81,8 @@ from .statements.unary import IncorrectUnaryExpressionDetection from .operations.missing_zero_address_validation import MissingZeroAddressValidation from .functions.dead_code import DeadCode +from .functions.unused_events import UnusedEvents +from .functions.unused_modifiers import UnusedModifiers from .statements.write_after_write import WriteAfterWrite from .statements.msg_value_in_loop import MsgValueInLoop from .statements.msg_value_in_nonpayable import MsgValueInNonPayable diff --git a/slither/detectors/functions/unused_events.py b/slither/detectors/functions/unused_events.py new file mode 100644 index 0000000000..51fc6cfaf6 --- /dev/null +++ b/slither/detectors/functions/unused_events.py @@ -0,0 +1,73 @@ +""" +Module detecting unused events +""" + +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + DETECTOR_INFO, +) +from slither.slithir.operations.event_call import EventCall +from slither.utils.output import Output + + +class UnusedEvents(AbstractDetector): + """ + Detect events that are declared but never emitted + """ + + ARGUMENT = "unused-events" + HELP = "Events that are not emitted" + IMPACT = DetectorClassification.INFORMATIONAL + CONFIDENCE = DetectorClassification.HIGH + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#unused-events" + + WIKI_TITLE = "Unused events" + WIKI_DESCRIPTION = "Detect events that are declared but never emitted." + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract Example { + event Unused(address indexed user); + event Used(uint256 value); + + function action() external { + emit Used(100); + } +} +``` +`Unused` is never emitted and should be removed.""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = "Remove unused events or emit them where appropriate." + + def _detect(self) -> list[Output]: + results = [] + + # Collect all emitted event names across the entire compilation unit + events_emitted: set[str] = set() + for contract in self.compilation_unit.contracts_derived: + for function in contract.functions_and_modifiers: + for node in function.nodes: + for ir in node.irs: + if isinstance(ir, EventCall): + events_emitted.add(str(ir.name)) + + for contract in sorted(self.compilation_unit.contracts, key=lambda x: x.name): + if contract.is_interface or contract.is_from_dependency(): + continue + + for event in sorted(contract.events_declared, key=lambda x: x.full_name): + if event.name not in events_emitted: + info: DETECTOR_INFO = [ + event, + " is never emitted in ", + contract, + "\n", + ] + res = self.generate_result(info) + results.append(res) + + return results diff --git a/slither/detectors/functions/unused_modifiers.py b/slither/detectors/functions/unused_modifiers.py new file mode 100644 index 0000000000..66c9309d3b --- /dev/null +++ b/slither/detectors/functions/unused_modifiers.py @@ -0,0 +1,105 @@ +""" +Module detecting unused modifiers +""" + +from slither.core.declarations import Modifier +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + DETECTOR_INFO, +) +from slither.utils.output import Output + + +class UnusedModifiers(AbstractDetector): + """ + Detect modifiers that are declared but never applied to any function + """ + + ARGUMENT = "unused-modifiers" + HELP = "Modifiers that are not applied" + IMPACT = DetectorClassification.INFORMATIONAL + CONFIDENCE = DetectorClassification.HIGH + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#unused-modifiers" + + WIKI_TITLE = "Unused modifiers" + WIKI_DESCRIPTION = "Detect modifiers that are declared but never applied to any function." + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract Example { + modifier unused() { + require(msg.sender == address(0)); + _; + } + + modifier used() { + require(msg.value > 0); + _; + } + + function pay() external payable used { + // ... + } +} +``` +`unused` is never applied and should be removed.""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = "Remove unused modifiers or apply them where appropriate." + + def _detect(self) -> list[Output]: + results = [] + + # Collect all modifiers that are applied to any function + modifiers_used: set[str] = set() + for contract in self.compilation_unit.contracts_derived: + for function in contract.functions: + for modifier in function.modifiers: + if isinstance(modifier, Modifier): + modifiers_used.add(modifier.canonical_name) + + # Collect virtual modifiers that are overridden in child contracts + # (overridden_by may not be populated for modifiers, so check manually) + overridden_modifiers: set[str] = set() + for contract in self.compilation_unit.contracts: + for inherited_mod in contract.modifiers_inherited: + for declared_mod in contract.modifiers_declared: + if inherited_mod.name == declared_mod.name: + overridden_modifiers.add(inherited_mod.canonical_name) + + for modifier in sorted(self.compilation_unit.modifiers, key=lambda x: x.canonical_name): + if not isinstance(modifier, Modifier): + continue + + if modifier.contract_declarer.is_from_dependency(): + continue + + if modifier.contract_declarer.is_interface: + continue + + # Skip if the modifier is used + if modifier.canonical_name in modifiers_used: + continue + + # Skip if not implemented (abstract) + if not modifier.is_implemented: + continue + + # Skip virtual modifiers that are overridden in child contracts + # (part of inheritance design pattern) + if modifier.is_virtual and ( + modifier.overridden_by or modifier.canonical_name in overridden_modifiers + ): + continue + + info: DETECTOR_INFO = [ + modifier, + " is never used and should be removed\n", + ] + res = self.generate_result(info) + results.append(res) + + return results diff --git a/tests/e2e/detectors/snapshots/detectors__detector_UnusedEvents_0_8_0_unused_events_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_UnusedEvents_0_8_0_unused_events_sol__0.txt new file mode 100644 index 0000000000..86d7d4cf00 --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_UnusedEvents_0_8_0_unused_events_sol__0.txt @@ -0,0 +1,6 @@ +UnusedEventContract.NeverEmitted(address) (unused_events.sol#7) is never emitted in UnusedEventContract (unused_events.sol#6-15) + +UnusedEventContract.AlsoNeverEmitted(uint256,string) (unused_events.sol#8) is never emitted in UnusedEventContract (unused_events.sol#6-15) + +BaseUnused.OrphanEvent(address) (unused_events.sol#46) is never emitted in BaseUnused (unused_events.sol#45-47) + diff --git a/tests/e2e/detectors/snapshots/detectors__detector_UnusedModifiers_0_8_0_unused_modifiers_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_UnusedModifiers_0_8_0_unused_modifiers_sol__0.txt new file mode 100644 index 0000000000..203c6c77ef --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_UnusedModifiers_0_8_0_unused_modifiers_sol__0.txt @@ -0,0 +1,6 @@ +UnusedModifierContract.alsoNeverUsed() (unused_modifiers.sol#14-17) is never used and should be removed + +UnusedVirtual.unusedVirtualMod() (unused_modifiers.sol#75-77) is never used and should be removed + +UnusedModifierContract.neverUsed() (unused_modifiers.sol#9-12) is never used and should be removed + diff --git a/tests/e2e/detectors/test_data/unused-events/0.8.0/unused_events.sol b/tests/e2e/detectors/test_data/unused-events/0.8.0/unused_events.sol new file mode 100644 index 0000000000..8cb005b963 --- /dev/null +++ b/tests/e2e/detectors/test_data/unused-events/0.8.0/unused_events.sol @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +// --- Should be flagged --- + +contract UnusedEventContract { + event NeverEmitted(address indexed user); + event AlsoNeverEmitted(uint256 value, string reason); + + event ActuallyEmitted(uint256 value); + + function action() external { + emit ActuallyEmitted(100); + } +} + +// --- Should NOT be flagged --- + +contract EmitsAll { + event Transfer(address indexed from, address indexed to, uint256 amount); + event Approval(address indexed owner, address indexed spender, uint256 amount); + + function transfer(address to, uint256 amount) external { + emit Transfer(msg.sender, to, amount); + } + + function approve(address spender, uint256 amount) external { + emit Approval(msg.sender, spender, amount); + } +} + +// --- Inheritance tests --- + +contract Base { + event BaseEvent(uint256 value); +} + +contract Child is Base { + function emitIt() external { + emit BaseEvent(42); + } +} + +// Unused event in base, never emitted anywhere +contract BaseUnused { + event OrphanEvent(address who); +} + +contract ChildNoEmit is BaseUnused { + function doNothing() external pure {} +} + +// --- Interface should not be flagged --- + +interface IExample { + event InterfaceEvent(uint256 id); +} + +contract Implementor is IExample { + // InterfaceEvent is inherited from interface, not declared here + function trigger() external { + emit InterfaceEvent(1); + } +} diff --git a/tests/e2e/detectors/test_data/unused-events/0.8.0/unused_events.sol-0.8.0.zip b/tests/e2e/detectors/test_data/unused-events/0.8.0/unused_events.sol-0.8.0.zip new file mode 100644 index 0000000000..cd6f4efd03 Binary files /dev/null and b/tests/e2e/detectors/test_data/unused-events/0.8.0/unused_events.sol-0.8.0.zip differ diff --git a/tests/e2e/detectors/test_data/unused-modifiers/0.8.0/unused_modifiers.sol b/tests/e2e/detectors/test_data/unused-modifiers/0.8.0/unused_modifiers.sol new file mode 100644 index 0000000000..a9ea2af5a5 --- /dev/null +++ b/tests/e2e/detectors/test_data/unused-modifiers/0.8.0/unused_modifiers.sol @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +// --- Should be flagged --- + +contract UnusedModifierContract { + address public owner; + + modifier neverUsed() { + require(msg.sender == owner); + _; + } + + modifier alsoNeverUsed() { + require(msg.value > 0); + _; + } + + modifier actuallyUsed() { + require(msg.sender != address(0)); + _; + } + + function doSomething() external actuallyUsed { + // uses actuallyUsed + } +} + +// --- Should NOT be flagged --- + +contract AllModifiersUsed { + address public owner; + + modifier onlyOwner() { + require(msg.sender == owner); + _; + } + + modifier nonZero(uint256 val) { + require(val > 0); + _; + } + + function restricted() external onlyOwner { + // uses onlyOwner + } + + function validated(uint256 x) external nonZero(x) { + // uses nonZero + } +} + +// --- Inheritance: virtual modifier overridden --- + +contract BaseModifier { + modifier baseMod() virtual { + _; + } +} + +contract ChildModifier is BaseModifier { + modifier baseMod() override { + require(msg.sender != address(0)); + _; + } + + function action() external baseMod { + // uses overridden baseMod + } +} + +// --- Unused virtual modifier NOT overridden --- + +contract UnusedVirtual { + modifier unusedVirtualMod() virtual { + _; + } + + function nothing() external pure {} +} diff --git a/tests/e2e/detectors/test_data/unused-modifiers/0.8.0/unused_modifiers.sol-0.8.0.zip b/tests/e2e/detectors/test_data/unused-modifiers/0.8.0/unused_modifiers.sol-0.8.0.zip new file mode 100644 index 0000000000..9d407d4aae Binary files /dev/null and b/tests/e2e/detectors/test_data/unused-modifiers/0.8.0/unused_modifiers.sol-0.8.0.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index 7bd6e34600..a58a0c3905 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -1366,6 +1366,16 @@ def id_test(test_item: Test): "dead-code-library.sol", "0.8.0", ), + Test( + all_detectors.UnusedEvents, + "unused_events.sol", + "0.8.0", + ), + Test( + all_detectors.UnusedModifiers, + "unused_modifiers.sol", + "0.8.0", + ), Test( all_detectors.WriteAfterWrite, "write-after-write.sol",