From 3dd07697cb6cb41e26ecf4b53a8945c427ee5ed9 Mon Sep 17 00:00:00 2001 From: Alex Roan Date: Thu, 6 Mar 2025 16:20:36 +0000 Subject: [PATCH] Fix False Positive: Unused errors that were used within require blocks (#811) --- aderyn_core/src/detect/low/useless_error.rs | 29 +++++++++---------- reports/report.json | 16 +++++----- reports/report.md | 10 +++---- reports/report.sarif | 4 +-- tests/contract-playground/src/UnusedError.sol | 7 ++++- 5 files changed, 34 insertions(+), 32 deletions(-) diff --git a/aderyn_core/src/detect/low/useless_error.rs b/aderyn_core/src/detect/low/useless_error.rs index 02935bd77..9f1e2d192 100644 --- a/aderyn_core/src/detect/low/useless_error.rs +++ b/aderyn_core/src/detect/low/useless_error.rs @@ -1,5 +1,5 @@ use crate::{ - ast::{Expression, NodeID}, + ast::NodeID, capture, context::workspace_context::WorkspaceContext, detect::detector::{IssueDetector, IssueDetectorNamePool, IssueSeverity}, @@ -19,27 +19,24 @@ pub struct UselessErrorDetector { impl IssueDetector for UselessErrorDetector { fn detect(&mut self, context: &WorkspaceContext) -> Result> { let error_definitions = context.error_definitions().into_iter().collect::>(); - let mut used_errors = HashSet::new(); + let mut referenced_ids = HashSet::new(); - // Collect all used errors from revert statements - for revert_stmt in context.revert_statements() { - // extract the ids directly from the expression of the function call - if let Expression::Identifier(identifier) = &*revert_stmt.error_call.expression { - if let Some(reference_id) = identifier.referenced_declaration { - used_errors.insert(reference_id); - } - } else if let Expression::MemberAccess(member_access) = - &*revert_stmt.error_call.expression - { - if let Some(reference_id) = member_access.referenced_declaration { - used_errors.insert(reference_id); - } + //Get all MemberAccess and Identifier nodes where the referenced_declaration is an ID of an + // error definition + for identifier in context.identifiers() { + if let Some(reference_id) = identifier.referenced_declaration { + referenced_ids.insert(reference_id); + } + } + for member_access in context.member_accesses() { + if let Some(reference_id) = member_access.referenced_declaration { + referenced_ids.insert(reference_id); } } // Identify unused errors by comparing defined and used error IDs for error_def in error_definitions { - if !used_errors.contains(&error_def.id) { + if !referenced_ids.contains(&error_def.id) { // Capture unused error instances capture!(self, context, error_def); } diff --git a/reports/report.json b/reports/report.json index a3db849d3..acf9d261b 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { "total_source_units": 112, - "total_sloc": 3957 + "total_sloc": 3961 }, "files_details": { "files_details": [ @@ -347,7 +347,7 @@ }, { "file_path": "src/UnusedError.sol", - "n_sloc": 19 + "n_sloc": 23 }, { "file_path": "src/UnusedImport.sol", @@ -5112,9 +5112,9 @@ }, { "contract_path": "src/UnusedError.sol", - "line_no": 13, - "src": "258:36", - "src_char": "258:36" + "line_no": 14, + "src": "286:36", + "src_char": "286:36" }, { "contract_path": "src/WrongOrderOfLayout.sol", @@ -6775,9 +6775,9 @@ }, { "contract_path": "src/UnusedError.sol", - "line_no": 15, - "src": "309:7", - "src_char": "309:7" + "line_no": 16, + "src": "337:7", + "src_char": "337:7" }, { "contract_path": "src/UnusedStateVariables.sol", diff --git a/reports/report.md b/reports/report.md index 3938a4408..aca573677 100644 --- a/reports/report.md +++ b/reports/report.md @@ -104,7 +104,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | | .sol Files | 112 | -| Total nSLOC | 3957 | +| Total nSLOC | 3961 | ## Files Details @@ -196,7 +196,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/UninitializedStateVariable.sol | 29 | | src/UnprotectedInitialize.sol | 28 | | src/UnsafeERC721Mint.sol | 18 | -| src/UnusedError.sol | 19 | +| src/UnusedError.sol | 23 | | src/UnusedImport.sol | 10 | | src/UnusedStateVariables.sol | 12 | | src/UsingSelfdestruct.sol | 6 | @@ -223,7 +223,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/reused_contract_name/ContractB.sol | 7 | | src/uniswap/UniswapV2Swapper.sol | 50 | | src/uniswap/UniswapV3Swapper.sol | 150 | -| **Total** | **3957** | +| **Total** | **3961** | ## Issue Summary @@ -5108,7 +5108,7 @@ it is recommended that the definition be removed when custom error is unused error UnusedLibraryError(); ``` -- Found in src/UnusedError.sol [Line: 13](../tests/contract-playground/src/UnusedError.sol#L13) +- Found in src/UnusedError.sol [Line: 14](../tests/contract-playground/src/UnusedError.sol#L14) ```solidity error UnusedError1(address account); @@ -6860,7 +6860,7 @@ State variable changes in this function but no event is emitted. function initializeWithoutModifierOrRevert() external { ``` -- Found in src/UnusedError.sol [Line: 15](../tests/contract-playground/src/UnusedError.sol#L15) +- Found in src/UnusedError.sol [Line: 16](../tests/contract-playground/src/UnusedError.sol#L16) ```solidity function perform() external { diff --git a/reports/report.sarif b/reports/report.sarif index 4f33e51ff..cf1bb4599 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -8293,7 +8293,7 @@ }, "region": { "byteLength": 36, - "byteOffset": 258 + "byteOffset": 286 } } }, @@ -11261,7 +11261,7 @@ }, "region": { "byteLength": 7, - "byteOffset": 309 + "byteOffset": 337 } } }, diff --git a/tests/contract-playground/src/UnusedError.sol b/tests/contract-playground/src/UnusedError.sol index 313b683d7..bbf85874e 100644 --- a/tests/contract-playground/src/UnusedError.sol +++ b/tests/contract-playground/src/UnusedError.sol @@ -1,8 +1,9 @@ // SPDX-License-Identifier: MIT -pragma solidity 0.8.19; +pragma solidity 0.8.27; library ErrorLibrary { error UnusedLibraryError(); + error UsedInARequire(); error LibraryError(); } @@ -23,4 +24,8 @@ contract UnusedError { function goodLibraryError() external pure { revert ErrorLibrary.LibraryError(); } + + function goodRequire() external pure { + require(false, ErrorLibrary.UsedInARequire()); + } }