diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 11b46f801..91264b88c 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -65,6 +65,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -126,6 +127,7 @@ pub(crate) enum IssueDetectorNamePool { RTLO, UncheckedReturn, DangerousUnaryOperator, + DeleteNestedMapping, // 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, @@ -261,6 +263,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::DeleteNestedMapping => { + Some(Box::::default()) + } IssueDetectorNamePool::Undecided => None, } } diff --git a/aderyn_core/src/detect/high/deletion_nested_mapping.rs b/aderyn_core/src/detect/high/deletion_nested_mapping.rs new file mode 100644 index 000000000..17820714f --- /dev/null +++ b/aderyn_core/src/detect/high/deletion_nested_mapping.rs @@ -0,0 +1,135 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{ + ASTNode, Expression, Identifier, IndexAccess, Mapping, NodeID, TypeName, UserDefinedTypeName, + VariableDeclaration, +}; + +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 DeletionNestedMappingDetector { + // 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 DeletionNestedMappingDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for delete_operation in context + .unary_operations() + .into_iter() + .filter(|op| op.operator == "delete") + { + if let Expression::IndexAccess(IndexAccess { + base_expression, .. + }) = delete_operation.sub_expression.as_ref() + { + if let Expression::Identifier(Identifier { + referenced_declaration: Some(referenced_id), + type_descriptions, + .. + }) = base_expression.as_ref() + { + // Check if we're deleting a value from mapping + if type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| type_string.starts_with("mapping")) + { + // Check if the value in the mapping is of type struct that has a member which is also a mapping + if let Some(ASTNode::VariableDeclaration(VariableDeclaration { + type_name: Some(TypeName::Mapping(Mapping { value_type, .. })), + .. + })) = context.nodes.get(referenced_id) + { + if let TypeName::UserDefinedTypeName(UserDefinedTypeName { + referenced_declaration, + .. + }) = value_type.as_ref() + { + if let Some(ASTNode::StructDefinition(structure)) = + context.nodes.get(referenced_declaration) + { + // Check that a member of a struct is of type mapping + if structure.members.iter().any(|member| { + member.type_descriptions.type_string.as_ref().is_some_and( + |type_string| type_string.starts_with("mapping"), + ) + }) { + capture!(self, context, delete_operation); + } + } + } + } + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Deletion from a nested mappping.") + } + + fn description(&self) -> String { + String::from("A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::DeleteNestedMapping.to_string() + } +} + +#[cfg(test)] +mod deletion_nested_mapping_tests { + use crate::detect::{ + detector::IssueDetector, high::deletion_nested_mapping::DeletionNestedMappingDetector, + }; + + #[test] + fn test_deletion_nested_mapping() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol", + ); + + let mut detector = DeletionNestedMappingDetector::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 high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Deletion from a nested mappping.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.") + ); + } +} diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index e247206b9..b23275b99 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -4,6 +4,7 @@ pub(crate) mod block_timestamp_deadline; pub(crate) mod dangerous_unary_operator; pub(crate) mod delegate_call_in_loop; pub(crate) mod delegate_call_no_address_check; +pub(crate) mod deletion_nested_mapping; pub(crate) mod dynamic_array_length_assignment; pub(crate) mod enumerable_loop_removal; pub(crate) mod experimental_encoder; @@ -31,6 +32,7 @@ pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; pub use dangerous_unary_operator::DangerousUnaryOperatorDetector; pub use delegate_call_in_loop::DelegateCallInLoopDetector; pub use delegate_call_no_address_check::DelegateCallOnUncheckedAddressDetector; +pub use deletion_nested_mapping::DeletionNestedMappingDetector; pub use dynamic_array_length_assignment::DynamicArrayLengthAssignmentDetector; pub use enumerable_loop_removal::EnumerableLoopRemovalDetector; pub use experimental_encoder::ExperimentalEncoderDetector; diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index d3797769c..5ade44f99 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -187,6 +187,7 @@ "tautological-compare", "rtlo", "unchecked-return", - "dangerous-unary-operator" + "dangerous-unary-operator", + "delete-nested-mapping" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index 713071135..96bc6360c 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 64, - "total_sloc": 1825 + "total_source_units": 65, + "total_sloc": 1836 }, "files_details": { "files_details": [ @@ -53,6 +53,10 @@ "file_path": "src/DelegateCallWithoutAddressCheck.sol", "n_sloc": 31 }, + { + "file_path": "src/DeletionNestedMappingStructureContract.sol", + "n_sloc": 11 + }, { "file_path": "src/DeprecatedOZFunctions.sol", "n_sloc": 32 @@ -264,7 +268,7 @@ ] }, "issue_count": { - "high": 26, + "high": 27, "low": 23 }, "high_issues": { @@ -1578,6 +1582,19 @@ "src_char": "247:10" } ] + }, + { + "title": "Deletion from a nested mappping.", + "description": "A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.", + "detector_name": "delete-nested-mapping", + "instances": [ + { + "contract_path": "src/DeletionNestedMappingStructureContract.sol", + "line_no": 17, + "src": "426:25", + "src_char": "426:25" + } + ] } ] }, @@ -1821,6 +1838,12 @@ "src": "32:21", "src_char": "32:21" }, + { + "contract_path": "src/DeletionNestedMappingStructureContract.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/InconsistentUints.sol", "line_no": 1, @@ -2477,6 +2500,12 @@ "src": "32:21", "src_char": "32:21" }, + { + "contract_path": "src/DeletionNestedMappingStructureContract.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/DeprecatedOZFunctions.sol", "line_no": 2, @@ -3319,6 +3348,7 @@ "tautological-compare", "rtlo", "unchecked-return", - "dangerous-unary-operator" + "dangerous-unary-operator", + "delete-nested-mapping" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index 0ccc72620..c4bf1036c 100644 --- a/reports/report.md +++ b/reports/report.md @@ -34,6 +34,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-24: RTLO character detected in file. \u{202e}](#h-24-rtlo-character-detected-in-file-u202e) - [H-25: Return value of the function call is not checked.](#h-25-return-value-of-the-function-call-is-not-checked) - [H-26: Dangerous unary operator found in assignment.](#h-26-dangerous-unary-operator-found-in-assignment) + - [H-27: Deletion from a nested mappping.](#h-27-deletion-from-a-nested-mappping) - [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) @@ -66,8 +67,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 64 | -| Total nSLOC | 1825 | +| .sol Files | 65 | +| Total nSLOC | 1836 | ## Files Details @@ -86,6 +87,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/CrazyPragma.sol | 4 | | src/DangerousUnaryOperator.sol | 13 | | src/DelegateCallWithoutAddressCheck.sol | 31 | +| src/DeletionNestedMappingStructureContract.sol | 11 | | src/DeprecatedOZFunctions.sol | 32 | | src/DivisionBeforeMultiplication.sol | 22 | | src/DynamicArrayLengthAssignment.sol | 16 | @@ -138,14 +140,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** | **1825** | +| **Total** | **1836** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 26 | +| High | 27 | | Low | 23 | @@ -1567,6 +1569,23 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in +## H-27: Deletion from a nested mappping. + +A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract. + +
1 Found Instances + + +- Found in src/DeletionNestedMappingStructureContract.sol [Line: 17](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L17) + + ```solidity + delete people[msg.sender]; + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -1799,7 +1818,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;` -
13 Found Instances +
14 Found Instances - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) @@ -1832,6 +1851,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.8; ``` +- Found in src/DeletionNestedMappingStructureContract.sol [Line: 2](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/InconsistentUints.sol [Line: 1](../tests/contract-playground/src/InconsistentUints.sol#L1) ```solidity @@ -2475,7 +2500,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. -
25 Found Instances +
26 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2508,6 +2533,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity ^0.8; ``` +- Found in src/DeletionNestedMappingStructureContract.sol [Line: 2](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/DeprecatedOZFunctions.sol [Line: 2](../tests/contract-playground/src/DeprecatedOZFunctions.sol#L2) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index 9ae28c6c8..d8bda7569 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2306,6 +2306,26 @@ }, "ruleId": "dangerous-unary-operator" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 25, + "byteOffset": 426 + } + } + } + ], + "message": { + "text": "A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract." + }, + "ruleId": "delete-nested-mapping" + }, { "level": "note", "locations": [ @@ -2717,6 +2737,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3889,6 +3920,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/reports/templegold-report.md b/reports/templegold-report.md index ebc825a93..97bac719f 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -14,6 +14,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-4: Contract Name Reused in Different Files](#h-4-contract-name-reused-in-different-files) - [H-5: Uninitialized State Variables](#h-5-uninitialized-state-variables) - [H-6: Return value of the function call is not checked.](#h-6-return-value-of-the-function-call-is-not-checked) + - [H-7: Deletion from a nested mappping.](#h-7-deletion-from-a-nested-mappping) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: `ecrecover` is susceptible to signature malleability](#l-2-ecrecover-is-susceptible-to-signature-malleability) @@ -185,7 +186,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 6 | +| High | 7 | | Low | 18 | @@ -485,6 +486,23 @@ Function returns a value but it is ignored. +## H-7: Deletion from a nested mappping. + +A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract. + +
1 Found Instances + + +- Found in contracts/v2/TreasuryReservesVault.sol [Line: 317](../tests/2024-07-templegold/protocol/contracts/v2/TreasuryReservesVault.sol#L317) + + ```solidity + delete strategies[strategy]; + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners diff --git a/tests/contract-playground/src/DeletionNestedMappingStructureContract.sol b/tests/contract-playground/src/DeletionNestedMappingStructureContract.sol new file mode 100644 index 000000000..ee2f3a057 --- /dev/null +++ b/tests/contract-playground/src/DeletionNestedMappingStructureContract.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + + +contract NestedMappingBalanceStructure { + + struct Person { + uint256[] names; + mapping(address => uint256) age; + } + + mapping(address => Person) private people; + + function remove() internal{ + // We are deleting from a mapping whose value is a struct which contains a member of type mapping. + // Therefore, capture it! + delete people[msg.sender]; + } + +} \ No newline at end of file