diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index eef406e72..80ed73457 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -66,6 +66,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -131,6 +132,7 @@ pub(crate) enum IssueDetectorNamePool { RTLO, UncheckedReturn, DangerousUnaryOperator, + PublicVariableReadInExternalContext, WeakRandomness, PreDeclaredLocalVariableUsage, DeleteNestedMapping, @@ -270,6 +272,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::PublicVariableReadInExternalContext => { + Some(Box::::default()) + } IssueDetectorNamePool::WeakRandomness => Some(Box::::default()), IssueDetectorNamePool::PreDeclaredLocalVariableUsage => { Some(Box::::default()) diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index 6b8270129..a20f696f5 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -8,6 +8,7 @@ pub(crate) mod empty_blocks; pub(crate) mod inconsistent_type_names; pub(crate) mod large_literal_value; pub(crate) mod non_reentrant_before_others; +pub(crate) mod public_variable_read_in_external_context; pub(crate) mod push_0_opcode; pub(crate) mod require_with_string; pub(crate) mod reverts_and_requries_in_loops; @@ -32,6 +33,7 @@ pub use empty_blocks::EmptyBlockDetector; pub use inconsistent_type_names::InconsistentTypeNamesDetector; pub use large_literal_value::LargeLiteralValueDetector; pub use non_reentrant_before_others::NonReentrantBeforeOthersDetector; +pub use public_variable_read_in_external_context::PublicVariableReadInExternalContextDetector; pub use push_0_opcode::PushZeroOpcodeDetector; pub use require_with_string::RequireWithStringDetector; pub use reverts_and_requries_in_loops::RevertsAndRequiresInLoopsDetector; diff --git a/aderyn_core/src/detect/low/public_variable_read_in_external_context.rs b/aderyn_core/src/detect/low/public_variable_read_in_external_context.rs new file mode 100644 index 000000000..a0c95573f --- /dev/null +++ b/aderyn_core/src/detect/low/public_variable_read_in_external_context.rs @@ -0,0 +1,170 @@ +use std::collections::{BTreeMap, HashSet}; +use std::error::Error; + +use crate::ast::{ + ASTNode, ContractDefinition, Expression, Identifier, MemberAccess, NodeID, Visibility, +}; + +use crate::capture; +use crate::context::browser::{ExtractFunctionCalls, ExtractVariableDeclarations}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::{eyre, Result}; + +#[derive(Default)] +pub struct PublicVariableReadInExternalContextDetector { + // 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 PublicVariableReadInExternalContextDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for contract in context.contract_definitions() { + // Public state variables including the base contracts + if let Ok(public_state_variable_names) = + find_all_public_state_variables_names_for_contract(context, contract) + { + // Find all the `X`s that appear with the pattern `this.X()` + let this_member_accesses = + find_all_public_member_names_called_using_this_keyword_in_contract( + context, contract, + ); + + for member_access in this_member_accesses { + if public_state_variable_names.contains(&member_access.member_name) { + capture!(self, context, member_access); + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::Low + } + + fn title(&self) -> String { + String::from("Public variables of a contract read in an external context (using `this`).") + } + + fn description(&self) -> String { + String::from( + "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage.", + ) + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::PublicVariableReadInExternalContext.to_string() + } +} + +fn find_all_public_member_names_called_using_this_keyword_in_contract<'a>( + context: &'a WorkspaceContext, + contract: &ContractDefinition, +) -> Vec<&'a MemberAccess> { + let mut member_names = vec![]; + + let function_calls = ExtractFunctionCalls::from(contract).extracted; + for function_call in function_calls { + if let Expression::MemberAccess(MemberAccess { id, expression, .. }) = + function_call.expression.as_ref() + { + if let Expression::Identifier(Identifier { name, .. }) = expression.as_ref() { + if name == "this" { + if let Some(ASTNode::MemberAccess(member_acess)) = context.nodes.get(id) { + member_names.push(member_acess) + } + } + } + } + } + + member_names +} + +// Scans the linearized base contracts and returns a list of all the NodeIDs of public variable declarations +fn find_all_public_state_variables_names_for_contract( + context: &WorkspaceContext, + contract: &ContractDefinition, +) -> Result, Box> { + let inheritance_ancestors = contract + .linearized_base_contracts + .as_ref() + .ok_or(eyre!("base contracts not found!"))?; + + Ok(inheritance_ancestors + .iter() + .flat_map(|ancestor_id| { + if let Some(ancestor) = context.nodes.get(ancestor_id) { + let public_variable_declaraions = + ExtractVariableDeclarations::from(ancestor).extracted; + return Some( + public_variable_declaraions + .into_iter() + .filter(|declaration| { + declaration.state_variable + && declaration.visibility == Visibility::Public + }) + .collect::>(), + ); + } + None + }) + .flatten() + .map(|v| v.name.clone()) + .collect()) +} + +#[cfg(test)] +mod public_variable_read_in_external_context_detector_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + low::public_variable_read_in_external_context::PublicVariableReadInExternalContextDetector, + }; + + #[test] + #[serial] + fn test_public_variable_read_in_external_context() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/PublicVariableReadInExternalContext.sol", + ); + + let mut detector = PublicVariableReadInExternalContextDetector::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(), 4); + // 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( + "Public variables of a contract read in an external context (using `this`)." + ) + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from( + "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage.", + ) + ); + } +} diff --git a/reports/report.json b/reports/report.json index caa2ecc85..c941b7001 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 68, - "total_sloc": 1922 + "total_source_units": 69, + "total_sloc": 1954 }, "files_details": { "files_details": [ @@ -121,6 +121,10 @@ "file_path": "src/PreDeclaredVarUsage.sol", "n_sloc": 9 }, + { + "file_path": "src/PublicVariableReadInExternalContext.sol", + "n_sloc": 32 + }, { "file_path": "src/RTLO.sol", "n_sloc": 7 @@ -281,7 +285,7 @@ }, "issue_count": { "high": 30, - "low": 23 + "low": 24 }, "high_issues": { "issues": [ @@ -1271,6 +1275,12 @@ "src": "355:7", "src_char": "355:7" }, + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 6, + "src": "130:26", + "src_char": "130:26" + }, { "contract_path": "src/StateShadowing.sol", "line_no": 5, @@ -3479,6 +3489,37 @@ "src_char": "541:5" } ] + }, + { + "title": "Public variables of a contract read in an external context (using `this`).", + "description": "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage.", + "detector_name": "public-variable-read-in-external-context", + "instances": [ + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 12, + "src": "355:14", + "src_char": "355:14" + }, + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 16, + "src": "457:16", + "src_char": "457:16" + }, + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 20, + "src": "553:12", + "src_char": "553:12" + }, + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 42, + "src": "1175:14", + "src_char": "1175:14" + } + ] } ] }, @@ -3533,6 +3574,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "public-variable-read-in-external-context", "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping" diff --git a/reports/report.md b/reports/report.md index dba746648..b813542af 100644 --- a/reports/report.md +++ b/reports/report.md @@ -62,6 +62,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: Public variables of a contract read in an external context (using `this`).](#l-24-public-variables-of-a-contract-read-in-an-external-context-using-this) # Summary @@ -70,8 +71,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 68 | -| Total nSLOC | 1922 | +| .sol Files | 69 | +| Total nSLOC | 1954 | ## Files Details @@ -107,6 +108,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/MultipleConstructorSchemes.sol | 10 | | src/OnceModifierExample.sol | 8 | | src/PreDeclaredVarUsage.sol | 9 | +| src/PublicVariableReadInExternalContext.sol | 32 | | src/RTLO.sol | 7 | | src/RevertsAndRequriesInLoops.sol | 27 | | src/SendEtherNoChecks.sol | 58 | @@ -146,7 +148,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** | **1922** | +| **Total** | **1954** | ## Issue Summary @@ -154,7 +156,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 30 | -| Low | 23 | +| Low | 24 | # High Issues @@ -1176,7 +1178,7 @@ If the length of a dynamic array (storage variable) directly assigned to, it may Solidity does initialize variables by default when you declare them, however it's good practice to explicitly declare an initial value. For example, if you transfer money to an address we must make sure that the address has been initialized. -
13 Found Instances +
14 Found Instances - Found in src/AssemblyExample.sol [Line: 5](../tests/contract-playground/src/AssemblyExample.sol#L5) @@ -1209,6 +1211,12 @@ Solidity does initialize variables by default when you declare them, however it' uint256 private s_first; ``` +- Found in src/PublicVariableReadInExternalContext.sol [Line: 6](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L6) + + ```solidity + uint256 public testUint256; + ``` + - Found in src/StateShadowing.sol [Line: 5](../tests/contract-playground/src/StateShadowing.sol#L5) ```solidity @@ -3557,3 +3565,38 @@ Division operations followed directly by multiplication operations can lead to p +## L-24: Public variables of a contract read in an external context (using `this`). + +The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage. + +
4 Found Instances + + +- Found in src/PublicVariableReadInExternalContext.sol [Line: 12](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L12) + + ```solidity + return this.testArray(0); + ``` + +- Found in src/PublicVariableReadInExternalContext.sol [Line: 16](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L16) + + ```solidity + return this.testUint256(); + ``` + +- Found in src/PublicVariableReadInExternalContext.sol [Line: 20](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L20) + + ```solidity + return this.testMap(0); + ``` + +- Found in src/PublicVariableReadInExternalContext.sol [Line: 42](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L42) + + ```solidity + return this.testArray(0); + ``` + +
+ + + diff --git a/reports/report.sarif b/reports/report.sarif index ddc8f5e5d..3d415b864 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1748,6 +1748,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 26, + "byteOffset": 130 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5650,6 +5661,59 @@ "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/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 14, + "byteOffset": 355 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 457 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 12, + "byteOffset": 553 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 14, + "byteOffset": 1175 + } + } + } + ], + "message": { + "text": "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage." + }, + "ruleId": "public-variable-read-in-external-context" } ], "tool": { diff --git a/tests/contract-playground/src/PublicVariableReadInExternalContext.sol b/tests/contract-playground/src/PublicVariableReadInExternalContext.sol new file mode 100644 index 000000000..7635d4db9 --- /dev/null +++ b/tests/contract-playground/src/PublicVariableReadInExternalContext.sol @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.5.0; + +contract PublicVariableReadUsingThis { + string[] public testArray; + uint256 public testUint256; + mapping(uint256 => bool) public testMap; + + // BAD ways of reading (using this.) + + function readStringArray() external view returns (string memory) { + return this.testArray(0); + } + + function readUint256() external view returns (uint256) { + return this.testUint256(); + } + + function readMap() external view returns (bool) { + return this.testMap(0); + } + + // GOOD ways of reading (using ) + + function readStringArrayGood() external view returns (string memory) { + return testArray[0]; + } + + function readUint256Good() external view returns (uint256) { + return testUint256; + } + + function readMapGood() external view returns (bool) { + return testMap[0]; + } +} + +contract DerivedFromPublicVariableReadUsingThis is PublicVariableReadUsingThis { + // BAD ways of reading (using this.) + + function readStringArray() external view returns (string memory) { + return this.testArray(0); + } + + // GOOD ways of reading (using ) + + function readStringArrayGood() external view returns (string memory) { + return testArray[0]; + } +}