Skip to content

Commit c9c251d

Browse files
DavidDrobalexroan
andauthored
Detector: Weak Randomness (#618)
Co-authored-by: Alex Roan <[email protected]>
1 parent fc3d760 commit c9c251d

File tree

9 files changed

+640
-16
lines changed

9 files changed

+640
-16
lines changed

aderyn_core/src/detect/detector.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
6565
Box::<RTLODetector>::default(),
6666
Box::<UncheckedReturnDetector>::default(),
6767
Box::<DangerousUnaryOperatorDetector>::default(),
68+
Box::<WeakRandomnessDetector>::default(),
6869
Box::<PreDeclaredLocalVariableUsageDetector>::default(),
6970
Box::<DeletionNestedMappingDetector>::default(),
7071
]
@@ -128,6 +129,7 @@ pub(crate) enum IssueDetectorNamePool {
128129
RTLO,
129130
UncheckedReturn,
130131
DangerousUnaryOperator,
132+
WeakRandomness,
131133
PreDeclaredLocalVariableUsage,
132134
DeleteNestedMapping,
133135
// 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<Box<dyn Iss
265267
IssueDetectorNamePool::DangerousUnaryOperator => {
266268
Some(Box::<DangerousUnaryOperatorDetector>::default())
267269
}
270+
IssueDetectorNamePool::WeakRandomness => Some(Box::<WeakRandomnessDetector>::default()),
268271
IssueDetectorNamePool::PreDeclaredLocalVariableUsage => {
269272
Some(Box::<PreDeclaredLocalVariableUsageDetector>::default())
270273
}

aderyn_core/src/detect/high/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ pub(crate) mod unchecked_return;
2525
pub(crate) mod uninitialized_state_variable;
2626
pub(crate) mod unprotected_init_function;
2727
pub(crate) mod unsafe_casting;
28+
pub(crate) mod weak_randomness;
2829
pub(crate) mod yul_return;
2930

3031
pub use arbitrary_transfer_from::ArbitraryTransferFromDetector;
@@ -54,4 +55,5 @@ pub use unchecked_return::UncheckedReturnDetector;
5455
pub use uninitialized_state_variable::UninitializedStateVariableDetector;
5556
pub use unprotected_init_function::UnprotectedInitializerDetector;
5657
pub use unsafe_casting::UnsafeCastingDetector;
58+
pub use weak_randomness::WeakRandomnessDetector;
5759
pub use yul_return::YulReturnDetector;
Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
use std::collections::BTreeMap;
2+
use std::error::Error;
3+
4+
use crate::ast::{Expression, FunctionCall, FunctionCallKind, NodeID};
5+
6+
use crate::capture;
7+
use crate::detect::detector::IssueDetectorNamePool;
8+
use crate::{
9+
context::workspace_context::{ASTNode, WorkspaceContext},
10+
detect::detector::{IssueDetector, IssueSeverity},
11+
};
12+
use eyre::Result;
13+
14+
#[derive(Default)]
15+
pub struct WeakRandomnessDetector {
16+
// Keys are: [0] source file name, [1] line number, [2] character location of node.
17+
// Do not add items manually, use `capture!` to add nodes to this BTreeMap.
18+
found_instances: BTreeMap<(String, usize, String), NodeID>,
19+
}
20+
21+
impl IssueDetector for WeakRandomnessDetector {
22+
fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
23+
let keccaks: Vec<&FunctionCall> = context.function_calls()
24+
.into_iter()
25+
.filter(|x| matches!(*x.expression, Expression::Identifier(ref id) if id.name == "keccak256"))
26+
.collect();
27+
28+
for keccak in keccaks {
29+
// keccak256 must have exactly one argument
30+
if let Some(arg) = keccak.arguments.first() {
31+
if let Expression::FunctionCall(ref function_call) = *arg {
32+
if check_encode(function_call) {
33+
capture!(self, context, keccak);
34+
}
35+
}
36+
// get variable definition
37+
else if let Expression::Identifier(ref i) = *arg {
38+
if let Some(node_id) = i.referenced_declaration {
39+
let decleration = context.get_parent(node_id);
40+
41+
if let Some(ASTNode::VariableDeclarationStatement(var)) = decleration {
42+
if let Some(Expression::FunctionCall(function_call)) =
43+
&var.initial_value
44+
{
45+
if check_encode(function_call) {
46+
capture!(self, context, keccak);
47+
}
48+
}
49+
}
50+
}
51+
}
52+
}
53+
}
54+
55+
// check for modulo operations on block.timestamp, block.number and blockhash
56+
for binary_operation in context
57+
.binary_operations()
58+
.into_iter()
59+
.filter(|b| b.operator == "%")
60+
{
61+
// if left operand is a variable, get its definition and perform check
62+
if let Expression::Identifier(ref i) = *binary_operation.left_expression {
63+
if let Some(node_id) = i.referenced_declaration {
64+
let decleration = context.get_parent(node_id);
65+
66+
if let Some(ASTNode::VariableDeclarationStatement(var)) = decleration {
67+
if let Some(expression) = &var.initial_value {
68+
if check_operand(expression) {
69+
capture!(self, context, binary_operation);
70+
continue;
71+
}
72+
}
73+
}
74+
}
75+
}
76+
// otherwise perform check directly on the expression
77+
else if check_operand(&binary_operation.left_expression) {
78+
capture!(self, context, binary_operation);
79+
}
80+
}
81+
82+
// check if contract uses block.prevrandao
83+
for member_access in context.member_accesses() {
84+
if member_access.member_name == "prevrandao"
85+
&& matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block")
86+
{
87+
capture!(self, context, member_access);
88+
}
89+
}
90+
91+
Ok(!self.found_instances.is_empty())
92+
}
93+
94+
fn severity(&self) -> IssueSeverity {
95+
IssueSeverity::High
96+
}
97+
98+
fn title(&self) -> String {
99+
String::from("Weak Randomness")
100+
}
101+
102+
fn description(&self) -> String {
103+
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.")
104+
}
105+
106+
fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
107+
self.found_instances.clone()
108+
}
109+
110+
fn name(&self) -> String {
111+
format!("{}", IssueDetectorNamePool::WeakRandomness)
112+
}
113+
}
114+
115+
// returns whether block.timestamp or block.number is used in encode function
116+
fn check_encode(function_call: &FunctionCall) -> bool {
117+
if let Expression::MemberAccess(ref member_access) = *function_call.expression {
118+
if member_access.member_name == "encodePacked" || member_access.member_name == "encode" {
119+
for argument in &function_call.arguments {
120+
if let Expression::MemberAccess(ref member_access) = *argument {
121+
if ["timestamp", "number"].iter().any(|ma| {
122+
ma == &member_access.member_name &&
123+
matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block")
124+
}) {
125+
return true;
126+
}
127+
}
128+
}
129+
}
130+
}
131+
false
132+
}
133+
134+
// returns whether operand is dependent on block.timestamp, block.number or blockhash
135+
fn check_operand(operand: &Expression) -> bool {
136+
match operand {
137+
Expression::MemberAccess(member_access) => {
138+
if ["timestamp", "number"].iter().any(|ma| {
139+
ma == &member_access.member_name &&
140+
matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block")
141+
}) {
142+
return true;
143+
}
144+
},
145+
Expression::FunctionCall(function_call) => {
146+
if function_call.kind == FunctionCallKind::TypeConversion {
147+
// type conversion must have exactly one argument
148+
if let Some(Expression::FunctionCall(ref inner_function_call)) = function_call.arguments.first() {
149+
if matches!(*inner_function_call.expression, Expression::Identifier(ref id) if id.name == "blockhash") {
150+
return true;
151+
}
152+
}
153+
}
154+
},
155+
_ => ()
156+
}
157+
158+
false
159+
}
160+
161+
#[cfg(test)]
162+
mod weak_randomness_detector_tests {
163+
use crate::detect::{detector::IssueDetector, high::weak_randomness::WeakRandomnessDetector};
164+
165+
#[test]
166+
fn test_weak_randomness_detector() {
167+
let context = crate::detect::test_utils::load_solidity_source_unit(
168+
"../tests/contract-playground/src/WeakRandomness.sol",
169+
);
170+
171+
let mut detector = WeakRandomnessDetector::default();
172+
let found = detector.detect(&context).unwrap();
173+
// assert that the detector found an issue
174+
assert!(found);
175+
// assert that the detector found the correct number of instances
176+
assert_eq!(detector.instances().len(), 9);
177+
// assert the severity is high
178+
assert_eq!(
179+
detector.severity(),
180+
crate::detect::detector::IssueSeverity::High
181+
);
182+
// assert the title is correct
183+
assert_eq!(detector.title(), String::from("Weak Randomness"));
184+
// assert the description is correct
185+
assert_eq!(
186+
detector.description(),
187+
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.")
188+
);
189+
}
190+
}

