From 6f33d0d5946f46ab3d604d396b0acab10203331e Mon Sep 17 00:00:00 2001 From: TilakMaddy Date: Thu, 25 Jul 2024 12:08:51 +0530 Subject: [PATCH] deletion nested mapping --- aderyn_core/src/detect/detector.rs | 23 +-- .../detect/high/deletion_nested_mapping.rs | 135 ++++++++++++++++++ aderyn_core/src/detect/high/mod.rs | 2 + .../adhoc-sol-files-highs-only-report.json | 3 +- reports/report.json | 38 ++++- reports/report.md | 43 +++++- reports/report.sarif | 42 ++++++ reports/templegold-report.md | 20 ++- ...DeletionNestedMappingStructureContract.sol | 20 +++ 9 files changed, 305 insertions(+), 21 deletions(-) create mode 100644 aderyn_core/src/detect/high/deletion_nested_mapping.rs create mode 100644 tests/contract-playground/src/DeletionNestedMappingStructureContract.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 2ba2c48b..1d7d7b31 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -8,15 +8,15 @@ use crate::{ high::{ ArbitraryTransferFromDetector, AvoidAbiEncodePackedDetector, BlockTimestampDeadlineDetector, DangerousUnaryOperatorDetector, - DelegateCallInLoopDetector, DynamicArrayLengthAssignmentDetector, - EnumerableLoopRemovalDetector, ExperimentalEncoderDetector, - IncorrectShiftOrderDetector, IncorrectUseOfCaretOperatorDetector, - MultipleConstructorsDetector, NestedStructInMappingDetector, RTLODetector, - ReusedContractNameDetector, SelfdestructIdentifierDetector, - StateVariableShadowingDetector, StorageArrayEditWithMemoryDetector, - TautologicalCompareDetector, UncheckedReturnDetector, - UninitializedStateVariableDetector, UnprotectedInitializerDetector, - UnsafeCastingDetector, YulReturnDetector, + DelegateCallInLoopDetector, DeletionNestedMappingDetector, + DynamicArrayLengthAssignmentDetector, EnumerableLoopRemovalDetector, + ExperimentalEncoderDetector, IncorrectShiftOrderDetector, + IncorrectUseOfCaretOperatorDetector, MultipleConstructorsDetector, + NestedStructInMappingDetector, RTLODetector, ReusedContractNameDetector, + SelfdestructIdentifierDetector, StateVariableShadowingDetector, + StorageArrayEditWithMemoryDetector, TautologicalCompareDetector, + UncheckedReturnDetector, UninitializedStateVariableDetector, + UnprotectedInitializerDetector, UnsafeCastingDetector, YulReturnDetector, }, low::{ CentralizationRiskDetector, ConstantsInsteadOfLiteralsDetector, @@ -87,6 +87,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -145,6 +146,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, @@ -273,6 +275,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 00000000..17820714 --- /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 c7fa87a4..847923b0 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -3,6 +3,7 @@ pub(crate) mod avoid_abi_encode_packed; pub(crate) mod block_timestamp_deadline; pub(crate) mod dangerous_unary_operator; pub(crate) mod delegate_call_in_loop; +pub(crate) mod deletion_nested_mapping; pub(crate) mod dynamic_array_length_assignment; pub(crate) mod enumerable_loop_removal; pub(crate) mod experimental_encoder; @@ -27,6 +28,7 @@ pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector; pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; pub use dangerous_unary_operator::DangerousUnaryOperatorDetector; pub use delegate_call_in_loop::DelegateCallInLoopDetector; +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 6a54a00c..17c07987 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -171,6 +171,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 8e01d3d6..b262e65f 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 60, - "total_sloc": 1620 + "total_source_units": 61, + "total_sloc": 1631 }, "files_details": { "files_details": [ @@ -45,6 +45,10 @@ "file_path": "src/DangerousUnaryOperator.sol", "n_sloc": 13 }, + { + "file_path": "src/DeletionNestedMappingStructureContract.sol", + "n_sloc": 11 + }, { "file_path": "src/DeprecatedOZFunctions.sol", "n_sloc": 32 @@ -248,7 +252,7 @@ ] }, "issue_count": { - "high": 23, + "high": 24, "low": 23 }, "high_issues": { @@ -1427,6 +1431,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" + } + ] } ] }, @@ -1658,6 +1675,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/DeletionNestedMappingStructureContract.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/InconsistentUints.sol", "line_no": 1, @@ -2242,6 +2265,12 @@ "src": "32:32", "src_char": "32:32" }, + { + "contract_path": "src/DeletionNestedMappingStructureContract.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/DeprecatedOZFunctions.sol", "line_no": 2, @@ -2949,6 +2978,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 bea26761..aa89504e 100644 --- a/reports/report.md +++ b/reports/report.md @@ -31,6 +31,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-21: RTLO character detected in file. \u{202e}](#h-21-rtlo-character-detected-in-file-u202e) - [H-22: Return value of the function call is not checked.](#h-22-return-value-of-the-function-call-is-not-checked) - [H-23: Dangerous unary operator found in assignment.](#h-23-dangerous-unary-operator-found-in-assignment) + - [H-24: Deletion from a nested mappping.](#h-24-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) @@ -63,8 +64,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 60 | -| Total nSLOC | 1620 | +| .sol Files | 61 | +| Total nSLOC | 1631 | ## Files Details @@ -81,6 +82,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/Counter.sol | 20 | | src/CrazyPragma.sol | 4 | | src/DangerousUnaryOperator.sol | 13 | +| src/DeletionNestedMappingStructureContract.sol | 11 | | src/DeprecatedOZFunctions.sol | 32 | | src/DivisionBeforeMultiplication.sol | 22 | | src/DynamicArrayLengthAssignment.sol | 16 | @@ -131,14 +133,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** | **1620** | +| **Total** | **1631** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 23 | +| High | 24 | | Low | 23 | @@ -1413,6 +1415,23 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in +## H-24: 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 @@ -1639,7 +1658,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;` -
12 Found Instances +
13 Found Instances - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) @@ -1666,6 +1685,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.4.0; ``` +- 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 @@ -2249,7 +2274,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. -
24 Found Instances +
25 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2276,6 +2301,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity >=0.8.19 <0.9.1; ``` +- 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 d1edc11f..e005ea3d 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2070,6 +2070,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": [ @@ -2459,6 +2479,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3499,6 +3530,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 ebc825a9..97bac719 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 00000000..ee2f3a05 --- /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