diff --git a/aderyn_core/src/detect/high/send_ether_no_checks.rs b/aderyn_core/src/detect/high/send_ether_no_checks.rs index 174fcf4a8..48108693b 100644 --- a/aderyn_core/src/detect/high/send_ether_no_checks.rs +++ b/aderyn_core/src/detect/high/send_ether_no_checks.rs @@ -5,6 +5,7 @@ use crate::ast::NodeID; use crate::{ capture, context::{ + browser::ExtractModifierInvocations, graph::{CallGraph, CallGraphDirection, CallGraphVisitor}, workspace_context::{ASTNode, WorkspaceContext}, }, @@ -29,7 +30,20 @@ impl IssueDetector for SendEtherNoChecksDetector { let callgraph = CallGraph::new(context, &[&(func.into())], CallGraphDirection::Inward)?; callgraph.accept(context, &mut tracker)?; - if tracker.sends_native_eth && !tracker.has_binary_checks_on_some_address { + // Hacky way to check if the modifier is a know msg.sender checking modifier + // This is because our Callgraph doesn't navigate inside contracts that are outside + // the scope, this includes imported contracts. + let has_oz_modifier = + ExtractModifierInvocations::from(func).extracted.iter().any(|invocation| { + invocation.modifier_name.name().contains("onlyRole") + || invocation.modifier_name.name() == "onlyOwner" + || invocation.modifier_name.name() == "requiresAuth" + }); + + if tracker.sends_native_eth + && !tracker.has_binary_checks_on_some_address + && !has_oz_modifier + { capture!(self, context, func); } } @@ -58,6 +72,26 @@ impl IssueDetector for SendEtherNoChecksDetector { } } +#[derive(Default)] +pub struct AddressChecksAndCallWithValueTracker { + pub has_binary_checks_on_some_address: bool, + pub sends_native_eth: bool, +} + +impl CallGraphVisitor for AddressChecksAndCallWithValueTracker { + fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> { + if !self.has_binary_checks_on_some_address + && helpers::has_binary_checks_on_some_address(node) + { + self.has_binary_checks_on_some_address = true; + } + if !self.sends_native_eth && helpers::has_calls_that_sends_native_eth(node) { + self.sends_native_eth = true; + } + eyre::Ok(()) + } +} + #[cfg(test)] mod send_ether_no_checks_detector_tests { use serial_test::serial; @@ -66,6 +100,23 @@ mod send_ether_no_checks_detector_tests { detector::IssueDetector, high::send_ether_no_checks::SendEtherNoChecksDetector, }; + #[test] + #[serial] + fn test_send_ether_no_checks_lib_import() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/SendEtherNoChecksLibImport.sol", + ); + + let mut detector = SendEtherNoChecksDetector::default(); + let found = detector.detect(&context).unwrap(); + // assert that the detector found an issue + assert!(!found); + // assert that the detector found the correct number of instances + assert_eq!(detector.instances().len(), 0); + // assert the severity is high + assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::High); + } + #[test] #[serial] fn test_send_ether_no_checks() { @@ -83,23 +134,3 @@ mod send_ether_no_checks_detector_tests { assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::High); } } - -#[derive(Default)] -pub struct AddressChecksAndCallWithValueTracker { - pub has_binary_checks_on_some_address: bool, - pub sends_native_eth: bool, -} - -impl CallGraphVisitor for AddressChecksAndCallWithValueTracker { - fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> { - if !self.has_binary_checks_on_some_address - && helpers::has_binary_checks_on_some_address(node) - { - self.has_binary_checks_on_some_address = true; - } - if !self.sends_native_eth && helpers::has_calls_that_sends_native_eth(node) { - self.sends_native_eth = true; - } - eyre::Ok(()) - } -} diff --git a/reports/report.json b/reports/report.json index 9998dbc05..d2a40d3f9 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 113, - "total_sloc": 3976 + "total_source_units": 114, + "total_sloc": 3993 }, "files_details": { "files_details": [ @@ -249,6 +249,10 @@ "file_path": "src/SendEtherNoChecks.sol", "n_sloc": 58 }, + { + "file_path": "src/SendEtherNoChecksLibImport.sol", + "n_sloc": 17 + }, { "file_path": "src/StateShadowing.sol", "n_sloc": 17 @@ -2225,6 +2229,30 @@ "src": "250:9", "src_char": "250:9" }, + { + "contract_path": "src/SendEtherNoChecksLibImport.sol", + "line_no": 7, + "src": "276:7", + "src_char": "276:7" + }, + { + "contract_path": "src/SendEtherNoChecksLibImport.sol", + "line_no": 7, + "src": "285:13", + "src_char": "285:13" + }, + { + "contract_path": "src/SendEtherNoChecksLibImport.sol", + "line_no": 16, + "src": "516:9", + "src_char": "516:9" + }, + { + "contract_path": "src/SendEtherNoChecksLibImport.sol", + "line_no": 20, + "src": "614:28", + "src_char": "614:28" + }, { "contract_path": "src/Trump.sol", "line_no": 92, @@ -3807,6 +3835,12 @@ "src": "920:6", "src_char": "920:6" }, + { + "contract_path": "src/SendEtherNoChecksLibImport.sol", + "line_no": 12, + "src": "451:6", + "src_char": "451:6" + }, { "contract_path": "src/StateShadowing.sol", "line_no": 8, diff --git a/reports/report.md b/reports/report.md index 4f5163446..0df36228f 100644 --- a/reports/report.md +++ b/reports/report.md @@ -103,8 +103,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 113 | -| Total nSLOC | 3976 | +| .sol Files | 114 | +| Total nSLOC | 3993 | ## Files Details @@ -172,6 +172,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/ReturnBomb.sol | 44 | | src/RevertsAndRequriesInLoops.sol | 27 | | src/SendEtherNoChecks.sol | 58 | +| src/SendEtherNoChecksLibImport.sol | 17 | | src/StateShadowing.sol | 17 | | src/StateVariableCouldBeDeclaredConstant.sol | 27 | | src/StateVariableCouldBeDeclaredImmutable.sol | 22 | @@ -224,7 +225,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** | **3976** | +| **Total** | **3993** | ## Issue Summary @@ -2118,7 +2119,7 @@ Using `delegatecall` in loop may consume excessive gas Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds. -
17 Found Instances +
21 Found Instances - Found in src/AdminContract.sol [Line: 7](../tests/contract-playground/src/AdminContract.sol#L7) @@ -2157,6 +2158,24 @@ Contracts have owners with privileged rights to perform admin tasks and need to function setValue(uint256 _newValue) external onlyOwner { ``` +- Found in src/SendEtherNoChecksLibImport.sol [Line: 7](../tests/contract-playground/src/SendEtherNoChecksLibImport.sol#L7) + + ```solidity + contract SendEtherCheckOZOnlyOWner is Ownable, AccessControl { + ``` + +- Found in src/SendEtherNoChecksLibImport.sol [Line: 16](../tests/contract-playground/src/SendEtherNoChecksLibImport.sol#L16) + + ```solidity + function send(address x) external onlyOwner { + ``` + +- Found in src/SendEtherNoChecksLibImport.sol [Line: 20](../tests/contract-playground/src/SendEtherNoChecksLibImport.sol#L20) + + ```solidity + function sendWithRole(address x) external onlyRole(DEFAULT_ADMIN_ROLE) { + ``` + - Found in src/Trump.sol [Line: 92](../tests/contract-playground/src/Trump.sol#L92) ```solidity @@ -3671,7 +3690,7 @@ Index event fields make the field more quickly accessible to off-chain tools tha Use descriptive reason strings or custom errors for revert paths. -
23 Found Instances +
24 Found Instances - Found in src/CallGraphTests.sol [Line: 7](../tests/contract-playground/src/CallGraphTests.sol#L7) @@ -3758,6 +3777,12 @@ Use descriptive reason strings or custom errors for revert paths. revert(); ``` +- Found in src/SendEtherNoChecksLibImport.sol [Line: 12](../tests/contract-playground/src/SendEtherNoChecksLibImport.sol#L12) + + ```solidity + revert(); + ``` + - Found in src/StateShadowing.sol [Line: 8](../tests/contract-playground/src/StateShadowing.sol#L8) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index 97c2fe263..aeb40ffbb 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -3070,6 +3070,50 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecksLibImport.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 276 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecksLibImport.sol" + }, + "region": { + "byteLength": 13, + "byteOffset": 285 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecksLibImport.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 516 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecksLibImport.sol" + }, + "region": { + "byteLength": 28, + "byteOffset": 614 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5932,6 +5976,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecksLibImport.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 451 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/tests/contract-playground/src/SendEtherNoChecksLibImport.sol b/tests/contract-playground/src/SendEtherNoChecksLibImport.sol new file mode 100644 index 000000000..70eaedb43 --- /dev/null +++ b/tests/contract-playground/src/SendEtherNoChecksLibImport.sol @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import {Ownable} from "../lib/openzeppelin-contracts/contracts/access/Ownable.sol"; +import {AccessControl} from "../lib/openzeppelin-contracts/contracts/access/AccessControl.sol"; + +contract SendEtherCheckOZOnlyOWner is Ownable, AccessControl { + + function callAndSendNativeEth(address x) internal { + (bool success,) = x.call{value: 10}("calldata"); + if (!success) { + revert(); + } + } + + function send(address x) external onlyOwner { + callAndSendNativeEth(x); + } + + function sendWithRole(address x) external onlyRole(DEFAULT_ADMIN_ROLE) { + callAndSendNativeEth(x); + } +}