Skip to content

Commit

Permalink
Detector: Usage of pre-declared variables. (#617)
Browse files Browse the repository at this point in the history
Co-authored-by: Alex Roan <[email protected]>
  • Loading branch information
TilakMaddy and alexroan authored Jul 28, 2024
1 parent bb4cecf commit fc3d760
Show file tree
Hide file tree
Showing 36 changed files with 397 additions and 11 deletions.
5 changes: 5 additions & 0 deletions aderyn_core/src/detect/detector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
Box::<RTLODetector>::default(),
Box::<UncheckedReturnDetector>::default(),
Box::<DangerousUnaryOperatorDetector>::default(),
Box::<PreDeclaredLocalVariableUsageDetector>::default(),
Box::<DeletionNestedMappingDetector>::default(),
]
}
Expand Down Expand Up @@ -127,6 +128,7 @@ pub(crate) enum IssueDetectorNamePool {
RTLO,
UncheckedReturn,
DangerousUnaryOperator,
PreDeclaredLocalVariableUsage,
DeleteNestedMapping,
// NOTE: `Undecided` will be the default name (for new bots).
// If it's accepted, a new variant will be added to this enum before normalizing it in aderyn
Expand Down Expand Up @@ -263,6 +265,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
IssueDetectorNamePool::DangerousUnaryOperator => {
Some(Box::<DangerousUnaryOperatorDetector>::default())
}
IssueDetectorNamePool::PreDeclaredLocalVariableUsage => {
Some(Box::<PreDeclaredLocalVariableUsageDetector>::default())
}
IssueDetectorNamePool::DeleteNestedMapping => {
Some(Box::<DeletionNestedMappingDetector>::default())
}
Expand Down
13 changes: 13 additions & 0 deletions aderyn_core/src/detect/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,19 @@ pub fn get_literal_value_or_constant_variable_value(
None
}

pub fn get_node_offset(node: &ASTNode) -> Option<usize> {
let src_location = node.src()?;

let chopped_location = match src_location.rfind(':') {
Some(index) => &src_location[..index],
None => src_location, // No colon found, return the original string
}
.to_string();

let (offset, _) = chopped_location.split_once(':').unwrap();
offset.parse::<usize>().ok()
}

/*
Check if expression in constant
Expression::Literal whose value is false/true
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/_template.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,12 @@ impl IssueDetector for TemplateDetector {

#[cfg(test)]
mod template_detector_tests {
use serial_test::serial;

use crate::detect::{detector::IssueDetector, high::template_detector::TemplateDetector};

#[test]
#[serial]
fn test_template_detector() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/ArbitraryTransferFrom.sol",
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/dangerous_unary_operator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,14 @@ impl IssueDetector for DangerousUnaryOperatorDetector {

#[cfg(test)]
mod dangerous_unary_expression_tests {
use serial_test::serial;

use crate::detect::{
detector::IssueDetector, high::dangerous_unary_operator::DangerousUnaryOperatorDetector,
};

#[test]
#[serial]
fn test_dangerous_unary_operator() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/DangerousUnaryOperator.sol",
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/delegate_call_no_address_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,15 @@ impl<'a> StandardInvestigatorVisitor for DelegateCallNoAddressChecksTracker<'a>

#[cfg(test)]
mod delegate_call_no_address_check_tests {
use serial_test::serial;

use crate::detect::{
detector::IssueDetector,
high::delegate_call_no_address_check::DelegateCallOnUncheckedAddressDetector,
};

#[test]
#[serial]
fn test_delegate_call_without_checks() {
let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs(
"../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol",
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/deletion_nested_mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,14 @@ impl IssueDetector for DeletionNestedMappingDetector {

#[cfg(test)]
mod deletion_nested_mapping_tests {
use serial_test::serial;

use crate::detect::{
detector::IssueDetector, high::deletion_nested_mapping::DeletionNestedMappingDetector,
};

#[test]
#[serial]
fn test_deletion_nested_mapping() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,15 @@ impl IssueDetector for DynamicArrayLengthAssignmentDetector {

#[cfg(test)]
mod dynamic_array_length_assignment_tests {
use serial_test::serial;

use crate::detect::{
detector::IssueDetector, high::DynamicArrayLengthAssignmentDetector,
test_utils::load_solidity_source_unit,
};

#[test]
#[serial]
fn test_reused_contract_name_detector() {
let context = load_solidity_source_unit(
"../tests/contract-playground/src/DynamicArrayLengthAssignment.sol",
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/enumerable_loop_removal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,12 @@ Consider using a different data structure or removing items by collecting them d

#[cfg(test)]
mod enuemrable_loop_removal_tests {
use serial_test::serial;

use crate::detect::{detector::IssueDetector, high::EnumerableLoopRemovalDetector};

#[test]
#[serial]
fn test_enumerable_loop_detector() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/EnumerableSetIteration.sol",
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/experimental_encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,14 @@ impl IssueDetector for ExperimentalEncoderDetector {

#[cfg(test)]
mod storage_array_encode_compiler_bug_detector_tests {
use serial_test::serial;

use crate::detect::{
detector::IssueDetector, high::experimental_encoder::ExperimentalEncoderDetector,
};

#[test]
#[serial]
fn test_storage_array_encode_compiler_bug_detector() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/ExperimentalEncoder.sol",
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/incorrect_caret_operator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,12 @@ impl IssueDetector for IncorrectUseOfCaretOperatorDetector {

#[cfg(test)]
mod incorrect_use_of_caret_operator_tests {
use serial_test::serial;

use crate::detect::{detector::IssueDetector, high::IncorrectUseOfCaretOperatorDetector};

#[test]
#[serial]
fn test_incorrect_use_of_operator_detector() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/IncorrectCaretOperator.sol",
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/incorrect_shift_order.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,12 @@ impl IssueDetector for IncorrectShiftOrderDetector {

#[cfg(test)]
mod incorrect_shift_order_detector_tests {
use serial_test::serial;

use crate::detect::{detector::IssueDetector, high::IncorrectShiftOrderDetector};

#[test]
#[serial]
fn test_incorrect_shift_order_detector() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/IncorrectShift.sol",
Expand Down
3 changes: 3 additions & 0 deletions aderyn_core/src/detect/high/misused_boolean.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,12 @@ issue_detector! {

#[cfg(test)]
mod misused_boolean_tests {
use serial_test::serial;

use crate::detect::{detector::IssueDetector, high::misused_boolean::MisusedBooleanDetector};

#[test]
#[serial]
fn test_misused_boolean_by_loading_contract_directly() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/MisusedBoolean.sol",
Expand Down
2 changes: 2 additions & 0 deletions aderyn_core/src/detect/high/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ pub(crate) mod incorrect_shift_order;
pub(crate) mod misused_boolean;
pub(crate) mod multiple_constructors;
pub(crate) mod nested_struct_in_mapping;
pub(crate) mod pre_declared_variable_usage;
pub(crate) mod reused_contract_name;
pub(crate) mod rtlo;
pub(crate) mod selfdestruct;
Expand Down Expand Up @@ -41,6 +42,7 @@ pub use incorrect_shift_order::IncorrectShiftOrderDetector;
pub use misused_boolean::MisusedBooleanDetector;
pub use multiple_constructors::MultipleConstructorsDetector;
pub use nested_struct_in_mapping::NestedStructInMappingDetector;
pub use pre_declared_variable_usage::PreDeclaredLocalVariableUsageDetector;
pub use reused_contract_name::ReusedContractNameDetector;
pub use rtlo::RTLODetector;
pub use selfdestruct::SelfdestructIdentifierDetector;
Expand Down
4 changes: 4 additions & 0 deletions aderyn_core/src/detect/high/multiple_constructors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,12 @@ impl IssueDetector for MultipleConstructorsDetector {

#[cfg(test)]
mod multiple_constructors_detector_tests {
use serial_test::serial;

use crate::detect::{detector::IssueDetector, high::MultipleConstructorsDetector};

#[test]
#[serial]
fn test_multiple_constructors_detector() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/MultipleConstructorSchemes.sol",
Expand Down Expand Up @@ -96,6 +99,7 @@ mod multiple_constructors_detector_tests {
}

#[test]
#[serial]
fn test_multiple_constructors_detector_no_issue() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/ArbitraryTransferFrom.sol",
Expand Down
4 changes: 4 additions & 0 deletions aderyn_core/src/detect/high/nested_struct_in_mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,12 @@ impl IssueDetector for NestedStructInMappingDetector {

#[cfg(test)]
mod nested_struct_in_mapping_detector_tests {
use serial_test::serial;

use crate::detect::{detector::IssueDetector, high::NestedStructInMappingDetector};

#[test]
#[serial]
fn test_nested_struct_in_mapping_detector() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/nested_mappings/NestedMappings.sol",
Expand Down Expand Up @@ -141,6 +144,7 @@ mod nested_struct_in_mapping_detector_tests {
}

#[test]
#[serial]
fn test_nested_struct_in_mapping_detector_no_issue() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/nested_mappings/LaterVersion.sol",
Expand Down
139 changes: 139 additions & 0 deletions aderyn_core/src/detect/high/pre_declared_variable_usage.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
use std::collections::{BTreeMap, HashSet};
use std::error::Error;

use crate::ast::{ASTNode, NodeID};

use crate::capture;
use crate::context::browser::{ExtractIdentifiers, ExtractVariableDeclarations};
use crate::detect::detector::IssueDetectorNamePool;
use crate::detect::helpers;
use crate::{
context::workspace_context::WorkspaceContext,
detect::detector::{IssueDetector, IssueSeverity},
};
use eyre::Result;

// HOW TO USE THIS TEMPLATE:
// 1. Copy this file and rename it to the snake_case version of the issue you are detecting.
// 2. Rename the PreDeclaredLocalVariableUsageDetector struct and impl to your new issue name.
// 3. Add this file and detector struct to the mod.rs file in the same directory.
// 4. Implement the detect function to find instances of the issue.

#[derive(Default)]
pub struct PreDeclaredLocalVariableUsageDetector {
// 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 PreDeclaredLocalVariableUsageDetector {
fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
// Since this is restricted to local variables, we examine each function independently
for function in context
.function_definitions()
.into_iter()
.filter(|&f| f.implemented)
{
let local_variable_declaration_ids = ExtractVariableDeclarations::from(function)
.extracted
.iter()
.map(|vd| vd.id)
.collect::<HashSet<_>>();

let used_local_variables = ExtractIdentifiers::from(function).extracted;

let used_local_variables = used_local_variables
.iter()
.filter(|identifier| {
identifier
.referenced_declaration
.is_some_and(|referenced_declaration| {
local_variable_declaration_ids.contains(&referenced_declaration)
})
})
.collect::<HashSet<_>>();

for used in used_local_variables {
if let Some(id) = used.referenced_declaration {
if let Some(ASTNode::VariableDeclaration(variable_declaration)) =
context.nodes.get(&id)
{
let used_offset = helpers::get_node_offset(&used.into());
let declaration_offset =
helpers::get_node_offset(&variable_declaration.into());

if let (Some(used_offset), Some(declaration_offset)) =
(used_offset, declaration_offset)
{
if used_offset < declaration_offset {
capture!(self, context, used);
}
}
}
}
}
}

Ok(!self.found_instances.is_empty())
}

fn severity(&self) -> IssueSeverity {
IssueSeverity::High
}

fn title(&self) -> String {
String::from("Usage of variable before declaration.")
}

fn description(&self) -> String {
String::from("This is a bad practice that may lead to unintended consequences. Please declare the variable before using it.")
}

fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
self.found_instances.clone()
}

fn name(&self) -> String {
IssueDetectorNamePool::PreDeclaredLocalVariableUsage.to_string()
}
}

#[cfg(test)]
mod pre_declared_variable_usage_tests {
use serial_test::serial;

use crate::detect::{
detector::IssueDetector,
high::pre_declared_variable_usage::PreDeclaredLocalVariableUsageDetector,
};

#[test]
#[serial]
fn test_pre_declared_variable_usage() {
let context = crate::detect::test_utils::load_solidity_source_unit(
"../tests/contract-playground/src/PreDeclaredVarUsage.sol",
);

let mut detector = PreDeclaredLocalVariableUsageDetector::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(), 1);
// 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("Usage of variable before declaration.")
);
// assert the description is correct
assert_eq!(
detector.description(),
String::from("This is a bad practice that may lead to unintended consequences. Please declare the variable before using it.")
);
}
}
Loading

0 comments on commit fc3d760

Please sign in to comment.