diff --git a/crates/forge/tests/cli/lint.rs b/crates/forge/tests/cli/lint.rs index 113eddd347f43..bdb7bb7a35495 100644 --- a/crates/forge/tests/cli/lint.rs +++ b/crates/forge/tests/cli/lint.rs @@ -18,9 +18,9 @@ uint256 constant screaming_snake_case_info = 0; contract ContractWithLints { uint256 VARIABLE_MIXED_CASE_INFO; - function incorrectShiftHigh() public { - uint256 localValue = 50; - uint256 result = 8 >> localValue; + function incorrectShiftHigh() public pure { + uint256 result = 8; + assembly { result := shr(result, 8) } } function divideBeforeMultiplyMedium() public { (1 / 2) * 3; @@ -666,10 +666,10 @@ forgetest!(can_override_config_lint, |prj, cmd| { cmd.arg("lint").args(["--only-lint", "incorrect-shift"]).assert_success().stderr_eq(str![[ r#" warning[incorrect-shift]: the order of args in a shift operation is incorrect - [FILE]:13:26 + [FILE]:13:30 │ -13 │ uint256 result = 8 >> localValue; - │ ━━━━━━━━━━━━━━━ +13 │ assembly { result := shr(result, 8) } + │ ━━━━━━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/incorrect-shift @@ -707,24 +707,12 @@ warning[divide-before-multiply]: multiplication should occur before division to [COMPILING_FILES] with [SOLC_VERSION] [SOLC_VERSION] [ELAPSED] Compiler run successful with warnings: -Warning (2072): Unused local variable. - [FILE]:13:9: - | -13 | uint256 result = 8 >> localValue; - | ^^^^^^^^^^^^^^ - Warning (6133): Statement has no effect. [FILE]:16:9: | 16 | (1 / 2) * 3; | ^^^^^^^^^^^ -Warning (2018): Function state mutability can be restricted to pure - [FILE]:11:5: - | -11 | function incorrectShiftHigh() public { - | ^ (Relevant source part starts here and spans across multiple lines). - Warning (2018): Function state mutability can be restricted to pure [FILE]:15:5: | @@ -806,24 +794,12 @@ forgetest!(build_respects_lint_on_build_false, |prj, cmd| { [COMPILING_FILES] with [SOLC_VERSION] [SOLC_VERSION] [ELAPSED] Compiler run successful with warnings: -Warning (2072): Unused local variable. - [FILE]:13:9: - | -13 | uint256 result = 8 >> localValue; - | ^^^^^^^^^^^^^^ - Warning (6133): Statement has no effect. [FILE]:16:9: | 16 | (1 / 2) * 3; | ^^^^^^^^^^^ -Warning (2018): Function state mutability can be restricted to pure - [FILE]:11:5: - | -11 | function incorrectShiftHigh() public { - | ^ (Relevant source part starts here and spans across multiple lines). - Warning (2018): Function state mutability can be restricted to pure [FILE]:15:5: | @@ -882,24 +858,12 @@ forgetest!(build_no_lint_flag_skips_lint, |prj, cmd| { [COMPILING_FILES] with [SOLC_VERSION] [SOLC_VERSION] [ELAPSED] Compiler run successful with warnings: -Warning (2072): Unused local variable. - [FILE]:13:9: - | -13 | uint256 result = 8 >> localValue; - | ^^^^^^^^^^^^^^ - Warning (6133): Statement has no effect. [FILE]:16:9: | 16 | (1 / 2) * 3; | ^^^^^^^^^^^ -Warning (2018): Function state mutability can be restricted to pure - [FILE]:11:5: - | -11 | function incorrectShiftHigh() public { - | ^ (Relevant source part starts here and spans across multiple lines). - Warning (2018): Function state mutability can be restricted to pure [FILE]:15:5: | @@ -1427,7 +1391,7 @@ forgetest!(pragma_inconsistent_cross_file, |prj, cmd| { cmd.arg("lint").args(["--only-lint", "pragma-inconsistent"]).assert_success().stderr_eq(str![ [r#" -note[pragma-inconsistent]: 'pragma solidity ^0.8.20;' conflicts with other version requirements in the project: 0.8.20 +note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: 0.8.20, ^0.8.20 [FILE]:3:1 │ 3 │ pragma solidity ^0.8.20; @@ -1435,14 +1399,6 @@ note[pragma-inconsistent]: 'pragma solidity ^0.8.20;' conflicts with other versi │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20 - [FILE]:3:1 - │ -3 │ pragma solidity 0.8.20; - │ ━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - "#] ]); @@ -1538,7 +1494,7 @@ forgetest!(pragma_inconsistent_duplicates_among_conflict, |prj, cmd| { cmd.arg("lint").args(["--only-lint", "pragma-inconsistent"]).assert_success().stderr_eq(str![ [r#" -note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20 +note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: 0.8.20, ^0.8.20 [FILE]:3:1 │ 3 │ pragma solidity 0.8.20; @@ -1546,22 +1502,6 @@ note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other versio │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20 - [FILE]:3:1 - │ -3 │ pragma solidity 0.8.20; - │ ━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - -note[pragma-inconsistent]: 'pragma solidity ^0.8.20;' conflicts with other version requirements in the project: 0.8.20 - [FILE]:3:1 - │ -3 │ pragma solidity ^0.8.20; - │ ━━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - "#] ]); @@ -1578,7 +1518,7 @@ forgetest!(pragma_inconsistent_files_without_pragma, |prj, cmd| { cmd.arg("lint").args(["--only-lint", "pragma-inconsistent"]).assert_success().stderr_eq(str![ [r#" -note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20 +note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: 0.8.20, ^0.8.20 [FILE]:3:1 │ 3 │ pragma solidity 0.8.20; @@ -1586,14 +1526,6 @@ note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other versio │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity ^0.8.20;' conflicts with other version requirements in the project: 0.8.20 - [FILE]:3:1 - │ -3 │ pragma solidity ^0.8.20; - │ ━━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - "#] ]); diff --git a/crates/lint/docs/incorrect-shift.md b/crates/lint/docs/incorrect-shift.md index a9a70c7f93128..ac0c7f2eee4bc 100644 --- a/crates/lint/docs/incorrect-shift.md +++ b/crates/lint/docs/incorrect-shift.md @@ -1,37 +1,39 @@ -# Incorrect shift order +# Incorrect Yul shift order **Severity**: `High` **ID**: `incorrect-shift` -Flags shift operations where a literal appears on the left and a non-literal on the right, which -is almost always the wrong operand order. +Flags Yul `shl` and `shr` calls whose operands appear to be reversed. ## What it does -Warns when the left-hand operand of `<<` or `>>` is a numeric literal and the right-hand operand -is a non-literal expression (e.g. a variable, function call, or composite expression). +Warns when the first argument to a Yul `shl` or `shr` call is dynamic and the second argument is a +literal. Yul shift calls take the shift amount first and the value second, so `shr(value, 8)` +shifts the literal `8` by `value`; the usual intended expression is `shr(8, value)`. ## Why is this bad? -Shift expressions like `2 << x` are usually a typo for `x << 2`. In the former, the *value being -shifted* is a tiny constant and the *shift amount* is dynamic — almost never the intended -behavior, and a known source of bugs in production contracts. +Yul's shift argument order is easy to confuse with high-level Solidity operators. Reversing the +arguments silently changes which value is shifted and can produce incorrect arithmetic, +bit-packing, or bounds logic. ## Example ### Bad ```solidity -result = 2 << stateValue; // shift amount comes from state -result = 8 >> localValue; // shift amount comes from a local -result = 16 << (stateValue + 1); // shift amount is a dynamic expression +assembly { + result := shl(value, 8) + result := shr(add(value, 1), 16) +} ``` ### Good ```solidity -result = stateValue << 2; -result = localValue >> 3; -result = stateValue << localShiftAmount; -result = 1 << 8; // both literals — fine +assembly { + result := shl(8, value) + result := shr(16, add(value, 1)) + result := shl(8, 1) // both literals +} ``` diff --git a/crates/lint/docs/pragma-inconsistent.md b/crates/lint/docs/pragma-inconsistent.md index 095f45783773d..3b22e3825407a 100644 --- a/crates/lint/docs/pragma-inconsistent.md +++ b/crates/lint/docs/pragma-inconsistent.md @@ -8,9 +8,9 @@ pragmas. ## What it does -Inspects every `pragma solidity ...;` directive across all input source files and reports when +Inspects every `pragma solidity ...;` directive across all input source files and reports once when their version requirements are inconsistent (different exact versions, mixed caret/tilde/range -shapes, etc.). +shapes, etc.). The diagnostic lists the distinct requirements seen in the project. ## Why is this bad? diff --git a/crates/lint/src/sol/analysis/interface.rs b/crates/lint/src/sol/analysis/interface.rs index 6a889821338d7..3eb513ec76d1b 100644 --- a/crates/lint/src/sol/analysis/interface.rs +++ b/crates/lint/src/sol/analysis/interface.rs @@ -10,7 +10,7 @@ pub fn is_elementary(hir: &hir::Hir<'_>, id: VariableId, abi: &str) -> bool { /// Static contract type of a method-call receiver: a contract-typed variable /// or an `IFoo(addr)` interface wrap. pub fn receiver_contract_id(hir: &hir::Hir<'_>, recv: &Expr<'_>) -> Option { - match &recv.kind { + match &recv.peel_parens().kind { ExprKind::Ident([Res::Item(ItemId::Variable(id)), ..]) => { if let TypeKind::Custom(ItemId::Contract(cid)) = hir.variable(*id).ty.kind { Some(cid) @@ -18,10 +18,14 @@ pub fn receiver_contract_id(hir: &hir::Hir<'_>, recv: &Expr<'_>) -> Option Some(*cid), + ExprKind::Call(callee, ..) => { + if let ExprKind::Ident([Res::Item(ItemId::Contract(cid))]) = &callee.peel_parens().kind + { + Some(*cid) + } else { + None + } + } _ => None, } } diff --git a/crates/lint/src/sol/high/incorrect_shift.rs b/crates/lint/src/sol/high/incorrect_shift.rs index 8e7fcbf868109..8f36ef4f46be8 100644 --- a/crates/lint/src/sol/high/incorrect_shift.rs +++ b/crates/lint/src/sol/high/incorrect_shift.rs @@ -3,7 +3,10 @@ use crate::{ linter::{EarlyLintPass, LintContext}, sol::{Severity, SolLint}, }; -use solar::ast::{BinOp, BinOpKind, Expr, ExprKind}; +use solar::{ + ast::{Stmt, StmtKind, yul}, + interface::kw, +}; declare_forge_lint!( INCORRECT_SHIFT, @@ -13,28 +16,62 @@ declare_forge_lint!( ); impl<'ast> EarlyLintPass<'ast> for IncorrectShift { - fn check_expr(&mut self, ctx: &LintContext, expr: &'ast Expr<'ast>) { - if let ExprKind::Binary( - left_expr, - BinOp { kind: BinOpKind::Shl | BinOpKind::Shr, .. }, - right_expr, - ) = &expr.kind - && contains_incorrect_shift(left_expr, right_expr) - { - ctx.emit(&INCORRECT_SHIFT, expr.span); + fn check_stmt(&mut self, ctx: &LintContext, stmt: &'ast Stmt<'ast>) { + if let StmtKind::Assembly(assembly) = &stmt.kind { + check_yul_block(ctx, &assembly.block); } } } -// TODO: come up with a better heuristic. Treat initial impl as a PoC. -// Checks if the left operand is a literal and the right operand is not, indicating a potential -// reversed shift operation. -const fn contains_incorrect_shift<'ast>( - left_expr: &'ast Expr<'ast>, - right_expr: &'ast Expr<'ast>, -) -> bool { - let is_left_literal = matches!(left_expr.kind, ExprKind::Lit(..)); - let is_right_not_literal = !matches!(right_expr.kind, ExprKind::Lit(..)); +fn check_yul_block(ctx: &LintContext, block: &yul::Block<'_>) { + for stmt in block.stmts.iter() { + check_yul_stmt(ctx, stmt); + } +} - is_left_literal && is_right_not_literal +fn check_yul_stmt(ctx: &LintContext, stmt: &yul::Stmt<'_>) { + match &stmt.kind { + yul::StmtKind::Block(block) => check_yul_block(ctx, block), + yul::StmtKind::AssignSingle(_, expr) + | yul::StmtKind::AssignMulti(_, expr) + | yul::StmtKind::Expr(expr) => check_yul_expr(ctx, expr), + yul::StmtKind::If(cond, block) => { + check_yul_expr(ctx, cond); + check_yul_block(ctx, block); + } + yul::StmtKind::For(for_stmt) => { + check_yul_block(ctx, &for_stmt.init); + check_yul_expr(ctx, &for_stmt.cond); + check_yul_block(ctx, &for_stmt.step); + check_yul_block(ctx, &for_stmt.body); + } + yul::StmtKind::Switch(switch) => { + check_yul_expr(ctx, &switch.selector); + for case in switch.cases.iter() { + check_yul_block(ctx, &case.body); + } + } + yul::StmtKind::FunctionDef(func) => check_yul_block(ctx, &func.body), + yul::StmtKind::VarDecl(_, Some(init)) => check_yul_expr(ctx, init), + yul::StmtKind::Leave + | yul::StmtKind::Break + | yul::StmtKind::Continue + | yul::StmtKind::VarDecl(_, None) => {} + } +} + +fn check_yul_expr(ctx: &LintContext, expr: &yul::Expr<'_>) { + let yul::ExprKind::Call(call) = &expr.kind else { return }; + + if matches!(call.name.name, kw::Shl | kw::Shr | kw::Sar) + && let [left, right] = call.arguments.as_ref() + && !matches!(left.kind, yul::ExprKind::Lit(_)) + && matches!(right.kind, yul::ExprKind::Lit(_)) + { + ctx.emit(&INCORRECT_SHIFT, expr.span); + } + + for arg in call.arguments.iter() { + check_yul_expr(ctx, arg); + } } diff --git a/crates/lint/src/sol/info/pragma_directive.rs b/crates/lint/src/sol/info/pragma_directive.rs index b66b6bcff6ade..7f30e15adad7f 100644 --- a/crates/lint/src/sol/info/pragma_directive.rs +++ b/crates/lint/src/sol/info/pragma_directive.rs @@ -40,18 +40,13 @@ impl<'ast> ProjectLintPass<'ast> for PragmaDirective { return; } - for (idx, span, req_str) in &entries { - let others = distinct - .iter() - .filter(|v| **v != req_str.as_str()) - .copied() - .collect::>() - .join(", "); - let msg = format!( - "'pragma solidity {req_str};' conflicts with other version requirements in the project: {others}" - ); - ctx.emit_with_msg(&sources[*idx], &PRAGMA_INCONSISTENT, *span, msg); - } + let (idx, span, _) = entries[0]; + let versions = distinct.join(", "); + let msg = format!( + "{} different Solidity pragma version requirements are used: {versions}", + distinct.len() + ); + ctx.emit_with_msg(&sources[idx], &PRAGMA_INCONSISTENT, span, msg); } } diff --git a/crates/lint/src/sol/info/too_many_digits.rs b/crates/lint/src/sol/info/too_many_digits.rs index 3ba9e8abba2de..8206090d46783 100644 --- a/crates/lint/src/sol/info/too_many_digits.rs +++ b/crates/lint/src/sol/info/too_many_digits.rs @@ -3,7 +3,7 @@ use crate::{ linter::{EarlyLintPass, LintContext}, sol::{Severity, SolLint}, }; -use solar::ast::{Expr, ExprKind, LitKind}; +use solar::ast::{Expr, ExprKind, Lit, LitKind, Stmt, StmtKind, yul}; declare_forge_lint!( TOO_MANY_DIGITS, @@ -14,37 +14,108 @@ declare_forge_lint!( ); impl<'ast> EarlyLintPass<'ast> for TooManyDigits { + fn check_stmt(&mut self, ctx: &LintContext, stmt: &'ast Stmt<'ast>) { + if let StmtKind::Assembly(assembly) = &stmt.kind { + check_yul_block(ctx, &assembly.block); + } + } + fn check_expr(&mut self, ctx: &LintContext, expr: &'ast Expr<'ast>) { let ExprKind::Lit(lit, sub_denom) = &expr.kind else { return }; + check_lit(ctx, lit, sub_denom.is_some()); + } +} - // Only plain integer literals. `LitKind::Address` (40-hex-digit address) is a - // distinct variant and is therefore skipped automatically. - if !matches!(lit.kind, LitKind::Number(_)) { - return; - } +fn check_lit(ctx: &LintContext, lit: &Lit<'_>, has_sub_denom: bool) { + // Only plain integer literals. `LitKind::Address` (40-hex-digit address) is a + // distinct variant and is therefore skipped automatically. + if !matches!(lit.kind, LitKind::Number(_)) { + return; + } - // Skip literals with a sub-denomination, e.g. `1000000 gwei`, `5 minutes`. - if sub_denom.is_some() { - return; - } + // Skip literals with a sub-denomination, e.g. `1000000 gwei`, `5 minutes`. + if has_sub_denom { + return; + } - let s = lit.symbol.as_str(); + let s = lit.symbol.as_str(); + let is_hex = is_hex_literal(s); - // Skip hex literals — long zero runs in hex are usually intentional (masks, - // selectors, bit patterns) and there is no scientific-notation alternative. - if s.starts_with("0x") || s.starts_with("0X") { - return; - } + // Match Slither's detector: skip only address-shaped hex constants, not all hex + // constants. Long padded masks/selectors are still hard to review. + if is_hex_address(s) { + return; + } - // Skip if the user already used scientific notation (`1e18`). - if s.contains('e') || s.contains('E') { - return; + // Skip if the user already used scientific notation (`1e18`). + if !is_hex && (s.contains('e') || s.contains('E')) { + return; + } + + // 5+ consecutive zeros in the literal as written. Underscores are + // preserved, so `1_000_000` passes while `1_000000` is flagged. + if s.contains("00000") { + ctx.emit(&TOO_MANY_DIGITS, lit.span); + } +} + +fn is_hex_literal(s: &str) -> bool { + s.starts_with("0x") || s.starts_with("0X") +} + +fn is_hex_address(s: &str) -> bool { + let Some(hex) = s.strip_prefix("0x").or_else(|| s.strip_prefix("0X")) else { return false }; + hex.len() == 40 && hex.bytes().all(|b| b.is_ascii_hexdigit()) +} + +fn check_yul_block(ctx: &LintContext, block: &yul::Block<'_>) { + for stmt in block.stmts.iter() { + check_yul_stmt(ctx, stmt); + } +} + +fn check_yul_stmt(ctx: &LintContext, stmt: &yul::Stmt<'_>) { + match &stmt.kind { + yul::StmtKind::Block(block) => check_yul_block(ctx, block), + yul::StmtKind::AssignSingle(_, expr) + | yul::StmtKind::AssignMulti(_, expr) + | yul::StmtKind::Expr(expr) => check_yul_expr(ctx, expr), + yul::StmtKind::If(cond, block) => { + check_yul_expr(ctx, cond); + check_yul_block(ctx, block); + } + yul::StmtKind::For(for_stmt) => { + check_yul_block(ctx, &for_stmt.init); + check_yul_expr(ctx, &for_stmt.cond); + check_yul_block(ctx, &for_stmt.step); + check_yul_block(ctx, &for_stmt.body); } + yul::StmtKind::Switch(switch) => { + check_yul_expr(ctx, &switch.selector); + for case in switch.cases.iter() { + if let Some(lit) = &case.constant { + check_lit(ctx, lit, false); + } + check_yul_block(ctx, &case.body); + } + } + yul::StmtKind::FunctionDef(func) => check_yul_block(ctx, &func.body), + yul::StmtKind::VarDecl(_, Some(init)) => check_yul_expr(ctx, init), + yul::StmtKind::Leave + | yul::StmtKind::Break + | yul::StmtKind::Continue + | yul::StmtKind::VarDecl(_, None) => {} + } +} - // 5+ consecutive zeros in the literal as written. Underscores are - // preserved, so `1_000_000` passes while `1_000000` is flagged. - if s.contains("00000") { - ctx.emit(&TOO_MANY_DIGITS, lit.span); +fn check_yul_expr(ctx: &LintContext, expr: &yul::Expr<'_>) { + match &expr.kind { + yul::ExprKind::Call(call) => { + for arg in call.arguments.iter() { + check_yul_expr(ctx, arg); + } } + yul::ExprKind::Lit(lit) => check_lit(ctx, lit, false), + yul::ExprKind::Path(_) => {} } } diff --git a/crates/lint/src/sol/low/block_timestamp.rs b/crates/lint/src/sol/low/block_timestamp.rs index b4ba3c7712217..3833a4051b8bd 100644 --- a/crates/lint/src/sol/low/block_timestamp.rs +++ b/crates/lint/src/sol/low/block_timestamp.rs @@ -1,9 +1,21 @@ use super::BlockTimestamp; use crate::{ - linter::{EarlyLintPass, LintContext}, + linter::{LateLintPass, LintContext}, sol::{Severity, SolLint}, }; -use solar::ast::{BinOp, BinOpKind, Expr, ExprKind}; +use solar::{ + ast, + interface::{kw, sym}, + sema::{ + Gcx, Hir, + builtins::Builtin, + hir::{ + BinOpKind, Block, ContractId, Expr, ExprKind, Function, FunctionId, ItemId, Res, Stmt, + StmtKind, VariableId, + }, + }, +}; +use std::collections::HashSet; declare_forge_lint!( BLOCK_TIMESTAMP, @@ -12,14 +24,242 @@ declare_forge_lint!( "usage of `block.timestamp` in a comparison may be manipulated by validators" ); -impl<'ast> EarlyLintPass<'ast> for BlockTimestamp { - fn check_expr(&mut self, ctx: &LintContext, expr: &'ast Expr<'ast>) { - if let ExprKind::Binary(lhs, BinOp { kind, .. }, rhs) = &expr.kind - && is_cmp(*kind) - && (contains_block_timestamp(lhs) || contains_block_timestamp(rhs)) - { - ctx.emit(&BLOCK_TIMESTAMP, expr.span); +impl<'hir> LateLintPass<'hir> for BlockTimestamp { + fn check_function( + &mut self, + ctx: &LintContext, + _gcx: Gcx<'hir>, + hir: &'hir Hir<'hir>, + func: &'hir Function<'hir>, + ) { + if let Some(body) = func.body { + let helpers = timestamp_helpers(hir, func.contract); + let mut aliases = HashSet::new(); + check_block(ctx, hir, &helpers, body, &mut aliases); + } + } +} + +fn check_block<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + helpers: &HashSet, + block: Block<'hir>, + aliases: &mut HashSet, +) -> bool { + for stmt in block.stmts { + if !check_stmt(ctx, hir, helpers, stmt, aliases) { + return false; + } + } + true +} + +fn check_stmt<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + helpers: &HashSet, + stmt: &'hir Stmt<'hir>, + aliases: &mut HashSet, +) -> bool { + match &stmt.kind { + StmtKind::DeclSingle(var_id) => { + if let Some(init) = hir.variable(*var_id).initializer { + check_expr(ctx, hir, helpers, init, aliases); + update_alias( + hir, + *var_id, + expr_value_is_timestamp_source(helpers, init, aliases), + aliases, + ); + } + true + } + StmtKind::DeclMulti(vars, expr) => { + check_expr(ctx, hir, helpers, expr, aliases); + update_multi_aliases(hir, helpers, vars, expr, aliases); + true + } + StmtKind::Expr(expr) => { + check_expr(ctx, hir, helpers, expr, aliases); + !is_revert_call(expr) + } + StmtKind::Emit(expr) => { + check_expr(ctx, hir, helpers, expr, aliases); + true + } + StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { + check_expr(ctx, hir, helpers, expr, aliases); + false + } + StmtKind::If(cond, then_stmt, else_stmt) => { + check_expr(ctx, hir, helpers, cond, aliases); + + let baseline = aliases.clone(); + let mut merged = HashSet::new(); + let mut falls_through = false; + + let mut then_aliases = baseline.clone(); + if check_stmt(ctx, hir, helpers, then_stmt, &mut then_aliases) { + merged.extend(then_aliases); + falls_through = true; + } + + if let Some(else_stmt) = else_stmt { + let mut else_aliases = baseline; + if check_stmt(ctx, hir, helpers, else_stmt, &mut else_aliases) { + merged.extend(else_aliases); + falls_through = true; + } + } else { + merged.extend(baseline); + falls_through = true; + } + + if falls_through { + *aliases = merged; + } + falls_through + } + StmtKind::Loop(block, _) => { + let baseline = aliases.clone(); + let mut loop_aliases = baseline.clone(); + *aliases = if check_block(ctx, hir, helpers, *block, &mut loop_aliases) { + baseline.union(&loop_aliases).copied().collect() + } else { + baseline + }; + true + } + StmtKind::Try(try_stmt) => { + check_expr(ctx, hir, helpers, &try_stmt.expr, aliases); + + let baseline = aliases.clone(); + let mut merged = baseline.clone(); + for clause in try_stmt.clauses { + let mut clause_aliases = baseline.clone(); + if check_block(ctx, hir, helpers, clause.block, &mut clause_aliases) { + merged.extend(clause_aliases); + } + } + *aliases = merged; + true + } + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { + check_block(ctx, hir, helpers, *block, aliases) + } + StmtKind::AssemblyBlock(block) => check_block(ctx, hir, helpers, *block, aliases), + StmtKind::Switch(switch) => { + check_expr(ctx, hir, helpers, switch.selector, aliases); + + let baseline = aliases.clone(); + let mut merged = baseline.clone(); + for case in switch.cases { + let mut case_aliases = baseline.clone(); + if check_block(ctx, hir, helpers, case.body, &mut case_aliases) { + merged.extend(case_aliases); + } + } + *aliases = merged; + true + } + StmtKind::Return(None) => false, + StmtKind::Break | StmtKind::Continue | StmtKind::Placeholder | StmtKind::Err(_) => true, + } +} + +fn check_expr<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + helpers: &HashSet, + expr: &'hir Expr<'hir>, + aliases: &mut HashSet, +) { + match &expr.peel_parens().kind { + ExprKind::Assign(lhs, op, rhs) => { + check_expr(ctx, hir, helpers, rhs, aliases); + let rhs_is_timestamp = expr_value_is_timestamp_source(helpers, rhs, aliases); + + if op.is_some() { + check_expr(ctx, hir, helpers, lhs, aliases); + update_lhs_aliases( + hir, + lhs, + rhs_is_timestamp || expr_value_is_timestamp_source(helpers, lhs, aliases), + aliases, + ); + } else { + update_assignment_aliases(hir, helpers, lhs, rhs, aliases); + } + } + ExprKind::Binary(lhs, op, rhs) => { + if is_cmp(op.kind) + && (expr_contains_timestamp_source(helpers, lhs, aliases) + || expr_contains_timestamp_source(helpers, rhs, aliases)) + { + ctx.emit(&BLOCK_TIMESTAMP, expr.span); + } + + check_expr(ctx, hir, helpers, lhs, aliases); + check_expr(ctx, hir, helpers, rhs, aliases); } + ExprKind::Call(callee, args, options) => { + check_expr(ctx, hir, helpers, callee, aliases); + if let Some(options) = options { + for arg in options.args { + check_expr(ctx, hir, helpers, &arg.value, aliases); + } + } + for arg in args.exprs() { + check_expr(ctx, hir, helpers, arg, aliases); + } + } + ExprKind::Unary(_, inner) + | ExprKind::Delete(inner) + | ExprKind::Member(inner, _) + | ExprKind::Payable(inner) + | ExprKind::YulMember(inner, _) => check_expr(ctx, hir, helpers, inner, aliases), + ExprKind::Ternary(cond, then_expr, else_expr) => { + check_expr(ctx, hir, helpers, cond, aliases); + + let baseline = aliases.clone(); + let mut then_aliases = baseline.clone(); + check_expr(ctx, hir, helpers, then_expr, &mut then_aliases); + let mut else_aliases = baseline; + check_expr(ctx, hir, helpers, else_expr, &mut else_aliases); + *aliases = then_aliases.union(&else_aliases).copied().collect(); + } + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().flatten() { + check_expr(ctx, hir, helpers, expr, aliases); + } + } + ExprKind::Array(exprs) => { + for expr in *exprs { + check_expr(ctx, hir, helpers, expr, aliases); + } + } + ExprKind::Index(base, index) => { + check_expr(ctx, hir, helpers, base, aliases); + if let Some(index) = index { + check_expr(ctx, hir, helpers, index, aliases); + } + } + ExprKind::Slice(base, start, end) => { + check_expr(ctx, hir, helpers, base, aliases); + if let Some(start) = start { + check_expr(ctx, hir, helpers, start, aliases); + } + if let Some(end) = end { + check_expr(ctx, hir, helpers, end, aliases); + } + } + ExprKind::Ident(_) + | ExprKind::Lit(_) + | ExprKind::New(_) + | ExprKind::TypeCall(_) + | ExprKind::Type(_) + | ExprKind::Err(_) => {} } } @@ -35,36 +275,305 @@ const fn is_cmp(kind: BinOpKind) -> bool { ) } -/// Returns `true` if `expr` is `block.timestamp`. +fn update_multi_aliases( + hir: &Hir<'_>, + helpers: &HashSet, + vars: &[Option], + expr: &Expr<'_>, + aliases: &mut HashSet, +) { + if let ExprKind::Tuple(exprs) = &expr.peel_parens().kind + && exprs.len() == vars.len() + { + let rhs_aliases: Vec<_> = exprs + .iter() + .map(|expr| { + expr.is_some_and(|expr| expr_value_is_timestamp_source(helpers, expr, aliases)) + }) + .collect(); + for (var_id, rhs_is_timestamp) in vars.iter().zip(rhs_aliases) { + if let Some(var_id) = var_id { + update_alias(hir, *var_id, rhs_is_timestamp, aliases); + } + } + return; + } + + let rhs_is_timestamp = expr_value_is_timestamp_source(helpers, expr, aliases); + for var_id in vars.iter().flatten() { + update_alias(hir, *var_id, rhs_is_timestamp, aliases); + } +} + +fn update_assignment_aliases( + hir: &Hir<'_>, + helpers: &HashSet, + lhs: &Expr<'_>, + rhs: &Expr<'_>, + aliases: &mut HashSet, +) { + if let (ExprKind::Tuple(lhs_exprs), ExprKind::Tuple(rhs_exprs)) = + (&lhs.peel_parens().kind, &rhs.peel_parens().kind) + && lhs_exprs.len() == rhs_exprs.len() + { + let rhs_aliases: Vec<_> = rhs_exprs + .iter() + .map(|rhs| rhs.is_some_and(|rhs| expr_value_is_timestamp_source(helpers, rhs, aliases))) + .collect(); + for (lhs, rhs_is_timestamp) in lhs_exprs.iter().zip(rhs_aliases) { + if let Some(lhs) = lhs { + update_lhs_aliases(hir, lhs, rhs_is_timestamp, aliases); + } + } + return; + } + + let rhs_is_timestamp = expr_value_is_timestamp_source(helpers, rhs, aliases); + update_lhs_aliases(hir, lhs, rhs_is_timestamp, aliases); +} + +fn update_lhs_aliases( + hir: &Hir<'_>, + lhs: &Expr<'_>, + is_timestamp: bool, + aliases: &mut HashSet, +) { + match &lhs.peel_parens().kind { + ExprKind::Ident(resolutions) => { + for res in *resolutions { + if let Res::Item(ItemId::Variable(var_id)) = res { + update_alias(hir, *var_id, is_timestamp, aliases); + } + } + } + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().flatten() { + update_lhs_aliases(hir, expr, is_timestamp, aliases); + } + } + _ => {} + } +} + +fn update_alias( + hir: &Hir<'_>, + var_id: VariableId, + is_timestamp: bool, + aliases: &mut HashSet, +) { + if !hir.variable(var_id).is_local_or_return() { + return; + } + if is_timestamp { + aliases.insert(var_id); + } else { + aliases.remove(&var_id); + } +} + +fn expr_contains_timestamp_source( + helpers: &HashSet, + expr: &Expr<'_>, + aliases: &HashSet, +) -> bool { + if expr_value_is_timestamp_source(helpers, expr, aliases) { + return true; + } + + match &expr.peel_parens().kind { + ExprKind::Assign(lhs, _, rhs) | ExprKind::Binary(lhs, _, rhs) => { + expr_contains_timestamp_source(helpers, lhs, aliases) + || expr_contains_timestamp_source(helpers, rhs, aliases) + } + ExprKind::Call(callee, args, options) => { + expr_contains_timestamp_source(helpers, callee, aliases) + || options.is_some_and(|options| { + options + .args + .iter() + .any(|arg| expr_contains_timestamp_source(helpers, &arg.value, aliases)) + }) + || args.exprs().any(|arg| expr_contains_timestamp_source(helpers, arg, aliases)) + } + ExprKind::Unary(_, inner) + | ExprKind::Delete(inner) + | ExprKind::Member(inner, _) + | ExprKind::Payable(inner) + | ExprKind::YulMember(inner, _) => expr_contains_timestamp_source(helpers, inner, aliases), + ExprKind::Ternary(cond, then_expr, else_expr) => { + expr_contains_timestamp_source(helpers, cond, aliases) + || expr_contains_timestamp_source(helpers, then_expr, aliases) + || expr_contains_timestamp_source(helpers, else_expr, aliases) + } + ExprKind::Tuple(exprs) => exprs + .iter() + .flatten() + .any(|expr| expr_contains_timestamp_source(helpers, expr, aliases)), + ExprKind::Array(exprs) => { + exprs.iter().any(|expr| expr_contains_timestamp_source(helpers, expr, aliases)) + } + ExprKind::Index(base, index) => { + expr_contains_timestamp_source(helpers, base, aliases) + || index + .is_some_and(|index| expr_contains_timestamp_source(helpers, index, aliases)) + } + ExprKind::Slice(base, start, end) => { + expr_contains_timestamp_source(helpers, base, aliases) + || start + .is_some_and(|start| expr_contains_timestamp_source(helpers, start, aliases)) + || end.is_some_and(|end| expr_contains_timestamp_source(helpers, end, aliases)) + } + ExprKind::Ident(_) + | ExprKind::Lit(_) + | ExprKind::New(_) + | ExprKind::TypeCall(_) + | ExprKind::Type(_) + | ExprKind::Err(_) => false, + } +} + +fn expr_value_is_timestamp_source( + helpers: &HashSet, + expr: &Expr<'_>, + aliases: &HashSet, +) -> bool { + if is_block_timestamp(expr) || is_timestamp_helper_call(helpers, expr) { + return true; + } + + match &expr.peel_parens().kind { + ExprKind::Ident(resolutions) => resolutions.iter().any( + |res| matches!(res, Res::Item(ItemId::Variable(var_id)) if aliases.contains(var_id)), + ), + ExprKind::Binary(lhs, op, rhs) if !is_cmp(op.kind) => { + expr_value_is_timestamp_source(helpers, lhs, aliases) + || expr_value_is_timestamp_source(helpers, rhs, aliases) + } + ExprKind::Unary(_, inner) | ExprKind::Payable(inner) | ExprKind::YulMember(inner, _) => { + expr_value_is_timestamp_source(helpers, inner, aliases) + } + ExprKind::Ternary(_, then_expr, else_expr) => { + expr_value_is_timestamp_source(helpers, then_expr, aliases) + || expr_value_is_timestamp_source(helpers, else_expr, aliases) + } + ExprKind::Tuple([Some(inner)]) => expr_value_is_timestamp_source(helpers, inner, aliases), + _ => false, + } +} + fn is_block_timestamp(expr: &Expr<'_>) -> bool { - matches!( - &expr.kind, - ExprKind::Member(base, member) - if member.as_str() == "timestamp" - && matches!(&base.kind, ExprKind::Ident(ident) if ident.as_str() == "block") - ) + match &expr.peel_parens().kind { + ExprKind::Member(base, member) if member.name == kw::Timestamp => { + let ExprKind::Ident(reses) = &base.peel_parens().kind else { return false }; + reses + .iter() + .any(|res| matches!(res, Res::Builtin(builtin) if builtin.name() == sym::block)) + } + ExprKind::Ident(reses) => { + reses.iter().any(|res| matches!(res, Res::Builtin(Builtin::BlockTimestamp))) + } + _ => false, + } +} + +fn timestamp_helpers(hir: &Hir<'_>, contract: Option) -> HashSet { + let Some(contract) = contract else { return HashSet::new() }; + hir.contract_item_ids(contract) + .filter_map(|item| item.as_function()) + .filter(|fid| { + let helper = hir.function(*fid); + helper.contract == Some(contract) + && matches!(helper.visibility, ast::Visibility::Internal | ast::Visibility::Private) + && helper.body.is_some_and(block_directly_returns_timestamp) + }) + .collect() +} + +fn is_timestamp_helper_call(helpers: &HashSet, expr: &Expr<'_>) -> bool { + let ExprKind::Call(callee, _, _) = &expr.peel_parens().kind else { return false }; + helper_function_ids(callee).into_iter().any(|fid| helpers.contains(&fid)) +} + +fn is_revert_call(expr: &Expr<'_>) -> bool { + let ExprKind::Call(callee, _, _) = &expr.peel_parens().kind else { return false }; + let ExprKind::Ident(resolutions) = &callee.peel_parens().kind else { return false }; + resolutions.iter().any(|res| matches!(res, Res::Builtin(Builtin::Revert | Builtin::RevertMsg))) +} + +fn helper_function_ids(callee: &Expr<'_>) -> Vec { + let ExprKind::Ident(resolutions) = &callee.peel_parens().kind else { return Vec::new() }; + resolutions + .iter() + .filter_map( + |res| { + if let Res::Item(ItemId::Function(fid)) = res { Some(*fid) } else { None } + }, + ) + .collect() } -/// Recursively checks if an expression tree contains `block.timestamp`. -fn contains_block_timestamp(expr: &Expr<'_>) -> bool { +fn block_directly_returns_timestamp(block: Block<'_>) -> bool { + block.stmts.iter().any(stmt_directly_returns_timestamp) +} + +fn stmt_directly_returns_timestamp(stmt: &Stmt<'_>) -> bool { + match &stmt.kind { + StmtKind::Return(Some(expr)) => expr_contains_direct_block_timestamp(expr), + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { + block_directly_returns_timestamp(*block) + } + StmtKind::If(_, then_stmt, else_stmt) => { + stmt_directly_returns_timestamp(then_stmt) + || else_stmt.is_some_and(stmt_directly_returns_timestamp) + } + _ => false, + } +} + +fn expr_contains_direct_block_timestamp(expr: &Expr<'_>) -> bool { if is_block_timestamp(expr) { return true; } - match &expr.kind { - ExprKind::Unary(_, inner) => contains_block_timestamp(inner), - ExprKind::Binary(lhs, _, rhs) => { - contains_block_timestamp(lhs) || contains_block_timestamp(rhs) + + match &expr.peel_parens().kind { + ExprKind::Assign(lhs, _, rhs) | ExprKind::Binary(lhs, _, rhs) => { + expr_contains_direct_block_timestamp(lhs) || expr_contains_direct_block_timestamp(rhs) } - ExprKind::Tuple(elems) => elems.iter().any(|e| { - if let solar::interface::SpannedOption::Some(inner) = e.as_ref() { - contains_block_timestamp(inner) - } else { - false - } - }), - ExprKind::Call(callee, args) => { - contains_block_timestamp(callee) || args.exprs().any(|e| contains_block_timestamp(e)) + ExprKind::Call(callee, args, options) => { + expr_contains_direct_block_timestamp(callee) + || options.is_some_and(|options| { + options.args.iter().any(|arg| expr_contains_direct_block_timestamp(&arg.value)) + }) + || args.exprs().any(expr_contains_direct_block_timestamp) } - _ => false, + ExprKind::Unary(_, inner) + | ExprKind::Delete(inner) + | ExprKind::Member(inner, _) + | ExprKind::Payable(inner) + | ExprKind::YulMember(inner, _) => expr_contains_direct_block_timestamp(inner), + ExprKind::Ternary(cond, then_expr, else_expr) => { + expr_contains_direct_block_timestamp(cond) + || expr_contains_direct_block_timestamp(then_expr) + || expr_contains_direct_block_timestamp(else_expr) + } + ExprKind::Tuple(exprs) => { + exprs.iter().flatten().any(|expr| expr_contains_direct_block_timestamp(expr)) + } + ExprKind::Array(exprs) => exprs.iter().any(expr_contains_direct_block_timestamp), + ExprKind::Index(base, index) => { + expr_contains_direct_block_timestamp(base) + || index.is_some_and(expr_contains_direct_block_timestamp) + } + ExprKind::Slice(base, start, end) => { + expr_contains_direct_block_timestamp(base) + || start.is_some_and(expr_contains_direct_block_timestamp) + || end.is_some_and(expr_contains_direct_block_timestamp) + } + ExprKind::Ident(_) + | ExprKind::Lit(_) + | ExprKind::New(_) + | ExprKind::TypeCall(_) + | ExprKind::Type(_) + | ExprKind::Err(_) => false, } } diff --git a/crates/lint/src/sol/low/mod.rs b/crates/lint/src/sol/low/mod.rs index 344e741296628..ec99565a73e56 100644 --- a/crates/lint/src/sol/low/mod.rs +++ b/crates/lint/src/sol/low/mod.rs @@ -30,7 +30,7 @@ mod reentrancy_events; use reentrancy_events::REENTRANCY_EVENTS; register_lints!( - (BlockTimestamp, early, (BLOCK_TIMESTAMP)), + (BlockTimestamp, late, (BLOCK_TIMESTAMP)), (CallsLoop, late, (CALLS_LOOP)), (DelegatecallLoop, late, (DELEGATECALL_LOOP)), (MsgValueLoop, late, (MSG_VALUE_LOOP)), diff --git a/crates/lint/src/sol/med/div_mul.rs b/crates/lint/src/sol/med/div_mul.rs index a9be610d6e4c3..21e6ea3cd9f42 100644 --- a/crates/lint/src/sol/med/div_mul.rs +++ b/crates/lint/src/sol/med/div_mul.rs @@ -1,12 +1,19 @@ use super::DivideBeforeMultiply; use crate::{ - linter::{EarlyLintPass, LintContext}, + linter::{LateLintPass, LintContext}, sol::{Severity, SolLint}, }; use solar::{ - ast::{BinOp, BinOpKind, Expr, ExprKind}, - interface::SpannedOption, + ast::UnOpKind, + sema::{ + Gcx, Hir, + builtins::Builtin, + hir::{ + BinOpKind, Block, Expr, ExprKind, Function, ItemId, Res, Stmt, StmtKind, VariableId, + }, + }, }; +use std::collections::HashSet; declare_forge_lint!( DIVIDE_BEFORE_MULTIPLY, @@ -15,26 +22,408 @@ declare_forge_lint!( "multiplication should occur before division to avoid loss of precision" ); -impl<'ast> EarlyLintPass<'ast> for DivideBeforeMultiply { - fn check_expr(&mut self, ctx: &LintContext, expr: &'ast Expr<'ast>) { - if let ExprKind::Binary(left_expr, BinOp { kind: BinOpKind::Mul, .. }, _) = &expr.kind - && contains_division(left_expr) - { - ctx.emit(&DIVIDE_BEFORE_MULTIPLY, expr.span); +impl<'hir> LateLintPass<'hir> for DivideBeforeMultiply { + fn check_function( + &mut self, + ctx: &LintContext, + _gcx: Gcx<'hir>, + hir: &'hir Hir<'hir>, + func: &'hir Function<'hir>, + ) { + if let Some(body) = func.body { + let mut tainted = HashSet::default(); + check_block(ctx, hir, body, &mut tainted); } } } -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 SpannedOption::Some(inner_expr) = opt_expr.as_ref() { - contains_division(inner_expr) +fn check_block<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + block: Block<'hir>, + tainted: &mut HashSet, +) -> bool { + for stmt in block.stmts { + if !check_stmt(ctx, hir, stmt, tainted) { + return false; + } + } + true +} + +fn check_stmt<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + stmt: &'hir Stmt<'hir>, + tainted: &mut HashSet, +) -> bool { + match &stmt.kind { + StmtKind::DeclSingle(var_id) => { + if let Some(init) = hir.variable(*var_id).initializer { + check_expr(ctx, hir, init, tainted); + update_taint( + hir, + *var_id, + expr_value_is_division_or_tainted(init, tainted), + tainted, + ); + } + true + } + StmtKind::DeclMulti(vars, expr) => { + check_expr(ctx, hir, expr, tainted); + update_multi_decl_taint(hir, vars, expr, tainted); + true + } + StmtKind::Expr(expr) => { + check_expr(ctx, hir, expr, tainted); + !is_revert_call(expr) + } + StmtKind::Emit(expr) => { + check_expr(ctx, hir, expr, tainted); + true + } + StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { + check_expr(ctx, hir, expr, tainted); + false + } + StmtKind::If(cond, then_stmt, else_stmt) => { + check_expr(ctx, hir, cond, tainted); + + let baseline = tainted.clone(); + let mut merged_taint = HashSet::default(); + let mut falls_through = false; + + let mut then_tainted = baseline.clone(); + if check_stmt(ctx, hir, then_stmt, &mut then_tainted) { + merged_taint = union_taints(&merged_taint, &then_tainted); + falls_through = true; + } + + if let Some(else_stmt) = else_stmt { + let mut else_tainted = baseline; + if check_stmt(ctx, hir, else_stmt, &mut else_tainted) { + merged_taint = union_taints(&merged_taint, &else_tainted); + falls_through = true; + } } else { - false + merged_taint = union_taints(&merged_taint, &baseline); + falls_through = true; + } + + if falls_through { + *tainted = merged_taint; + } + falls_through + } + StmtKind::Loop(block, _) => { + let baseline = tainted.clone(); + let mut loop_tainted = baseline.clone(); + *tainted = if check_block(ctx, hir, *block, &mut loop_tainted) { + union_taints(&baseline, &loop_tainted) + } else { + baseline + }; + true + } + StmtKind::Try(try_stmt) => { + check_expr(ctx, hir, &try_stmt.expr, tainted); + let mut merged_taint = tainted.clone(); + for clause in try_stmt.clauses { + let mut clause_tainted = tainted.clone(); + if check_block(ctx, hir, clause.block, &mut clause_tainted) { + merged_taint = union_taints(&merged_taint, &clause_tainted); + } + } + *tainted = merged_taint; + true + } + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { + check_block(ctx, hir, *block, tainted) + } + StmtKind::AssemblyBlock(block) => check_block(ctx, hir, *block, tainted), + StmtKind::Switch(switch) => { + check_expr(ctx, hir, switch.selector, tainted); + let mut merged_taint = tainted.clone(); + for case in switch.cases { + let mut case_tainted = tainted.clone(); + if check_block(ctx, hir, case.body, &mut case_tainted) { + merged_taint = union_taints(&merged_taint, &case_tainted); + } + } + *tainted = merged_taint; + true + } + StmtKind::Return(None) => false, + StmtKind::Break | StmtKind::Continue | StmtKind::Placeholder | StmtKind::Err(_) => true, + } +} + +fn check_expr<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + expr: &'hir Expr<'hir>, + tainted: &mut HashSet, +) { + match &expr.peel_parens().kind { + ExprKind::Assign(lhs, op, rhs) => { + check_expr(ctx, hir, rhs, tainted); + check_expr(ctx, hir, lhs, tainted); + + match op { + None => { + update_assignment_taint(hir, lhs, rhs, tainted); + } + Some(op) if op.kind == BinOpKind::Mul => { + let lhs_tainted = expr_is_division_result_or_tainted(lhs, tainted); + let rhs_tainted = expr_is_division_result_or_tainted(rhs, tainted); + if lhs_tainted || rhs_tainted { + ctx.emit(&DIVIDE_BEFORE_MULTIPLY, expr.span); + } + update_lhs_taint(hir, lhs, lhs_tainted || rhs_tainted, tainted); + } + Some(op) if op.kind == BinOpKind::Div => { + update_lhs_taint(hir, lhs, true, tainted); + } + Some(_) => update_lhs_taint(hir, lhs, false, tainted), + } + } + ExprKind::Binary(left, op, right) => { + check_expr(ctx, hir, left, tainted); + check_expr(ctx, hir, right, tainted); + + if op.kind == BinOpKind::Mul + && (expr_is_division_result_or_tainted(left, tainted) + || expr_is_division_result_or_tainted(right, tainted)) + { + ctx.emit(&DIVIDE_BEFORE_MULTIPLY, expr.span); + } + } + ExprKind::Array(exprs) => { + for expr in *exprs { + check_expr(ctx, hir, expr, tainted); + } + } + ExprKind::Call(callee, args, named_args) => { + check_expr(ctx, hir, callee, tainted); + for arg in args.exprs() { + check_expr(ctx, hir, arg, tainted); + } + if let Some(named_args) = named_args { + for arg in named_args.args { + check_expr(ctx, hir, &arg.value, tainted); + } + } + + if is_yul_multiplication_call(expr) + && args.exprs().any(|arg| expr_is_division_result_or_tainted(arg, tainted)) + { + ctx.emit(&DIVIDE_BEFORE_MULTIPLY, expr.span); + } + } + ExprKind::Delete(inner) + | ExprKind::Index(inner, None) + | ExprKind::Member(inner, _) + | ExprKind::Payable(inner) => check_expr(ctx, hir, inner, tainted), + ExprKind::Index(base, Some(index)) => { + check_expr(ctx, hir, base, tainted); + check_expr(ctx, hir, index, tainted); + } + ExprKind::Slice(base, start, end) => { + check_expr(ctx, hir, base, tainted); + if let Some(start) = start { + check_expr(ctx, hir, start, tainted); + } + if let Some(end) = end { + check_expr(ctx, hir, end, tainted); + } + } + ExprKind::Ternary(cond, then_expr, else_expr) => { + check_expr(ctx, hir, cond, tainted); + let mut then_tainted = tainted.clone(); + check_expr(ctx, hir, then_expr, &mut then_tainted); + let mut else_tainted = tainted.clone(); + check_expr(ctx, hir, else_expr, &mut else_tainted); + *tainted = union_taints(&then_tainted, &else_tainted); + } + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().flatten() { + check_expr(ctx, hir, expr, tainted); } - }), + } + ExprKind::Unary(op, inner) => { + check_expr(ctx, hir, inner, tainted); + if is_inc_dec_op(op.kind) { + update_lhs_taint(hir, inner, false, tainted); + } + } + ExprKind::Ident(_) + | ExprKind::Lit(_) + | ExprKind::New(_) + | ExprKind::TypeCall(_) + | ExprKind::Type(_) => {} + ExprKind::YulMember(inner, _) => check_expr(ctx, hir, inner, tainted), + ExprKind::Err(_) => {} + } +} + +fn update_multi_decl_taint( + hir: &Hir<'_>, + vars: &[Option], + expr: &Expr<'_>, + tainted: &mut HashSet, +) { + if let ExprKind::Tuple(exprs) = &expr.peel_parens().kind + && exprs.len() == vars.len() + { + let rhs_taints: Vec<_> = exprs + .iter() + .map(|expr| expr.is_some_and(|expr| expr_value_is_division_or_tainted(expr, tainted))) + .collect(); + for (var_id, rhs_tainted) in vars.iter().zip(rhs_taints) { + if let Some(var_id) = var_id { + update_taint(hir, *var_id, rhs_tainted, tainted); + } + } + return; + } + + let rhs_tainted = expr_value_is_division_or_tainted(expr, tainted); + for var_id in vars.iter().flatten() { + update_taint(hir, *var_id, rhs_tainted, tainted); + } +} + +fn update_assignment_taint( + hir: &Hir<'_>, + lhs: &Expr<'_>, + rhs: &Expr<'_>, + tainted: &mut HashSet, +) { + if let (ExprKind::Tuple(lhs_exprs), ExprKind::Tuple(rhs_exprs)) = + (&lhs.peel_parens().kind, &rhs.peel_parens().kind) + && lhs_exprs.len() == rhs_exprs.len() + { + let rhs_taints: Vec<_> = rhs_exprs + .iter() + .map(|rhs| rhs.is_some_and(|rhs| expr_value_is_division_or_tainted(rhs, tainted))) + .collect(); + for (lhs, rhs_tainted) in lhs_exprs.iter().zip(rhs_taints) { + if let Some(lhs) = lhs { + update_lhs_taint(hir, lhs, rhs_tainted, tainted); + } + } + return; + } + + update_lhs_taint(hir, lhs, expr_value_is_division_or_tainted(rhs, tainted), tainted); +} + +fn union_taints(left: &HashSet, right: &HashSet) -> HashSet { + left.union(right).copied().collect() +} + +fn update_lhs_taint( + hir: &Hir<'_>, + lhs: &Expr<'_>, + is_tainted: bool, + tainted: &mut HashSet, +) { + match &lhs.peel_parens().kind { + ExprKind::Ident(resolutions) => { + for res in *resolutions { + if let Res::Item(ItemId::Variable(var_id)) = res { + update_taint(hir, *var_id, is_tainted, tainted); + } + } + } + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().flatten() { + update_lhs_taint(hir, expr, is_tainted, tainted); + } + } + _ => {} + } +} + +fn update_taint( + hir: &Hir<'_>, + var_id: VariableId, + is_tainted: bool, + tainted: &mut HashSet, +) { + if !hir.variable(var_id).is_local_or_return() { + return; + } + if is_tainted { + tainted.insert(var_id); + } else { + tainted.remove(&var_id); + } +} + +fn expr_value_is_division_or_tainted(expr: &Expr<'_>, tainted: &HashSet) -> bool { + match &expr.peel_parens().kind { + ExprKind::Binary(_, op, _) => op.kind == BinOpKind::Div, + ExprKind::Ident(resolutions) => resolutions.iter().any( + |res| matches!(res, Res::Item(ItemId::Variable(var_id)) if tainted.contains(var_id)), + ), + ExprKind::Call(..) => is_yul_division_call(expr), + ExprKind::Tuple([Some(inner)]) => expr_value_is_division_or_tainted(inner, tainted), + ExprKind::YulMember(inner, _) => expr_value_is_division_or_tainted(inner, tainted), + ExprKind::Array(_) + | ExprKind::Assign(..) + | ExprKind::Delete(_) + | ExprKind::Index(..) + | ExprKind::Lit(_) + | ExprKind::Member(_, _) + | ExprKind::New(_) + | ExprKind::Payable(_) + | ExprKind::Slice(..) + | ExprKind::Ternary(..) + | ExprKind::TypeCall(_) + | ExprKind::Type(_) + | ExprKind::Unary(_, _) + | ExprKind::Tuple(_) => false, + ExprKind::Err(_) => false, + } +} + +fn expr_is_division_result_or_tainted(expr: &Expr<'_>, tainted: &HashSet) -> bool { + match &expr.peel_parens().kind { + ExprKind::Binary(_, op, _) => op.kind == BinOpKind::Div, + ExprKind::Call(..) => is_yul_division_call(expr), + ExprKind::Ident(resolutions) => resolutions.iter().any( + |res| matches!(res, Res::Item(ItemId::Variable(var_id)) if tainted.contains(var_id)), + ), + ExprKind::Tuple([Some(inner)]) => expr_is_division_result_or_tainted(inner, tainted), _ => false, } } + +fn is_yul_division_call(expr: &Expr<'_>) -> bool { + is_yul_builtin_call(expr, |builtin| matches!(builtin, Builtin::YulDiv | Builtin::YulSdiv)) +} + +fn is_yul_multiplication_call(expr: &Expr<'_>) -> bool { + is_yul_builtin_call(expr, |builtin| matches!(builtin, Builtin::YulMul)) +} + +fn is_revert_call(expr: &Expr<'_>) -> bool { + let ExprKind::Call(callee, _, _) = &expr.peel_parens().kind else { return false }; + let ExprKind::Ident(resolutions) = &callee.peel_parens().kind else { return false }; + resolutions.iter().any(|res| matches!(res, Res::Builtin(Builtin::Revert | Builtin::RevertMsg))) +} + +const fn is_inc_dec_op(kind: UnOpKind) -> bool { + matches!(kind, UnOpKind::PreInc | UnOpKind::PostInc | UnOpKind::PreDec | UnOpKind::PostDec) +} + +fn is_yul_builtin_call(expr: &Expr<'_>, predicate: impl Fn(Builtin) -> bool) -> bool { + let ExprKind::Call(callee, args, _) = &expr.peel_parens().kind else { return false }; + if args.len() != 2 { + return false; + } + let ExprKind::Ident(resolutions) = &callee.peel_parens().kind else { return false }; + resolutions.iter().any(|res| matches!(res, Res::Builtin(builtin) if predicate(*builtin))) +} diff --git a/crates/lint/src/sol/med/mod.rs b/crates/lint/src/sol/med/mod.rs index e2ae70e39b717..7376e3c2ca8e5 100644 --- a/crates/lint/src/sol/med/mod.rs +++ b/crates/lint/src/sol/med/mod.rs @@ -41,7 +41,7 @@ use weak_prng::WEAK_PRNG; register_lints!( (AssertStateChange, late, (ASSERT_STATE_CHANGE)), - (DivideBeforeMultiply, early, (DIVIDE_BEFORE_MULTIPLY)), + (DivideBeforeMultiply, late, (DIVIDE_BEFORE_MULTIPLY)), (IncorrectERC20Interface, late, (INCORRECT_ERC20_INTERFACE)), (IncorrectERC721Interface, late, (INCORRECT_ERC721_INTERFACE)), (IncorrectStrictEquality, late, (INCORRECT_STRICT_EQUALITY)), diff --git a/crates/lint/src/sol/med/unused_return.rs b/crates/lint/src/sol/med/unused_return.rs index 4a3d975ccea2d..7614113fe0090 100644 --- a/crates/lint/src/sol/med/unused_return.rs +++ b/crates/lint/src/sol/med/unused_return.rs @@ -1,11 +1,11 @@ use super::UnusedReturn; use crate::{ linter::{LateLintPass, LintContext}, - sol::{Severity, SolLint}, + sol::{Severity, SolLint, analysis::interface::receiver_contract_id}, }; use solar::sema::{ Gcx, Hir, - hir::{Expr, ExprKind, Function, ItemId, Res, Stmt, StmtKind, TypeKind, VariableId}, + hir::{Expr, ExprKind, Function, Stmt, StmtKind, TypeKind, VariableId}, }; declare_forge_lint!( @@ -23,14 +23,28 @@ impl<'hir> LateLintPass<'hir> for UnusedReturn { hir: &'hir Hir<'hir>, stmt: &'hir Stmt<'hir>, ) { - if let StmtKind::Expr(expr) = &stmt.kind - && is_unused_return_call(hir, expr) - { - ctx.emit(&UNUSED_RETURN, expr.span); + match &stmt.kind { + StmtKind::Expr(expr) + if is_unused_return_call(hir, expr) || is_ignored_tuple_assignment(hir, expr) => + { + ctx.emit(&UNUSED_RETURN, expr.span); + } + StmtKind::DeclMulti(vars, expr) + if vars.iter().any(Option::is_none) && is_unused_return_call(hir, expr) => + { + ctx.emit(&UNUSED_RETURN, expr.span); + } + _ => {} } } } +fn is_ignored_tuple_assignment(hir: &Hir<'_>, expr: &Expr<'_>) -> bool { + let ExprKind::Assign(lhs, None, rhs) = &expr.peel_parens().kind else { return false }; + matches!(&lhs.peel_parens().kind, ExprKind::Tuple(elems) if elems.iter().any(Option::is_none)) + && is_unused_return_call(hir, rhs) +} + /// Returns true if `expr` is a member call on a contract whose resolved function has return /// values, excluding ERC20 `transfer`/`transferFrom` (covered by `erc20-unchecked-transfer`). fn is_unused_return_call(hir: &Hir<'_>, expr: &Expr<'_>) -> bool { @@ -41,61 +55,42 @@ fn is_unused_return_call(hir: &Hir<'_>, expr: &Expr<'_>) -> bool { ) }; - let ExprKind::Call(callee, call_args, ..) = &expr.kind else { return false }; - let ExprKind::Member(contract_expr, func_ident) = &callee.kind else { return false }; + let ExprKind::Call(callee, call_args, ..) = &expr.peel_parens().kind else { return false }; + let ExprKind::Member(contract_expr, func_ident) = &callee.peel_parens().kind else { + return false; + }; // Arity from either positional or named args. let arity = call_args.kind.len(); - let Some(cid) = (match &contract_expr.kind { - // Pre-instantiated contract variable: `oracle.f()` - ExprKind::Ident([Res::Item(ItemId::Variable(id)), ..]) => { - if let TypeKind::Custom(ItemId::Contract(cid)) = hir.variable(*id).ty.kind { - Some(cid) - } else { - None - } - } - // Explicit interface cast: `IOracle(addr).f()` - ExprKind::Call( - Expr { kind: ExprKind::Ident([Res::Item(ItemId::Contract(cid))]), .. }, - .., - ) => Some(*cid), - _ => None, - }) else { - return false; - }; + let Some(cid) = receiver_contract_id(hir, contract_expr) else { return false }; - // Collect all functions in the contract matching this name and arity. - let candidates: Vec<&Function<'_>> = hir - .contract_item_ids(cid) - .filter_map(|item| { - let fid = item.as_function()?; - let func = hir.function(fid); - (func.name.is_some_and(|n| n.as_str() == func_ident.as_str()) - && func.kind.is_function() - && func.parameters.len() == arity) - .then_some(func) - }) - .collect(); + let mut has_candidate = false; + for item in hir.contract_item_ids(cid) { + let Some(fid) = item.as_function() else { continue }; + let func = hir.function(fid); + if func.name.is_none_or(|n| n.as_str() != func_ident.as_str()) + || !func.kind.is_function() + || func.parameters.len() != arity + { + continue; + } - // No matching candidate found, nothing to lint. - if candidates.is_empty() { - return false; - } + has_candidate = true; - // If any candidate returns nothing, we can't tell which overload is being called, - // skip to avoid a false positive. - if candidates.iter().any(|f| f.returns.is_empty()) { - return false; - } + // If any matching overload returns nothing, we can't tell which overload is being called, + // skip to avoid a false positive. + if func.returns.is_empty() { + return false; + } - // If any candidate is an ERC20 transfer/transferFrom, defer to erc20-unchecked-transfer. - if candidates.iter().any(|f| is_erc20_transfer_sig(f, func_ident.as_str(), &is_type)) { - return false; + // If any candidate is an ERC20 transfer/transferFrom, defer to erc20-unchecked-transfer. + if is_erc20_transfer_sig(func, func_ident.as_str(), &is_type) { + return false; + } } - true + has_candidate } /// Returns true if `func` matches the ERC20 `transfer` or `transferFrom` signature exactly. diff --git a/crates/lint/testdata/BlockTimestamp.sol b/crates/lint/testdata/BlockTimestamp.sol index 6e2e259884eaa..2245580f7047a 100644 --- a/crates/lint/testdata/BlockTimestamp.sol +++ b/crates/lint/testdata/BlockTimestamp.sol @@ -58,6 +58,20 @@ contract BlockTimestamp { return foo(block.timestamp) > 0; //~WARN: usage of `block.timestamp` in a comparison may be manipulated by validators } + function aliasedComparison() public view returns (bool) { + uint256 t = block.timestamp; + return t < deadline; //~WARN: usage of `block.timestamp` in a comparison may be manipulated by validators + } + + function aliasedRequire() public view { + uint256 t = block.timestamp; + require(t < deadline); //~WARN: usage of `block.timestamp` in a comparison may be manipulated by validators + } + + function helperComparison() public view returns (bool) { + return currentTime() < deadline; //~WARN: usage of `block.timestamp` in a comparison may be manipulated by validators + } + // SHOULD PASS: function assignOnly() public view returns (uint256) { @@ -77,9 +91,37 @@ contract BlockTimestamp { return block.timestamp + 100; } + function aliasOverwritten() public view returns (bool) { + uint256 t = block.timestamp; + t = deadline; + return t > 0; + } + + function returningBranchAliasDoesNotLeak(bool condition) public view returns (bool) { + uint256 t = deadline; + if (condition) { + t = block.timestamp; + return true; + } + return t > 0; + } + + function revertingBranchAliasDoesNotLeak(bool condition) public view returns (bool) { + uint256 t = deadline; + if (condition) { + t = block.timestamp; + revert("done"); + } + return t > 0; + } + function foo(uint256 x) internal pure returns (uint256) { return x; } + function currentTime() internal view returns (uint256) { + return block.timestamp; + } + event Timestamp(uint256 ts); } diff --git a/crates/lint/testdata/BlockTimestamp.stderr b/crates/lint/testdata/BlockTimestamp.stderr index 62ab588ae7340..112deb1e650ae 100644 --- a/crates/lint/testdata/BlockTimestamp.stderr +++ b/crates/lint/testdata/BlockTimestamp.stderr @@ -94,3 +94,27 @@ LL │ return foo(block.timestamp) > 0; │ ╰ help: https://getfoundry.sh/forge/linting/block-timestamp +warning[block-timestamp]: usage of `block.timestamp` in a comparison may be manipulated by validators + ╭▸ ROOT/testdata/BlockTimestamp.sol:LL:CC + │ +LL │ return t < deadline; + │ ━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/block-timestamp + +warning[block-timestamp]: usage of `block.timestamp` in a comparison may be manipulated by validators + ╭▸ ROOT/testdata/BlockTimestamp.sol:LL:CC + │ +LL │ require(t < deadline); + │ ━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/block-timestamp + +warning[block-timestamp]: usage of `block.timestamp` in a comparison may be manipulated by validators + ╭▸ ROOT/testdata/BlockTimestamp.sol:LL:CC + │ +LL │ return currentTime() < deadline; + │ ━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/block-timestamp + diff --git a/crates/lint/testdata/DivideBeforeMultiply.sol b/crates/lint/testdata/DivideBeforeMultiply.sol index 4910d43641a8d..aec789e8087f0 100644 --- a/crates/lint/testdata/DivideBeforeMultiply.sol +++ b/crates/lint/testdata/DivideBeforeMultiply.sol @@ -4,6 +4,8 @@ pragma solidity ^0.8.18; contract DivideBeforeMultiply { function arithmetic() public { (1 / 2) * 3; //~WARN: multiplication should occur before division to avoid loss of precision + 3 * (1 / 2); //~WARN: multiplication should occur before division to avoid loss of precision + 4 * ((1 + 2) / 3); //~WARN: multiplication should occur before division to avoid loss of precision (1 * 2) / 3; ((1 / 2) * 3) * 4; //~WARN: multiplication should occur before division to avoid loss of precision ((1 * 2) / 3) * 4; //~WARN: multiplication should occur before division to avoid loss of precision @@ -18,4 +20,121 @@ contract DivideBeforeMultiply { 1 / ((2 / 3) * 3); //~WARN: multiplication should occur before division to avoid loss of precision 1 / ((2 * 3) + 3); } + + function assigned(uint256 a, uint256 b, uint256 c) public pure returns (uint256 result) { + uint256 q = a / b; + result = q * c; //~WARN: multiplication should occur before division to avoid loss of precision + + q = a + b; + result = q * c; + } + + function propagated(uint256 a, uint256 b, uint256 c) public pure returns (uint256) { + uint256 q = a / b; + uint256 copy = q; + return c * copy; //~WARN: multiplication should occur before division to avoid loss of precision + } + + function branchPropagated(uint256 a, uint256 b, uint256 c, bool condition) + public + pure + returns (uint256 q) + { + if (condition) { + q = a / b; + } + return q * c; //~WARN: multiplication should occur before division to avoid loss of precision + } + + function loopPropagated(uint256 a, uint256 b, uint256 c) public pure returns (uint256 q) { + for (uint256 i = 0; i < 1; ++i) { + q = a / b; + } + return q * c; //~WARN: multiplication should occur before division to avoid loss of precision + } + + function returningBranchDoesNotLeak(uint256 a, uint256 b, uint256 c, bool condition) + public + pure + returns (uint256 q) + { + if (condition) { + q = a / b; + return q; + } + return q * c; + } + + function revertingBranchDoesNotLeak(uint256 a, uint256 b, uint256 c, bool condition) + public + pure + returns (uint256 q) + { + if (condition) { + q = a / b; + revert("done"); + } + return q * c; + } + + function compound(uint256 a, uint256 b, uint256 c) public pure returns (uint256 q) { + q = a / b; + q *= c; //~WARN: multiplication should occur before division to avoid loss of precision + + q = a + b; + q *= c; + + q = a; + q /= b; + q *= c; //~WARN: multiplication should occur before division to avoid loss of precision + } + + function compoundClearsTaint(uint256 a, uint256 b, uint256 c) public pure returns (uint256 q) { + q = a / b; + q += 1; + return q * c; + } + + function incrementClearsTaint(uint256 a, uint256 b, uint256 c) public pure returns (uint256 q) { + q = a / b; + q++; + return q * c; + } + + function tupleElementWise(uint256 a, uint256 b, uint256 c) + public + pure + returns (uint256 x, uint256 y) + { + (x, y) = (a / b, c); + x = x * c; //~WARN: multiplication should occur before division to avoid loss of precision + y = y * c; + } + + function noBroadRhsTaint(uint256 a, uint256 b, uint256 c) public pure returns (uint256) { + uint256 q = (a / b) + 1; + uint256 z = helper(a / b); + return (q * c) + (z * c); + } + + function helper(uint256 value) internal pure returns (uint256) { + return value; + } + + function yulDirect(uint256 a, uint256 b, uint256 c) public pure returns (uint256 result) { + assembly { + result := mul(div(a, b), c) //~WARN: multiplication should occur before division to avoid loss of precision + result := mul(c, sdiv(a, b)) //~WARN: multiplication should occur before division to avoid loss of precision + } + } + + function yulAssigned(uint256 a, uint256 b, uint256 c) public pure returns (uint256 result) { + assembly { + let q := div(a, b) + result := mul(q, c) //~WARN: multiplication should occur before division to avoid loss of precision + + q := add(a, b) + result := mul(q, c) + } + } } diff --git a/crates/lint/testdata/DivideBeforeMultiply.stderr b/crates/lint/testdata/DivideBeforeMultiply.stderr index 95022f65db874..9dd96b855fe9d 100644 --- a/crates/lint/testdata/DivideBeforeMultiply.stderr +++ b/crates/lint/testdata/DivideBeforeMultiply.stderr @@ -6,11 +6,27 @@ LL │ (1 / 2) * 3; │ ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ 3 * (1 / 2); + │ ━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ 4 * ((1 + 2) / 3); + │ ━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC │ LL │ ((1 / 2) * 3) * 4; - │ ━━━━━━━━━━━ + │ ━━━━━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply @@ -42,7 +58,87 @@ warning[divide-before-multiply]: multiplication should occur before division to ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC │ LL │ 1 / ((2 / 3) * 3); - │ ━━━━━━━━━━━ + │ ━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ result = q * c; + │ ━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ return c * copy; + │ ━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ return q * c; + │ ━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ return q * c; + │ ━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ q *= c; + │ ━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ q *= c; + │ ━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ x = x * c; + │ ━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ result := mul(div(a, b), c) + │ ━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ result := mul(c, sdiv(a, b)) + │ ━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply + +warning[divide-before-multiply]: multiplication should occur before division to avoid loss of precision + ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC + │ +LL │ result := mul(q, c) + │ ━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply diff --git a/crates/lint/testdata/IncorrectShift.sol b/crates/lint/testdata/IncorrectShift.sol index 9ca3297e101ce..a27964e4e7d94 100644 --- a/crates/lint/testdata/IncorrectShift.sol +++ b/crates/lint/testdata/IncorrectShift.sol @@ -15,14 +15,19 @@ contract IncorrectShift { uint256 localShiftAmount = 3; // SHOULD FAIL: - // - Literal << NonLiteral - // - Literal >> NonLiteral + // - Yul shl/shr with a dynamic first argument and literal second argument - result = 2 << stateValue; //~WARN: the order of args in a shift operation is incorrect - result = 8 >> localValue; //~WARN: the order of args in a shift operation is incorrect - result = 16 << (stateValue + 1); //~WARN: the order of args in a shift operation is incorrect - result = 32 >> getAmount(); //~WARN: the order of args in a shift operation is incorrect - result = 1 << (localValue > 10 ? localShiftAmount : stateShiftAmount); //~WARN: the order of args in a shift operation is incorrect + result = 2 << stateValue; + result = 8 >> localValue; + result = 16 << (stateValue + 1); + result = 32 >> getAmount(); + result = 1 << (localValue > 10 ? localShiftAmount : stateShiftAmount); + + assembly { + result := shl(result, 8) //~WARN: the order of args in a shift operation is incorrect + result := shr(add(result, 1), 16) //~WARN: the order of args in a shift operation is incorrect + result := sar(add(result, 1), 8) //~WARN: the order of args in a shift operation is incorrect + } // SHOULD PASS: result = stateValue << 2; @@ -34,5 +39,12 @@ contract IncorrectShift { result = 1 << 8; result = 255 >> 4; + + assembly { + result := shl(8, result) + result := shr(16, result) + result := sar(8, result) + result := shl(8, 1) + } } } diff --git a/crates/lint/testdata/IncorrectShift.stderr b/crates/lint/testdata/IncorrectShift.stderr index dfff32db897bb..cfadddc7b2067 100644 --- a/crates/lint/testdata/IncorrectShift.stderr +++ b/crates/lint/testdata/IncorrectShift.stderr @@ -1,40 +1,24 @@ warning[incorrect-shift]: the order of args in a shift operation is incorrect ╭▸ ROOT/testdata/IncorrectShift.sol:LL:CC │ -LL │ result = 2 << stateValue; - │ ━━━━━━━━━━━━━━━ +LL │ result := shl(result, 8) + │ ━━━━━━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/incorrect-shift warning[incorrect-shift]: the order of args in a shift operation is incorrect ╭▸ ROOT/testdata/IncorrectShift.sol:LL:CC │ -LL │ result = 8 >> localValue; - │ ━━━━━━━━━━━━━━━ +LL │ result := shr(add(result, 1), 16) + │ ━━━━━━━━━━━━━━━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/incorrect-shift warning[incorrect-shift]: the order of args in a shift operation is incorrect ╭▸ ROOT/testdata/IncorrectShift.sol:LL:CC │ -LL │ result = 16 << (stateValue + 1); - │ ━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/incorrect-shift - -warning[incorrect-shift]: the order of args in a shift operation is incorrect - ╭▸ ROOT/testdata/IncorrectShift.sol:LL:CC - │ -LL │ result = 32 >> getAmount(); - │ ━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/incorrect-shift - -warning[incorrect-shift]: the order of args in a shift operation is incorrect - ╭▸ ROOT/testdata/IncorrectShift.sol:LL:CC - │ -LL │ … result = 1 << (localValue > 10 ? localShiftAmount : stateShiftAmount); - │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +LL │ result := sar(add(result, 1), 8) + │ ━━━━━━━━━━━━━━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/incorrect-shift diff --git a/crates/lint/testdata/PragmaInconsistentCaretAboveExact.sol b/crates/lint/testdata/PragmaInconsistentCaretAboveExact.sol index bfc993baab794..0edb519ac4395 100644 --- a/crates/lint/testdata/PragmaInconsistentCaretAboveExact.sol +++ b/crates/lint/testdata/PragmaInconsistentCaretAboveExact.sol @@ -1,7 +1,7 @@ //@compile-flags: --only-lint pragma-inconsistent // SPDX-License-Identifier: MIT -pragma solidity ^0.8.0; //~NOTE: 'pragma solidity ^0.8.0;' conflicts with other version requirements in the project: 0.8.18 -pragma solidity 0.8.18; //~NOTE: 'pragma solidity 0.8.18;' conflicts with other version requirements in the project: ^0.8.0 +pragma solidity ^0.8.0; //~NOTE: 2 different Solidity pragma version requirements are used: 0.8.18, ^0.8.0 +pragma solidity 0.8.18; contract Main {} diff --git a/crates/lint/testdata/PragmaInconsistentCaretAboveExact.stderr b/crates/lint/testdata/PragmaInconsistentCaretAboveExact.stderr index c2c967dee792f..ee470c102a79e 100644 --- a/crates/lint/testdata/PragmaInconsistentCaretAboveExact.stderr +++ b/crates/lint/testdata/PragmaInconsistentCaretAboveExact.stderr @@ -1,4 +1,4 @@ -note[pragma-inconsistent]: 'pragma solidity ^0.8.0;' conflicts with other version requirements in the project: 0.8.18 +note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: 0.8.18, ^0.8.0 ╭▸ ROOT/testdata/PragmaInconsistentCaretAboveExact.sol:LL:CC │ LL │ pragma solidity ^0.8.0; @@ -6,11 +6,3 @@ LL │ pragma solidity ^0.8.0; │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity 0.8.18;' conflicts with other version requirements in the project: ^0.8.0 - ╭▸ ROOT/testdata/PragmaInconsistentCaretAboveExact.sol:LL:CC - │ -LL │ pragma solidity 0.8.18; - │ ━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - diff --git a/crates/lint/testdata/PragmaInconsistentCaretMatchesExact.sol b/crates/lint/testdata/PragmaInconsistentCaretMatchesExact.sol index 75bc17988accc..12bdbb4cb4106 100644 --- a/crates/lint/testdata/PragmaInconsistentCaretMatchesExact.sol +++ b/crates/lint/testdata/PragmaInconsistentCaretMatchesExact.sol @@ -1,7 +1,7 @@ //@compile-flags: --only-lint pragma-inconsistent // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; //~NOTE: 'pragma solidity ^0.8.20;' conflicts with other version requirements in the project: 0.8.20 -pragma solidity 0.8.20; //~NOTE: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20 +pragma solidity ^0.8.20; //~NOTE: 2 different Solidity pragma version requirements are used: 0.8.20, ^0.8.20 +pragma solidity 0.8.20; contract Main {} diff --git a/crates/lint/testdata/PragmaInconsistentCaretMatchesExact.stderr b/crates/lint/testdata/PragmaInconsistentCaretMatchesExact.stderr index f60361718ba9b..c7aae1720af99 100644 --- a/crates/lint/testdata/PragmaInconsistentCaretMatchesExact.stderr +++ b/crates/lint/testdata/PragmaInconsistentCaretMatchesExact.stderr @@ -1,4 +1,4 @@ -note[pragma-inconsistent]: 'pragma solidity ^0.8.20;' conflicts with other version requirements in the project: 0.8.20 +note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: 0.8.20, ^0.8.20 ╭▸ ROOT/testdata/PragmaInconsistentCaretMatchesExact.sol:LL:CC │ LL │ pragma solidity ^0.8.20; @@ -6,11 +6,3 @@ LL │ pragma solidity ^0.8.20; │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20 - ╭▸ ROOT/testdata/PragmaInconsistentCaretMatchesExact.sol:LL:CC - │ -LL │ pragma solidity 0.8.20; - │ ━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - diff --git a/crates/lint/testdata/PragmaInconsistentCaretVsTilde.sol b/crates/lint/testdata/PragmaInconsistentCaretVsTilde.sol index 37b06040c33a6..323d1b26e68c8 100644 --- a/crates/lint/testdata/PragmaInconsistentCaretVsTilde.sol +++ b/crates/lint/testdata/PragmaInconsistentCaretVsTilde.sol @@ -1,7 +1,7 @@ //@compile-flags: --only-lint pragma-inconsistent // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; //~NOTE: 'pragma solidity ^0.8.20;' conflicts with other version requirements in the project: ~0.8.20 -pragma solidity ~0.8.20; //~NOTE: 'pragma solidity ~0.8.20;' conflicts with other version requirements in the project: ^0.8.20 +pragma solidity ^0.8.20; //~NOTE: 2 different Solidity pragma version requirements are used: ^0.8.20, ~0.8.20 +pragma solidity ~0.8.20; contract Main {} diff --git a/crates/lint/testdata/PragmaInconsistentCaretVsTilde.stderr b/crates/lint/testdata/PragmaInconsistentCaretVsTilde.stderr index 6c46f2478208d..80f826818043c 100644 --- a/crates/lint/testdata/PragmaInconsistentCaretVsTilde.stderr +++ b/crates/lint/testdata/PragmaInconsistentCaretVsTilde.stderr @@ -1,4 +1,4 @@ -note[pragma-inconsistent]: 'pragma solidity ^0.8.20;' conflicts with other version requirements in the project: ~0.8.20 +note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: ^0.8.20, ~0.8.20 ╭▸ ROOT/testdata/PragmaInconsistentCaretVsTilde.sol:LL:CC │ LL │ pragma solidity ^0.8.20; @@ -6,11 +6,3 @@ LL │ pragma solidity ^0.8.20; │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity ~0.8.20;' conflicts with other version requirements in the project: ^0.8.20 - ╭▸ ROOT/testdata/PragmaInconsistentCaretVsTilde.sol:LL:CC - │ -LL │ pragma solidity ~0.8.20; - │ ━━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - diff --git a/crates/lint/testdata/PragmaInconsistentOrVsExact.sol b/crates/lint/testdata/PragmaInconsistentOrVsExact.sol index f85a477cc8744..b48e3f3ee8408 100644 --- a/crates/lint/testdata/PragmaInconsistentOrVsExact.sol +++ b/crates/lint/testdata/PragmaInconsistentOrVsExact.sol @@ -1,7 +1,7 @@ //@compile-flags: --only-lint pragma-inconsistent // SPDX-License-Identifier: MIT -pragma solidity 0.8.20 || 0.8.21; //~NOTE: 'pragma solidity 0.8.20 || 0.8.21;' conflicts with other version requirements in the project: 0.8.20 -pragma solidity 0.8.20; //~NOTE: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: 0.8.20 || 0.8.21 +pragma solidity 0.8.20 || 0.8.21; //~NOTE: 2 different Solidity pragma version requirements are used: 0.8.20, 0.8.20 || 0.8.21 +pragma solidity 0.8.20; contract Main {} diff --git a/crates/lint/testdata/PragmaInconsistentOrVsExact.stderr b/crates/lint/testdata/PragmaInconsistentOrVsExact.stderr index acf6bd7c2d6e0..a42fb30b27e59 100644 --- a/crates/lint/testdata/PragmaInconsistentOrVsExact.stderr +++ b/crates/lint/testdata/PragmaInconsistentOrVsExact.stderr @@ -1,4 +1,4 @@ -note[pragma-inconsistent]: 'pragma solidity 0.8.20 || 0.8.21;' conflicts with other version requirements in the project: 0.8.20 +note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: 0.8.20, 0.8.20 || 0.8.21 ╭▸ ROOT/testdata/PragmaInconsistentOrVsExact.sol:LL:CC │ LL │ pragma solidity 0.8.20 || 0.8.21; @@ -6,11 +6,3 @@ LL │ pragma solidity 0.8.20 || 0.8.21; │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: 0.8.20 || 0.8.21 - ╭▸ ROOT/testdata/PragmaInconsistentOrVsExact.sol:LL:CC - │ -LL │ pragma solidity 0.8.20; - │ ━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - diff --git a/crates/lint/testdata/PragmaInconsistentRangeVsExact.sol b/crates/lint/testdata/PragmaInconsistentRangeVsExact.sol index d8fcb7a0eb4b1..a2a029adf1d74 100644 --- a/crates/lint/testdata/PragmaInconsistentRangeVsExact.sol +++ b/crates/lint/testdata/PragmaInconsistentRangeVsExact.sol @@ -1,7 +1,7 @@ //@compile-flags: --only-lint pragma-inconsistent // SPDX-License-Identifier: MIT -pragma solidity >=0.8.0 <0.9.0; //~NOTE: 'pragma solidity >=0.8.0 <0.9.0;' conflicts with other version requirements in the project: 0.8.20 -pragma solidity 0.8.20; //~NOTE: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: >=0.8.0 <0.9.0 +pragma solidity >=0.8.0 <0.9.0; //~NOTE: 2 different Solidity pragma version requirements are used: 0.8.20, >=0.8.0 <0.9.0 +pragma solidity 0.8.20; contract Main {} diff --git a/crates/lint/testdata/PragmaInconsistentRangeVsExact.stderr b/crates/lint/testdata/PragmaInconsistentRangeVsExact.stderr index 5ac221b924c9a..73afa06e2af4e 100644 --- a/crates/lint/testdata/PragmaInconsistentRangeVsExact.stderr +++ b/crates/lint/testdata/PragmaInconsistentRangeVsExact.stderr @@ -1,4 +1,4 @@ -note[pragma-inconsistent]: 'pragma solidity >=0.8.0 <0.9.0;' conflicts with other version requirements in the project: 0.8.20 +note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: 0.8.20, >=0.8.0 <0.9.0 ╭▸ ROOT/testdata/PragmaInconsistentRangeVsExact.sol:LL:CC │ LL │ pragma solidity >=0.8.0 <0.9.0; @@ -6,11 +6,3 @@ LL │ pragma solidity >=0.8.0 <0.9.0; │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: >=0.8.0 <0.9.0 - ╭▸ ROOT/testdata/PragmaInconsistentRangeVsExact.sol:LL:CC - │ -LL │ pragma solidity 0.8.20; - │ ━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - diff --git a/crates/lint/testdata/PragmaInconsistentThreeDistinct.sol b/crates/lint/testdata/PragmaInconsistentThreeDistinct.sol index fe208e15efb63..2d19943e3bd8b 100644 --- a/crates/lint/testdata/PragmaInconsistentThreeDistinct.sol +++ b/crates/lint/testdata/PragmaInconsistentThreeDistinct.sol @@ -1,8 +1,8 @@ //@compile-flags: --only-lint pragma-inconsistent // SPDX-License-Identifier: MIT -pragma solidity >=0.8.0; //~NOTE: 'pragma solidity >=0.8.0;' conflicts with other version requirements in the project: ^0.8.0, ~0.8.0 -pragma solidity ^0.8.0; //~NOTE: 'pragma solidity ^0.8.0;' conflicts with other version requirements in the project: >=0.8.0, ~0.8.0 -pragma solidity ~0.8.0; //~NOTE: 'pragma solidity ~0.8.0;' conflicts with other version requirements in the project: >=0.8.0, ^0.8.0 +pragma solidity >=0.8.0; //~NOTE: 3 different Solidity pragma version requirements are used: >=0.8.0, ^0.8.0, ~0.8.0 +pragma solidity ^0.8.0; +pragma solidity ~0.8.0; contract Main {} diff --git a/crates/lint/testdata/PragmaInconsistentThreeDistinct.stderr b/crates/lint/testdata/PragmaInconsistentThreeDistinct.stderr index e1e5ad7333fb2..c76994f10f1ef 100644 --- a/crates/lint/testdata/PragmaInconsistentThreeDistinct.stderr +++ b/crates/lint/testdata/PragmaInconsistentThreeDistinct.stderr @@ -1,4 +1,4 @@ -note[pragma-inconsistent]: 'pragma solidity >=0.8.0;' conflicts with other version requirements in the project: ^0.8.0, ~0.8.0 +note[pragma-inconsistent]: 3 different Solidity pragma version requirements are used: >=0.8.0, ^0.8.0, ~0.8.0 ╭▸ ROOT/testdata/PragmaInconsistentThreeDistinct.sol:LL:CC │ LL │ pragma solidity >=0.8.0; @@ -6,19 +6,3 @@ LL │ pragma solidity >=0.8.0; │ ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent -note[pragma-inconsistent]: 'pragma solidity ^0.8.0;' conflicts with other version requirements in the project: >=0.8.0, ~0.8.0 - ╭▸ ROOT/testdata/PragmaInconsistentThreeDistinct.sol:LL:CC - │ -LL │ pragma solidity ^0.8.0; - │ ━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - -note[pragma-inconsistent]: 'pragma solidity ~0.8.0;' conflicts with other version requirements in the project: >=0.8.0, ^0.8.0 - ╭▸ ROOT/testdata/PragmaInconsistentThreeDistinct.sol:LL:CC - │ -LL │ pragma solidity ~0.8.0; - │ ━━━━━━━━━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent - diff --git a/crates/lint/testdata/TooManyDigits.sol b/crates/lint/testdata/TooManyDigits.sol index a56ad67fe379e..01ea940f7b5db 100644 --- a/crates/lint/testdata/TooManyDigits.sol +++ b/crates/lint/testdata/TooManyDigits.sol @@ -8,6 +8,7 @@ contract TooManyDigits { uint256 stateA = 1000000000000000000; //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators uint256 stateB = 100000; //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + uint256 fortyDigitDecimal = 1000000000000000000000000000000000000000; //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators function asReturn() public pure returns (uint256) { return 10000000; //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators @@ -56,9 +57,21 @@ contract TooManyDigits { // Address literal (distinct AST kind, not flagged). address adr = 0x1234567890123456789012345678901234567890; - // Hex literal — intentional zero patterns (mask / padded value). - bytes32 mask = 0x0000000000000000000000000000000000000000000000000000000000000001; - uint256 hexNum = 0x100000; + // Non-address hex literals with long zero runs are flagged for Slither parity. + bytes32 mask = 0x0000000000000000000000000000000000000000000000000000000000000001; //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + uint256 hexNum = 0x100000; //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + uint256 hexWithE = 0x7fffffe07fffffe03ff000000000000; //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + uint256 hexWithUpperE = 0x7FFFFFE07FFFFFE03FF000000000000; //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + + function inAssembly() public pure returns (uint256 result) { + assembly { + result := 100000 //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + result := add(result, 0x100000) //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + result := add(result, 0x7fffffe07fffffe03ff000000000000) //~NOTE: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + result := add(result, 10000) + result := add(result, 0x1234567890123456789012345678901234567890) + } + } // Small literals (< 5 consecutive zeros). uint256 small1 = 100; diff --git a/crates/lint/testdata/TooManyDigits.stderr b/crates/lint/testdata/TooManyDigits.stderr index 7e21a530776c2..f97bf6bd094c7 100644 --- a/crates/lint/testdata/TooManyDigits.stderr +++ b/crates/lint/testdata/TooManyDigits.stderr @@ -14,6 +14,14 @@ LL │ uint256 stateB = 100000; │ ╰ help: https://getfoundry.sh/forge/linting/too-many-digits +note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ uint256 fortyDigitDecimal = 1000000000000000000000000000000000000000; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/too-many-digits + note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC │ @@ -70,3 +78,67 @@ LL │ uint256 badGrouping2 = 1_00000; │ ╰ help: https://getfoundry.sh/forge/linting/too-many-digits +note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ bytes32 mask = 0x0000000000000000000000000000000000000000000000000000000000000001; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/too-many-digits + +note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ uint256 hexNum = 0x100000; + │ ━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/too-many-digits + +note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ uint256 hexWithE = 0x7fffffe07fffffe03ff000000000000; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/too-many-digits + +note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ uint256 hexWithUpperE = 0x7FFFFFE07FFFFFE03FF000000000000; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/too-many-digits + +note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ … result := 100000 + │ ━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/too-many-digits + +note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ … result := add(result, 0x100000) + │ ━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/too-many-digits + +note[too-many-digits]: numeric literal with many digits is error-prone; use scientific notation, sub-denominations, or underscore separators + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ … result := add(result, 0x7fffffe07fffffe03ff000000000000) + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/too-many-digits + +note[inline-assembly]: inline assembly used; review for memory safety and side effects + ╭▸ ROOT/testdata/TooManyDigits.sol:LL:CC + │ +LL │ assembly { + │ ━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/inline-assembly + diff --git a/crates/lint/testdata/UnusedReturn.sol b/crates/lint/testdata/UnusedReturn.sol index 5fd0662b3f217..59e6aeae0000d 100644 --- a/crates/lint/testdata/UnusedReturn.sol +++ b/crates/lint/testdata/UnusedReturn.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.20; interface IOracle { function getPrice(address token) external returns (uint256); + function latest(address token) external returns (uint256, bool); function update() external returns (bool); function noReturn() external; } @@ -82,4 +83,63 @@ contract UnusedReturn { function bad4(address t) external { oracle.getPrice({token: t}); //~WARN: Return value of an external call is not used } + + // SHOULD FAIL: parenthesized receiver + function bad5(address t) external { + (oracle).getPrice(t); //~WARN: Return value of an external call is not used + } + + // SHOULD FAIL: parenthesized interface cast receiver + function bad6(address oracleAddr, address t) external { + (IOracle(oracleAddr)).getPrice(t); //~WARN: Return value of an external call is not used + } + + // SHOULD FAIL: tuple return has an ignored slot + function bad7(address t) external { + (uint256 price, ) = oracle.latest(t); //~WARN: Return value of an external call is not used + price = price + 1; + } + + // SHOULD FAIL: tuple assignment has an ignored slot + function bad8(address t) external { + uint256 price; + (price, ) = oracle.latest(t); //~WARN: Return value of an external call is not used + price = price + 1; + } + + // SHOULD PASS: captured return alone is considered used + function good7(address t) external { + uint256 price = oracle.getPrice(t); + price = 1; + } + + // SHOULD PASS: captured return is read before overwrite + function good8(address t) external returns (uint256) { + uint256 price = oracle.getPrice(t); + uint256 out = price; + price = 1; + return out; + } + + // SHOULD PASS: captured return read on a branch is still used + function good9(address t, bool cond) external returns (uint256) { + uint256 price = oracle.getPrice(t); + if (cond) return price; + return 0; + } + + // SHOULD PASS: tuple return values are read + function good10(address t) external returns (uint256) { + (uint256 price, bool ok) = oracle.latest(t); + uint256 out = price; + bool ready = ok; + if (ready) return out; + return 0; + } + + // SHOULD PASS: captured ERC20 transfer remains excluded + function good11(address to, uint256 amt) external { + bool ok = token.transfer(to, amt); + ok = true; + } } diff --git a/crates/lint/testdata/UnusedReturn.stderr b/crates/lint/testdata/UnusedReturn.stderr index a30c6188951cf..a98499c6b0e72 100644 --- a/crates/lint/testdata/UnusedReturn.stderr +++ b/crates/lint/testdata/UnusedReturn.stderr @@ -30,3 +30,35 @@ LL │ oracle.getPrice({token: t}); │ ╰ help: https://getfoundry.sh/forge/linting/unused-return +warning[unused-return]: Return value of an external call is not used + ╭▸ ROOT/testdata/UnusedReturn.sol:LL:CC + │ +LL │ (oracle).getPrice(t); + │ ━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/unused-return + +warning[unused-return]: Return value of an external call is not used + ╭▸ ROOT/testdata/UnusedReturn.sol:LL:CC + │ +LL │ (IOracle(oracleAddr)).getPrice(t); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/unused-return + +warning[unused-return]: Return value of an external call is not used + ╭▸ ROOT/testdata/UnusedReturn.sol:LL:CC + │ +LL │ (uint256 price, ) = oracle.latest(t); + │ ━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/unused-return + +warning[unused-return]: Return value of an external call is not used + ╭▸ ROOT/testdata/UnusedReturn.sol:LL:CC + │ +LL │ (price, ) = oracle.latest(t); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/unused-return +