From 47272a0e016c9f6817eb3db4014a6936f121ba6e Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Fri, 30 Aug 2024 22:10:44 +0530 Subject: [PATCH] Detector: Function Pointers in constructors (#693) Co-authored-by: Alex Roan --- aderyn_core/src/detect/detector.rs | 5 + .../low/function_pointer_in_constructor.rs | 154 ++++++++++++++++++ aderyn_core/src/detect/low/mod.rs | 2 + reports/report.json | 26 ++- reports/report.md | 27 ++- reports/report.sarif | 20 +++ .../src/FunctionPointers.sol | 19 +++ 7 files changed, 245 insertions(+), 8 deletions(-) create mode 100644 aderyn_core/src/detect/low/function_pointer_in_constructor.rs create mode 100644 tests/contract-playground/src/FunctionPointers.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index a19c74c2..15f35850 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -93,6 +93,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -104,6 +105,7 @@ pub fn get_all_detectors_names() -> Vec { #[derive(Debug, PartialEq, EnumString, Display)] #[strum(serialize_all = "kebab-case")] pub(crate) enum IssueDetectorNamePool { + FunctionPointerInConstructor, DeadCode, FunctionSelectorCollision, CacheArrayLength, @@ -191,6 +193,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { + Some(Box::::default()) + } IssueDetectorNamePool::DeadCode => Some(Box::::default()), IssueDetectorNamePool::FunctionSelectorCollision => { Some(Box::::default()) diff --git a/aderyn_core/src/detect/low/function_pointer_in_constructor.rs b/aderyn_core/src/detect/low/function_pointer_in_constructor.rs new file mode 100644 index 00000000..6b324f4d --- /dev/null +++ b/aderyn_core/src/detect/low/function_pointer_in_constructor.rs @@ -0,0 +1,154 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{FunctionKind, NodeID}; + +use crate::capture; +use crate::context::browser::ExtractVariableDeclarations; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct FucntionPointerInConstructorDetector { + // 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 FucntionPointerInConstructorDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // PLAN: + // Catch all the function pointers in constructors that compile below 0.5.9 + + for func in context + .function_definitions() + .into_iter() + .filter(|f| f.kind() == &FunctionKind::Constructor) + .filter(|f| f.compiles_for_solc_below_0_5_9(context)) + { + let variable_declarations = ExtractVariableDeclarations::from(func).extracted; + + for variable_declaration in variable_declarations { + if variable_declaration + .type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| type_string.starts_with("function ")) + { + capture!(self, context, variable_declaration); + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::Low + } + + fn title(&self) -> String { + String::from("Function pointers used in constructors.") + } + + fn description(&self) -> String { + String::from("solc versions below 0.5.9 contain a compiler bug leading to unexpected behavior when calling uninitialized function pointers in constructors. It is recommended to not use function pointers in constructors.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::FunctionPointerInConstructor) + } +} + +mod func_compilation_solc_pragma_helper { + use std::str::FromStr; + + use semver::{Version, VersionReq}; + + use crate::{ + ast::{FunctionDefinition, NodeType}, + context::{ + browser::{ExtractPragmaDirectives, GetClosestAncestorOfTypeX}, + workspace_context::WorkspaceContext, + }, + detect::helpers, + }; + + impl FunctionDefinition { + pub fn compiles_for_solc_below_0_5_9(&self, context: &WorkspaceContext) -> bool { + if let Some(source_unit) = self.closest_ancestor_of_type(context, NodeType::SourceUnit) + { + let pragma_directives = ExtractPragmaDirectives::from(source_unit).extracted; + + if let Some(pragma_directive) = pragma_directives.first() { + if let Ok(pragma_semver) = helpers::pragma_directive_to_semver(pragma_directive) + { + if version_req_allows_below_0_5_9(&pragma_semver) { + return true; + } + } + } + } + false + } + } + + fn version_req_allows_below_0_5_9(version_req: &VersionReq) -> bool { + // If it matches any 0.4.0 to 0.4.26, return true + for i in 0..=26 { + let version = Version::from_str(&format!("0.4.{}", i)).unwrap(); + if version_req.matches(&version) { + return true; + } + } + + // If it matches any 0.5.0 to 0.5.8, return true + for i in 0..=8 { + let version = Version::from_str(&format!("0.5.{}", i)).unwrap(); + if version_req.matches(&version) { + return true; + } + } + + // Else, return false + false + } +} + +#[cfg(test)] +mod function_pointers_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + low::function_pointer_in_constructor::FucntionPointerInConstructorDetector, + }; + + #[test] + #[serial] + fn test_function_pointers() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/FunctionPointers.sol", + ); + + let mut detector = FucntionPointerInConstructorDetector::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 + ); + } +} diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index 9d803a3f..e398255c 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -13,6 +13,7 @@ pub(crate) mod division_before_multiplication; pub(crate) mod ecrecover; pub(crate) mod empty_blocks; pub(crate) mod function_init_state_vars; +pub(crate) mod function_pointer_in_constructor; pub(crate) mod inconsistent_type_names; pub(crate) mod large_literal_value; pub(crate) mod non_reentrant_before_others; @@ -50,6 +51,7 @@ pub use division_before_multiplication::DivisionBeforeMultiplicationDetector; pub use ecrecover::EcrecoverDetector; pub use empty_blocks::EmptyBlockDetector; pub use function_init_state_vars::FunctionInitializingStateDetector; +pub use function_pointer_in_constructor::FucntionPointerInConstructorDetector; pub use inconsistent_type_names::InconsistentTypeNamesDetector; pub use large_literal_value::LargeLiteralValueDetector; pub use non_reentrant_before_others::NonReentrantBeforeOthersDetector; diff --git a/reports/report.json b/reports/report.json index 06808860..bf275776 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 95, - "total_sloc": 3336 + "total_source_units": 96, + "total_sloc": 3346 }, "files_details": { "files_details": [ @@ -137,6 +137,10 @@ "file_path": "src/FunctionInitializingState.sol", "n_sloc": 49 }, + { + "file_path": "src/FunctionPointers.sol", + "n_sloc": 10 + }, { "file_path": "src/FunctionSignatureCollision.sol", "n_sloc": 9 @@ -389,7 +393,7 @@ }, "issue_count": { "high": 41, - "low": 36 + "low": 37 }, "high_issues": { "issues": [ @@ -5623,6 +5627,19 @@ "src_char": "414:15" } ] + }, + { + "title": "Function pointers used in constructors.", + "description": "solc versions below 0.5.9 contain a compiler bug leading to unexpected behavior when calling uninitialized function pointers in constructors. It is recommended to not use function pointers in constructors.", + "detector_name": "function-pointer-in-constructor", + "instances": [ + { + "contract_path": "src/FunctionPointers.sol", + "line_no": 13, + "src": "330:50", + "src_char": "330:50" + } + ] } ] }, @@ -5703,6 +5720,7 @@ "costly-operations-inside-loops", "constant-function-changing-state", "builtin-symbol-shadow", - "function-selector-collision" + "function-selector-collision", + "function-pointer-in-constructor" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index 3a2dc308..7c735f79 100644 --- a/reports/report.md +++ b/reports/report.md @@ -86,6 +86,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-34: Incorrect use of `assert()`](#l-34-incorrect-use-of-assert) - [L-35: Costly operations inside loops.](#l-35-costly-operations-inside-loops) - [L-36: Builtin Symbol Shadowing](#l-36-builtin-symbol-shadowing) + - [L-37: Function pointers used in constructors.](#l-37-function-pointers-used-in-constructors) # Summary @@ -94,8 +95,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 95 | -| Total nSLOC | 3336 | +| .sol Files | 96 | +| Total nSLOC | 3346 | ## Files Details @@ -135,6 +136,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/EnumerableSetIteration.sol | 55 | | src/ExperimentalEncoder.sol | 4 | | src/FunctionInitializingState.sol | 49 | +| src/FunctionPointers.sol | 10 | | src/FunctionSignatureCollision.sol | 9 | | src/HugeConstants.sol | 36 | | src/InconsistentUints.sol | 17 | @@ -197,7 +199,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** | **3336** | +| **Total** | **3346** | ## Issue Summary @@ -205,7 +207,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 41 | -| Low | 36 | +| Low | 37 | # High Issues @@ -5731,3 +5733,20 @@ Name clashes with a built-in-symbol. Consider renaming it. +## L-37: Function pointers used in constructors. + +solc versions below 0.5.9 contain a compiler bug leading to unexpected behavior when calling uninitialized function pointers in constructors. It is recommended to not use function pointers in constructors. + +
1 Found Instances + + +- Found in src/FunctionPointers.sol [Line: 13](../tests/contract-playground/src/FunctionPointers.sol#L13) + + ```solidity + function(uint, uint) pure returns (uint) operation; + ``` + +
+ + + diff --git a/reports/report.sarif b/reports/report.sarif index bd994e63..cda671e0 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -9293,6 +9293,26 @@ "text": "Name clashes with a built-in-symbol. Consider renaming it." }, "ruleId": "builtin-symbol-shadow" + }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionPointers.sol" + }, + "region": { + "byteLength": 50, + "byteOffset": 330 + } + } + } + ], + "message": { + "text": "solc versions below 0.5.9 contain a compiler bug leading to unexpected behavior when calling uninitialized function pointers in constructors. It is recommended to not use function pointers in constructors." + }, + "ruleId": "function-pointer-in-constructor" } ], "tool": { diff --git a/tests/contract-playground/src/FunctionPointers.sol b/tests/contract-playground/src/FunctionPointers.sol new file mode 100644 index 00000000..f4b5256d --- /dev/null +++ b/tests/contract-playground/src/FunctionPointers.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.5.8; + +contract FunctionPointerExample { + + // A simple add function + function add(uint a, uint b) public pure returns (uint) { + return a + b; + } + + constructor() public { + // Declare a function type that takes two uint arguments and returns a uint + function(uint, uint) pure returns (uint) operation; + + // Assign the add function to the operation variable + operation = add; + } + +}