Skip to content

Commit

Permalink
Detector: Builtin Symbol Shadow (#665)
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 Aug 16, 2024
1 parent 54b7687 commit 48d7a5f
Show file tree
Hide file tree
Showing 9 changed files with 452 additions and 15 deletions.
2 changes: 1 addition & 1 deletion aderyn/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ pub fn aderyn_is_currently_running_newest_version() -> Result<bool, reqwest::Err

let data = latest_version_checker.json::<Value>()?;
let newest =
Version::parse(data["tag_name"].as_str().unwrap().replace("v", "").as_str()).unwrap();
Version::parse(data["tag_name"].as_str().unwrap().replace('v', "").as_str()).unwrap();
let current = Version::parse(env!("CARGO_PKG_VERSION")).unwrap();

Ok(current >= newest)
Expand Down
5 changes: 5 additions & 0 deletions aderyn_core/src/detect/detector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
Box::<ReturnBombDetector>::default(),
Box::<OutOfOrderRetryableDetector>::default(),
Box::<FunctionInitializingStateDetector>::default(),
Box::<BuiltinSymbolShadowDetector>::default(),
]
}

Expand All @@ -97,6 +98,7 @@ pub fn get_all_detectors_names() -> Vec<String> {
#[derive(Debug, PartialEq, EnumString, Display)]
#[strum(serialize_all = "kebab-case")]
pub(crate) enum IssueDetectorNamePool {
BuiltinSymbolShadow,
IncorrectERC721Interface,
FunctionInitializingState,
DelegateCallInLoop,
Expand Down Expand Up @@ -176,6 +178,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
// Expects a valid detector_name
let detector_name = IssueDetectorNamePool::from_str(detector_name).ok()?;
match detector_name {
IssueDetectorNamePool::BuiltinSymbolShadow => {
Some(Box::<BuiltinSymbolShadowDetector>::default())
}
IssueDetectorNamePool::IncorrectERC721Interface => {
Some(Box::<IncorrectERC721InterfaceDetector>::default())
}
Expand Down
166 changes: 166 additions & 0 deletions aderyn_core/src/detect/low/builtin_symbol_shadowing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
use std::collections::BTreeMap;
use std::error::Error;

use crate::ast::NodeID;

use crate::capture;
use crate::detect::detector::IssueDetectorNamePool;
use phf::phf_set;

use crate::{
context::workspace_context::WorkspaceContext,
detect::detector::{IssueDetector, IssueSeverity},
};
use eyre::Result;

#[derive(Default)]
pub struct BuiltinSymbolShadowDetector {
// 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 BuiltinSymbolShadowDetector {
fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
// Variable Declaration names
for variable_declaration in context.variable_declarations() {
if DENY_LIST.contains(&variable_declaration.name) {
capture!(self, context, variable_declaration);
}
}

// Function Definition names
for function in context.function_definitions() {
if DENY_LIST.contains(&function.name) {
capture!(self, context, function);
}
}

// Modifier definition names
for modifier in context.modifier_definitions() {
if DENY_LIST.contains(&modifier.name) {
capture!(self, context, modifier);
}
}

// Event definition names
for event in context.event_definitions() {
if DENY_LIST.contains(&event.name) {
capture!(self, context, event);
}
}

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

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

fn title(&self) -> String {
String::from("Builtin Symbol Shadowing")
}

fn description(&self) -> String {
String::from("Name clashes with a built-in-symbol. Consider renaming it.")
}

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

fn name(&self) -> String {
format!("{}", IssueDetectorNamePool::BuiltinSymbolShadow)
}
}

// Copied from SLITHER
static DENY_LIST: phf::Set<&'static str> = phf_set! {
"assert",
"require",
"revert",
"block",
"blockhash",
"gasleft",
"msg",
"now",
"tx",
"this",
"addmod",
"mulmod",
"keccak256",
"sha256",
"sha3",
"ripemd160",
"ecrecover",
"selfdestruct",
"suicide",
"abi",
"fallback",
"receive",
"abstract",
"after",
"alias",
"apply",
"auto",
"case",
"catch",
"copyof",
"default",
"define",
"final",
"immutable",
"implements",
"in",
"inline",
"let",
"macro",
"match",
"mutable",
"null",
"of",
"override",
"partial",
"promise",
"reference",
"relocatable",
"sealed",
"sizeof",
"static",
"supports",
"switch",
"try",
"type",
"typedef",
"typeof",
"unchecked",
};

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

use crate::detect::{
detector::IssueDetector, low::builtin_symbol_shadowing::BuiltinSymbolShadowDetector,
};

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

let mut detector = BuiltinSymbolShadowDetector::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(), 4);
// assert the severity is low
assert_eq!(
detector.severity(),
crate::detect::detector::IssueSeverity::Low
);
}
}
2 changes: 1 addition & 1 deletion aderyn_core/src/detect/low/constant_funcs_assembly.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ impl StandardInvestigatorVisitor for AssemblyTracker {
// Ignore checking functions that start with `_`
// Example - templegold contains math functions like `_rpow()`, etc that are used by view functions
// That should be okay .. I guess? (idk ... it's open for dicussion)
if function.name.starts_with("_") {
if function.name.starts_with('_') {
return Ok(());
}
}
Expand Down
2 changes: 2 additions & 0 deletions aderyn_core/src/detect/low/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pub(crate) mod boolean_equality;
pub(crate) mod builtin_symbol_shadowing;
pub(crate) mod centralization_risk;
pub(crate) mod constant_funcs_assembly;
pub(crate) mod constants_instead_of_literals;
Expand Down Expand Up @@ -30,6 +31,7 @@ pub(crate) mod useless_public_function;
pub(crate) mod zero_address_check;

pub use boolean_equality::BooleanEqualityDetector;
pub use builtin_symbol_shadowing::BuiltinSymbolShadowDetector;
pub use centralization_risk::CentralizationRiskDetector;
pub use constant_funcs_assembly::ConstantFunctionContainsAssemblyDetector;
pub use constants_instead_of_literals::ConstantsInsteadOfLiteralsDetector;
Expand Down
74 changes: 70 additions & 4 deletions reports/report.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"files_summary": {
"total_source_units": 85,
"total_sloc": 2876
"total_source_units": 86,
"total_sloc": 2890
},
"files_details": {
"files_details": [
Expand All @@ -25,6 +25,10 @@
"file_path": "src/BooleanEquality.sol",
"n_sloc": 27
},
{
"file_path": "src/BuiltinSymbolShadow.sol",
"n_sloc": 14
},
{
"file_path": "src/CallGraphTests.sol",
"n_sloc": 49
Expand Down Expand Up @@ -349,7 +353,7 @@
},
"issue_count": {
"high": 39,
"low": 30
"low": 31
},
"high_issues": {
"issues": [
Expand Down Expand Up @@ -1315,6 +1319,12 @@
"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,
Expand Down Expand Up @@ -2464,6 +2474,12 @@
"description": "Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;`",
"detector_name": "unspecific-solidity-pragma",
"instances": [
{
"contract_path": "src/BuiltinSymbolShadow.sol",
"line_no": 2,
"src": "32:23",
"src_char": "32:23"
},
{
"contract_path": "src/CompilerBugStorageSignedIntegerArray.sol",
"line_no": 2,
Expand Down Expand Up @@ -2712,6 +2728,12 @@
"src": "113:1",
"src_char": "113:1"
},
{
"contract_path": "src/BuiltinSymbolShadow.sol",
"line_no": 8,
"src": "125:41",
"src_char": "125:41"
},
{
"contract_path": "src/ContractLocksEther.sol",
"line_no": 20,
Expand Down Expand Up @@ -3763,6 +3785,12 @@
"description": "",
"detector_name": "useless-modifier",
"instances": [
{
"contract_path": "src/BuiltinSymbolShadow.sol",
"line_no": 17,
"src": "358:39",
"src_char": "358:39"
},
{
"contract_path": "src/CallGraphTests.sol",
"line_no": 10,
Expand Down Expand Up @@ -3848,6 +3876,12 @@
"src": "457:23",
"src_char": "457:23"
},
{
"contract_path": "src/BuiltinSymbolShadow.sol",
"line_no": 8,
"src": "125:41",
"src_char": "125:41"
},
{
"contract_path": "src/CallGraphTests.sol",
"line_no": 16,
Expand Down Expand Up @@ -4817,6 +4851,37 @@
"src_char": "267:21"
}
]
},
{
"title": "Builtin Symbol Shadowing",
"description": "Name clashes with a built-in-symbol. Consider renaming it.",
"detector_name": "builtin-symbol-shadow",
"instances": [
{
"contract_path": "src/BuiltinSymbolShadow.sol",
"line_no": 5,
"src": "92:8",
"src_char": "92:8"
},
{
"contract_path": "src/BuiltinSymbolShadow.sol",
"line_no": 8,
"src": "125:41",
"src_char": "125:41"
},
{
"contract_path": "src/BuiltinSymbolShadow.sol",
"line_no": 17,
"src": "358:39",
"src_char": "358:39"
},
{
"contract_path": "src/BuiltinSymbolShadow.sol",
"line_no": 22,
"src": "414:15",
"src_char": "414:15"
}
]
}
]
},
Expand Down Expand Up @@ -4889,6 +4954,7 @@
"incorrect-erc20-interface",
"return-bomb",
"out-of-order-retryable",
"function-initializing-state"
"function-initializing-state",
"builtin-symbol-shadow"
]
}
Loading

0 comments on commit 48d7a5f

Please sign in to comment.