diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 5fb7ee43..49ad141f 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -65,6 +65,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), ] @@ -128,6 +129,7 @@ pub(crate) enum IssueDetectorNamePool { RTLO, UncheckedReturn, DangerousUnaryOperator, + WeakRandomness, PreDeclaredLocalVariableUsage, DeleteNestedMapping, // NOTE: `Undecided` will be the default name (for new bots). @@ -265,6 +267,7 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::WeakRandomness => Some(Box::::default()), IssueDetectorNamePool::PreDeclaredLocalVariableUsage => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 8d88f74e..f5b60972 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -25,6 +25,7 @@ pub(crate) mod unchecked_return; pub(crate) mod uninitialized_state_variable; pub(crate) mod unprotected_init_function; pub(crate) mod unsafe_casting; +pub(crate) mod weak_randomness; pub(crate) mod yul_return; pub use arbitrary_transfer_from::ArbitraryTransferFromDetector; @@ -54,4 +55,5 @@ pub use unchecked_return::UncheckedReturnDetector; pub use uninitialized_state_variable::UninitializedStateVariableDetector; pub use unprotected_init_function::UnprotectedInitializerDetector; pub use unsafe_casting::UnsafeCastingDetector; +pub use weak_randomness::WeakRandomnessDetector; pub use yul_return::YulReturnDetector; diff --git a/aderyn_core/src/detect/high/weak_randomness.rs b/aderyn_core/src/detect/high/weak_randomness.rs new file mode 100644 index 00000000..079fd2cc --- /dev/null +++ b/aderyn_core/src/detect/high/weak_randomness.rs @@ -0,0 +1,190 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{Expression, FunctionCall, FunctionCallKind, NodeID}; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::{ASTNode, WorkspaceContext}, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct WeakRandomnessDetector { + // 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 WeakRandomnessDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + let keccaks: Vec<&FunctionCall> = context.function_calls() + .into_iter() + .filter(|x| matches!(*x.expression, Expression::Identifier(ref id) if id.name == "keccak256")) + .collect(); + + for keccak in keccaks { + // keccak256 must have exactly one argument + if let Some(arg) = keccak.arguments.first() { + if let Expression::FunctionCall(ref function_call) = *arg { + if check_encode(function_call) { + capture!(self, context, keccak); + } + } + // get variable definition + else if let Expression::Identifier(ref i) = *arg { + if let Some(node_id) = i.referenced_declaration { + let decleration = context.get_parent(node_id); + + if let Some(ASTNode::VariableDeclarationStatement(var)) = decleration { + if let Some(Expression::FunctionCall(function_call)) = + &var.initial_value + { + if check_encode(function_call) { + capture!(self, context, keccak); + } + } + } + } + } + } + } + + // check for modulo operations on block.timestamp, block.number and blockhash + for binary_operation in context + .binary_operations() + .into_iter() + .filter(|b| b.operator == "%") + { + // if left operand is a variable, get its definition and perform check + if let Expression::Identifier(ref i) = *binary_operation.left_expression { + if let Some(node_id) = i.referenced_declaration { + let decleration = context.get_parent(node_id); + + if let Some(ASTNode::VariableDeclarationStatement(var)) = decleration { + if let Some(expression) = &var.initial_value { + if check_operand(expression) { + capture!(self, context, binary_operation); + continue; + } + } + } + } + } + // otherwise perform check directly on the expression + else if check_operand(&binary_operation.left_expression) { + capture!(self, context, binary_operation); + } + } + + // check if contract uses block.prevrandao + for member_access in context.member_accesses() { + if member_access.member_name == "prevrandao" + && matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block") + { + capture!(self, context, member_access); + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Weak Randomness") + } + + fn description(&self) -> String { + String::from("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.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::WeakRandomness) + } +} + +// returns whether block.timestamp or block.number is used in encode function +fn check_encode(function_call: &FunctionCall) -> bool { + if let Expression::MemberAccess(ref member_access) = *function_call.expression { + if member_access.member_name == "encodePacked" || member_access.member_name == "encode" { + for argument in &function_call.arguments { + if let Expression::MemberAccess(ref member_access) = *argument { + if ["timestamp", "number"].iter().any(|ma| { + ma == &member_access.member_name && + matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block") + }) { + return true; + } + } + } + } + } + false +} + +// returns whether operand is dependent on block.timestamp, block.number or blockhash +fn check_operand(operand: &Expression) -> bool { + match operand { + Expression::MemberAccess(member_access) => { + if ["timestamp", "number"].iter().any(|ma| { + ma == &member_access.member_name && + matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block") + }) { + return true; + } + }, + Expression::FunctionCall(function_call) => { + if function_call.kind == FunctionCallKind::TypeConversion { + // type conversion must have exactly one argument + if let Some(Expression::FunctionCall(ref inner_function_call)) = function_call.arguments.first() { + if matches!(*inner_function_call.expression, Expression::Identifier(ref id) if id.name == "blockhash") { + return true; + } + } + } + }, + _ => () + } + + false +} + +#[cfg(test)] +mod weak_randomness_detector_tests { + use crate::detect::{detector::IssueDetector, high::weak_randomness::WeakRandomnessDetector}; + + #[test] + fn test_weak_randomness_detector() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/WeakRandomness.sol", + ); + + let mut detector = WeakRandomnessDetector::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(), 9); + // 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("Weak Randomness")); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("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.") + ); + } +} diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 24dab397..5b6f1358 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -188,6 +188,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping" ] diff --git a/reports/report.json b/reports/report.json index b73738b3..1e716194 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 66, - "total_sloc": 1845 + "total_source_units": 67, + "total_sloc": 1904 }, "files_details": { "files_details": [ @@ -185,6 +185,10 @@ "file_path": "src/UsingSelfdestruct.sol", "n_sloc": 6 }, + { + "file_path": "src/WeakRandomness.sol", + "n_sloc": 59 + }, { "file_path": "src/WrongOrderOfLayout.sol", "n_sloc": 13 @@ -272,7 +276,7 @@ ] }, "issue_count": { - "high": 28, + "high": 29, "low": 23 }, "high_issues": { @@ -1587,6 +1591,67 @@ } ] }, + { + "title": "Weak Randomness", + "description": "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.", + "detector_name": "weak-randomness", + "instances": [ + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 6, + "src": "188:70", + "src_char": "188:70" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 11, + "src": "386:41", + "src_char": "386:41" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 16, + "src": "597:20", + "src_char": "597:20" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 21, + "src": "793:20", + "src_char": "793:20" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 25, + "src": "915:20", + "src_char": "915:20" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 31, + "src": "1095:5", + "src_char": "1095:5" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 35, + "src": "1217:37", + "src_char": "1217:37" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 41, + "src": "1434:9", + "src_char": "1434:9" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 45, + "src": "1563:16", + "src_char": "1563:16" + } + ] + }, { "title": "Usage of variable before declaration.", "description": "This is a bad practice that may lead to unintended consequences. Please declare the variable before using it.", @@ -2284,6 +2349,24 @@ "src": "1190:7", "src_char": "1190:7" }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 25, + "src": "933:2", + "src_char": "933:2" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 35, + "src": "1252:2", + "src_char": "1252:2" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 41, + "src": "1441:2", + "src_char": "1441:2" + }, { "contract_path": "src/eth2/DepositContract.sol", "line_no": 113, @@ -2589,6 +2672,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/cloc/AnotherHeavilyCommentedContract.sol", "line_no": 6, @@ -3384,6 +3473,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping" ] diff --git a/reports/report.md b/reports/report.md index 38e6b06b..d2cd2eec 100644 --- a/reports/report.md +++ b/reports/report.md @@ -34,8 +34,9 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-24: RTLO character detected in file. \u{202e}](#h-24-rtlo-character-detected-in-file-u202e) - [H-25: Return value of the function call is not checked.](#h-25-return-value-of-the-function-call-is-not-checked) - [H-26: Dangerous unary operator found in assignment.](#h-26-dangerous-unary-operator-found-in-assignment) - - [H-27: Usage of variable before declaration.](#h-27-usage-of-variable-before-declaration) - - [H-28: Deletion from a nested mappping.](#h-28-deletion-from-a-nested-mappping) + - [H-27: Weak Randomness](#h-27-weak-randomness) + - [H-28: Usage of variable before declaration.](#h-28-usage-of-variable-before-declaration) + - [H-29: Deletion from a nested mappping.](#h-29-deletion-from-a-nested-mappping) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: Solmate's SafeTransferLib does not check for token contract's existence](#l-2-solmates-safetransferlib-does-not-check-for-token-contracts-existence) @@ -68,8 +69,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 66 | -| Total nSLOC | 1845 | +| .sol Files | 67 | +| Total nSLOC | 1904 | ## Files Details @@ -121,6 +122,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/UnsafeERC721Mint.sol | 18 | | src/UnusedError.sol | 19 | | src/UsingSelfdestruct.sol | 6 | +| src/WeakRandomness.sol | 59 | | src/WrongOrderOfLayout.sol | 13 | | src/YulReturn.sol | 8 | | src/ZeroAddressCheck.sol | 41 | @@ -142,14 +144,14 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/reused_contract_name/ContractB.sol | 7 | | src/uniswap/UniswapV2Swapper.sol | 50 | | src/uniswap/UniswapV3Swapper.sol | 150 | -| **Total** | **1845** | +| **Total** | **1904** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 28 | +| High | 29 | | Low | 23 | @@ -1571,7 +1573,72 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-27: Usage of variable before declaration. +## H-27: 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. + +
9 Found Instances + + +- Found in src/WeakRandomness.sol [Line: 6](../tests/contract-playground/src/WeakRandomness.sol#L6) + + ```solidity + uint256 randomNumber = uint256(keccak256(abi.encodePacked(msg.sender, block.number, block.timestamp))); + ``` + +- Found in src/WeakRandomness.sol [Line: 11](../tests/contract-playground/src/WeakRandomness.sol#L11) + + ```solidity + return uint256(keccak256(abi.encodePacked(block.number))); + ``` + +- Found in src/WeakRandomness.sol [Line: 16](../tests/contract-playground/src/WeakRandomness.sol#L16) + + ```solidity + return uint256(keccak256(someBytes)); + ``` + +- Found in src/WeakRandomness.sol [Line: 21](../tests/contract-playground/src/WeakRandomness.sol#L21) + + ```solidity + return uint256(keccak256(someBytes)); + ``` + +- Found in src/WeakRandomness.sol [Line: 25](../tests/contract-playground/src/WeakRandomness.sol#L25) + + ```solidity + return block.timestamp % 10; + ``` + +- Found in src/WeakRandomness.sol [Line: 31](../tests/contract-playground/src/WeakRandomness.sol#L31) + + ```solidity + return a % b; + ``` + +- Found in src/WeakRandomness.sol [Line: 35](../tests/contract-playground/src/WeakRandomness.sol#L35) + + ```solidity + uint256 randomNumber = uint256(blockhash(block.number)) % 10; + ``` + +- Found in src/WeakRandomness.sol [Line: 41](../tests/contract-playground/src/WeakRandomness.sol#L41) + + ```solidity + return hash % 10; + ``` + +- Found in src/WeakRandomness.sol [Line: 45](../tests/contract-playground/src/WeakRandomness.sol#L45) + + ```solidity + uint256 randomNumber = block.prevrandao; + ``` + +
+ + + +## H-28: Usage of variable before declaration. This is a bad practice that may lead to unintended consequences. Please declare the variable before using it. @@ -1588,7 +1655,7 @@ This is a bad practice that may lead to unintended consequences. Please declare -## H-28: Deletion from a nested mappping. +## H-29: 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. @@ -2134,7 +2201,7 @@ Instead of marking a function as `public`, consider marking it as `external` if If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract. -
31 Found Instances +
34 Found Instances - Found in src/Casting.sol [Line: 16](../tests/contract-playground/src/Casting.sol#L16) @@ -2305,6 +2372,24 @@ If the same constant literal value is used multiple times, create a constant sta if (UncheckedHelperExternal(address(0x12345)).two() != 2) { ``` +- Found in src/WeakRandomness.sol [Line: 25](../tests/contract-playground/src/WeakRandomness.sol#L25) + + ```solidity + return block.timestamp % 10; + ``` + +- Found in src/WeakRandomness.sol [Line: 35](../tests/contract-playground/src/WeakRandomness.sol#L35) + + ```solidity + uint256 randomNumber = uint256(blockhash(block.number)) % 10; + ``` + +- Found in src/WeakRandomness.sol [Line: 41](../tests/contract-playground/src/WeakRandomness.sol#L41) + + ```solidity + return hash % 10; + ``` + - Found in src/eth2/DepositContract.sol [Line: 113](../tests/contract-playground/src/eth2/DepositContract.sol#L113) ```solidity @@ -2537,7 +2622,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support Solc compiler version 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail. -
26 Found Instances +
27 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2624,6 +2709,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity 0.8.20; ``` +- Found in src/WeakRandomness.sol [Line: 2](../tests/contract-playground/src/WeakRandomness.sol#L2) + + ```solidity + pragma solidity 0.8.20; + ``` + - Found in src/cloc/AnotherHeavilyCommentedContract.sol [Line: 6](../tests/contract-playground/src/cloc/AnotherHeavilyCommentedContract.sol#L6) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index 1e050069..914985a7 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2306,6 +2306,114 @@ }, "ruleId": "dangerous-unary-operator" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 70, + "byteOffset": 188 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 41, + "byteOffset": 386 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 597 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 793 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 915 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1095 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 37, + "byteOffset": 1217 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 1434 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 1563 + } + } + } + ], + "message": { + "text": "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." + }, + "ruleId": "weak-randomness" + }, { "level": "warning", "locations": [ @@ -3532,6 +3640,39 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 933 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 1252 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 1441 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4072,6 +4213,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/reports/templegold-report.md b/reports/templegold-report.md index 97bac719..b7a152e8 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -14,7 +14,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-4: Contract Name Reused in Different Files](#h-4-contract-name-reused-in-different-files) - [H-5: Uninitialized State Variables](#h-5-uninitialized-state-variables) - [H-6: Return value of the function call is not checked.](#h-6-return-value-of-the-function-call-is-not-checked) - - [H-7: Deletion from a nested mappping.](#h-7-deletion-from-a-nested-mappping) + - [H-7: Weak Randomness](#h-7-weak-randomness) + - [H-8: Deletion from a nested mappping.](#h-8-deletion-from-a-nested-mappping) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: `ecrecover` is susceptible to signature malleability](#l-2-ecrecover-is-susceptible-to-signature-malleability) @@ -186,7 +187,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 7 | +| High | 8 | | Low | 18 | @@ -486,7 +487,24 @@ Function returns a value but it is ignored. -## H-7: Deletion from a nested mappping. +## H-7: 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. + +
1 Found Instances + + +- Found in contracts/amm/TempleUniswapV2Pair.sol [Line: 82](../tests/2024-07-templegold/protocol/contracts/amm/TempleUniswapV2Pair.sol#L82) + + ```solidity + uint32 blockTimestamp = uint32(block.timestamp % 2**32); + ``` + +
+ + + +## H-8: 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. diff --git a/tests/contract-playground/src/WeakRandomness.sol b/tests/contract-playground/src/WeakRandomness.sol new file mode 100644 index 00000000..ec8438fa --- /dev/null +++ b/tests/contract-playground/src/WeakRandomness.sol @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.20; + +contract WeakRandomness { + function getRandomNumber1() external view returns (uint256) { + uint256 randomNumber = uint256(keccak256(abi.encodePacked(msg.sender, block.number, block.timestamp))); + return randomNumber; + } + + function getRandomNumber2() external view returns (uint256) { + return uint256(keccak256(abi.encodePacked(block.number))); + } + + function getRandomNumber3() external view returns (uint256) { + bytes memory someBytes = abi.encode(block.number, msg.sender); + return uint256(keccak256(someBytes)); + } + + function getRandomNumber4() external view returns (uint256) { + bytes memory someBytes = abi.encodePacked(block.number, msg.sender); + return uint256(keccak256(someBytes)); + } + + function getRandomNumberUsingModulo1() external view returns (uint256) { + return block.timestamp % 10; + } + + function getRandomNumberUsingModulo2() external view returns (uint256) { + uint256 a = block.number; + uint256 b = 123; + return a % b; + } + + function getRandomNumberUsingModulo3() external view returns (uint256) { + uint256 randomNumber = uint256(blockhash(block.number)) % 10; + return randomNumber; + } + + function getRandomNumberUsingModulo4() external view returns (uint256) { + uint256 hash = uint256(blockhash(12345)); + return hash % 10; + } + + function getRandomNumberUsingPrevrandao() external view returns (uint256) { + uint256 randomNumber = block.prevrandao; + return randomNumber; + } + + // good + function timestamp() external view returns (uint256) { + return block.timestamp; + } + + function number() external view returns (uint256) { + return block.number; + } + + function encode() external view returns (bytes memory) { + return abi.encode(block.timestamp, block.number); + } + + function encodePacked() external view returns (bytes memory) { + return abi.encodePacked(block.timestamp, block.number); + } + + function moduloOperation(uint256 a, uint256 b) external pure returns (uint256) { + return a % b; + } + + function getBlockHashAsUint(uint256 blockNumber) external view returns (uint256) { + return uint256(blockhash(blockNumber)); + } + + function getDifficulty() external view returns (uint256) { + return block.difficulty; + } +}