diff --git a/Cargo.lock b/Cargo.lock index ec194355f5e7..3e6ef311ea6b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3316,6 +3316,7 @@ dependencies = [ "eyre", "forge-doc", "forge-fmt", + "forge-lint", "forge-script", "forge-script-sequence", "forge-sol-macro-gen", @@ -3354,6 +3355,7 @@ dependencies = [ "similar", "similar-asserts", "solang-parser", + "solar-interface", "solar-parse", "soldeer-commands", "strum", @@ -3413,6 +3415,26 @@ dependencies = [ "tracing-subscriber", ] +[[package]] +name = "forge-lint" +version = "0.3.0" +dependencies = [ + "auto_impl", + "clap", + "eyre", + "foundry-common", + "foundry-compilers", + "rayon", + "regex", + "serde", + "serde_json", + "solar-ast", + "solar-interface", + "solar-parse", + "thiserror 2.0.9", + "yansi", +] + [[package]] name = "forge-script" version = "0.3.0" diff --git a/Cargo.toml b/Cargo.toml index 636f4fd6291b..d4a9f5edc680 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,8 @@ members = [ "crates/script-sequence/", "crates/macros/", "crates/test-utils/", + "crates/lint/", + ] resolver = "2" @@ -147,6 +149,7 @@ forge = { path = "crates/forge" } forge-doc = { path = "crates/doc" } forge-fmt = { path = "crates/fmt" } +forge-lint = { path = "crates/lint" } forge-verify = { path = "crates/verify" } forge-script = { path = "crates/script" } forge-sol-macro-gen = { path = "crates/sol-macro-gen" } @@ -174,7 +177,9 @@ foundry-block-explorers = { version = "0.9.0", default-features = false } foundry-compilers = { version = "0.12.9", default-features = false } foundry-fork-db = "0.10.0" solang-parser = "=0.3.3" +solar-ast = { version = "=0.1.1", default-features = false } solar-parse = { version = "=0.1.1", default-features = false } +solar-interface = { version = "=0.1.1", default-features = false } ## revm revm = { version = "19.0.0", default-features = false } diff --git a/crates/forge/Cargo.toml b/crates/forge/Cargo.toml index a8b7f56c020d..05418e1324c4 100644 --- a/crates/forge/Cargo.toml +++ b/crates/forge/Cargo.toml @@ -50,6 +50,7 @@ chrono.workspace = true # bin forge-doc.workspace = true forge-fmt.workspace = true +forge-lint.workspace = true forge-verify.workspace = true forge-script.workspace = true forge-sol-macro-gen.workspace = true @@ -88,6 +89,7 @@ serde_json.workspace = true similar = { version = "2", features = ["inline"] } solang-parser.workspace = true solar-parse.workspace = true +solar-interface.workspace = true strum = { workspace = true, features = ["derive"] } thiserror.workspace = true tokio = { workspace = true, features = ["time"] } diff --git a/crates/forge/bin/cmd/lint.rs b/crates/forge/bin/cmd/lint.rs new file mode 100644 index 000000000000..d84827dc878a --- /dev/null +++ b/crates/forge/bin/cmd/lint.rs @@ -0,0 +1,76 @@ +use clap::{Parser, ValueHint}; +use eyre::Result; +use forge_lint::{ + linter::{ProjectLinter, Severity}, + sol::SolidityLinter, +}; +use foundry_cli::utils::LoadConfig; +use foundry_config::impl_figment_convert_basic; +use std::{collections::HashSet, path::PathBuf}; + +/// CLI arguments for `forge lint`. +#[derive(Clone, Debug, Parser)] +pub struct LintArgs { + /// The project's root path. + /// + /// By default root of the Git repository, if in one, + /// or the current working directory. + #[arg(long, value_hint = ValueHint::DirPath, value_name = "PATH")] + root: Option, + + /// Include only the specified files when linting. + #[arg(long, value_hint = ValueHint::FilePath, value_name = "FILES", num_args(1..))] + include: Option>, + + /// Exclude the specified files when linting. + #[arg(long, value_hint = ValueHint::FilePath, value_name = "FILES", num_args(1..))] + exclude: Option>, + + /// Specifies which lints to run based on severity. + /// + /// Supported values: `high`, `med`, `low`, `info`, `gas`. + #[arg(long, value_name = "SEVERITY", num_args(1..))] + severity: Option>, +} + +impl_figment_convert_basic!(LintArgs); + +impl LintArgs { + pub fn run(self) -> Result<()> { + let config = self.try_load_config_emit_warnings()?; + let project = config.project()?; + + // Get all source files from the project + let mut sources = + project.paths.read_input_files()?.keys().cloned().collect::>(); + + // Add included paths to sources + if let Some(include_paths) = &self.include { + let included = + include_paths.iter().filter(|path| path.exists()).cloned().collect::>(); + sources.extend(included); + } + + // Remove excluded files from sources + if let Some(exclude_paths) = &self.exclude { + let excluded = exclude_paths.iter().cloned().collect::>(); + sources.retain(|path| !excluded.contains(path)); + } + + if sources.is_empty() { + sh_println!("Nothing to lint")?; + std::process::exit(0); + } + + let linter = if project.compiler.solc.is_some() { + SolidityLinter::new().with_severity(self.severity) + } else { + todo!("Linting not supported for this language"); + }; + + let output = ProjectLinter::new(linter).lint(&sources)?; + sh_println!("{}", &output)?; + + Ok(()) + } +} diff --git a/crates/forge/bin/cmd/mod.rs b/crates/forge/bin/cmd/mod.rs index 6885819f81c6..3c5dc72c6d37 100644 --- a/crates/forge/bin/cmd/mod.rs +++ b/crates/forge/bin/cmd/mod.rs @@ -57,6 +57,7 @@ pub mod generate; pub mod init; pub mod inspect; pub mod install; +pub mod lint; pub mod remappings; pub mod remove; pub mod selectors; diff --git a/crates/forge/bin/main.rs b/crates/forge/bin/main.rs index 60a55af7a82d..b29b8caf9e00 100644 --- a/crates/forge/bin/main.rs +++ b/crates/forge/bin/main.rs @@ -99,6 +99,7 @@ fn run() -> Result<()> { } } ForgeSubcommand::Fmt(cmd) => cmd.run(), + ForgeSubcommand::Lint(cmd) => cmd.run(), ForgeSubcommand::Config(cmd) => cmd.run(), ForgeSubcommand::Flatten(cmd) => cmd.run(), ForgeSubcommand::Inspect(cmd) => cmd.run(), diff --git a/crates/forge/bin/opts.rs b/crates/forge/bin/opts.rs index e211d03b78f3..529b35e01d5d 100644 --- a/crates/forge/bin/opts.rs +++ b/crates/forge/bin/opts.rs @@ -1,7 +1,7 @@ use crate::cmd::{ bind::BindArgs, bind_json, build::BuildArgs, cache::CacheArgs, clone::CloneArgs, compiler::CompilerArgs, config, coverage, create::CreateArgs, doc::DocArgs, eip712, flatten, - fmt::FmtArgs, geiger, generate, init::InitArgs, inspect, install::InstallArgs, + fmt::FmtArgs, geiger, generate, init::InitArgs, inspect, install::InstallArgs, lint::LintArgs, remappings::RemappingArgs, remove::RemoveArgs, selectors::SelectorsSubcommands, snapshot, soldeer, test, tree, update, }; @@ -140,6 +140,10 @@ pub enum ForgeSubcommand { /// Format Solidity source files. Fmt(FmtArgs), + /// Lint Solidity source files + #[command(visible_alias = "l")] + Lint(LintArgs), + /// Get specialized information about a smart contract. #[command(visible_alias = "in")] Inspect(inspect::InspectArgs), diff --git a/crates/lint/Cargo.toml b/crates/lint/Cargo.toml new file mode 100644 index 000000000000..93458cebabab --- /dev/null +++ b/crates/lint/Cargo.toml @@ -0,0 +1,33 @@ + +[package] +name = "forge-lint" + +version.workspace = true +edition.workspace = true +rust-version.workspace = true +authors.workspace = true +license.workspace = true +homepage.workspace = true +repository.workspace = true + +[lints] +workspace = true + +[dependencies] +# lib +foundry-common.workspace = true +foundry-compilers.workspace = true + +solar-parse.workspace = true +solar-ast.workspace = true +solar-interface.workspace = true + +eyre.workspace = true +rayon.workspace = true +thiserror.workspace = true +serde_json.workspace = true +auto_impl.workspace = true +yansi.workspace = true +serde = { workspace = true, features = ["derive"] } +regex = "1.11" +clap = { version = "4", features = ["derive"] } diff --git a/crates/lint/src/lib.rs b/crates/lint/src/lib.rs new file mode 100644 index 000000000000..73e1b3c95427 --- /dev/null +++ b/crates/lint/src/lib.rs @@ -0,0 +1,5 @@ +//! # forge-lint +//! +//! Types, traits, and utilities for linting Solidity projects. +pub mod linter; +pub mod sol; diff --git a/crates/lint/src/linter.rs b/crates/lint/src/linter.rs new file mode 100644 index 000000000000..a38892c27bf1 --- /dev/null +++ b/crates/lint/src/linter.rs @@ -0,0 +1,275 @@ +use clap::ValueEnum; +use core::fmt; +use foundry_compilers::Language; +use solar_ast::Span; +use std::{ + collections::{BTreeMap, HashMap}, + error::Error, + hash::Hash, + ops::{Deref, DerefMut}, + path::PathBuf, +}; +use yansi::Paint; + +/// Trait representing a generic linter for analyzing and reporting issues in smart contract source +/// code files. A linter can be implemented for any smart contract language supported by Foundry. +/// +/// # Type Parameters +/// +/// - `Language`: Represents the target programming language. Must implement the [`Language`] trait. +/// - `Lint`: Represents the types of lints performed by the linter. Must implement the [`Lint`] +/// trait. +/// - `LinterError`: Represents errors that can occur during the linting process. +/// +/// # Required Methods +/// +/// - `lint`: Scans the provided source files and returns a [`LinterOutput`] containing categorized +/// findings or an error if linting fails. +pub trait Linter: Send + Sync + Clone { + type Language: Language; + type Lint: Lint + Ord; + type LinterError: Error + Send + Sync + 'static; + + fn lint(&self, input: &[PathBuf]) -> Result, Self::LinterError>; +} + +pub struct ProjectLinter +where + L: Linter, +{ + pub linter: L, +} + +impl ProjectLinter +where + L: Linter, +{ + pub fn new(linter: L) -> Self { + Self { linter } + } + + pub fn lint(self, input: &[PathBuf]) -> eyre::Result> { + Ok(self.linter.lint(input)?) + } +} + +#[derive(Default)] +pub struct LinterOutput(pub BTreeMap>); + +impl LinterOutput { + pub fn new() -> Self { + Self(BTreeMap::new()) + } +} + +impl Deref for LinterOutput { + type Target = BTreeMap>; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for LinterOutput { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl Extend<(L::Lint, Vec)> for LinterOutput { + fn extend)>>(&mut self, iter: T) { + for (lint, findings) in iter { + self.0.entry(lint).or_default().extend(findings); + } + } +} + +impl fmt::Display for LinterOutput { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + writeln!(f)?; + + for (lint, locations) in &self.0 { + let severity = lint.severity(); + let name = lint.name(); + let description = lint.description(); + + let mut file_contents = HashMap::new(); + + for location in locations { + let file_content = + file_contents.entry(location.file.clone()).or_insert_with(|| { + std::fs::read_to_string(&location.file) + .expect("Could not read file for source location") + }); + + let ((start_line, start_column), (end_line, end_column)) = + match location.location(file_content) { + Some(pos) => pos, + None => continue, + }; + + let max_line_number_width = start_line.to_string().len(); + + writeln!( + f, + "{}: {}: {}", + severity, + Paint::white(name).bold(), + Paint::white(description).bold() + )?; + writeln!( + f, + "{} {}:{}:{}", + Paint::blue(" -->").bold(), + location.file.display(), + start_line, + start_column + )?; + writeln!( + f, + "{:width$}{}", + "", + Paint::blue("|").bold(), + width = max_line_number_width + 1 + )?; + + let lines = file_content.lines().collect::>(); + for line_number in start_line..=end_line { + let line = lines.get(line_number - 1).unwrap_or(&""); + + writeln!( + f, + "{:>width$} {} {}", + if line_number == start_line { + line_number.to_string() + } else { + String::new() + }, + Paint::blue("|").bold(), + line, + width = max_line_number_width + )?; + + if line_number == start_line { + let caret = severity.color(&"^".repeat(end_column - start_column)); + writeln!( + f, + "{:width$}{} {}{}", + "", + Paint::blue("|").bold(), + " ".repeat(start_column - 1), + caret, + width = max_line_number_width + 1 + )?; + } + } + + if let Some(url) = lint.url() { + writeln!( + f, + "{:width$}{} {} {} {}", + "", + Paint::blue("=").bold(), + Paint::white("help:").bold(), + Paint::white("for further information visit"), + url, + width = max_line_number_width + 1 + )?; + } + + writeln!(f)?; + } + } + + Ok(()) + } +} + +pub trait Lint: Hash { + fn name(&self) -> &'static str; + fn description(&self) -> &'static str; + fn url(&self) -> Option<&'static str>; + fn severity(&self) -> Severity; +} + +#[derive(Clone, Debug, PartialEq, Eq, ValueEnum)] +pub enum Severity { + High, + Med, + Low, + Info, + Gas, +} + +impl Severity { + pub fn color(&self, message: &str) -> String { + match self { + Self::High => Paint::red(message).bold().to_string(), + Self::Med => Paint::rgb(message, 255, 135, 61).bold().to_string(), + Self::Low => Paint::yellow(message).bold().to_string(), + Self::Info => Paint::cyan(message).bold().to_string(), + Self::Gas => Paint::green(message).bold().to_string(), + } + } +} + +impl fmt::Display for Severity { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let colored = match self { + Self::High => self.color("High"), + Self::Med => self.color("Med"), + Self::Low => self.color("Low"), + Self::Info => self.color("Info"), + Self::Gas => self.color("Gas"), + }; + write!(f, "{colored}") + } +} + +/// Represents the location of a specific AST node in the specified `file`. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct SourceLocation { + pub file: PathBuf, + pub span: Span, +} + +impl SourceLocation { + pub fn new(file: PathBuf, span: Span) -> Self { + Self { file, span } + } + + /// Find the start and end position of the span in the file content. + pub fn location(&self, file_content: &str) -> Option<((usize, usize), (usize, usize))> { + let lo = self.span.lo().0 as usize; + let hi = self.span.hi().0 as usize; + + // Ensure offsets are valid + if lo > file_content.len() || hi > file_content.len() || lo > hi { + return None; + } + + let mut offset = 0; + let mut start_line = 0; + let mut start_column = 0; + + for (line_number, line) in file_content.lines().enumerate() { + let line_length = line.len() + 1; + + // Check start position + if offset <= lo && lo < offset + line_length { + start_line = line_number + 1; + start_column = lo - offset + 1; + } + + // Check end position + if offset <= hi && hi < offset + line_length { + // Return if both positions are found + return Some(((start_line, start_column), (line_number + 1, hi - offset + 1))); + } + + offset += line_length; + } + + None + } +} diff --git a/crates/lint/src/sol/gas.rs b/crates/lint/src/sol/gas.rs new file mode 100644 index 000000000000..3c8d2c0cfbc1 --- /dev/null +++ b/crates/lint/src/sol/gas.rs @@ -0,0 +1,54 @@ +use std::ops::ControlFlow; + +use solar_ast::{visit::Visit, Expr, ExprKind}; + +use super::AsmKeccak256; + +impl<'ast> Visit<'ast> for AsmKeccak256 { + type BreakValue = (); + + fn visit_expr(&mut self, expr: &'ast Expr<'ast>) -> ControlFlow { + if let ExprKind::Call(expr, _) = &expr.kind { + if let ExprKind::Ident(ident) = &expr.kind { + if ident.name.as_str() == "keccak256" { + self.results.push(expr.span); + } + } + } + self.walk_expr(expr); + ControlFlow::Continue(()) + } +} + +#[cfg(test)] +mod test { + use solar_ast::{visit::Visit, Arena}; + use solar_interface::{ColorChoice, Session}; + use std::path::Path; + + use super::AsmKeccak256; + + #[test] + fn test_keccak256() -> eyre::Result<()> { + let sess = Session::builder().with_buffer_emitter(ColorChoice::Auto).build(); + + let _ = sess.enter(|| -> solar_interface::Result<()> { + let arena = Arena::new(); + + let mut parser = + solar_parse::Parser::from_file(&sess, &arena, Path::new("testdata/Keccak256.sol"))?; + + // Parse the file. + let ast = parser.parse_file().map_err(|e| e.emit())?; + + let mut pattern = AsmKeccak256::default(); + pattern.visit_source_unit(&ast); + + assert_eq!(pattern.results.len(), 2); + + Ok(()) + }); + + Ok(()) + } +} diff --git a/crates/lint/src/sol/high.rs b/crates/lint/src/sol/high.rs new file mode 100644 index 000000000000..b0e9e70a10ab --- /dev/null +++ b/crates/lint/src/sol/high.rs @@ -0,0 +1,14 @@ +use std::ops::ControlFlow; + +use solar_ast::{visit::Visit, Expr}; + +use super::IncorrectShift; + +impl<'ast> Visit<'ast> for IncorrectShift { + type BreakValue = (); + fn visit_expr(&mut self, expr: &'ast Expr<'ast>) -> ControlFlow { + // TODO: + self.walk_expr(expr); + ControlFlow::Continue(()) + } +} diff --git a/crates/lint/src/sol/info.rs b/crates/lint/src/sol/info.rs new file mode 100644 index 000000000000..0edc27d9481d --- /dev/null +++ b/crates/lint/src/sol/info.rs @@ -0,0 +1,175 @@ +use std::ops::ControlFlow; + +use regex::Regex; + +use solar_ast::{visit::Visit, ItemStruct, VariableDefinition}; + +use super::{ScreamingSnakeCase, StructPascalCase, VariableMixedCase}; + +impl<'ast> Visit<'ast> for VariableMixedCase { + type BreakValue = (); + + fn visit_variable_definition( + &mut self, + var: &'ast VariableDefinition<'ast>, + ) -> ControlFlow { + if var.mutability.is_none() { + if let Some(name) = var.name { + let name = name.as_str(); + if !is_mixed_case(name) && name.len() > 1 { + self.results.push(var.span); + } + } + } + + self.walk_variable_definition(var); + ControlFlow::Continue(()) + } +} + +impl<'ast> Visit<'ast> for ScreamingSnakeCase { + type BreakValue = (); + + fn visit_variable_definition( + &mut self, + var: &'ast VariableDefinition<'ast>, + ) -> ControlFlow { + if let Some(mutability) = var.mutability { + if mutability.is_constant() || mutability.is_immutable() { + if let Some(name) = var.name { + let name = name.as_str(); + if !is_screaming_snake_case(name) && name.len() > 1 { + self.results.push(var.span); + } + } + } + } + self.walk_variable_definition(var); + ControlFlow::Continue(()) + } +} + +impl<'ast> Visit<'ast> for StructPascalCase { + type BreakValue = (); + + fn visit_item_struct( + &mut self, + strukt: &'ast ItemStruct<'ast>, + ) -> ControlFlow { + let name = strukt.name.as_str(); + + if !is_pascal_case(name) && name.len() > 1 { + self.results.push(strukt.name.span); + } + + self.walk_item_struct(strukt); + ControlFlow::Continue(()) + } +} + +// Check if a string is mixedCase +pub fn is_mixed_case(s: &str) -> bool { + let re = Regex::new(r"^[a-z_][a-zA-Z0-9]*$").unwrap(); + re.is_match(s) && s.chars().any(|c| c.is_uppercase()) +} + +// Check if a string is PascalCase +pub fn is_pascal_case(s: &str) -> bool { + let re = Regex::new(r"^[A-Z][a-z]+(?:[A-Z][a-z]+)*$").unwrap(); + re.is_match(s) +} + +// Check if a string is SCREAMING_SNAKE_CASE +pub fn is_screaming_snake_case(s: &str) -> bool { + let re = Regex::new(r"^[A-Z_][A-Z0-9_]*$").unwrap(); + re.is_match(s) && s.contains('_') +} + +#[cfg(test)] +mod test { + use solar_ast::{visit::Visit, Arena}; + use solar_interface::{ColorChoice, Session}; + use std::path::Path; + + use crate::sol::StructPascalCase; + + use super::{ScreamingSnakeCase, VariableMixedCase}; + + #[test] + fn test_variable_mixed_case() -> eyre::Result<()> { + let sess = Session::builder().with_buffer_emitter(ColorChoice::Auto).build(); + + let _ = sess.enter(|| -> solar_interface::Result<()> { + let arena = Arena::new(); + + let mut parser = solar_parse::Parser::from_file( + &sess, + &arena, + Path::new("testdata/VariableMixedCase.sol"), + )?; + + let ast = parser.parse_file().map_err(|e| e.emit())?; + + let mut pattern = VariableMixedCase::default(); + pattern.visit_source_unit(&ast); + + assert_eq!(pattern.results.len(), 6); + + Ok(()) + }); + + Ok(()) + } + + #[test] + fn test_screaming_snake_case() -> eyre::Result<()> { + let sess = Session::builder().with_buffer_emitter(ColorChoice::Auto).build(); + + let _ = sess.enter(|| -> solar_interface::Result<()> { + let arena = Arena::new(); + + let mut parser = solar_parse::Parser::from_file( + &sess, + &arena, + Path::new("testdata/ScreamingSnakeCase.sol"), + )?; + + let ast = parser.parse_file().map_err(|e| e.emit())?; + + let mut pattern = ScreamingSnakeCase::default(); + pattern.visit_source_unit(&ast); + + assert_eq!(pattern.results.len(), 10); + + Ok(()) + }); + + Ok(()) + } + + #[test] + fn test_struct_pascal_case() -> eyre::Result<()> { + let sess = Session::builder().with_buffer_emitter(ColorChoice::Auto).build(); + + let _ = sess.enter(|| -> solar_interface::Result<()> { + let arena = Arena::new(); + + let mut parser = solar_parse::Parser::from_file( + &sess, + &arena, + Path::new("testdata/StructPascalCase.sol"), + )?; + + let ast = parser.parse_file().map_err(|e| e.emit())?; + + let mut pattern = StructPascalCase::default(); + pattern.visit_source_unit(&ast); + + assert_eq!(pattern.results.len(), 5); + + Ok(()) + }); + + Ok(()) + } +} diff --git a/crates/lint/src/sol/med.rs b/crates/lint/src/sol/med.rs new file mode 100644 index 000000000000..e69975462afe --- /dev/null +++ b/crates/lint/src/sol/med.rs @@ -0,0 +1,70 @@ +use std::ops::ControlFlow; + +use solar_ast::{visit::Visit, BinOp, BinOpKind, Expr, ExprKind}; + +use super::DivideBeforeMultiply; + +impl<'ast> Visit<'ast> for DivideBeforeMultiply { + type BreakValue = (); + + fn visit_expr(&mut self, expr: &'ast Expr<'ast>) -> ControlFlow { + if let ExprKind::Binary(left_expr, BinOp { kind: BinOpKind::Mul, .. }, _) = &expr.kind { + if contains_division(left_expr) { + self.results.push(expr.span); + } + } + + self.walk_expr(expr); + ControlFlow::Continue(()) + } +} + +fn contains_division<'ast>(expr: &'ast Expr<'ast>) -> bool { + match &expr.kind { + ExprKind::Binary(_, BinOp { kind: BinOpKind::Div, .. }, _) => true, + ExprKind::Tuple(inner_exprs) => inner_exprs.iter().any(|opt_expr| { + if let Some(inner_expr) = opt_expr { + contains_division(inner_expr) + } else { + false + } + }), + _ => false, + } +} + +#[cfg(test)] +mod test { + use solar_ast::{visit::Visit, Arena}; + use solar_interface::{ColorChoice, Session}; + use std::path::Path; + + use super::DivideBeforeMultiply; + + #[test] + fn test_divide_before_multiply() -> eyre::Result<()> { + let sess = Session::builder().with_buffer_emitter(ColorChoice::Auto).build(); + + let _ = sess.enter(|| -> solar_interface::Result<()> { + let arena = Arena::new(); + + let mut parser = solar_parse::Parser::from_file( + &sess, + &arena, + Path::new("testdata/DivideBeforeMultiply.sol"), + )?; + + // Parse the file. + let ast = parser.parse_file().map_err(|e| e.emit())?; + + let mut pattern = DivideBeforeMultiply::default(); + pattern.visit_source_unit(&ast); + + assert_eq!(pattern.results.len(), 6); + + Ok(()) + }); + + Ok(()) + } +} diff --git a/crates/lint/src/sol/mod.rs b/crates/lint/src/sol/mod.rs new file mode 100644 index 000000000000..fd5121c41240 --- /dev/null +++ b/crates/lint/src/sol/mod.rs @@ -0,0 +1,256 @@ +pub mod gas; +pub mod high; +pub mod info; +pub mod med; + +use std::{ + hash::{Hash, Hasher}, + ops::ControlFlow, + path::PathBuf, +}; + +use foundry_compilers::solc::SolcLanguage; +use rayon::iter::{IntoParallelIterator, ParallelIterator}; +use solar_ast::{visit::Visit, Arena, SourceUnit}; +use solar_interface::{ColorChoice, Session, Span}; +use thiserror::Error; + +use crate::linter::{Lint, Linter, LinterOutput, Severity, SourceLocation}; + +/// Linter implementation to analyze Solidity source code responsible for identifying +/// vulnerabilities gas optimizations, and best practices. +#[derive(Debug, Clone, Default)] +pub struct SolidityLinter { + pub severity: Option>, + pub description: bool, +} + +impl SolidityLinter { + pub fn new() -> Self { + Self::default() + } + + pub fn with_description(mut self, description: bool) -> Self { + self.description = description; + self + } + + pub fn with_severity(mut self, severity: Option>) -> Self { + self.severity = severity; + self + } +} + +impl Linter for SolidityLinter { + type Language = SolcLanguage; + type Lint = SolLint; + type LinterError = SolLintError; + + fn lint(&self, input: &[PathBuf]) -> Result, Self::LinterError> { + let all_findings = input + .into_par_iter() + .map(|file| { + let mut lints = if let Some(severity) = &self.severity { + SolLint::with_severity(severity.to_owned()) + } else { + SolLint::all() + }; + + // Initialize session and parsing environment + let sess = Session::builder().with_buffer_emitter(ColorChoice::Auto).build(); + let arena = Arena::new(); + + // Enter the session context for this thread + let _ = sess.enter(|| -> solar_interface::Result<()> { + let mut parser = solar_parse::Parser::from_file(&sess, &arena, file)?; + let ast = + parser.parse_file().map_err(|e| e.emit()).expect("Failed to parse file"); + + // Run all lints on the parsed AST and collect findings + for lint in lints.iter_mut() { + lint.lint(&ast); + } + + Ok(()) + }); + + (file.to_owned(), lints) + }) + .collect::)>>(); + + let mut output = LinterOutput::new(); + for (file, lints) in all_findings { + for lint in lints { + let source_locations = lint + .results() + .iter() + .map(|span| SourceLocation::new(file.clone(), *span)) + .collect::>(); + + if source_locations.is_empty() { + continue; + } + + output.insert(lint, source_locations); + } + } + + Ok(output) + } +} + +#[derive(Error, Debug)] +pub enum SolLintError {} + +/// Macro for defining lints and relevant metadata for the Solidity linter. +/// +/// This macro generates the [`SolLint`] enum with each lint along with utility methods and +/// corresponding structs for each lint specified. +/// +/// # Parameters +/// +/// Each lint is defined as a tuple with the following fields: +/// - `$name`: Identitifier used for the struct and enum variant created for the lint. +/// - `$severity`: The [`Severity`] of the lint (e.g. `High`, `Med`, `Low`, `Info`, `Gas`). +/// - `$lint_name`: A string identifier for the lint used during reporting. +/// - `$description`: A short description of the lint. +/// - `$url`: URL providing additional information about the lint or best practices. +macro_rules! declare_sol_lints { + ($(($name:ident, $severity:expr, $lint_name:expr, $description:expr, $url:expr)),* $(,)?) => { + #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] + pub enum SolLint { + $( + $name($name), + )* + } + + impl SolLint { + pub fn all() -> Vec { + vec![ + $( + SolLint::$name($name::new()), + )* + ] + } + + pub fn results(&self) -> &[Span] { + match self { + $( + SolLint::$name(lint) => &lint.results, + )* + } + } + + pub fn with_severity(severity: Vec) -> Vec { + Self::all() + .into_iter() + .filter(|lint| severity.contains(&lint.severity())) + .collect() + } + + pub fn lint(&mut self, source_unit: &SourceUnit<'_>) -> Vec { + match self { + $( + SolLint::$name(lint) => { + lint.visit_source_unit(source_unit); + lint.results.clone() + }, + )* + } + } + } + + impl<'ast> Visit<'ast> for SolLint { + type BreakValue = (); + fn visit_source_unit(&mut self, source_unit: &SourceUnit<'ast>) -> ControlFlow { + match self { + $( + SolLint::$name(lint) => lint.visit_source_unit(source_unit), + )* + } + } + } + + impl Hash for SolLint { + fn hash(&self, state: &mut H) { + self.name().hash(state); + } + } + + impl Lint for SolLint { + fn name(&self) -> &'static str { + match self { + $( + SolLint::$name(_) => $lint_name, + )* + } + } + + fn description(&self) -> &'static str { + match self { + $( + SolLint::$name(_) => $description, + )* + } + } + + fn severity(&self) -> Severity { + match self { + $( + SolLint::$name(_) => $severity, + )* + } + } + + fn url(&self) -> Option<&'static str> { + match self { + $( + SolLint::$name(_) => { + if !$url.is_empty() { + Some($url) + } else { + None + } + }, + )* + } + } + } + + $( + #[derive(Debug, Default, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] + pub struct $name { + pub results: Vec, + } + + impl $name { + pub fn new() -> Self { + Self { results: Vec::new() } + } + } + )* + }; +} + +declare_sol_lints!( + //High + (IncorrectShift, Severity::High, "incorrect-shift", "The order of args in a shift operation is incorrect.", ""), + // Med + + (DivideBeforeMultiply, Severity::Med, "divide-before-multiply", "Multiplication should occur before division to avoid loss of precision.", ""), + // Low + + // Info + (VariableMixedCase, Severity::Info, "variable-mixed-case", "Variables should follow `camelCase` naming conventions unless they are constants or immutables.", ""), + (ScreamingSnakeCase, Severity::Info, "screaming-snake-case", "Constants and immutables should be named with all capital letters with underscores separating words.", "https://docs.soliditylang.org/en/latest/style-guide.html#contract-and-library-names"), + (StructPascalCase, Severity::Info, "struct-pascal-case", "Structs should be named using PascalCase. Examples: MyCoin, Position", "https://docs.soliditylang.org/en/latest/style-guide.html#struct-names"), + // TODO: FunctionMixedCase + + // Gas Optimizations + (AsmKeccak256, Severity::Gas, "asm-keccak256", "Hashing via keccak256 can be done with inline assembly to save gas.", "https://placeholder.xyz"), + // TODO: PackStorageVariables + // TODO: PackStructs + // TODO: UseConstantVariable + // TODO: UseImmutableVariable + // TODO: UseCalldataInsteadOfMemory +); diff --git a/crates/lint/testdata/DivideBeforeMultiply.sol b/crates/lint/testdata/DivideBeforeMultiply.sol new file mode 100644 index 000000000000..3bde4b9cf6c9 --- /dev/null +++ b/crates/lint/testdata/DivideBeforeMultiply.sol @@ -0,0 +1,18 @@ +contract DivideBeforeMultiply { + function arithmetic() public { + (1 / 2) * 3; // Unsafe + (1 * 2) / 3; // Safe + ((1 / 2) * 3) * 4; // Unsafe + ((1 * 2) / 3) * 4; // Unsafe + (1 / 2 / 3) * 4; // Unsafe + (1 / (2 + 3)) * 4; // Unsafe + (1 / 2 + 3) * 4; // Safe + (1 / 2 - 3) * 4; // Safe + (1 + 2 / 3) * 4; // Safe + (1 / 2 - 3) * 4; // Safe + ((1 / 2) % 3) * 4; // Safe + 1 / (2 * 3 + 3); // Safe + 1 / ((2 / 3) * 3); // Unsafe + 1 / ((2 * 3) + 3); // Safe + } +} diff --git a/crates/lint/testdata/Keccak256.sol b/crates/lint/testdata/Keccak256.sol new file mode 100644 index 000000000000..12fcbc4c00c2 --- /dev/null +++ b/crates/lint/testdata/Keccak256.sol @@ -0,0 +1,18 @@ +contract AsmKeccak256 { + constructor(uint256 a, uint256 b) { + keccak256(abi.encodePacked(a, b)); + } + + function solidityHash(uint256 a, uint256 b) public view { + keccak256(abi.encodePacked(a, b)); + } + + function assemblyHash(uint256 a, uint256 b) public view { + //optimized + assembly { + mstore(0x00, a) + mstore(0x20, b) + let hashedVal := keccak256(0x00, 0x40) + } + } +} diff --git a/crates/lint/testdata/ScreamingSnakeCase.sol b/crates/lint/testdata/ScreamingSnakeCase.sol new file mode 100644 index 000000000000..ca4d72fb6d06 --- /dev/null +++ b/crates/lint/testdata/ScreamingSnakeCase.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +contract ScreamingSnakeCaseTest { + // Passes + uint256 constant _SCREAMING_SNAKE_CASE = 0; + uint256 constant SCREAMING_SNAKE_CASE = 0; + uint256 immutable _SCREAMING_SNAKE_CASE_1 = 0; + uint256 immutable SCREAMING_SNAKE_CASE_1 = 0; + + // Fails + uint256 constant SCREAMINGSNAKECASE = 0; + uint256 constant screamingSnakeCase = 0; + uint256 constant screaming_snake_case = 0; + uint256 constant ScreamingSnakeCase = 0; + uint256 constant SCREAMING_snake_case = 0; + uint256 immutable SCREAMINGSNAKECASE0 = 0; + uint256 immutable screamingSnakeCase0 = 0; + uint256 immutable screaming_snake_case0 = 0; + uint256 immutable ScreamingSnakeCase0 = 0; + uint256 immutable SCREAMING_snake_case_0 = 0; +} diff --git a/crates/lint/testdata/StructPascalCase.sol b/crates/lint/testdata/StructPascalCase.sol new file mode 100644 index 000000000000..a79758f24baf --- /dev/null +++ b/crates/lint/testdata/StructPascalCase.sol @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +contract StructPascalCaseTest { + // Passes + struct PascalCase { + uint256 a; + } + + // Fails + struct _PascalCase { + uint256 a; + } + + struct pascalCase { + uint256 a; + } + + struct pascalcase { + uint256 a; + } + + struct pascal_case { + uint256 a; + } + + struct PASCAL_CASE { + uint256 a; + } + + struct PASCALCASE { + uint256 a; + } + + struct PascalCAse { + uint256 a; + } +} diff --git a/crates/lint/testdata/VariableMixedCase.sol b/crates/lint/testdata/VariableMixedCase.sol new file mode 100644 index 000000000000..77f28b226196 --- /dev/null +++ b/crates/lint/testdata/VariableMixedCase.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +contract VariableMixedCaseTest { + // Passes + uint256 variableMixedCase; + uint256 _variableMixedCase; + + // Fails + uint256 Variablemixedcase; + uint256 VARIABLE_MIXED_CASE; + uint256 variablemixedcase; + uint256 VariableMixedCase; + + function foo() public { + // Passes + uint256 testVal; + uint256 testVAL; + uint256 testVal123; + + // Fails + uint256 TestVal; + uint256 TESTVAL; + } +}