From a883e6bfe17c8e2d85c48f96f6607fd6eb59a6bc Mon Sep 17 00:00:00 2001 From: TilakMaddy Date: Fri, 18 Oct 2024 13:46:37 +0530 Subject: [PATCH] purge uninitialized state variable --- aderyn_core/src/detect/detector.rs | 5 - aderyn_core/src/detect/high/mod.rs | 2 - .../high/uninitialized_state_variable.rs | 145 ------- .../adhoc-sol-files-highs-only-report.json | 40 +- reports/adhoc-sol-files-report.md | 52 +-- reports/ccip-functions-report.md | 26 +- reports/hardhat-playground-report.md | 40 +- reports/report.json | 220 +--------- reports/report.md | 324 +++----------- reports/report.sarif | 394 ------------------ reports/templegold-report.md | 68 +-- 11 files changed, 71 insertions(+), 1245 deletions(-) delete mode 100644 aderyn_core/src/detect/high/uninitialized_state_variable.rs diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index d97fabd6..7693cc98 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -54,7 +54,6 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), - Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -169,7 +168,6 @@ pub(crate) enum IssueDetectorNamePool { NestedStructInMapping, SelfdestructIdentifier, DynamicArrayLengthAssignment, - UninitializedStateVariable, IncorrectCaretOperator, YulReturn, StateVariableShadowing, @@ -375,9 +373,6 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option::default()) } - IssueDetectorNamePool::UninitializedStateVariable => { - Some(Box::::default()) - } IssueDetectorNamePool::IncorrectCaretOperator => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 757cc413..e47edf40 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -33,7 +33,6 @@ pub(crate) mod tautology_or_contradiction; pub(crate) mod tx_origin_used_for_auth; pub(crate) mod unchecked_calls; pub(crate) mod unchecked_send; -pub(crate) mod uninitialized_state_variable; pub(crate) mod unprotected_init_function; pub(crate) mod unsafe_casting; pub(crate) mod weak_randomness; @@ -74,7 +73,6 @@ pub use tautology_or_contradiction::TautologyOrContraditionDetector; pub use tx_origin_used_for_auth::TxOriginUsedForAuthDetector; pub use unchecked_calls::UncheckedLowLevelCallDetector; pub use unchecked_send::UncheckedSendDetector; -pub use uninitialized_state_variable::UninitializedStateVariableDetector; pub use unprotected_init_function::UnprotectedInitializerDetector; pub use unsafe_casting::UnsafeCastingDetector; pub use weak_randomness::WeakRandomnessDetector; diff --git a/aderyn_core/src/detect/high/uninitialized_state_variable.rs b/aderyn_core/src/detect/high/uninitialized_state_variable.rs deleted file mode 100644 index 067a5c0d..00000000 --- a/aderyn_core/src/detect/high/uninitialized_state_variable.rs +++ /dev/null @@ -1,145 +0,0 @@ -use std::{ - collections::{BTreeMap, HashSet}, - error::Error, -}; - -use crate::ast::{Expression, NodeID}; - -use crate::{ - capture, - context::workspace_context::WorkspaceContext, - detect::detector::{IssueDetector, IssueDetectorNamePool, IssueSeverity}, -}; -use eyre::Result; - -#[derive(Default)] -pub struct UninitializedStateVariableDetector { - // 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 UninitializedStateVariableDetector { - fn detect(&mut self, context: &WorkspaceContext) -> Result> { - /* - * Plan (Maybe it can be improved) - * - Gather all the storage variables (VariableDeclarations) - * - Fitler out / Remove the ones where `value` property is not `None` - * - Fitler out / Remove the ones that are arrays, mappings and structs - * - Now, we're left with state variables that are not initialized at the same line - * where they are declared. - * - Gather all the `Assignments` and collect all the `referencedDeclarations` on - * `Identifier` expressions when they appear on LHS of the assginments - * - Remove the above ids from the initial storage variables list - * - Now we're left with storage variables that have never been initialized - */ - - let mut state_variable_ids = HashSet::new(); - - for var_decl in context - .variable_declarations() - .into_iter() - .filter(|s| s.state_variable && s.value.is_none()) - .filter(|s| { - s.type_descriptions - .type_string - .as_ref() - .is_some_and(|type_string| !type_string.starts_with("mapping")) - }) - .filter(|s| { - s.type_descriptions - .type_string - .as_ref() - .is_some_and(|type_string| !type_string.ends_with(']')) - }) - .filter(|s| { - s.type_descriptions - .type_string - .as_ref() - .is_some_and(|type_string| !type_string.starts_with("struct")) - }) - { - state_variable_ids.insert(var_decl.id); - } - - for assignment in context.assignments() { - if let Expression::Identifier(identifier) = assignment.left_hand_side.as_ref() { - if let Some(refers_to) = identifier.referenced_declaration { - let _ = state_variable_ids.remove(&refers_to); - } - } - } - - for id in state_variable_ids { - context.nodes.get(&id).inspect(|&x| capture!(self, context, x)); - } - - Ok(!self.found_instances.is_empty()) - } - - fn severity(&self) -> IssueSeverity { - IssueSeverity::High - } - - fn title(&self) -> String { - String::from("Uninitialized State Variables") - } - - fn description(&self) -> String { - String::from( - "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." - ) - } - - fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { - self.found_instances.clone() - } - - fn name(&self) -> String { - IssueDetectorNamePool::UninitializedStateVariable.to_string() - } -} - -#[cfg(test)] -mod uninitialized_state_variable_tests { - use serial_test::serial; - - use crate::detect::{ - detector::IssueDetector, - high::uninitialized_state_variable::UninitializedStateVariableDetector, - }; - - #[test] - #[serial] - fn test_uninitialized_state_variables() { - let context: crate::context::workspace_context::WorkspaceContext = - crate::detect::test_utils::load_solidity_source_unit( - "../tests/contract-playground/src/UninitializedStateVariable.sol", - ); - - let mut detector = UninitializedStateVariableDetector::default(); - let found = detector.detect(&context).unwrap(); - - println!("{:?}", detector.instances()); - - // 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); - // assert the title is correct - assert_eq!(detector.title(), String::from("Uninitialized State Variables")); - // assert the description is correct - assert_eq!( - detector.description(), - String::from( - "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/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index c38712b2..6bc3b4bd 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -88,7 +88,7 @@ ] }, "issue_count": { - "high": 4, + "high": 3, "low": 0 }, "high_issues": { @@ -106,43 +106,6 @@ } ] }, - { - "title": "Uninitialized State Variables", - "description": "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.", - "detector_name": "uninitialized-state-variable", - "instances": [ - { - "contract_path": "InconsistentUints.sol", - "line_no": 7, - "src": "197:11", - "src_char": "197:11" - }, - { - "contract_path": "InconsistentUints.sol", - "line_no": 8, - "src": "233:14", - "src_char": "233:14" - }, - { - "contract_path": "StateVariables.sol", - "line_no": 8, - "src": "199:19", - "src_char": "199:19" - }, - { - "contract_path": "StateVariables.sol", - "line_no": 9, - "src": "241:20", - "src_char": "241:20" - }, - { - "contract_path": "StateVariables.sol", - "line_no": 10, - "src": "282:18", - "src_char": "282:18" - } - ] - }, { "title": "Delegatecall made by the function without checks on any address.", "description": "Introduce checks on the address", @@ -189,7 +152,6 @@ "nested-struct-in-mapping", "selfdestruct-identifier", "dynamic-array-length-assignment", - "uninitialized-state-variable", "incorrect-caret-operator", "yul-return", "state-variable-shadowing", diff --git a/reports/adhoc-sol-files-report.md b/reports/adhoc-sol-files-report.md index 54f80353..db45e444 100644 --- a/reports/adhoc-sol-files-report.md +++ b/reports/adhoc-sol-files-report.md @@ -9,9 +9,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Issue Summary](#issue-summary) - [High Issues](#high-issues) - [H-1: Using `delegatecall` in loop](#h-1-using-delegatecall-in-loop) - - [H-2: Uninitialized State Variables](#h-2-uninitialized-state-variables) - - [H-3: Delegatecall made by the function without checks on any address.](#h-3-delegatecall-made-by-the-function-without-checks-on-any-address) - - [H-4: Unchecked Low level calls](#h-4-unchecked-low-level-calls) + - [H-2: Delegatecall made by the function without checks on any address.](#h-2-delegatecall-made-by-the-function-without-checks-on-any-address) + - [H-3: Unchecked Low level calls](#h-3-unchecked-low-level-calls) - [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) @@ -78,7 +77,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 4 | +| High | 3 | | Low | 22 | @@ -101,48 +100,7 @@ When calling `delegatecall` the same `msg.value` amount will be accredited multi -## 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. - -
5 Found Instances - - -- Found in InconsistentUints.sol [Line: 7](../tests/adhoc-sol-files/InconsistentUints.sol#L7) - - ```solidity - int public intVariable; // 1 - ``` - -- Found in InconsistentUints.sol [Line: 8](../tests/adhoc-sol-files/InconsistentUints.sol#L8) - - ```solidity - int256 public int256Variable; // 1 - ``` - -- Found in StateVariables.sol [Line: 8](../tests/adhoc-sol-files/StateVariables.sol#L8) - - ```solidity - uint256 private staticPrivateNumber; - ``` - -- Found in StateVariables.sol [Line: 9](../tests/adhoc-sol-files/StateVariables.sol#L9) - - ```solidity - uint256 internal staticInternalNumber; - ``` - -- Found in StateVariables.sol [Line: 10](../tests/adhoc-sol-files/StateVariables.sol#L10) - - ```solidity - uint256 public staticPublicNumber; - ``` - -
- - - -## H-3: Delegatecall made by the function without checks on any address. +## H-2: Delegatecall made by the function without checks on any address. Introduce checks on the address @@ -159,7 +117,7 @@ Introduce checks on the address -## H-4: Unchecked Low level calls +## H-3: Unchecked Low level calls The return value of the low-level call is not checked, so if the call fails, the Ether will be locked in the contract. If the low level is used to prevent blocking operations, consider logging failed calls. Ensure that the return value of a low-level call is checked or logged. diff --git a/reports/ccip-functions-report.md b/reports/ccip-functions-report.md index c6a976fd..41637b56 100644 --- a/reports/ccip-functions-report.md +++ b/reports/ccip-functions-report.md @@ -10,7 +10,6 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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) - [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,7 +104,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 3 | +| High | 2 | | Low | 18 | @@ -469,29 +468,6 @@ When compiling contracts with certain development frameworks (for example: Truff -## H-3: 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. - -
2 Found Instances - - -- Found in src/v0.8/functions/dev/v1_X/FunctionsSubscriptions.sol [Line: 39](../tests/ccip-contracts/contracts/src/v0.8/functions/dev/v1_X/FunctionsSubscriptions.sol#L39) - - ```solidity - uint64 private s_currentSubscriptionId; - ``` - -- Found in src/v0.8/functions/v1_0_0/FunctionsSubscriptions.sol [Line: 39](../tests/ccip-contracts/contracts/src/v0.8/functions/v1_0_0/FunctionsSubscriptions.sol#L39) - - ```solidity - uint64 private s_currentSubscriptionId; - ``` - -
- - - # Low Issues ## L-1: Centralization Risk for trusted owners diff --git a/reports/hardhat-playground-report.md b/reports/hardhat-playground-report.md index 044fb7da..1d37df15 100644 --- a/reports/hardhat-playground-report.md +++ b/reports/hardhat-playground-report.md @@ -10,9 +10,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [High Issues](#high-issues) - [H-1: Using `delegatecall` in loop](#h-1-using-delegatecall-in-loop) - [H-2: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`](#h-2-abiencodepacked-should-not-be-used-with-dynamic-types-when-passing-the-result-to-a-hash-function-such-as-keccak256) - - [H-3: Uninitialized State Variables](#h-3-uninitialized-state-variables) - - [H-4: Delegatecall made by the function without checks on any address.](#h-4-delegatecall-made-by-the-function-without-checks-on-any-address) - - [H-5: Unchecked Low level calls](#h-5-unchecked-low-level-calls) + - [H-3: Delegatecall made by the function without checks on any address.](#h-3-delegatecall-made-by-the-function-without-checks-on-any-address) + - [H-4: Unchecked Low level calls](#h-4-unchecked-low-level-calls) - [Low Issues](#low-issues) - [L-1: `ecrecover` is susceptible to signature malleability](#l-1-ecrecover-is-susceptible-to-signature-malleability) - [L-2: Unsafe ERC20 Operations should not be used](#l-2-unsafe-erc20-operations-should-not-be-used) @@ -57,7 +56,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 5 | +| High | 4 | | Low | 13 | @@ -110,36 +109,7 @@ If all arguments are strings and or bytes, `bytes.concat()` should be used inste -## H-3: 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. - -
3 Found Instances - - -- Found in contracts/StateVariables.sol [Line: 8](../tests/hardhat-js-playground/contracts/StateVariables.sol#L8) - - ```solidity - uint256 private staticPrivateNumber; - ``` - -- Found in contracts/StateVariables.sol [Line: 9](../tests/hardhat-js-playground/contracts/StateVariables.sol#L9) - - ```solidity - uint256 internal staticInternalNumber; - ``` - -- Found in contracts/StateVariables.sol [Line: 10](../tests/hardhat-js-playground/contracts/StateVariables.sol#L10) - - ```solidity - uint256 public staticPublicNumber; - ``` - -
- - - -## H-4: Delegatecall made by the function without checks on any address. +## H-3: Delegatecall made by the function without checks on any address. Introduce checks on the address @@ -156,7 +126,7 @@ Introduce checks on the address -## H-5: Unchecked Low level calls +## H-4: Unchecked Low level calls The return value of the low-level call is not checked, so if the call fails, the Ether will be locked in the contract. If the low level is used to prevent blocking operations, consider logging failed calls. Ensure that the return value of a low-level call is checked or logged. diff --git a/reports/report.json b/reports/report.json index cdc663f8..df85b688 100644 --- a/reports/report.json +++ b/reports/report.json @@ -456,7 +456,7 @@ ] }, "issue_count": { - "high": 40, + "high": 39, "low": 47 }, "high_issues": { @@ -1315,223 +1315,6 @@ } ] }, - { - "title": "Uninitialized State Variables", - "description": "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.", - "detector_name": "uninitialized-state-variable", - "instances": [ - { - "contract_path": "src/AssemblyExample.sol", - "line_no": 5, - "src": "97:1", - "src_char": "97:1" - }, - { - "contract_path": "src/BuiltinSymbolShadow.sol", - "line_no": 5, - "src": "92:8", - "src_char": "92:8" - }, - { - "contract_path": "src/ConstantFuncsAssembly.sol", - "line_no": 6, - "src": "110:20", - "src_char": "110:20" - }, - { - "contract_path": "src/DelegateCallWithoutAddressCheck.sol", - "line_no": 9, - "src": "337:7", - "src_char": "337:7" - }, - { - "contract_path": "src/InconsistentUints.sol", - "line_no": 7, - "src": "197:11", - "src_char": "197:11" - }, - { - "contract_path": "src/InconsistentUints.sol", - "line_no": 8, - "src": "233:14", - "src_char": "233:14" - }, - { - "contract_path": "src/IncorrectCaretOperator.sol", - "line_no": 10, - "src": "355:7", - "src_char": "355:7" - }, - { - "contract_path": "src/IncorrectERC721.sol", - "line_no": 147, - "src": "4076:11", - "src_char": "4076:11" - }, - { - "contract_path": "src/LocalVariableShadow.sol", - "line_no": 7, - "src": "129:5", - "src_char": "129:5" - }, - { - "contract_path": "src/LocalVariableShadow.sol", - "line_no": 25, - "src": "532:4", - "src_char": "532:4" - }, - { - "contract_path": "src/PublicVariableReadInExternalContext.sol", - "line_no": 6, - "src": "130:26", - "src_char": "130:26" - }, - { - "contract_path": "src/ReturnBomb.sol", - "line_no": 61, - "src": "1623:7", - "src_char": "1623:7" - }, - { - "contract_path": "src/StateShadowing.sol", - "line_no": 5, - "src": "87:13", - "src_char": "87:13" - }, - { - "contract_path": "src/StateVariables.sol", - "line_no": 8, - "src": "199:19", - "src_char": "199:19" - }, - { - "contract_path": "src/StateVariables.sol", - "line_no": 9, - "src": "241:20", - "src_char": "241:20" - }, - { - "contract_path": "src/StateVariables.sol", - "line_no": 10, - "src": "282:18", - "src_char": "282:18" - }, - { - "contract_path": "src/StateVariablesManipulation.sol", - "line_no": 8, - "src": "184:10", - "src_char": "184:10" - }, - { - "contract_path": "src/StateVariablesManipulation.sol", - "line_no": 9, - "src": "214:9", - "src_char": "214:9" - }, - { - "contract_path": "src/StateVariablesManipulation.sol", - "line_no": 10, - "src": "241:10", - "src_char": "241:10" - }, - { - "contract_path": "src/StateVariablesManipulation.sol", - "line_no": 11, - "src": "272:13", - "src_char": "272:13" - }, - { - "contract_path": "src/StateVariablesManipulation.sol", - "line_no": 12, - "src": "314:20", - "src_char": "314:20" - }, - { - "contract_path": "src/StateVariablesManipulation.sol", - "line_no": 13, - "src": "354:12", - "src_char": "354:12" - }, - { - "contract_path": "src/StateVariablesManipulation.sol", - "line_no": 14, - "src": "385:11", - "src_char": "385:11" - }, - { - "contract_path": "src/TautologyOrContradiction.sol", - "line_no": 6, - "src": "133:6", - "src_char": "133:6" - }, - { - "contract_path": "src/TautologyOrContradiction.sol", - "line_no": 7, - "src": "145:9", - "src_char": "145:9" - }, - { - "contract_path": "src/UninitializedLocalVariables.sol", - "line_no": 5, - "src": "93:12", - "src_char": "93:12" - }, - { - "contract_path": "src/UninitializedStateVariable.sol", - "line_no": 7, - "src": "122:8", - "src_char": "122:8" - }, - { - "contract_path": "src/UninitializedStateVariable.sol", - "line_no": 15, - "src": "529:11", - "src_char": "529:11" - }, - { - "contract_path": "src/UnusedStateVariables.sol", - "line_no": 6, - "src": "147:13", - "src_char": "147:13" - }, - { - "contract_path": "src/UnusedStateVariables.sol", - "line_no": 7, - "src": "183:13", - "src_char": "183:13" - }, - { - "contract_path": "src/UnusedStateVariables.sol", - "line_no": 8, - "src": "215:10", - "src_char": "215:10" - }, - { - "contract_path": "src/UnusedStateVariables.sol", - "line_no": 9, - "src": "246:12", - "src_char": "246:12" - }, - { - "contract_path": "src/UnusedStateVariables.sol", - "line_no": 12, - "src": "314:11", - "src_char": "314:11" - }, - { - "contract_path": "src/WrongOrderOfLayout.sol", - "line_no": 11, - "src": "257:10", - "src_char": "257:10" - }, - { - "contract_path": "src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol", - "line_no": 68, - "src": "1971:5", - "src_char": "1971:5" - } - ] - }, { "title": "Incorrect use of caret operator on a non hexadcimal constant", "description": "The caret operator is usually mistakenly thought of as an exponentiation operator but actually, it's a bitwise xor operator.", @@ -7402,7 +7185,6 @@ "nested-struct-in-mapping", "selfdestruct-identifier", "dynamic-array-length-assignment", - "uninitialized-state-variable", "incorrect-caret-operator", "yul-return", "state-variable-shadowing", diff --git a/reports/report.md b/reports/report.md index f44c2a82..cc8d293a 100644 --- a/reports/report.md +++ b/reports/report.md @@ -22,32 +22,31 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-12: Nested Structs in Mappings pre-0.5.0](#h-12-nested-structs-in-mappings-pre-050) - [H-13: Depracated EVM Instruction for `selfdestruct` should not be used.](#h-13-depracated-evm-instruction-for-selfdestruct-should-not-be-used) - [H-14: Array length value has a direct assignment.](#h-14-array-length-value-has-a-direct-assignment) - - [H-15: Uninitialized State Variables](#h-15-uninitialized-state-variables) - - [H-16: Incorrect use of caret operator on a non hexadcimal constant](#h-16-incorrect-use-of-caret-operator-on-a-non-hexadcimal-constant) - - [H-17: Yul block contains `return` function call.](#h-17-yul-block-contains-return-function-call) - - [H-18: Shadowed State Variables in Inheritance Hierarchy](#h-18-shadowed-state-variables-in-inheritance-hierarchy) - - [H-19: Unchecked `bool success` value for send call.](#h-19-unchecked-bool-success-value-for-send-call) - - [H-20: Misused boolean with logical operators](#h-20-misused-boolean-with-logical-operators) - - [H-21: Functions send eth away from contract but performs no checks on any address.](#h-21-functions-send-eth-away-from-contract-but-performs-no-checks-on-any-address) - - [H-22: Delegatecall made by the function without checks on any address.](#h-22-delegatecall-made-by-the-function-without-checks-on-any-address) - - [H-23: Tautological comparison.](#h-23-tautological-comparison) - - [H-24: RTLO character detected in file. \u{202e}](#h-24-rtlo-character-detected-in-file-u202e) - - [H-25: Dangerous unary operator found in assignment.](#h-25-dangerous-unary-operator-found-in-assignment) - - [H-26: Tautology or Contradiction in comparison.](#h-26-tautology-or-contradiction-in-comparison) - - [H-27: Dangerous strict equality checks on contract balances.](#h-27-dangerous-strict-equality-checks-on-contract-balances) - - [H-28: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10`](#h-28-compiler-bug-signed-array-in-storage-detected-for-compiler-version-0510) - - [H-29: Weak Randomness](#h-29-weak-randomness) - - [H-30: Usage of variable before declaration.](#h-30-usage-of-variable-before-declaration) - - [H-31: Deletion from a nested mappping.](#h-31-deletion-from-a-nested-mappping) - - [H-32: Potential use of `tx.origin` for authentication.](#h-32-potential-use-of-txorigin-for-authentication) - - [H-33: Loop contains `msg.value`.](#h-33-loop-contains-msgvalue) - - [H-34: Contract locks Ether without a withdraw function.](#h-34-contract-locks-ether-without-a-withdraw-function) - - [H-35: Incorrect ERC721 interface.](#h-35-incorrect-erc721-interface) - - [H-36: Incorrect ERC20 interface.](#h-36-incorrect-erc20-interface) - - [H-37: Out of order retryable transactions.](#h-37-out-of-order-retryable-transactions) - - [H-38: Constant functions changing state](#h-38-constant-functions-changing-state) - - [H-39: Function selector collides with other functions](#h-39-function-selector-collides-with-other-functions) - - [H-40: Unchecked Low level calls](#h-40-unchecked-low-level-calls) + - [H-15: Incorrect use of caret operator on a non hexadcimal constant](#h-15-incorrect-use-of-caret-operator-on-a-non-hexadcimal-constant) + - [H-16: Yul block contains `return` function call.](#h-16-yul-block-contains-return-function-call) + - [H-17: Shadowed State Variables in Inheritance Hierarchy](#h-17-shadowed-state-variables-in-inheritance-hierarchy) + - [H-18: Unchecked `bool success` value for send call.](#h-18-unchecked-bool-success-value-for-send-call) + - [H-19: Misused boolean with logical operators](#h-19-misused-boolean-with-logical-operators) + - [H-20: Functions send eth away from contract but performs no checks on any address.](#h-20-functions-send-eth-away-from-contract-but-performs-no-checks-on-any-address) + - [H-21: Delegatecall made by the function without checks on any address.](#h-21-delegatecall-made-by-the-function-without-checks-on-any-address) + - [H-22: Tautological comparison.](#h-22-tautological-comparison) + - [H-23: RTLO character detected in file. \u{202e}](#h-23-rtlo-character-detected-in-file-u202e) + - [H-24: Dangerous unary operator found in assignment.](#h-24-dangerous-unary-operator-found-in-assignment) + - [H-25: Tautology or Contradiction in comparison.](#h-25-tautology-or-contradiction-in-comparison) + - [H-26: Dangerous strict equality checks on contract balances.](#h-26-dangerous-strict-equality-checks-on-contract-balances) + - [H-27: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10`](#h-27-compiler-bug-signed-array-in-storage-detected-for-compiler-version-0510) + - [H-28: Weak Randomness](#h-28-weak-randomness) + - [H-29: Usage of variable before declaration.](#h-29-usage-of-variable-before-declaration) + - [H-30: Deletion from a nested mappping.](#h-30-deletion-from-a-nested-mappping) + - [H-31: Potential use of `tx.origin` for authentication.](#h-31-potential-use-of-txorigin-for-authentication) + - [H-32: Loop contains `msg.value`.](#h-32-loop-contains-msgvalue) + - [H-33: Contract locks Ether without a withdraw function.](#h-33-contract-locks-ether-without-a-withdraw-function) + - [H-34: Incorrect ERC721 interface.](#h-34-incorrect-erc721-interface) + - [H-35: Incorrect ERC20 interface.](#h-35-incorrect-erc20-interface) + - [H-36: Out of order retryable transactions.](#h-36-out-of-order-retryable-transactions) + - [H-37: Constant functions changing state](#h-37-constant-functions-changing-state) + - [H-38: Function selector collides with other functions](#h-38-function-selector-collides-with-other-functions) + - [H-39: Unchecked Low level calls](#h-39-unchecked-low-level-calls) - [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) @@ -231,7 +230,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 40 | +| High | 39 | | Low | 47 | @@ -1149,228 +1148,7 @@ If the length of a dynamic array (storage variable) directly assigned to, it may -## H-15: 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. - -
35 Found Instances - - -- Found in src/AssemblyExample.sol [Line: 5](../tests/contract-playground/src/AssemblyExample.sol#L5) - - ```solidity - uint b; - ``` - -- Found in src/BuiltinSymbolShadow.sol [Line: 5](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L5) - - ```solidity - uint now; // BAD - ``` - -- Found in src/ConstantFuncsAssembly.sol [Line: 6](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L6) - - ```solidity - uint256 public value; - ``` - -- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 9](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L9) - - ```solidity - address public manager; - ``` - -- Found in src/InconsistentUints.sol [Line: 7](../tests/contract-playground/src/InconsistentUints.sol#L7) - - ```solidity - int public intVariable; // 1 - ``` - -- Found in src/InconsistentUints.sol [Line: 8](../tests/contract-playground/src/InconsistentUints.sol#L8) - - ```solidity - int256 public int256Variable; // 1 - ``` - -- Found in src/IncorrectCaretOperator.sol [Line: 10](../tests/contract-playground/src/IncorrectCaretOperator.sol#L10) - - ```solidity - uint256 private s_first; - ``` - -- Found in src/IncorrectERC721.sol [Line: 147](../tests/contract-playground/src/IncorrectERC721.sol#L147) - - ```solidity - uint256 public totalSupply; - ``` - -- Found in src/LocalVariableShadow.sol [Line: 7](../tests/contract-playground/src/LocalVariableShadow.sol#L7) - - ```solidity - uint owner; - ``` - -- Found in src/LocalVariableShadow.sol [Line: 25](../tests/contract-playground/src/LocalVariableShadow.sol#L25) - - ```solidity - uint internal roll; - ``` - -- Found in src/PublicVariableReadInExternalContext.sol [Line: 6](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L6) - - ```solidity - uint256 public testUint256; - ``` - -- Found in src/ReturnBomb.sol [Line: 61](../tests/contract-playground/src/ReturnBomb.sol#L61) - - ```solidity - address goodGuy; - ``` - -- Found in src/StateShadowing.sol [Line: 5](../tests/contract-playground/src/StateShadowing.sol#L5) - - ```solidity - address owner; - ``` - -- Found in src/StateVariables.sol [Line: 8](../tests/contract-playground/src/StateVariables.sol#L8) - - ```solidity - uint256 private staticPrivateNumber; - ``` - -- Found in src/StateVariables.sol [Line: 9](../tests/contract-playground/src/StateVariables.sol#L9) - - ```solidity - uint256 internal staticInternalNumber; - ``` - -- Found in src/StateVariables.sol [Line: 10](../tests/contract-playground/src/StateVariables.sol#L10) - - ```solidity - uint256 public staticPublicNumber; - ``` - -- Found in src/StateVariablesManipulation.sol [Line: 8](../tests/contract-playground/src/StateVariablesManipulation.sol#L8) - - ```solidity - uint256 public simpleUint; - ``` - -- Found in src/StateVariablesManipulation.sol [Line: 9](../tests/contract-playground/src/StateVariablesManipulation.sol#L9) - - ```solidity - int256 public simpleInt; - ``` - -- Found in src/StateVariablesManipulation.sol [Line: 10](../tests/contract-playground/src/StateVariablesManipulation.sol#L10) - - ```solidity - bool public simpleBool; - ``` - -- Found in src/StateVariablesManipulation.sol [Line: 11](../tests/contract-playground/src/StateVariablesManipulation.sol#L11) - - ```solidity - address public simpleAddress; - ``` - -- Found in src/StateVariablesManipulation.sol [Line: 12](../tests/contract-playground/src/StateVariablesManipulation.sol#L12) - - ```solidity - address payable public simplePayableAddress; - ``` - -- Found in src/StateVariablesManipulation.sol [Line: 13](../tests/contract-playground/src/StateVariablesManipulation.sol#L13) - - ```solidity - string public simpleString; - ``` - -- Found in src/StateVariablesManipulation.sol [Line: 14](../tests/contract-playground/src/StateVariablesManipulation.sol#L14) - - ```solidity - bytes public simpleBytes; - ``` - -- Found in src/TautologyOrContradiction.sol [Line: 6](../tests/contract-playground/src/TautologyOrContradiction.sol#L6) - - ```solidity - uint x; - ``` - -- Found in src/TautologyOrContradiction.sol [Line: 7](../tests/contract-playground/src/TautologyOrContradiction.sol#L7) - - ```solidity - uint256 y; - ``` - -- Found in src/UninitializedLocalVariables.sol [Line: 5](../tests/contract-playground/src/UninitializedLocalVariables.sol#L5) - - ```solidity - uint256 stateVarUint; - ``` - -- Found in src/UninitializedStateVariable.sol [Line: 7](../tests/contract-playground/src/UninitializedStateVariable.sol#L7) - - ```solidity - string public s_author; // BAD (because it's used without initializing) - ``` - -- Found in src/UninitializedStateVariable.sol [Line: 15](../tests/contract-playground/src/UninitializedStateVariable.sol#L15) - - ```solidity - address destination; // BAD - ``` - -- Found in src/UnusedStateVariables.sol [Line: 6](../tests/contract-playground/src/UnusedStateVariables.sol#L6) - - ```solidity - uint256 internal unusedUint256; - ``` - -- Found in src/UnusedStateVariables.sol [Line: 7](../tests/contract-playground/src/UnusedStateVariables.sol#L7) - - ```solidity - address internal unusedAddress; - ``` - -- Found in src/UnusedStateVariables.sol [Line: 8](../tests/contract-playground/src/UnusedStateVariables.sol#L8) - - ```solidity - bool private unusedBool; - ``` - -- Found in src/UnusedStateVariables.sol [Line: 9](../tests/contract-playground/src/UnusedStateVariables.sol#L9) - - ```solidity - string private unusedString; - ``` - -- Found in src/UnusedStateVariables.sol [Line: 12](../tests/contract-playground/src/UnusedStateVariables.sol#L12) - - ```solidity - bytes32 public usedBytes32; // External contracts may want to interact with it by calling it as a function - ``` - -- Found in src/WrongOrderOfLayout.sol [Line: 11](../tests/contract-playground/src/WrongOrderOfLayout.sol#L11) - - ```solidity - uint256 public multiplier; - ``` - -- Found in src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol [Line: 68](../tests/contract-playground/src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol#L68) - - ```solidity - address public owner; - ``` - -
- - - -## H-16: Incorrect use of caret operator on a non hexadcimal constant +## H-15: Incorrect use of caret operator on a non hexadcimal constant The caret operator is usually mistakenly thought of as an exponentiation operator but actually, it's a bitwise xor operator. @@ -1411,7 +1189,7 @@ The caret operator is usually mistakenly thought of as an exponentiation operato -## H-17: Yul block contains `return` function call. +## H-16: Yul block contains `return` function call. Remove this, as this causes execution to halt. Nothing after that call will execute, including code following the assembly block. @@ -1428,7 +1206,7 @@ Remove this, as this causes execution to halt. Nothing after that call will exec -## H-18: Shadowed State Variables in Inheritance Hierarchy +## H-17: Shadowed State Variables in Inheritance Hierarchy This vulnerability arises when a derived contract unintentionally shadows a state variable from a parent contract by declaring a variable with the same name. This can be misleading. To prevent this, ensure variable names are unique across the inheritance hierarchy or use proper visibility and scope controls. @@ -1445,7 +1223,7 @@ This vulnerability arises when a derived contract unintentionally shadows a stat -## H-19: Unchecked `bool success` value for send call. +## H-18: Unchecked `bool success` value for send call. The transaction `address(payable?).send(address)` may fail because of reasons like out-of-gas, invalid receipient address or revert from the recipient. Therefore, the boolean returned by this function call must be checked to be `true` in order to verify that the transaction was successful @@ -1462,7 +1240,7 @@ The transaction `address(payable?).send(address)` may fail because of reasons li -## H-20: Misused boolean with logical operators +## H-19: Misused boolean with logical operators The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively. @@ -1533,7 +1311,7 @@ The patterns `if (… || true)` and `if (.. && false)` will always evaluate to t -## H-21: Functions send eth away from contract but performs no checks on any address. +## H-20: Functions send eth away from contract but performs no checks on any address. Consider introducing checks for `msg.sender` to ensure the recipient of the money is as intended. @@ -1664,7 +1442,7 @@ Consider introducing checks for `msg.sender` to ensure the recipient of the mone -## H-22: Delegatecall made by the function without checks on any address. +## H-21: Delegatecall made by the function without checks on any address. Introduce checks on the address @@ -1705,7 +1483,7 @@ Introduce checks on the address -## H-23: Tautological comparison. +## H-22: Tautological comparison. The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false. @@ -1740,7 +1518,7 @@ The left hand side and the right hand side of the binary operation has the same -## H-24: RTLO character detected in file. \u{202e} +## H-23: RTLO character detected in file. \u{202e} Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments! @@ -1757,7 +1535,7 @@ Right to left override character may be misledaing and cause potential attacks b -## H-25: Dangerous unary operator found in assignment. +## H-24: Dangerous unary operator found in assignment. Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between. @@ -1780,7 +1558,7 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-26: Tautology or Contradiction in comparison. +## H-25: Tautology or Contradiction in comparison. The condition has been determined to be either always true or always false due to the integer range in which we're operating. @@ -1803,7 +1581,7 @@ The condition has been determined to be either always true or always false due t -## H-27: Dangerous strict equality checks on contract balances. +## H-26: Dangerous strict equality checks on contract balances. A contract's balance can be forcibly manipulated by another selfdestructing contract. Therefore, it's recommended to use >, <, >= or <= instead of strict equality. @@ -1832,7 +1610,7 @@ A contract's balance can be forcibly manipulated by another selfdestructing cont -## H-28: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10` +## H-27: Compiler Bug: Signed array in storage detected for compiler version `<0.5.10` If you want to leverage signed arrays in storage by assigning a literal array with at least one negative number, then you mus use solidity version 0.5.10 or above. This is because of a bug in older compilers. @@ -1849,7 +1627,7 @@ If you want to leverage signed arrays in storage by assigning a literal array wi -## H-29: Weak Randomness +## H-28: 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. @@ -1914,7 +1692,7 @@ The use of keccak256 hash functions on predictable values like block.timestamp, -## H-30: Usage of variable before declaration. +## H-29: Usage of variable before declaration. This is a bad practice that may lead to unintended consequences. Please declare the variable before using it. @@ -1931,7 +1709,7 @@ This is a bad practice that may lead to unintended consequences. Please declare -## H-31: Deletion from a nested mappping. +## H-30: 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. @@ -1948,7 +1726,7 @@ A deletion in a structure containing a mapping will not delete the mapping. The -## H-32: Potential use of `tx.origin` for authentication. +## H-31: Potential use of `tx.origin` for authentication. Using `tx.origin` may lead to problems when users are interacting via smart contract with your protocol. It is recommended to use `msg.sender` for authentication. @@ -1977,7 +1755,7 @@ Using `tx.origin` may lead to problems when users are interacting via smart cont -## H-33: Loop contains `msg.value`. +## H-32: Loop contains `msg.value`. Provide an explicit array of amounts alongside the receivers array, and check that the sum of all amounts matches `msg.value`. @@ -2012,7 +1790,7 @@ Provide an explicit array of amounts alongside the receivers array, and check th -## H-34: Contract locks Ether without a withdraw function. +## H-33: 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. @@ -2083,7 +1861,7 @@ It appears that the contract includes a payable function to accept Ether but lac -## H-35: Incorrect ERC721 interface. +## H-34: Incorrect ERC721 interface. Incorrect return values for ERC721 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC721 functions. @@ -2142,7 +1920,7 @@ Incorrect return values for ERC721 functions. A contract compiled with Solidity -## H-36: Incorrect ERC20 interface. +## H-35: Incorrect ERC20 interface. Incorrect return values for ERC20 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC20 functions. @@ -2183,7 +1961,7 @@ Incorrect return values for ERC20 functions. A contract compiled with Solidity > -## H-37: Out of order retryable transactions. +## H-36: Out of order retryable transactions. Do not rely on the order or successful execution of retryable tickets. Functions like createRetryableTicket, outboundTransferCustomRefund, unsafeCreateRetryableTicket are free to be re-tried in any order if they fail in the first go. Since this operation happens off chain, the sequencer is in control of the @@ -2208,7 +1986,7 @@ Do not rely on the order or successful execution of retryable tickets. Functions -## H-38: Constant functions changing state +## H-37: Constant functions changing state Function is declared constant/view but it changes state. Ensure that the attributes of contract compiled prior to 0.5 are correct. @@ -2225,7 +2003,7 @@ Function is declared constant/view but it changes state. Ensure that the attribu -## H-39: Function selector collides with other functions +## H-38: 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. @@ -2250,7 +2028,7 @@ Function selector collides with other functions. This may cause the solidity fun -## H-40: Unchecked Low level calls +## H-39: Unchecked Low level calls The return value of the low-level call is not checked, so if the call fails, the Ether will be locked in the contract. If the low level is used to prevent blocking operations, consider logging failed calls. Ensure that the return value of a low-level call is checked or logged. diff --git a/reports/report.sarif b/reports/report.sarif index 9498df3e..b8af7938 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1516,400 +1516,6 @@ }, "ruleId": "dynamic-array-length-assignment" }, - { - "level": "warning", - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/AssemblyExample.sol" - }, - "region": { - "byteLength": 1, - "byteOffset": 97 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/BuiltinSymbolShadow.sol" - }, - "region": { - "byteLength": 8, - "byteOffset": 92 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/ConstantFuncsAssembly.sol" - }, - "region": { - "byteLength": 20, - "byteOffset": 110 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/DelegateCallWithoutAddressCheck.sol" - }, - "region": { - "byteLength": 7, - "byteOffset": 337 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/InconsistentUints.sol" - }, - "region": { - "byteLength": 11, - "byteOffset": 197 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/InconsistentUints.sol" - }, - "region": { - "byteLength": 14, - "byteOffset": 233 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/IncorrectCaretOperator.sol" - }, - "region": { - "byteLength": 7, - "byteOffset": 355 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/IncorrectERC721.sol" - }, - "region": { - "byteLength": 11, - "byteOffset": 4076 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/LocalVariableShadow.sol" - }, - "region": { - "byteLength": 5, - "byteOffset": 129 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/LocalVariableShadow.sol" - }, - "region": { - "byteLength": 4, - "byteOffset": 532 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/PublicVariableReadInExternalContext.sol" - }, - "region": { - "byteLength": 26, - "byteOffset": 130 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/ReturnBomb.sol" - }, - "region": { - "byteLength": 7, - "byteOffset": 1623 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateShadowing.sol" - }, - "region": { - "byteLength": 13, - "byteOffset": 87 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariables.sol" - }, - "region": { - "byteLength": 19, - "byteOffset": 199 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariables.sol" - }, - "region": { - "byteLength": 20, - "byteOffset": 241 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariables.sol" - }, - "region": { - "byteLength": 18, - "byteOffset": 282 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariablesManipulation.sol" - }, - "region": { - "byteLength": 10, - "byteOffset": 184 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariablesManipulation.sol" - }, - "region": { - "byteLength": 9, - "byteOffset": 214 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariablesManipulation.sol" - }, - "region": { - "byteLength": 10, - "byteOffset": 241 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariablesManipulation.sol" - }, - "region": { - "byteLength": 13, - "byteOffset": 272 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariablesManipulation.sol" - }, - "region": { - "byteLength": 20, - "byteOffset": 314 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariablesManipulation.sol" - }, - "region": { - "byteLength": 12, - "byteOffset": 354 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/StateVariablesManipulation.sol" - }, - "region": { - "byteLength": 11, - "byteOffset": 385 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/TautologyOrContradiction.sol" - }, - "region": { - "byteLength": 6, - "byteOffset": 133 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/TautologyOrContradiction.sol" - }, - "region": { - "byteLength": 9, - "byteOffset": 145 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/UninitializedLocalVariables.sol" - }, - "region": { - "byteLength": 12, - "byteOffset": 93 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/UninitializedStateVariable.sol" - }, - "region": { - "byteLength": 8, - "byteOffset": 122 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/UninitializedStateVariable.sol" - }, - "region": { - "byteLength": 11, - "byteOffset": 529 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/UnusedStateVariables.sol" - }, - "region": { - "byteLength": 13, - "byteOffset": 147 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/UnusedStateVariables.sol" - }, - "region": { - "byteLength": 13, - "byteOffset": 183 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/UnusedStateVariables.sol" - }, - "region": { - "byteLength": 10, - "byteOffset": 215 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/UnusedStateVariables.sol" - }, - "region": { - "byteLength": 12, - "byteOffset": 246 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/UnusedStateVariables.sol" - }, - "region": { - "byteLength": 11, - "byteOffset": 314 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/WrongOrderOfLayout.sol" - }, - "region": { - "byteLength": 10, - "byteOffset": 257 - } - } - }, - { - "physicalLocation": { - "artifactLocation": { - "uri": "src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol" - }, - "region": { - "byteLength": 5, - "byteOffset": 1971 - } - } - } - ], - "message": { - "text": "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." - }, - "ruleId": "uninitialized-state-variable" - }, { "level": "warning", "locations": [ diff --git a/reports/templegold-report.md b/reports/templegold-report.md index dbdc6831..54eeebd0 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -12,10 +12,9 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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-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 | @@ -337,60 +336,7 @@ When compiling contracts with certain development frameworks (for example: Truff -## H-5: 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. - -
7 Found Instances - - -- Found in contracts/amm/TempleUniswapV2Pair.sol [Line: 29](../tests/2024-07-templegold/protocol/contracts/amm/TempleUniswapV2Pair.sol#L29) - - ```solidity - uint public kLast; // reserve0 * reserve1, as of immediately after the most recent liquidity event - ``` - -- Found in contracts/fakes/templegold/TempleGoldStakingMock.sol [Line: 39](../tests/2024-07-templegold/protocol/contracts/fakes/templegold/TempleGoldStakingMock.sol#L39) - - ```solidity - uint256 public rewardPerTokenStored; - ``` - -- Found in contracts/fakes/templegold/TempleGoldStakingMock.sol [Line: 47](../tests/2024-07-templegold/protocol/contracts/fakes/templegold/TempleGoldStakingMock.sol#L47) - - ```solidity - uint256 public periodFinish; - ``` - -- Found in contracts/fakes/templegold/TempleGoldStakingMock.sol [Line: 48](../tests/2024-07-templegold/protocol/contracts/fakes/templegold/TempleGoldStakingMock.sol#L48) - - ```solidity - uint256 public lastUpdateTime; - ``` - -- Found in contracts/templegold/TempleGoldStaking.sol [Line: 40](../tests/2024-07-templegold/protocol/contracts/templegold/TempleGoldStaking.sol#L40) - - ```solidity - uint256 public override rewardPerTokenStored; - ``` - -- Found in contracts/templegold/TempleGoldStaking.sol [Line: 45](../tests/2024-07-templegold/protocol/contracts/templegold/TempleGoldStaking.sol#L45) - - ```solidity - uint256 public override periodFinish; - ``` - -- Found in contracts/templegold/TempleGoldStaking.sol [Line: 47](../tests/2024-07-templegold/protocol/contracts/templegold/TempleGoldStaking.sol#L47) - - ```solidity - uint256 public override lastUpdateTime; - ``` - -
- - - -## 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 +353,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 +370,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.