From a6c263717e3266ef6f96ffd4306da96cb6a5a3f9 Mon Sep 17 00:00:00 2001 From: TilakMaddy Date: Sun, 21 Jul 2024 16:41:10 +0530 Subject: [PATCH] sub without if --- aderyn_core/src/detect/detector.rs | 12 +- aderyn_core/src/detect/low/mod.rs | 2 + aderyn_core/src/detect/low/sub_without_if.rs | 150 +++++++++++++++++++ reports/report.json | 18 ++- reports/report.md | 20 ++- reports/report.sarif | 20 +++ 6 files changed, 216 insertions(+), 6 deletions(-) create mode 100644 aderyn_core/src/detect/low/sub_without_if.rs diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 492ed3d74..4d10fe361 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -36,9 +36,12 @@ use std::{ str::FromStr, }; -use super::high::{ - DelegateCallOnUncheckedAddressDetector, SelfdestructIdentifierDetector, - SendEtherNoChecksDetector, +use super::{ + high::{ + DelegateCallOnUncheckedAddressDetector, SelfdestructIdentifierDetector, + SendEtherNoChecksDetector, + }, + low::SubWithoutIfDetector, }; pub fn get_all_issue_detectors() -> Vec> { @@ -86,6 +89,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -140,6 +144,7 @@ pub(crate) enum IssueDetectorNamePool { StateVariableShadowing, SendEtherNoChecks, DelegateCallUncheckedAddress, + SubWithoutIf, // NOTE: `Undecided` will be the default name (for new bots). // If it's accepted, a new variant will be added to this enum before normalizing it in aderyn Undecided, @@ -262,6 +267,7 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::SubWithoutIf => Some(Box::::default()), IssueDetectorNamePool::Undecided => None, } } diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index 6b8270129..f2beb5819 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -12,6 +12,7 @@ pub(crate) mod push_0_opcode; pub(crate) mod require_with_string; pub(crate) mod reverts_and_requries_in_loops; pub(crate) mod solmate_safe_transfer_lib; +pub(crate) mod sub_without_if; pub(crate) mod unindexed_events; pub(crate) mod unsafe_erc20_functions; pub(crate) mod unsafe_oz_erc721_mint; @@ -36,6 +37,7 @@ pub use push_0_opcode::PushZeroOpcodeDetector; pub use require_with_string::RequireWithStringDetector; pub use reverts_and_requries_in_loops::RevertsAndRequiresInLoopsDetector; pub use solmate_safe_transfer_lib::SolmateSafeTransferLibDetector; +pub use sub_without_if::SubWithoutIfDetector; pub use unindexed_events::UnindexedEventsDetector; pub use unsafe_erc20_functions::UnsafeERC20FunctionsDetector; pub use unsafe_oz_erc721_mint::UnsafeERC721MintDetector; diff --git a/aderyn_core/src/detect/low/sub_without_if.rs b/aderyn_core/src/detect/low/sub_without_if.rs new file mode 100644 index 000000000..b413a643c --- /dev/null +++ b/aderyn_core/src/detect/low/sub_without_if.rs @@ -0,0 +1,150 @@ +use std::collections::{BTreeMap, HashSet}; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::context::browser::ExtractMemberAccesses; +use crate::context::investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, +}; + +use crate::context::workspace_context::ASTNode; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct SubWithoutIfDetector { + // Keys are: [0] source file name, [1] line number, [2] character location of node. + // Do not add items manually, use `capture!` to add nodes to this BTreeMap. + found_instances: BTreeMap<(String, usize, String), NodeID>, +} + +impl IssueDetector for SubWithoutIfDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // Plan: + // First, collect the if statements and investigate them for any `X.sub(Y)` patterns + // and capture all the IDs ... Set A (protected_subs) + // Then, collect all the IDS of X.sub(Y) pattern ... Set B (all_subs) + // Answer = SetB - SetA + // (We're left with unchecked sub method calls) + + let mut all_subs = context + .member_accesses() + .into_iter() + .filter(|member| member.member_name == "sub") + .map(|x| x.id) + .collect::>(); + + let mut protected_subs: HashSet = HashSet::new(); + + let if_statements = context + .if_statements() + .into_iter() + .map(|if_statement| if_statement.into()) + .collect::>(); + + let mut tracker = SubTracker { + sub_pattern_ids: vec![], + }; + let investigator = StandardInvestigator::new( + context, + &if_statements.iter().collect::>(), + StandardInvestigationStyle::Downstream, + )?; + investigator.investigate(context, &mut tracker)?; + protected_subs.extend(tracker.sub_pattern_ids.iter()); + + for protected_sub in protected_subs { + let _ = all_subs.remove(&protected_sub); + } + + // Now, we have the unprotected subs remaining in all_subs + for sub in all_subs { + context + .nodes + .get(&sub) + .inspect(|&node| capture!(self, context, node)); + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::Low + } + + fn title(&self) -> String { + String::from("Potential unchecked call made to `x.sub(y)`.") + } + + fn description(&self) -> String { + String::from("Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::SubWithoutIf.to_string() + } +} + +struct SubTracker { + sub_pattern_ids: Vec, +} + +impl StandardInvestigatorVisitor for SubTracker { + fn visit_fallback( + &mut self, + node: &crate::context::workspace_context::ASTNode, + ) -> eyre::Result<()> { + let member_accesses = ExtractMemberAccesses::from(node).extracted; + self.sub_pattern_ids.extend( + member_accesses + .into_iter() + .filter(|member| member.member_name == "sub") + .map(|x| x.id), + ); + Ok(()) + } +} + +#[cfg(test)] +mod sub_without_if_tests { + use crate::detect::{detector::IssueDetector, low::sub_without_if::SubWithoutIfDetector}; + + #[test] + fn test_sub_without_if() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/SubWithoutIf.sol", + ); + + let mut detector = SubWithoutIfDetector::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(), 1); + // assert the severity is low + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::Low + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Potential unchecked call made to `x.sub(y)`.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts"), + ); + } +} diff --git a/reports/report.json b/reports/report.json index 71cbe59c0..ad264dcf8 100644 --- a/reports/report.json +++ b/reports/report.json @@ -241,7 +241,7 @@ }, "issue_count": { "high": 20, - "low": 23 + "low": 24 }, "high_issues": { "issues": [ @@ -2967,6 +2967,19 @@ "src_char": "541:5" } ] + }, + { + "title": "Potential unchecked call made to `x.sub(y)`.", + "description": "Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts", + "detector_name": "sub-without-if", + "instances": [ + { + "contract_path": "src/SubWithoutIf.sol", + "line_no": 17, + "src": "549:23", + "src_char": "549:23" + } + ] } ] }, @@ -3013,6 +3026,7 @@ "yul-return", "state-variable-shadowing", "send-ether-no-checks", - "delegate-call-unchecked-address" + "delegate-call-unchecked-address", + "sub-without-if" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index d3b937ded..d0999bbde 100644 --- a/reports/report.md +++ b/reports/report.md @@ -52,6 +52,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-21: Unused Custom Error](#l-21-unused-custom-error) - [L-22: Loop contains `require`/`revert` statements](#l-22-loop-contains-requirerevert-statements) - [L-23: Incorrect Order of Division and Multiplication](#l-23-incorrect-order-of-division-and-multiplication) + - [L-24: Potential unchecked call made to `x.sub(y)`.](#l-24-potential-unchecked-call-made-to-xsuby) # Summary @@ -134,7 +135,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 20 | -| Low | 23 | +| Low | 24 | # High Issues @@ -3007,3 +3008,20 @@ Division operations followed directly by multiplication operations can lead to p +## L-24: Potential unchecked call made to `x.sub(y)`. + +Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts + +
1 Found Instances + + +- Found in src/SubWithoutIf.sol [Line: 17](../tests/contract-playground/src/SubWithoutIf.sol#L17) + + ```solidity + uint256 magicNumber = subtractionContract.sub(a, b) + 4 ** 7; + ``` + +
+ + + diff --git a/reports/report.sarif b/reports/report.sarif index 3678cd8c0..5a41d2653 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -4823,6 +4823,26 @@ "text": "Division operations followed directly by multiplication operations can lead to precision loss due to the way integer arithmetic is handled in Solidity." }, "ruleId": "division-before-multiplication" + }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SubWithoutIf.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 549 + } + } + } + ], + "message": { + "text": "Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts" + }, + "ruleId": "sub-without-if" } ], "tool": {