diff --git a/aderyn_core/src/detect/high/unprotected_init_function.rs b/aderyn_core/src/detect/high/unprotected_init_function.rs index c67babc8..d52a2f20 100644 --- a/aderyn_core/src/detect/high/unprotected_init_function.rs +++ b/aderyn_core/src/detect/high/unprotected_init_function.rs @@ -3,8 +3,15 @@ use std::{collections::BTreeMap, error::Error}; use crate::{ ast::NodeID, capture, - context::{browser::ExtractIdentifiers, workspace_context::WorkspaceContext}, - detect::detector::{IssueDetector, IssueDetectorNamePool, IssueSeverity}, + context::{ + browser::{ExtractIdentifiers, ExtractModifierInvocations, ExtractRevertStatements}, + graph::{CallGraph, CallGraphDirection, CallGraphVisitor}, + workspace_context::WorkspaceContext, + }, + detect::{ + detector::{IssueDetector, IssueDetectorNamePool, IssueSeverity}, + helpers, + }, }; use eyre::Result; @@ -17,15 +24,16 @@ pub struct UnprotectedInitializerDetector { impl IssueDetector for UnprotectedInitializerDetector { fn detect(&mut self, context: &WorkspaceContext) -> Result> { - for function in context.function_definitions() { - if function.name.to_lowercase().contains("init") { - let has_modifiers = !function.modifiers.is_empty(); - if !has_modifiers { - let identifiers = ExtractIdentifiers::from(function).extracted; - if !identifiers.iter().any(|x| x.name == "revert" || x.name == "require") { - capture!(self, context, function); - } + for func in helpers::get_implemented_external_and_public_functions(context) { + let callgraph = CallGraph::new(context, &[&func.into()], CallGraphDirection::Inward)?; + let mut tracker = UnprotectedInitializationTracker::default(); + callgraph.accept(context, &mut tracker)?; + + if func.name.starts_with("_init") || func.name.starts_with("init") { + if tracker.has_initializer_modifier || tracker.has_require_or_revert { + continue; } + capture!(self, context, func); } } @@ -53,6 +61,51 @@ impl IssueDetector for UnprotectedInitializerDetector { } } +#[derive(Default, Debug)] +struct UnprotectedInitializationTracker { + has_require_or_revert: bool, + has_initializer_modifier: bool, // devtooligan's suggestion +} + +impl CallGraphVisitor for UnprotectedInitializationTracker { + fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { + // Check for revert(), require(), revert SomeError() + let has_req_or_revert_calls = ExtractIdentifiers::from(node) + .extracted + .into_iter() + .any(|x| x.name == "require" || x.name == "revert"); + + let has_revert_stmnts = !ExtractRevertStatements::from(node).extracted.is_empty(); + + if has_req_or_revert_calls || has_revert_stmnts { + self.has_require_or_revert = true; + } + + // Check if modifier name is "initialized" and assume it works + // This is done becauase often times initialized comes from openzeppelin and it is out of + // scope when running aderyn due to it being a library + + let modifier_invocations = ExtractModifierInvocations::from(node).extracted; + + for inv in modifier_invocations { + match inv.modifier_name { + crate::ast::IdentifierOrIdentifierPath::Identifier(n) => { + if n.name == "initializer" { + self.has_initializer_modifier = true; + } + } + crate::ast::IdentifierOrIdentifierPath::IdentifierPath(n) => { + if n.name == "initializer" { + self.has_initializer_modifier = true; + } + } + } + } + + Ok(()) + } +} + #[cfg(test)] mod unprotected_initializer { use serial_test::serial; @@ -69,8 +122,7 @@ mod unprotected_initializer { ); let mut detector = UnprotectedInitializerDetector::default(); - let found = detector.detect(&context).unwrap(); - // assert that the detector found an abi encode packed + let found = detector.detect(&context).unwrap(); // assert that the detector found an abi encode packed assert!(found); // println!("{:?}", detector.instances()); assert_eq!(detector.instances().len(), 1); diff --git a/reports/ccip-functions-report.md b/reports/ccip-functions-report.md index c6a976fd..1a180b35 100644 --- a/reports/ccip-functions-report.md +++ b/reports/ccip-functions-report.md @@ -8,9 +8,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Files Details](#files-details) - [Issue Summary](#issue-summary) - [High Issues](#high-issues) - - [H-1: Unprotected initializer](#h-1-unprotected-initializer) - - [H-2: Contract Name Reused in Different Files](#h-2-contract-name-reused-in-different-files) - - [H-3: Uninitialized State Variables](#h-3-uninitialized-state-variables) + - [H-1: Contract Name Reused in Different Files](#h-1-contract-name-reused-in-different-files) + - [H-2: Uninitialized State Variables](#h-2-uninitialized-state-variables) - [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) @@ -105,48 +104,13 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 3 | +| High | 2 | | Low | 18 | # High Issues -## H-1: Unprotected initializer - -Consider protecting the initializer functions with modifiers. - -
4 Found Instances - - -- Found in src/v0.8/functions/dev/v1_X/libraries/FunctionsRequest.sol [Line: 91](../tests/ccip-contracts/contracts/src/v0.8/functions/dev/v1_X/libraries/FunctionsRequest.sol#L91) - - ```solidity - function _initializeRequest( - ``` - -- Found in src/v0.8/functions/dev/v1_X/libraries/FunctionsRequest.sol [Line: 108](../tests/ccip-contracts/contracts/src/v0.8/functions/dev/v1_X/libraries/FunctionsRequest.sol#L108) - - ```solidity - function _initializeRequestForInlineJavaScript(Request memory self, string memory javaScriptSource) internal pure { - ``` - -- Found in src/v0.8/functions/v1_0_0/libraries/FunctionsRequest.sol [Line: 91](../tests/ccip-contracts/contracts/src/v0.8/functions/v1_0_0/libraries/FunctionsRequest.sol#L91) - - ```solidity - function initializeRequest( - ``` - -- Found in src/v0.8/functions/v1_0_0/libraries/FunctionsRequest.sol [Line: 108](../tests/ccip-contracts/contracts/src/v0.8/functions/v1_0_0/libraries/FunctionsRequest.sol#L108) - - ```solidity - function initializeRequestForInlineJavaScript(Request memory self, string memory javaScriptSource) internal pure { - ``` - -
- - - -## H-2: Contract Name Reused in Different Files +## H-1: Contract Name Reused in Different Files When compiling contracts with certain development frameworks (for example: Truffle), having contracts with the same name across different files can lead to one being overwritten. @@ -469,7 +433,7 @@ When compiling contracts with certain development frameworks (for example: Truff -## H-3: Uninitialized State Variables +## H-2: Uninitialized State Variables 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. diff --git a/reports/report.json b/reports/report.json index cdc663f8..90d62004 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { "total_source_units": 112, - "total_sloc": 3954 + "total_sloc": 3957 }, "files_details": { "files_details": [ @@ -339,7 +339,7 @@ }, { "file_path": "src/UnprotectedInitialize.sol", - "n_sloc": 25 + "n_sloc": 28 }, { "file_path": "src/UnsafeERC721Mint.sol", @@ -549,9 +549,9 @@ "instances": [ { "contract_path": "src/UnprotectedInitialize.sol", - "line_no": 35, - "src": "820:33", - "src_char": "820:33" + "line_no": 37, + "src": "916:33", + "src_char": "916:33" } ] }, @@ -4615,9 +4615,9 @@ }, { "contract_path": "src/UnprotectedInitialize.sol", - "line_no": 13, - "src": "222:21", - "src_char": "222:21" + "line_no": 15, + "src": "320:21", + "src_char": "320:21" } ] }, @@ -4776,6 +4776,12 @@ "src": "915:65", "src_char": "915:65" }, + { + "contract_path": "src/UnprotectedInitialize.sol", + "line_no": 43, + "src": "1071:38", + "src_char": "1071:38" + }, { "contract_path": "src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol", "line_no": 11, @@ -6980,21 +6986,21 @@ }, { "contract_path": "src/UnprotectedInitialize.sol", - "line_no": 19, - "src": "354:22", - "src_char": "354:22" + "line_no": 21, + "src": "452:22", + "src_char": "452:22" }, { "contract_path": "src/UnprotectedInitialize.sol", - "line_no": 25, - "src": "522:20", - "src_char": "522:20" + "line_no": 27, + "src": "618:20", + "src_char": "618:20" }, { "contract_path": "src/UnprotectedInitialize.sol", - "line_no": 35, - "src": "820:33", - "src_char": "820:33" + "line_no": 37, + "src": "916:33", + "src_char": "916:33" }, { "contract_path": "src/UnusedError.sol", @@ -7239,9 +7245,9 @@ }, { "contract_path": "src/UnprotectedInitialize.sol", - "line_no": 7, - "src": "146:5", - "src_char": "146:5" + "line_no": 9, + "src": "244:5", + "src_char": "244:5" }, { "contract_path": "src/auditor_mode/ExternalCalls.sol", diff --git a/reports/report.md b/reports/report.md index f44c2a82..f84c0fe6 100644 --- a/reports/report.md +++ b/reports/report.md @@ -105,7 +105,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | | .sol Files | 112 | -| Total nSLOC | 3954 | +| Total nSLOC | 3957 | ## Files Details @@ -195,7 +195,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/UncheckedSend.sol | 18 | | src/UninitializedLocalVariables.sol | 62 | | src/UninitializedStateVariable.sol | 29 | -| src/UnprotectedInitialize.sol | 25 | +| src/UnprotectedInitialize.sol | 28 | | src/UnsafeERC721Mint.sol | 18 | | src/UnusedError.sol | 19 | | src/UnusedImport.sol | 10 | @@ -224,7 +224,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** | **3954** | +| **Total** | **3957** | ## Issue Summary @@ -338,7 +338,7 @@ Consider protecting the initializer functions with modifiers.
1 Found Instances -- Found in src/UnprotectedInitialize.sol [Line: 35](../tests/contract-playground/src/UnprotectedInitialize.sol#L35) +- Found in src/UnprotectedInitialize.sol [Line: 37](../tests/contract-playground/src/UnprotectedInitialize.sol#L37) ```solidity function initializeWithoutModifierOrRevert() external { @@ -4592,7 +4592,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai modifier lockTheSwap { ``` -- Found in src/UnprotectedInitialize.sol [Line: 13](../tests/contract-playground/src/UnprotectedInitialize.sol#L13) +- Found in src/UnprotectedInitialize.sol [Line: 15](../tests/contract-playground/src/UnprotectedInitialize.sol#L15) ```solidity modifier firstTimeInitializing() { @@ -4606,7 +4606,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai Consider removing empty blocks. -
34 Found Instances +
35 Found Instances - Found in src/AderynIgnoreCustomDetectors.sol [Line: 7](../tests/contract-playground/src/AderynIgnoreCustomDetectors.sol#L7) @@ -4759,6 +4759,12 @@ Consider removing empty blocks. function doSomething(bool success) internal pure { ``` +- Found in src/UnprotectedInitialize.sol [Line: 43](../tests/contract-playground/src/UnprotectedInitialize.sol#L43) + + ```solidity + function initializeWithModifierNamedInitiliazer() external initializer { + ``` + - Found in src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol [Line: 11](../tests/contract-playground/src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol#L11) ```solidity @@ -7070,19 +7076,19 @@ State variable changes in this function but no event is emitted. function revokeUser(address _user) external { ``` -- Found in src/UnprotectedInitialize.sol [Line: 19](../tests/contract-playground/src/UnprotectedInitialize.sol#L19) +- Found in src/UnprotectedInitialize.sol [Line: 21](../tests/contract-playground/src/UnprotectedInitialize.sol#L21) ```solidity - function initializeWithModifier() external firstTimeInitializing() { + function initializeWithModifier() external firstTimeInitializing { ``` -- Found in src/UnprotectedInitialize.sol [Line: 25](../tests/contract-playground/src/UnprotectedInitialize.sol#L25) +- Found in src/UnprotectedInitialize.sol [Line: 27](../tests/contract-playground/src/UnprotectedInitialize.sol#L27) ```solidity function initializeWithRevert() external { ``` -- Found in src/UnprotectedInitialize.sol [Line: 35](../tests/contract-playground/src/UnprotectedInitialize.sol#L35) +- Found in src/UnprotectedInitialize.sol [Line: 37](../tests/contract-playground/src/UnprotectedInitialize.sol#L37) ```solidity function initializeWithoutModifierOrRevert() external { @@ -7333,7 +7339,7 @@ State variables that are should be declared immutable to save gas. Add the `immu uint256 public myVar; // initialized in extension, hence not captured ``` -- Found in src/UnprotectedInitialize.sol [Line: 7](../tests/contract-playground/src/UnprotectedInitialize.sol#L7) +- Found in src/UnprotectedInitialize.sol [Line: 9](../tests/contract-playground/src/UnprotectedInitialize.sol#L9) ```solidity address private owner; diff --git a/reports/report.sarif b/reports/report.sarif index 9498df3e..3f7c11a8 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -151,7 +151,7 @@ }, "region": { "byteLength": 33, - "byteOffset": 820 + "byteOffset": 916 } } } @@ -7401,7 +7401,7 @@ }, "region": { "byteLength": 21, - "byteOffset": 222 + "byteOffset": 320 } } } @@ -7689,6 +7689,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UnprotectedInitialize.sol" + }, + "region": { + "byteLength": 38, + "byteOffset": 1071 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -11633,7 +11644,7 @@ }, "region": { "byteLength": 22, - "byteOffset": 354 + "byteOffset": 452 } } }, @@ -11644,7 +11655,7 @@ }, "region": { "byteLength": 20, - "byteOffset": 522 + "byteOffset": 618 } } }, @@ -11655,7 +11666,7 @@ }, "region": { "byteLength": 33, - "byteOffset": 820 + "byteOffset": 916 } } }, @@ -12104,7 +12115,7 @@ }, "region": { "byteLength": 5, - "byteOffset": 146 + "byteOffset": 244 } } }, diff --git a/reports/templegold-report.md b/reports/templegold-report.md index dbdc6831..26e01646 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -9,13 +9,12 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Issue Summary](#issue-summary) - [High Issues](#high-issues) - [H-1: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`)](#h-1-arbitrary-from-passed-to-transferfrom-or-safetransferfrom) - - [H-2: Unprotected initializer](#h-2-unprotected-initializer) - - [H-3: Unsafe Casting](#h-3-unsafe-casting) - - [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: Weak Randomness](#h-6-weak-randomness) - - [H-7: Deletion from a nested mappping.](#h-7-deletion-from-a-nested-mappping) - - [H-8: Contract locks Ether without a withdraw function.](#h-8-contract-locks-ether-without-a-withdraw-function) + - [H-2: Unsafe Casting](#h-2-unsafe-casting) + - [H-3: Contract Name Reused in Different Files](#h-3-contract-name-reused-in-different-files) + - [H-4: Uninitialized State Variables](#h-4-uninitialized-state-variables) + - [H-5: Weak Randomness](#h-5-weak-randomness) + - [H-6: Deletion from a nested mappping.](#h-6-deletion-from-a-nested-mappping) + - [H-7: Contract locks Ether without a withdraw function.](#h-7-contract-locks-ether-without-a-withdraw-function) - [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) @@ -197,7 +196,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 8 | +| High | 7 | | Low | 28 | @@ -238,48 +237,7 @@ Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) ca -## H-2: Unprotected initializer - -Consider protecting the initializer functions with modifiers. - -
5 Found Instances - - -- Found in contracts/amo/test/external/IStashRewards.sol [Line: 9](../tests/2024-07-templegold/protocol/contracts/amo/test/external/IStashRewards.sol#L9) - - ```solidity - function initialize(uint256 _pid, address _operator, address _staker, address _gauge, address _rewardFactory) external; - ``` - -- Found in contracts/util/ABDKMathQuad.sol [Line: 422](../tests/2024-07-templegold/protocol/contracts/util/ABDKMathQuad.sol#L422) - - ```solidity - function isInfinity (bytes16 x) internal pure returns (bool) { - ``` - -- Found in contracts/v2/TempleDebtToken.sol [Line: 597](../tests/2024-07-templegold/protocol/contracts/v2/TempleDebtToken.sol#L597) - - ```solidity - function _initBaseCache(BaseCache memory _baseCache) private view returns (bool dirty) { - ``` - -- Found in contracts/v2/TempleDebtToken.sol [Line: 672](../tests/2024-07-templegold/protocol/contracts/v2/TempleDebtToken.sol#L672) - - ```solidity - function _initDebtorCache( - ``` - -- Found in contracts/v2/templeLineOfCredit/TempleLineOfCredit.sol [Line: 618](../tests/2024-07-templegold/protocol/contracts/v2/templeLineOfCredit/TempleLineOfCredit.sol#L618) - - ```solidity - function _initDebtTokenCache(DebtTokenCache memory _cache) private view returns (bool dirty) { - ``` - -
- - - -## H-3: Unsafe Casting +## H-2: Unsafe Casting Downcasting int/uints in Solidity can be unsafe due to the potential for data loss and unintended behavior.When downcasting a larger integer type to a smaller one (e.g., uint256 to uint128), the value may exceed the range of the target type,leading to truncation and loss of significant digits. Use OpenZeppelin's SafeCast library to safely downcast integers. @@ -302,7 +260,7 @@ Downcasting int/uints in Solidity can be unsafe due to the potential for data lo -## H-4: Contract Name Reused in Different Files +## H-3: Contract Name Reused in Different Files When compiling contracts with certain development frameworks (for example: Truffle), having contracts with the same name across different files can lead to one being overwritten. @@ -337,7 +295,7 @@ When compiling contracts with certain development frameworks (for example: Truff -## H-5: Uninitialized State Variables +## H-4: Uninitialized State Variables 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. @@ -390,7 +348,7 @@ Solidity does initialize variables by default when you declare them, however it' -## H-6: Weak Randomness +## H-5: Weak Randomness The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity. @@ -407,7 +365,7 @@ The use of keccak256 hash functions on predictable values like block.timestamp, -## H-7: Deletion from a nested mappping. +## H-6: 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. @@ -424,7 +382,7 @@ A deletion in a structure containing a mapping will not delete the mapping. The -## H-8: Contract locks Ether without a withdraw function. +## H-7: Contract locks Ether without a withdraw function. It appears that the contract includes a payable function to accept Ether but lacks a corresponding function to withdraw it, which leads to the Ether being locked in the contract. To resolve this issue, please implement a public or external function that allows for the withdrawal of Ether from the contract. diff --git a/tests/contract-playground/src/UnprotectedInitialize.sol b/tests/contract-playground/src/UnprotectedInitialize.sol index 7f72a33f..8bbebc8b 100644 --- a/tests/contract-playground/src/UnprotectedInitialize.sol +++ b/tests/contract-playground/src/UnprotectedInitialize.sol @@ -1,8 +1,10 @@ // SPDX-License-Identifier: No License +import "../lib/openzeppelin-contracts/contracts/proxy/utils/Initializable.sol"; + pragma solidity 0.8.19; -contract InitializedContract { +contract InitializedContract is Initializable { bool private initialized; address private owner; @@ -16,7 +18,7 @@ contract InitializedContract { } // GOOD - function initializeWithModifier() external firstTimeInitializing() { + function initializeWithModifier() external firstTimeInitializing { initialized = true; // Additional initialization logic here } @@ -36,4 +38,9 @@ contract InitializedContract { initialized = true; // Additional initialization logic here } + + // GOOD + function initializeWithModifierNamedInitiliazer() external initializer { + // Additional initialization logic here + } }