From 9fcabc085b5b92cc49e1c1a65cd7335d910219eb Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Mon, 19 Aug 2024 15:28:39 +0530 Subject: [PATCH] Detector: Function signature collision (#670) Co-authored-by: Alex Roan --- aderyn_core/src/detect/detector.rs | 5 + .../high/function_selector_collision.rs | 117 ++++++++++++++++++ aderyn_core/src/detect/high/mod.rs | 2 + .../adhoc-sol-files-highs-only-report.json | 3 +- reports/report.json | 56 ++++++++- reports/report.md | 63 ++++++++-- reports/report.sarif | 75 +++++++++++ .../src/FunctionSignatureCollision.sol | 18 +++ 8 files changed, 327 insertions(+), 12 deletions(-) create mode 100644 aderyn_core/src/detect/high/function_selector_collision.rs create mode 100644 tests/contract-playground/src/FunctionSignatureCollision.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 5d9c047c..f534193a 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -92,6 +92,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -103,6 +104,7 @@ pub fn get_all_detectors_names() -> Vec { #[derive(Debug, PartialEq, EnumString, Display)] #[strum(serialize_all = "kebab-case")] pub(crate) enum IssueDetectorNamePool { + FunctionSelectorCollision, CacheArrayLength, AssertStateChange, CostlyOperationsInsideLoops, @@ -187,6 +189,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { + Some(Box::::default()) + } IssueDetectorNamePool::CacheArrayLength => Some(Box::::default()), IssueDetectorNamePool::AssertStateChange => { Some(Box::::default()) diff --git a/aderyn_core/src/detect/high/function_selector_collision.rs b/aderyn_core/src/detect/high/function_selector_collision.rs new file mode 100644 index 00000000..3862b3bc --- /dev/null +++ b/aderyn_core/src/detect/high/function_selector_collision.rs @@ -0,0 +1,117 @@ +use std::collections::{BTreeMap, HashMap}; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct FunctionSelectorCollisionDetector { + // 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 FunctionSelectorCollisionDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // function_selector -> (function_name -> function_id) + let mut selectors: HashMap>> = HashMap::new(); + + // PLAN + // If we have > 1 function_name entries for any function_selector, then capture all the corresponding NodeIDs + + for function in context.function_definitions() { + if let Some(selector) = function.function_selector.as_ref() { + let name = &function.name; + match selectors.entry(selector.clone()) { + std::collections::hash_map::Entry::Occupied(mut o) => { + match o.get_mut().entry(name.clone()) { + std::collections::hash_map::Entry::Occupied(mut o) => { + o.get_mut().push(function.id); + } + std::collections::hash_map::Entry::Vacant(v) => { + v.insert(vec![function.id]); + } + }; + } + std::collections::hash_map::Entry::Vacant(v) => { + let mut nested_entry = HashMap::new(); + nested_entry.insert(name.clone(), vec![function.id]); + v.insert(nested_entry); + } + } + } + } + + for function_entries in selectors.values() { + if function_entries.len() >= 2 { + // Now we know that there is a collision + at least 2 different function names found for that selector. + for function_ids in function_entries.values() { + for function_id in function_ids { + if let Some(node) = context.nodes.get(function_id) { + capture!(self, context, node); + } + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Function selector collides with other functions") + } + + fn description(&self) -> String { + String::from("Function selector collides with other functions. This may cause the solidity function dispatcher to invoke the wrong function if the functions happen to be included in the same contract through an inheritance hirearchy later down the line. It is recommended to rename this function or change its parameters.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::FunctionSelectorCollision) + } +} + +#[cfg(test)] +mod function_signature_collision { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + high::function_selector_collision::FunctionSelectorCollisionDetector, + }; + + #[test] + #[serial] + fn test_function_signature_collision() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/FunctionSignatureCollision.sol", + ); + + let mut detector = FunctionSelectorCollisionDetector::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(), 2); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + } +} diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 5267c7a8..55cb6178 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -11,6 +11,7 @@ pub(crate) mod deletion_nested_mapping; pub(crate) mod dynamic_array_length_assignment; pub(crate) mod enumerable_loop_removal; pub(crate) mod experimental_encoder; +pub(crate) mod function_selector_collision; pub(crate) mod incorrect_caret_operator; pub(crate) mod incorrect_erc20_interface; pub(crate) mod incorrect_erc721_interface; @@ -52,6 +53,7 @@ pub use deletion_nested_mapping::DeletionNestedMappingDetector; pub use dynamic_array_length_assignment::DynamicArrayLengthAssignmentDetector; pub use enumerable_loop_removal::EnumerableLoopRemovalDetector; pub use experimental_encoder::ExperimentalEncoderDetector; +pub use function_selector_collision::FunctionSelectorCollisionDetector; pub use incorrect_caret_operator::IncorrectUseOfCaretOperatorDetector; pub use incorrect_erc20_interface::IncorrectERC20InterfaceDetector; pub use incorrect_erc721_interface::IncorrectERC721InterfaceDetector; diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index a69540c7..3bbb5f34 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -201,6 +201,7 @@ "incorrect-erc721-interface", "incorrect-erc20-interface", "out-of-order-retryable", - "constant-function-changing-state" + "constant-function-changing-state", + "function-selector-collision" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index 7a0fa50b..d310caea 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 91, - "total_sloc": 3221 + "total_source_units": 92, + "total_sloc": 3230 }, "files_details": { "files_details": [ @@ -129,6 +129,10 @@ "file_path": "src/FunctionInitializingState.sol", "n_sloc": 49 }, + { + "file_path": "src/FunctionSignatureCollision.sol", + "n_sloc": 9 + }, { "file_path": "src/HugeConstants.sol", "n_sloc": 36 @@ -372,7 +376,7 @@ ] }, "issue_count": { - "high": 40, + "high": 41, "low": 34 }, "high_issues": { @@ -2324,6 +2328,25 @@ "src_char": "173:112" } ] + }, + { + "title": "Function selector collides with other functions", + "description": "Function selector collides with other functions. This may cause the solidity function dispatcher to invoke the wrong function if the functions happen to be included in the same contract through an inheritance hirearchy later down the line. It is recommended to rename this function or change its parameters.", + "detector_name": "function-selector-collision", + "instances": [ + { + "contract_path": "src/FunctionSignatureCollision.sol", + "line_no": 7, + "src": "166:8", + "src_char": "166:8" + }, + { + "contract_path": "src/FunctionSignatureCollision.sol", + "line_no": 13, + "src": "231:22", + "src_char": "231:22" + } + ] } ] }, @@ -2639,6 +2662,12 @@ "src": "32:21", "src_char": "32:21" }, + { + "contract_path": "src/FunctionSignatureCollision.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/InconsistentUints.sol", "line_no": 1, @@ -3871,6 +3900,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/FunctionSignatureCollision.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/InconsistentUints.sol", "line_no": 1, @@ -4221,6 +4256,18 @@ "src": "1219:41", "src_char": "1219:41" }, + { + "contract_path": "src/FunctionSignatureCollision.sol", + "line_no": 7, + "src": "166:8", + "src_char": "166:8" + }, + { + "contract_path": "src/FunctionSignatureCollision.sol", + "line_no": 13, + "src": "231:22", + "src_char": "231:22" + }, { "contract_path": "src/OnceModifierExample.sol", "line_no": 10, @@ -5361,6 +5408,7 @@ "assert-state-change", "costly-operations-inside-loops", "constant-function-changing-state", - "builtin-symbol-shadow" + "builtin-symbol-shadow", + "function-selector-collision" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index 00856220..3c3320ae 100644 --- a/reports/report.md +++ b/reports/report.md @@ -48,6 +48,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-38: Incorrect ERC20 interface.](#h-38-incorrect-erc20-interface) - [H-39: Out of order retryable transactions.](#h-39-out-of-order-retryable-transactions) - [H-40: Constant functions changing state](#h-40-constant-functions-changing-state) + - [H-41: Function selector collides with other functions](#h-41-function-selector-collides-with-other-functions) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: Solmate's SafeTransferLib does not check for token contract's existence](#l-2-solmates-safetransferlib-does-not-check-for-token-contracts-existence) @@ -91,8 +92,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 91 | -| Total nSLOC | 3221 | +| .sol Files | 92 | +| Total nSLOC | 3230 | ## Files Details @@ -130,6 +131,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/FunctionSignatureCollision.sol | 9 | | src/HugeConstants.sol | 36 | | src/InconsistentUints.sol | 17 | | src/IncorrectCaretOperator.sol | 16 | @@ -190,14 +192,14 @@ 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** | **3221** | +| **Total** | **3230** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 40 | +| High | 41 | | Low | 34 | @@ -2315,6 +2317,29 @@ Function is declared constant/view but it changes state. Ensure that the attribu +## H-41: Function selector collides with other functions + +Function selector collides with other functions. This may cause the solidity function dispatcher to invoke the wrong function if the functions happen to be included in the same contract through an inheritance hirearchy later down the line. It is recommended to rename this function or change its parameters. + +
2 Found Instances + + +- Found in src/FunctionSignatureCollision.sol [Line: 7](../tests/contract-playground/src/FunctionSignatureCollision.sol#L7) + + ```solidity + function withdraw(uint256) external { + ``` + +- Found in src/FunctionSignatureCollision.sol [Line: 13](../tests/contract-playground/src/FunctionSignatureCollision.sol#L13) + + ```solidity + function OwnerTransferV7b711143(uint256) external { + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -2565,7 +2590,7 @@ ERC20 functions may not behave as expected. For example: return values are not a Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;` -
33 Found Instances +
34 Found Instances - Found in src/BuiltinSymbolShadow.sol [Line: 2](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L2) @@ -2652,6 +2677,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.4; ``` +- Found in src/FunctionSignatureCollision.sol [Line: 2](../tests/contract-playground/src/FunctionSignatureCollision.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/InconsistentUints.sol [Line: 1](../tests/contract-playground/src/InconsistentUints.sol#L1) ```solidity @@ -3823,7 +3854,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support Solc compiler version 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail. -
39 Found Instances +
40 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -3892,6 +3923,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity 0.8.20; ``` +- Found in src/FunctionSignatureCollision.sol [Line: 2](../tests/contract-playground/src/FunctionSignatureCollision.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/InconsistentUints.sol [Line: 1](../tests/contract-playground/src/InconsistentUints.sol#L1) ```solidity @@ -4157,7 +4194,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai Consider removing empty blocks. -
31 Found Instances +
33 Found Instances - Found in src/AdminContract.sol [Line: 14](../tests/contract-playground/src/AdminContract.sol#L14) @@ -4250,6 +4287,18 @@ Consider removing empty blocks. function emptyBlockWithCommentInsideNormalFunction() external { ``` +- Found in src/FunctionSignatureCollision.sol [Line: 7](../tests/contract-playground/src/FunctionSignatureCollision.sol#L7) + + ```solidity + function withdraw(uint256) external { + ``` + +- Found in src/FunctionSignatureCollision.sol [Line: 13](../tests/contract-playground/src/FunctionSignatureCollision.sol#L13) + + ```solidity + function OwnerTransferV7b711143(uint256) external { + ``` + - Found in src/OnceModifierExample.sol [Line: 10](../tests/contract-playground/src/OnceModifierExample.sol#L10) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index 8b6208c2..256c9baa 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -3422,6 +3422,37 @@ }, "ruleId": "constant-function-changing-state" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionSignatureCollision.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 166 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionSignatureCollision.sol" + }, + "region": { + "byteLength": 22, + "byteOffset": 231 + } + } + } + ], + "message": { + "text": "Function selector collides with other functions. This may cause the solidity function dispatcher to invoke the wrong function if the functions happen to be included in the same contract through an inheritance hirearchy later down the line. It is recommended to rename this function or change its parameters." + }, + "ruleId": "function-selector-collision" + }, { "level": "note", "locations": [ @@ -3965,6 +3996,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionSignatureCollision.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -6193,6 +6235,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionSignatureCollision.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -6827,6 +6880,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionSignatureCollision.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 166 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionSignatureCollision.sol" + }, + "region": { + "byteLength": 22, + "byteOffset": 231 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/tests/contract-playground/src/FunctionSignatureCollision.sol b/tests/contract-playground/src/FunctionSignatureCollision.sol new file mode 100644 index 00000000..076c1817 --- /dev/null +++ b/tests/contract-playground/src/FunctionSignatureCollision.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +// The following functions have the same function signature with different names. + +contract A { + function withdraw(uint256) external { + + } +} + +contract B { + function OwnerTransferV7b711143(uint256) external { + + } +} + +