reports/adhoc-sol-files-highs-only-report.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,7 @@
188188
"rtlo",
189189
"unchecked-return",
190190
"dangerous-unary-operator",
191+
"weak-randomness",
191192
"pre-declared-local-variable-usage",
192193
"delete-nested-mapping"
193194
]

reports/report.json

Lines changed: 93 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"files_summary": {
3-
"total_source_units": 66,
4-
"total_sloc": 1845
3+
"total_source_units": 67,
4+
"total_sloc": 1904
55
},
66
"files_details": {
77
"files_details": [
@@ -185,6 +185,10 @@
185185
"file_path": "src/UsingSelfdestruct.sol",
186186
"n_sloc": 6
187187
},
188+
{
189+
"file_path": "src/WeakRandomness.sol",
190+
"n_sloc": 59
191+
},
188192
{
189193
"file_path": "src/WrongOrderOfLayout.sol",
190194
"n_sloc": 13
@@ -272,7 +276,7 @@
272276
]
273277
},
274278
"issue_count": {
275-
"high": 28,
279+
"high": 29,
276280
"low": 23
277281
},
278282
"high_issues": {
@@ -1587,6 +1591,67 @@
15871591
}
15881592
]
15891593
},
1594+
{
1595+
"title": "Weak Randomness",
1596+
"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.",
1597+
"detector_name": "weak-randomness",
1598+
"instances": [
1599+
{
1600+
"contract_path": "src/WeakRandomness.sol",
1601+
"line_no": 6,
1602+
"src": "188:70",
1603+
"src_char": "188:70"
1604+
},
1605+
{
1606+
"contract_path": "src/WeakRandomness.sol",
1607+
"line_no": 11,
1608+
"src": "386:41",
1609+
"src_char": "386:41"
1610+
},
1611+
{
1612+
"contract_path": "src/WeakRandomness.sol",
1613+
"line_no": 16,
1614+
"src": "597:20",
1615+
"src_char": "597:20"
1616+
},
1617+
{
1618+
"contract_path": "src/WeakRandomness.sol",
1619+
"line_no": 21,
1620+
"src": "793:20",
1621+
"src_char": "793:20"
1622+
},
1623+
{
1624+
"contract_path": "src/WeakRandomness.sol",
1625+
"line_no": 25,
1626+
"src": "915:20",
1627+
"src_char": "915:20"
1628+
},
1629+
{
1630+
"contract_path": "src/WeakRandomness.sol",
1631+
"line_no": 31,
1632+
"src": "1095:5",
1633+
"src_char": "1095:5"
1634+
},
1635+
{
1636+
"contract_path": "src/WeakRandomness.sol",
1637+
"line_no": 35,
1638+
"src": "1217:37",
1639+
"src_char": "1217:37"
1640+
},
1641+
{
1642+
"contract_path": "src/WeakRandomness.sol",
1643+
"line_no": 41,
1644+
"src": "1434:9",
1645+
"src_char": "1434:9"
1646+
},
1647+
{
1648+
"contract_path": "src/WeakRandomness.sol",
1649+
"line_no": 45,
1650+
"src": "1563:16",
1651+
"src_char": "1563:16"
1652+
}
1653+
]
1654+
},
15901655
{
15911656
"title": "Usage of variable before declaration.",
15921657
"description": "This is a bad practice that may lead to unintended consequences. Please declare the variable before using it.",
@@ -2284,6 +2349,24 @@
22842349
"src": "1190:7",
22852350
"src_char": "1190:7"
22862351
},
2352+
{
2353+
"contract_path": "src/WeakRandomness.sol",
2354+
"line_no": 25,
2355+
"src": "933:2",
2356+
"src_char": "933:2"
2357+
},
2358+
{
2359+
"contract_path": "src/WeakRandomness.sol",
2360+
"line_no": 35,
2361+
"src": "1252:2",
2362+
"src_char": "1252:2"
2363+
},
2364+
{
2365+
"contract_path": "src/WeakRandomness.sol",
2366+
"line_no": 41,
2367+
"src": "1441:2",
2368+
"src_char": "1441:2"
2369+
},
22872370
{
22882371
"contract_path": "src/eth2/DepositContract.sol",
22892372
"line_no": 113,
@@ -2589,6 +2672,12 @@
25892672
"src": "32:23",
25902673
"src_char": "32:23"
25912674
},
2675+
{
2676+
"contract_path": "src/WeakRandomness.sol",
2677+
"line_no": 2,
2678+
"src": "32:23",
2679+
"src_char": "32:23"
2680+
},
25922681
{
25932682
"contract_path": "src/cloc/AnotherHeavilyCommentedContract.sol",
25942683
"line_no": 6,
@@ -3384,6 +3473,7 @@
33843473
"rtlo",
33853474
"unchecked-return",
33863475
"dangerous-unary-operator",
3476+
"weak-randomness",
33873477
"pre-declared-local-variable-usage",
33883478
"delete-nested-mapping"
33893479
]

0 commit comments

Comments
 (0)