From c7ff6b810ccbb211fdfc73731d42c7e5fb8dd715 Mon Sep 17 00:00:00 2001 From: Gustavo Figueiredo Date: Fri, 5 Jun 2026 15:12:30 +0100 Subject: [PATCH 1/7] Improve lint parity with Slither --- crates/forge/tests/cli/lint.rs | 48 +------ crates/lint/docs/incorrect-shift.md | 32 ++--- crates/lint/src/sol/high/incorrect_shift.rs | 77 +++++++++--- crates/lint/src/sol/info/too_many_digits.rs | 117 ++++++++++++++---- crates/lint/src/sol/med/div_mul.rs | 5 +- crates/lint/testdata/DivideBeforeMultiply.sol | 2 + .../lint/testdata/DivideBeforeMultiply.stderr | 16 +++ crates/lint/testdata/IncorrectShift.sol | 24 ++-- crates/lint/testdata/IncorrectShift.stderr | 32 +---- crates/lint/testdata/TooManyDigits.sol | 19 ++- crates/lint/testdata/TooManyDigits.stderr | 72 +++++++++++ 11 files changed, 304 insertions(+), 140 deletions(-) diff --git a/crates/forge/tests/cli/lint.rs b/crates/forge/tests/cli/lint.rs index 113eddd347f43..451d249c0bd72 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: | 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/src/sol/high/incorrect_shift.rs b/crates/lint/src/sol/high/incorrect_shift.rs index 8e7fcbf868109..49bfd3499ac12 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) + && 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/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/med/div_mul.rs b/crates/lint/src/sol/med/div_mul.rs index a9be610d6e4c3..c26f3ad7f6ac5 100644 --- a/crates/lint/src/sol/med/div_mul.rs +++ b/crates/lint/src/sol/med/div_mul.rs @@ -17,8 +17,9 @@ declare_forge_lint!( 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) + if let ExprKind::Binary(left_expr, BinOp { kind: BinOpKind::Mul, .. }, right_expr) = + &expr.kind + && (contains_division(left_expr) || contains_division(right_expr)) { ctx.emit(&DIVIDE_BEFORE_MULTIPLY, expr.span); } diff --git a/crates/lint/testdata/DivideBeforeMultiply.sol b/crates/lint/testdata/DivideBeforeMultiply.sol index 4910d43641a8d..4c983419eaff4 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 diff --git a/crates/lint/testdata/DivideBeforeMultiply.stderr b/crates/lint/testdata/DivideBeforeMultiply.stderr index 95022f65db874..ab4adb4149f09 100644 --- a/crates/lint/testdata/DivideBeforeMultiply.stderr +++ b/crates/lint/testdata/DivideBeforeMultiply.stderr @@ -6,6 +6,22 @@ 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 │ diff --git a/crates/lint/testdata/IncorrectShift.sol b/crates/lint/testdata/IncorrectShift.sol index 9ca3297e101ce..2b49bf4902d20 100644 --- a/crates/lint/testdata/IncorrectShift.sol +++ b/crates/lint/testdata/IncorrectShift.sol @@ -15,14 +15,18 @@ 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 + } // SHOULD PASS: result = stateValue << 2; @@ -34,5 +38,11 @@ contract IncorrectShift { result = 1 << 8; result = 255 >> 4; + + assembly { + result := shl(8, result) + result := shr(16, result) + result := shl(8, 1) + } } } diff --git a/crates/lint/testdata/IncorrectShift.stderr b/crates/lint/testdata/IncorrectShift.stderr index dfff32db897bb..39e3f1ecde004 100644 --- a/crates/lint/testdata/IncorrectShift.stderr +++ b/crates/lint/testdata/IncorrectShift.stderr @@ -1,40 +1,16 @@ 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; - │ ━━━━━━━━━━━━━━━ - │ - ╰ 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 := shr(add(result, 1), 16) + │ ━━━━━━━━━━━━━━━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/incorrect-shift 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 + From 28c657b4eb4f4a64f78aa75e0a4f50a3371f164a Mon Sep 17 00:00:00 2001 From: Gustavo Figueiredo Date: Fri, 5 Jun 2026 16:10:16 +0100 Subject: [PATCH 2/7] Improve Slither lint parity --- crates/lint/docs/pragma-inconsistent.md | 4 +- crates/lint/src/sol/analysis/interface.rs | 14 +- crates/lint/src/sol/high/incorrect_shift.rs | 2 +- crates/lint/src/sol/info/pragma_directive.rs | 19 +- crates/lint/src/sol/med/div_mul.rs | 332 ++++++++++++++++-- crates/lint/src/sol/med/mod.rs | 2 +- crates/lint/src/sol/med/unused_return.rs | 29 +- crates/lint/testdata/DivideBeforeMultiply.sol | 39 ++ .../lint/testdata/DivideBeforeMultiply.stderr | 52 ++- crates/lint/testdata/IncorrectShift.sol | 2 + crates/lint/testdata/IncorrectShift.stderr | 8 + .../PragmaInconsistentCaretAboveExact.sol | 4 +- .../PragmaInconsistentCaretAboveExact.stderr | 10 +- .../PragmaInconsistentCaretMatchesExact.sol | 4 +- ...PragmaInconsistentCaretMatchesExact.stderr | 10 +- .../PragmaInconsistentCaretVsTilde.sol | 4 +- .../PragmaInconsistentCaretVsTilde.stderr | 10 +- .../testdata/PragmaInconsistentOrVsExact.sol | 4 +- .../PragmaInconsistentOrVsExact.stderr | 10 +- .../PragmaInconsistentRangeVsExact.sol | 4 +- .../PragmaInconsistentRangeVsExact.stderr | 10 +- .../PragmaInconsistentThreeDistinct.sol | 6 +- .../PragmaInconsistentThreeDistinct.stderr | 18 +- crates/lint/testdata/UnusedReturn.sol | 10 + crates/lint/testdata/UnusedReturn.stderr | 16 + 25 files changed, 483 insertions(+), 140 deletions(-) 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 49bfd3499ac12..8f36ef4f46be8 100644 --- a/crates/lint/src/sol/high/incorrect_shift.rs +++ b/crates/lint/src/sol/high/incorrect_shift.rs @@ -63,7 +63,7 @@ fn check_yul_stmt(ctx: &LintContext, stmt: &yul::Stmt<'_>) { 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) + 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(_)) 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/med/div_mul.rs b/crates/lint/src/sol/med/div_mul.rs index c26f3ad7f6ac5..a81ab0ea67a86 100644 --- a/crates/lint/src/sol/med/div_mul.rs +++ b/crates/lint/src/sol/med/div_mul.rs @@ -1,12 +1,14 @@ use super::DivideBeforeMultiply; use crate::{ - linter::{EarlyLintPass, LintContext}, + linter::{LateLintPass, LintContext}, sol::{Severity, SolLint}, }; -use solar::{ - ast::{BinOp, BinOpKind, Expr, ExprKind}, - interface::SpannedOption, +use solar::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,27 +17,317 @@ 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, .. }, right_expr) = - &expr.kind - && (contains_division(left_expr) || contains_division(right_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) - } else { - false +fn check_block<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + block: Block<'hir>, + tainted: &mut HashSet, +) { + for stmt in block.stmts { + check_stmt(ctx, hir, stmt, tainted); + } +} + +fn check_stmt<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + stmt: &'hir Stmt<'hir>, + tainted: &mut HashSet, +) { + 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_has_division_or_taint(init, tainted), tainted); + } + } + StmtKind::DeclMulti(vars, expr) => { + check_expr(ctx, hir, expr, tainted); + let rhs_tainted = expr_has_division_or_taint(expr, tainted); + for var_id in vars.iter().flatten() { + update_taint(hir, *var_id, rhs_tainted, tainted); + } + } + StmtKind::Expr(expr) => check_expr(ctx, hir, expr, tainted), + StmtKind::Emit(expr) | StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { + check_expr(ctx, hir, expr, tainted) + } + StmtKind::If(cond, then_stmt, else_stmt) => { + check_expr(ctx, hir, cond, tainted); + + let mut then_tainted = tainted.clone(); + check_stmt(ctx, hir, then_stmt, &mut then_tainted); + + if let Some(else_stmt) = else_stmt { + let mut else_tainted = tainted.clone(); + check_stmt(ctx, hir, else_stmt, &mut else_tainted); + } + + // Branch-local assignments may not execute; keep only the incoming taint set. + } + StmtKind::Loop(block, _) => { + let mut loop_tainted = tainted.clone(); + check_block(ctx, hir, *block, &mut loop_tainted); + } + StmtKind::Try(try_stmt) => { + check_expr(ctx, hir, &try_stmt.expr, tainted); + for clause in try_stmt.clauses { + let mut clause_tainted = tainted.clone(); + check_block(ctx, hir, clause.block, &mut clause_tainted); + } + } + 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); + for case in switch.cases { + let mut case_tainted = tainted.clone(); + check_block(ctx, hir, case.body, &mut case_tainted); + } + } + StmtKind::Return(None) + | StmtKind::Break + | StmtKind::Continue + | StmtKind::Placeholder + | StmtKind::Err(_) => {} + } +} + +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 => { + let rhs_tainted = expr_has_division_or_taint(rhs, tainted); + update_lhs_taint(hir, lhs, rhs_tainted, 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 || expr_has_division_or_taint(rhs, tainted), + tainted, + ); + } + Some(_) => {} + } + } + 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); + check_expr(ctx, hir, then_expr, &mut tainted.clone()); + check_expr(ctx, hir, else_expr, &mut tainted.clone()); + } + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().flatten() { + check_expr(ctx, hir, expr, tainted); + } + } + ExprKind::Unary(_, inner) => check_expr(ctx, hir, inner, tainted), + ExprKind::Ident(_) + | ExprKind::Lit(_) + | ExprKind::New(_) + | ExprKind::TypeCall(_) + | ExprKind::Type(_) => {} + ExprKind::YulMember(inner, _) => check_expr(ctx, hir, inner, tainted), + ExprKind::Err(_) => {} + } +} + +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_has_division_or_taint(expr: &Expr<'_>, tainted: &HashSet) -> bool { + match &expr.peel_parens().kind { + ExprKind::Binary(left, op, right) => { + op.kind == BinOpKind::Div + || expr_has_division_or_taint(left, tainted) + || expr_has_division_or_taint(right, tainted) + } + ExprKind::Ident(resolutions) => resolutions.iter().any( + |res| matches!(res, Res::Item(ItemId::Variable(var_id)) if tainted.contains(var_id)), + ), + ExprKind::Array(exprs) => { + exprs.iter().any(|expr| expr_has_division_or_taint(expr, tainted)) + } + ExprKind::Assign(lhs, _, rhs) => { + expr_has_division_or_taint(lhs, tainted) || expr_has_division_or_taint(rhs, tainted) + } + ExprKind::Call(callee, args, named_args) => { + is_yul_division_call(expr) + || expr_has_division_or_taint(callee, tainted) + || args.exprs().any(|arg| expr_has_division_or_taint(arg, tainted)) + || named_args.is_some_and(|args| { + args.args.iter().any(|arg| expr_has_division_or_taint(&arg.value, tainted)) + }) + } + ExprKind::Delete(inner) + | ExprKind::Index(inner, None) + | ExprKind::Member(inner, _) + | ExprKind::Payable(inner) + | ExprKind::Unary(_, inner) => expr_has_division_or_taint(inner, tainted), + ExprKind::Index(base, Some(index)) => { + expr_has_division_or_taint(base, tainted) || expr_has_division_or_taint(index, tainted) + } + ExprKind::Slice(base, start, end) => { + expr_has_division_or_taint(base, tainted) + || start.is_some_and(|expr| expr_has_division_or_taint(expr, tainted)) + || end.is_some_and(|expr| expr_has_division_or_taint(expr, tainted)) + } + ExprKind::Ternary(cond, then_expr, else_expr) => { + expr_has_division_or_taint(cond, tainted) + || expr_has_division_or_taint(then_expr, tainted) + || expr_has_division_or_taint(else_expr, tainted) + } + ExprKind::Tuple(exprs) => { + exprs.iter().flatten().any(|expr| expr_has_division_or_taint(expr, tainted)) + } + ExprKind::Lit(_) | ExprKind::New(_) | ExprKind::TypeCall(_) | ExprKind::Type(_) => false, + ExprKind::YulMember(inner, _) => expr_has_division_or_taint(inner, tainted), + 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_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..a56c389c090c2 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!( @@ -41,30 +41,15 @@ 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 diff --git a/crates/lint/testdata/DivideBeforeMultiply.sol b/crates/lint/testdata/DivideBeforeMultiply.sol index 4c983419eaff4..d4a52970f453e 100644 --- a/crates/lint/testdata/DivideBeforeMultiply.sol +++ b/crates/lint/testdata/DivideBeforeMultiply.sol @@ -20,4 +20,43 @@ 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 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; + } + + 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 ab4adb4149f09..b73a198aa87a0 100644 --- a/crates/lint/testdata/DivideBeforeMultiply.stderr +++ b/crates/lint/testdata/DivideBeforeMultiply.stderr @@ -26,7 +26,7 @@ warning[divide-before-multiply]: multiplication should occur before division to ╭▸ ROOT/testdata/DivideBeforeMultiply.sol:LL:CC │ LL │ ((1 / 2) * 3) * 4; - │ ━━━━━━━━━━━ + │ ━━━━━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/divide-before-multiply @@ -58,7 +58,55 @@ 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 │ 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 │ 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 2b49bf4902d20..a27964e4e7d94 100644 --- a/crates/lint/testdata/IncorrectShift.sol +++ b/crates/lint/testdata/IncorrectShift.sol @@ -26,6 +26,7 @@ contract IncorrectShift { 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: @@ -42,6 +43,7 @@ contract IncorrectShift { 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 39e3f1ecde004..cfadddc7b2067 100644 --- a/crates/lint/testdata/IncorrectShift.stderr +++ b/crates/lint/testdata/IncorrectShift.stderr @@ -14,3 +14,11 @@ 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 := 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/UnusedReturn.sol b/crates/lint/testdata/UnusedReturn.sol index 5fd0662b3f217..0ef895da913ed 100644 --- a/crates/lint/testdata/UnusedReturn.sol +++ b/crates/lint/testdata/UnusedReturn.sol @@ -82,4 +82,14 @@ 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 + } } diff --git a/crates/lint/testdata/UnusedReturn.stderr b/crates/lint/testdata/UnusedReturn.stderr index a30c6188951cf..79a944bf6f2c7 100644 --- a/crates/lint/testdata/UnusedReturn.stderr +++ b/crates/lint/testdata/UnusedReturn.stderr @@ -30,3 +30,19 @@ 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 + From dfd514e056a8999e18c7efae924b11c04b8485bd Mon Sep 17 00:00:00 2001 From: Gustavo Figueiredo Date: Fri, 5 Jun 2026 17:04:47 +0100 Subject: [PATCH 3/7] test(forge): update lint parity snapshots Co-authored-by: Amp Amp-Thread-ID: https://ampcode.com/threads/T-019e9872-f047-7033-acf4-1f985641d25e --- crates/forge/tests/cli/lint.rs | 38 +++------------------------------- 1 file changed, 3 insertions(+), 35 deletions(-) diff --git a/crates/forge/tests/cli/lint.rs b/crates/forge/tests/cli/lint.rs index 451d249c0bd72..bdb7bb7a35495 100644 --- a/crates/forge/tests/cli/lint.rs +++ b/crates/forge/tests/cli/lint.rs @@ -1391,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; @@ -1399,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 - "#] ]); @@ -1502,15 +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 - [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 +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; @@ -1518,14 +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 - "#] ]); @@ -1542,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; @@ -1550,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 - "#] ]); From 4d0b9aca11db05d68e53cab74cb8487376a12554 Mon Sep 17 00:00:00 2001 From: Gustavo Figueiredo Date: Fri, 5 Jun 2026 18:05:19 +0100 Subject: [PATCH 4/7] fix(lint): tighten divide-before-multiply taint Amp-Thread-ID: https://ampcode.com/threads/T-019e9872-f047-7033-acf4-1f985641d25e Co-authored-by: Amp --- crates/lint/src/sol/med/div_mul.rs | 162 +++++++++++------- crates/lint/testdata/DivideBeforeMultiply.sol | 42 +++++ .../lint/testdata/DivideBeforeMultiply.stderr | 32 ++++ 3 files changed, 176 insertions(+), 60 deletions(-) diff --git a/crates/lint/src/sol/med/div_mul.rs b/crates/lint/src/sol/med/div_mul.rs index a81ab0ea67a86..c4a742889861d 100644 --- a/crates/lint/src/sol/med/div_mul.rs +++ b/crates/lint/src/sol/med/div_mul.rs @@ -53,15 +53,17 @@ fn check_stmt<'hir>( 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_has_division_or_taint(init, tainted), tainted); + update_taint( + hir, + *var_id, + expr_value_is_division_or_tainted(init, tainted), + tainted, + ); } } StmtKind::DeclMulti(vars, expr) => { check_expr(ctx, hir, expr, tainted); - let rhs_tainted = expr_has_division_or_taint(expr, tainted); - for var_id in vars.iter().flatten() { - update_taint(hir, *var_id, rhs_tainted, tainted); - } + update_multi_decl_taint(hir, vars, expr, tainted); } StmtKind::Expr(expr) => check_expr(ctx, hir, expr, tainted), StmtKind::Emit(expr) | StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { @@ -76,20 +78,25 @@ fn check_stmt<'hir>( if let Some(else_stmt) = else_stmt { let mut else_tainted = tainted.clone(); check_stmt(ctx, hir, else_stmt, &mut else_tainted); + *tainted = union_taints(&then_tainted, &else_tainted); + } else { + *tainted = union_taints(tainted, &then_tainted); } - - // Branch-local assignments may not execute; keep only the incoming taint set. } StmtKind::Loop(block, _) => { let mut loop_tainted = tainted.clone(); check_block(ctx, hir, *block, &mut loop_tainted); + *tainted = union_taints(tainted, &loop_tainted); } 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(); check_block(ctx, hir, clause.block, &mut clause_tainted); + merged_taint = union_taints(&merged_taint, &clause_tainted); } + *tainted = merged_taint; } StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { check_block(ctx, hir, *block, tainted) @@ -97,10 +104,13 @@ fn check_stmt<'hir>( 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(); check_block(ctx, hir, case.body, &mut case_tainted); + merged_taint = union_taints(&merged_taint, &case_tainted); } + *tainted = merged_taint; } StmtKind::Return(None) | StmtKind::Break @@ -123,8 +133,7 @@ fn check_expr<'hir>( match op { None => { - let rhs_tainted = expr_has_division_or_taint(rhs, tainted); - update_lhs_taint(hir, lhs, rhs_tainted, tainted); + 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); @@ -132,12 +141,10 @@ fn check_expr<'hir>( if lhs_tainted || rhs_tainted { ctx.emit(&DIVIDE_BEFORE_MULTIPLY, expr.span); } - update_lhs_taint( - hir, - lhs, - lhs_tainted || expr_has_division_or_taint(rhs, tainted), - tainted, - ); + 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(_) => {} } @@ -194,8 +201,11 @@ fn check_expr<'hir>( } ExprKind::Ternary(cond, then_expr, else_expr) => { check_expr(ctx, hir, cond, tainted); - check_expr(ctx, hir, then_expr, &mut tainted.clone()); - check_expr(ctx, hir, else_expr, &mut tainted.clone()); + 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() { @@ -213,6 +223,62 @@ fn check_expr<'hir>( } } +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<'_>, @@ -252,53 +318,29 @@ fn update_taint( } } -fn expr_has_division_or_taint(expr: &Expr<'_>, tainted: &HashSet) -> bool { +fn expr_value_is_division_or_tainted(expr: &Expr<'_>, tainted: &HashSet) -> bool { match &expr.peel_parens().kind { - ExprKind::Binary(left, op, right) => { - op.kind == BinOpKind::Div - || expr_has_division_or_taint(left, tainted) - || expr_has_division_or_taint(right, tainted) - } + 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::Array(exprs) => { - exprs.iter().any(|expr| expr_has_division_or_taint(expr, tainted)) - } - ExprKind::Assign(lhs, _, rhs) => { - expr_has_division_or_taint(lhs, tainted) || expr_has_division_or_taint(rhs, tainted) - } - ExprKind::Call(callee, args, named_args) => { - is_yul_division_call(expr) - || expr_has_division_or_taint(callee, tainted) - || args.exprs().any(|arg| expr_has_division_or_taint(arg, tainted)) - || named_args.is_some_and(|args| { - args.args.iter().any(|arg| expr_has_division_or_taint(&arg.value, tainted)) - }) - } - ExprKind::Delete(inner) - | ExprKind::Index(inner, None) - | ExprKind::Member(inner, _) - | ExprKind::Payable(inner) - | ExprKind::Unary(_, inner) => expr_has_division_or_taint(inner, tainted), - ExprKind::Index(base, Some(index)) => { - expr_has_division_or_taint(base, tainted) || expr_has_division_or_taint(index, tainted) - } - ExprKind::Slice(base, start, end) => { - expr_has_division_or_taint(base, tainted) - || start.is_some_and(|expr| expr_has_division_or_taint(expr, tainted)) - || end.is_some_and(|expr| expr_has_division_or_taint(expr, tainted)) - } - ExprKind::Ternary(cond, then_expr, else_expr) => { - expr_has_division_or_taint(cond, tainted) - || expr_has_division_or_taint(then_expr, tainted) - || expr_has_division_or_taint(else_expr, tainted) - } - ExprKind::Tuple(exprs) => { - exprs.iter().flatten().any(|expr| expr_has_division_or_taint(expr, tainted)) - } - ExprKind::Lit(_) | ExprKind::New(_) | ExprKind::TypeCall(_) | ExprKind::Type(_) => false, - ExprKind::YulMember(inner, _) => expr_has_division_or_taint(inner, tainted), + 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, } } diff --git a/crates/lint/testdata/DivideBeforeMultiply.sol b/crates/lint/testdata/DivideBeforeMultiply.sol index d4a52970f453e..4e169c3dbcd4f 100644 --- a/crates/lint/testdata/DivideBeforeMultiply.sol +++ b/crates/lint/testdata/DivideBeforeMultiply.sol @@ -35,12 +35,54 @@ contract DivideBeforeMultiply { 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 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 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) { diff --git a/crates/lint/testdata/DivideBeforeMultiply.stderr b/crates/lint/testdata/DivideBeforeMultiply.stderr index b73a198aa87a0..9dd96b855fe9d 100644 --- a/crates/lint/testdata/DivideBeforeMultiply.stderr +++ b/crates/lint/testdata/DivideBeforeMultiply.stderr @@ -78,6 +78,30 @@ 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 │ @@ -86,6 +110,14 @@ 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 │ From 525c004e24feb5633f0c55d3b38ba8702693de58 Mon Sep 17 00:00:00 2001 From: Gustavo Figueiredo Date: Fri, 5 Jun 2026 18:46:02 +0100 Subject: [PATCH 5/7] fix(lint): improve Slither parity for return and timestamp --- crates/lint/src/sol/low/block_timestamp.rs | 535 +++++++++++++++++++-- crates/lint/src/sol/low/mod.rs | 2 +- crates/lint/src/sol/med/unused_return.rs | 431 ++++++++++++++++- crates/lint/testdata/BlockTimestamp.sol | 24 + crates/lint/testdata/BlockTimestamp.stderr | 24 + crates/lint/testdata/UnusedReturn.sol | 43 ++ crates/lint/testdata/UnusedReturn.stderr | 24 + 7 files changed, 1041 insertions(+), 42 deletions(-) diff --git a/crates/lint/src/sol/low/block_timestamp.rs b/crates/lint/src/sol/low/block_timestamp.rs index b4ba3c7712217..572fdb7225540 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,17 +24,213 @@ 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, +) { + for stmt in block.stmts { + check_stmt(ctx, hir, helpers, stmt, aliases); + } +} + +fn check_stmt<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + helpers: &HashSet, + stmt: &'hir Stmt<'hir>, + aliases: &mut HashSet, +) { + 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, + ); + } + } + StmtKind::DeclMulti(vars, expr) => { + check_expr(ctx, hir, helpers, expr, aliases); + update_multi_aliases(hir, helpers, vars, expr, aliases); + } + StmtKind::Expr(expr) => check_expr(ctx, hir, helpers, expr, aliases), + StmtKind::Emit(expr) | StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { + check_expr(ctx, hir, helpers, expr, aliases); + } + StmtKind::If(cond, then_stmt, else_stmt) => { + check_expr(ctx, hir, helpers, cond, aliases); + + let baseline = aliases.clone(); + let mut then_aliases = baseline.clone(); + check_stmt(ctx, hir, helpers, then_stmt, &mut then_aliases); + + if let Some(else_stmt) = else_stmt { + let mut else_aliases = baseline; + check_stmt(ctx, hir, helpers, else_stmt, &mut else_aliases); + *aliases = then_aliases.union(&else_aliases).copied().collect(); + } else { + *aliases = baseline.union(&then_aliases).copied().collect(); + } + } + StmtKind::Loop(block, _) => { + let baseline = aliases.clone(); + let mut loop_aliases = baseline.clone(); + check_block(ctx, hir, helpers, *block, &mut loop_aliases); + *aliases = baseline.union(&loop_aliases).copied().collect(); + } + 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(); + check_block(ctx, hir, helpers, clause.block, &mut clause_aliases); + merged.extend(clause_aliases); + } + *aliases = merged; + } + 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(); + check_block(ctx, hir, helpers, case.body, &mut case_aliases); + merged.extend(case_aliases); + } + *aliases = merged; + } + StmtKind::Return(None) + | StmtKind::Break + | StmtKind::Continue + | StmtKind::Placeholder + | StmtKind::Err(_) => {} + } +} + +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(_) => {} + } +} + const fn is_cmp(kind: BinOpKind) -> bool { matches!( kind, @@ -35,36 +243,299 @@ 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 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() +} + +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, + } } -/// Recursively checks if an expression tree contains `block.timestamp`. -fn contains_block_timestamp(expr: &Expr<'_>) -> bool { +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/unused_return.rs b/crates/lint/src/sol/med/unused_return.rs index a56c389c090c2..04992c89627f5 100644 --- a/crates/lint/src/sol/med/unused_return.rs +++ b/crates/lint/src/sol/med/unused_return.rs @@ -3,10 +3,14 @@ use crate::{ linter::{LateLintPass, LintContext}, sol::{Severity, SolLint, analysis::interface::receiver_contract_id}, }; -use solar::sema::{ - Gcx, Hir, - hir::{Expr, ExprKind, Function, Stmt, StmtKind, TypeKind, VariableId}, +use solar::{ + interface::Span, + sema::{ + Gcx, Hir, + hir::{Block, Expr, ExprKind, Function, ItemId, Res, Stmt, StmtKind, TypeKind, VariableId}, + }, }; +use std::collections::HashSet; declare_forge_lint!( UNUSED_RETURN, @@ -16,21 +20,430 @@ declare_forge_lint!( ); impl<'hir> LateLintPass<'hir> for UnusedReturn { - fn check_stmt( + fn check_function( &mut self, ctx: &LintContext, _gcx: Gcx<'hir>, hir: &'hir Hir<'hir>, - stmt: &'hir Stmt<'hir>, + func: &'hir Function<'hir>, ) { - if let StmtKind::Expr(expr) = &stmt.kind - && is_unused_return_call(hir, expr) - { - ctx.emit(&UNUSED_RETURN, expr.span); + if let Some(body) = func.body { + let mut state = ReturnUseState::default(); + check_block(ctx, hir, body, &mut state); + state.finish(ctx); } } } +#[derive(Clone, Copy, PartialEq, Eq)] +struct PendingReturn { + var_id: VariableId, + span: Span, +} + +#[derive(Clone, Default)] +struct ReturnUseState { + pending: Vec, + emitted: HashSet, +} + +impl ReturnUseState { + fn emit(&mut self, ctx: &LintContext, span: Span) { + if self.emitted.insert(span) { + ctx.emit(&UNUSED_RETURN, span); + } + } + + fn add_pending(&mut self, ctx: &LintContext, var_id: VariableId, span: Span) { + if let Some(pending) = self.pending.iter_mut().find(|p| p.var_id == var_id) { + let old_span = pending.span; + pending.span = span; + self.emit(ctx, old_span); + } else { + self.pending.push(PendingReturn { var_id, span }); + } + } + + fn mark_read(&mut self, var_id: VariableId) { + if let Some(idx) = self.pending.iter().position(|p| p.var_id == var_id) { + self.pending.remove(idx); + } + } + + fn mark_overwritten(&mut self, ctx: &LintContext, var_id: VariableId) { + if let Some(idx) = self.pending.iter().position(|p| p.var_id == var_id) { + let pending = self.pending.remove(idx); + self.emit(ctx, pending.span); + } + } + + fn merge_from(&mut self, other: Self) { + self.emitted.extend(other.emitted); + for pending in other.pending { + if !self.pending.contains(&pending) { + self.pending.push(pending); + } + } + } + + fn finish(&mut self, ctx: &LintContext) { + for pending in std::mem::take(&mut self.pending) { + self.emit(ctx, pending.span); + } + } +} + +fn check_block<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + block: Block<'hir>, + state: &mut ReturnUseState, +) { + for stmt in block.stmts { + check_stmt(ctx, hir, stmt, state); + } +} + +fn check_stmt<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + stmt: &'hir Stmt<'hir>, + state: &mut ReturnUseState, +) { + match &stmt.kind { + StmtKind::DeclSingle(var_id) => { + if let Some(init) = hir.variable(*var_id).initializer { + check_expr(ctx, hir, init, state); + if is_unused_return_call(hir, init) { + add_pending_var(ctx, hir, *var_id, init.span, state); + } + } + } + StmtKind::DeclMulti(vars, expr) => { + check_expr(ctx, hir, expr, state); + update_multi_capture(ctx, hir, vars, expr, state); + } + StmtKind::Expr(expr) => { + if is_unused_return_call(hir, expr) { + state.emit(ctx, expr.span); + } + check_expr(ctx, hir, expr, state); + } + StmtKind::Emit(expr) | StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { + check_expr(ctx, hir, expr, state); + } + StmtKind::If(cond, then_stmt, else_stmt) => { + check_expr(ctx, hir, cond, state); + + let baseline = state.clone(); + let mut then_state = baseline.clone(); + check_stmt(ctx, hir, then_stmt, &mut then_state); + + let mut merged = ReturnUseState::default(); + merged.merge_from(then_state); + + if let Some(else_stmt) = else_stmt { + let mut else_state = baseline; + check_stmt(ctx, hir, else_stmt, &mut else_state); + merged.merge_from(else_state); + } else { + merged.merge_from(baseline); + } + + *state = merged; + } + StmtKind::Loop(block, _) => { + let baseline = state.clone(); + let mut loop_state = baseline.clone(); + check_block(ctx, hir, *block, &mut loop_state); + + let mut merged = baseline; + merged.merge_from(loop_state); + *state = merged; + } + StmtKind::Try(try_stmt) => { + check_expr(ctx, hir, &try_stmt.expr, state); + + let baseline = state.clone(); + let mut merged = baseline.clone(); + for clause in try_stmt.clauses { + let mut clause_state = baseline.clone(); + check_block(ctx, hir, clause.block, &mut clause_state); + merged.merge_from(clause_state); + } + *state = merged; + } + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { + check_block(ctx, hir, *block, state); + } + StmtKind::AssemblyBlock(block) => check_block(ctx, hir, *block, state), + StmtKind::Switch(switch) => { + check_expr(ctx, hir, switch.selector, state); + + let baseline = state.clone(); + let mut merged = baseline.clone(); + for case in switch.cases { + let mut case_state = baseline.clone(); + check_block(ctx, hir, case.body, &mut case_state); + merged.merge_from(case_state); + } + *state = merged; + } + StmtKind::Return(None) + | StmtKind::Break + | StmtKind::Continue + | StmtKind::Placeholder + | StmtKind::Err(_) => {} + } +} + +fn check_expr<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + expr: &'hir Expr<'hir>, + state: &mut ReturnUseState, +) { + match &expr.peel_parens().kind { + ExprKind::Assign(lhs, op, rhs) => { + check_expr(ctx, hir, rhs, state); + if op.is_some() { + check_expr(ctx, hir, lhs, state); + overwrite_lhs(ctx, hir, lhs, state); + } else { + check_lhs_reads(ctx, hir, lhs, state); + update_assignment_capture(ctx, hir, lhs, rhs, state); + } + } + ExprKind::Call(callee, args, options) => { + check_expr(ctx, hir, callee, state); + if let Some(options) = options { + for arg in options.args { + check_expr(ctx, hir, &arg.value, state); + } + } + for arg in args.exprs() { + check_expr(ctx, hir, arg, state); + } + } + ExprKind::Binary(lhs, _, rhs) => { + check_expr(ctx, hir, lhs, state); + check_expr(ctx, hir, rhs, state); + } + ExprKind::Unary(_, inner) + | ExprKind::Delete(inner) + | ExprKind::Member(inner, _) + | ExprKind::Payable(inner) + | ExprKind::YulMember(inner, _) => check_expr(ctx, hir, inner, state), + ExprKind::Ternary(cond, then_expr, else_expr) => { + check_expr(ctx, hir, cond, state); + + let baseline = state.clone(); + let mut then_state = baseline.clone(); + check_expr(ctx, hir, then_expr, &mut then_state); + let mut else_state = baseline; + check_expr(ctx, hir, else_expr, &mut else_state); + + let mut merged = ReturnUseState::default(); + merged.merge_from(then_state); + merged.merge_from(else_state); + *state = merged; + } + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().flatten() { + check_expr(ctx, hir, expr, state); + } + } + ExprKind::Array(exprs) => { + for expr in *exprs { + check_expr(ctx, hir, expr, state); + } + } + ExprKind::Index(base, index) => { + check_expr(ctx, hir, base, state); + if let Some(index) = index { + check_expr(ctx, hir, index, state); + } + } + ExprKind::Slice(base, start, end) => { + check_expr(ctx, hir, base, state); + if let Some(start) = start { + check_expr(ctx, hir, start, state); + } + if let Some(end) = end { + check_expr(ctx, hir, end, state); + } + } + ExprKind::Ident(resolutions) => { + for res in *resolutions { + if let Res::Item(ItemId::Variable(var_id)) = res { + state.mark_read(*var_id); + } + } + } + ExprKind::Lit(_) + | ExprKind::New(_) + | ExprKind::TypeCall(_) + | ExprKind::Type(_) + | ExprKind::Err(_) => {} + } +} + +fn update_multi_capture( + ctx: &LintContext, + hir: &Hir<'_>, + vars: &[Option], + expr: &Expr<'_>, + state: &mut ReturnUseState, +) { + if let ExprKind::Tuple(exprs) = &expr.peel_parens().kind + && exprs.len() == vars.len() + { + for (var_id, rhs) in vars.iter().zip(*exprs) { + if let Some(var_id) = var_id { + if let Some(rhs) = rhs + && is_unused_return_call(hir, rhs) + { + add_pending_var(ctx, hir, *var_id, rhs.span, state); + } else { + state.mark_overwritten(ctx, *var_id); + } + } + } + return; + } + + if is_unused_return_call(hir, expr) { + for var_id in vars.iter().flatten() { + add_pending_var(ctx, hir, *var_id, expr.span, state); + } + } else { + for var_id in vars.iter().flatten() { + state.mark_overwritten(ctx, *var_id); + } + } +} + +fn update_assignment_capture( + ctx: &LintContext, + hir: &Hir<'_>, + lhs: &Expr<'_>, + rhs: &Expr<'_>, + state: &mut ReturnUseState, +) { + if let (ExprKind::Tuple(lhs_exprs), ExprKind::Tuple(rhs_exprs)) = + (&lhs.peel_parens().kind, &rhs.peel_parens().kind) + && lhs_exprs.len() == rhs_exprs.len() + { + for (lhs, rhs) in lhs_exprs.iter().zip(*rhs_exprs) { + if let Some(lhs) = lhs { + if let Some(rhs) = rhs + && is_unused_return_call(hir, rhs) + { + add_pending_lhs(ctx, hir, lhs, rhs.span, state); + } else { + overwrite_lhs(ctx, hir, lhs, state); + } + } + } + return; + } + + if is_unused_return_call(hir, rhs) { + add_pending_lhs(ctx, hir, lhs, rhs.span, state); + } else { + overwrite_lhs(ctx, hir, lhs, state); + } +} + +fn add_pending_lhs( + ctx: &LintContext, + hir: &Hir<'_>, + lhs: &Expr<'_>, + span: Span, + state: &mut ReturnUseState, +) { + let mut locals = Vec::new(); + collect_lhs_locals(hir, lhs, &mut locals); + for var_id in locals { + add_pending_var(ctx, hir, var_id, span, state); + } +} + +fn add_pending_var( + ctx: &LintContext, + hir: &Hir<'_>, + var_id: VariableId, + span: Span, + state: &mut ReturnUseState, +) { + if hir.variable(var_id).is_local_or_return() { + state.add_pending(ctx, var_id, span); + } +} + +fn overwrite_lhs(ctx: &LintContext, hir: &Hir<'_>, lhs: &Expr<'_>, state: &mut ReturnUseState) { + let mut locals = Vec::new(); + collect_lhs_locals(hir, lhs, &mut locals); + for var_id in locals { + state.mark_overwritten(ctx, var_id); + } +} + +fn collect_lhs_locals(hir: &Hir<'_>, lhs: &Expr<'_>, out: &mut Vec) { + match &lhs.peel_parens().kind { + ExprKind::Ident(resolutions) => { + for res in *resolutions { + if let Res::Item(ItemId::Variable(var_id)) = res + && hir.variable(*var_id).is_local_or_return() + { + out.push(*var_id); + } + } + } + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().flatten() { + collect_lhs_locals(hir, expr, out); + } + } + _ => {} + } +} + +fn check_lhs_reads<'hir>( + ctx: &LintContext, + hir: &'hir Hir<'hir>, + lhs: &'hir Expr<'hir>, + state: &mut ReturnUseState, +) { + match &lhs.peel_parens().kind { + ExprKind::Ident(_) => {} + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().flatten() { + check_lhs_reads(ctx, hir, expr, state); + } + } + ExprKind::Member(base, _) | ExprKind::YulMember(base, _) => { + check_expr(ctx, hir, base, state); + } + ExprKind::Index(base, index) => { + check_expr(ctx, hir, base, state); + if let Some(index) = index { + check_expr(ctx, hir, index, state); + } + } + ExprKind::Slice(base, start, end) => { + check_expr(ctx, hir, base, state); + if let Some(start) = start { + check_expr(ctx, hir, start, state); + } + if let Some(end) = end { + check_expr(ctx, hir, end, state); + } + } + _ => check_expr(ctx, hir, lhs, state), + } +} + /// 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 { diff --git a/crates/lint/testdata/BlockTimestamp.sol b/crates/lint/testdata/BlockTimestamp.sol index 6e2e259884eaa..ed23a7a8a1299 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,19 @@ contract BlockTimestamp { return block.timestamp + 100; } + function aliasOverwritten() public view returns (bool) { + uint256 t = block.timestamp; + t = deadline; + 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/UnusedReturn.sol b/crates/lint/testdata/UnusedReturn.sol index 0ef895da913ed..7fd8162cdad14 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; } @@ -92,4 +93,46 @@ contract UnusedReturn { function bad6(address oracleAddr, address t) external { (IOracle(oracleAddr)).getPrice(t); //~WARN: Return value of an external call is not used } + + // SHOULD FAIL: return value captured but never read + function bad7(address t) external { + uint256 price = oracle.getPrice(t); //~WARN: Return value of an external call is not used + price = 1; + } + + // SHOULD FAIL: bool return captured but never read + function bad8() external { + bool ok = oracle.update(); //~WARN: Return value of an external call is not used + ok = true; + } + + // SHOULD FAIL: tuple return values captured but never read + function bad9(address t) external { + (uint256 price, bool ok) = oracle.latest(t); //~WARN: Return value of an external call is not used + price = 1; + ok = true; + } + + // SHOULD PASS: captured return is read before overwrite + function good7(address t) external returns (uint256) { + uint256 price = oracle.getPrice(t); + uint256 out = price; + price = 1; + return out; + } + + // SHOULD PASS: tuple return values are read + function good8(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 good9(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 79a944bf6f2c7..76bc180578bb7 100644 --- a/crates/lint/testdata/UnusedReturn.stderr +++ b/crates/lint/testdata/UnusedReturn.stderr @@ -46,3 +46,27 @@ 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.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 │ bool ok = oracle.update(); + │ ━━━━━━━━━━━━━━━ + │ + ╰ 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, bool ok) = oracle.latest(t); + │ ━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/unused-return + From e0dfca341ab86fb8413787205eb357778f07ca4b Mon Sep 17 00:00:00 2001 From: Gustavo Figueiredo Date: Sat, 6 Jun 2026 00:11:01 +0100 Subject: [PATCH 6/7] fix(lint): avoid unused-return branch false positives --- crates/lint/src/sol/med/unused_return.rs | 483 ++--------------------- crates/lint/testdata/UnusedReturn.sol | 35 +- crates/lint/testdata/UnusedReturn.stderr | 16 +- 3 files changed, 65 insertions(+), 469 deletions(-) diff --git a/crates/lint/src/sol/med/unused_return.rs b/crates/lint/src/sol/med/unused_return.rs index 04992c89627f5..7614113fe0090 100644 --- a/crates/lint/src/sol/med/unused_return.rs +++ b/crates/lint/src/sol/med/unused_return.rs @@ -3,14 +3,10 @@ use crate::{ linter::{LateLintPass, LintContext}, sol::{Severity, SolLint, analysis::interface::receiver_contract_id}, }; -use solar::{ - interface::Span, - sema::{ - Gcx, Hir, - hir::{Block, Expr, ExprKind, Function, ItemId, Res, Stmt, StmtKind, TypeKind, VariableId}, - }, +use solar::sema::{ + Gcx, Hir, + hir::{Expr, ExprKind, Function, Stmt, StmtKind, TypeKind, VariableId}, }; -use std::collections::HashSet; declare_forge_lint!( UNUSED_RETURN, @@ -20,428 +16,33 @@ declare_forge_lint!( ); impl<'hir> LateLintPass<'hir> for UnusedReturn { - fn check_function( + fn check_stmt( &mut self, ctx: &LintContext, _gcx: Gcx<'hir>, hir: &'hir Hir<'hir>, - func: &'hir Function<'hir>, + stmt: &'hir Stmt<'hir>, ) { - if let Some(body) = func.body { - let mut state = ReturnUseState::default(); - check_block(ctx, hir, body, &mut state); - state.finish(ctx); - } - } -} - -#[derive(Clone, Copy, PartialEq, Eq)] -struct PendingReturn { - var_id: VariableId, - span: Span, -} - -#[derive(Clone, Default)] -struct ReturnUseState { - pending: Vec, - emitted: HashSet, -} - -impl ReturnUseState { - fn emit(&mut self, ctx: &LintContext, span: Span) { - if self.emitted.insert(span) { - ctx.emit(&UNUSED_RETURN, span); - } - } - - fn add_pending(&mut self, ctx: &LintContext, var_id: VariableId, span: Span) { - if let Some(pending) = self.pending.iter_mut().find(|p| p.var_id == var_id) { - let old_span = pending.span; - pending.span = span; - self.emit(ctx, old_span); - } else { - self.pending.push(PendingReturn { var_id, span }); - } - } - - fn mark_read(&mut self, var_id: VariableId) { - if let Some(idx) = self.pending.iter().position(|p| p.var_id == var_id) { - self.pending.remove(idx); - } - } - - fn mark_overwritten(&mut self, ctx: &LintContext, var_id: VariableId) { - if let Some(idx) = self.pending.iter().position(|p| p.var_id == var_id) { - let pending = self.pending.remove(idx); - self.emit(ctx, pending.span); - } - } - - fn merge_from(&mut self, other: Self) { - self.emitted.extend(other.emitted); - for pending in other.pending { - if !self.pending.contains(&pending) { - self.pending.push(pending); + 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); } - } - } - - fn finish(&mut self, ctx: &LintContext) { - for pending in std::mem::take(&mut self.pending) { - self.emit(ctx, pending.span); - } - } -} - -fn check_block<'hir>( - ctx: &LintContext, - hir: &'hir Hir<'hir>, - block: Block<'hir>, - state: &mut ReturnUseState, -) { - for stmt in block.stmts { - check_stmt(ctx, hir, stmt, state); - } -} - -fn check_stmt<'hir>( - ctx: &LintContext, - hir: &'hir Hir<'hir>, - stmt: &'hir Stmt<'hir>, - state: &mut ReturnUseState, -) { - match &stmt.kind { - StmtKind::DeclSingle(var_id) => { - if let Some(init) = hir.variable(*var_id).initializer { - check_expr(ctx, hir, init, state); - if is_unused_return_call(hir, init) { - add_pending_var(ctx, hir, *var_id, init.span, state); - } + StmtKind::DeclMulti(vars, expr) + if vars.iter().any(Option::is_none) && is_unused_return_call(hir, expr) => + { + ctx.emit(&UNUSED_RETURN, expr.span); } + _ => {} } - StmtKind::DeclMulti(vars, expr) => { - check_expr(ctx, hir, expr, state); - update_multi_capture(ctx, hir, vars, expr, state); - } - StmtKind::Expr(expr) => { - if is_unused_return_call(hir, expr) { - state.emit(ctx, expr.span); - } - check_expr(ctx, hir, expr, state); - } - StmtKind::Emit(expr) | StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { - check_expr(ctx, hir, expr, state); - } - StmtKind::If(cond, then_stmt, else_stmt) => { - check_expr(ctx, hir, cond, state); - - let baseline = state.clone(); - let mut then_state = baseline.clone(); - check_stmt(ctx, hir, then_stmt, &mut then_state); - - let mut merged = ReturnUseState::default(); - merged.merge_from(then_state); - - if let Some(else_stmt) = else_stmt { - let mut else_state = baseline; - check_stmt(ctx, hir, else_stmt, &mut else_state); - merged.merge_from(else_state); - } else { - merged.merge_from(baseline); - } - - *state = merged; - } - StmtKind::Loop(block, _) => { - let baseline = state.clone(); - let mut loop_state = baseline.clone(); - check_block(ctx, hir, *block, &mut loop_state); - - let mut merged = baseline; - merged.merge_from(loop_state); - *state = merged; - } - StmtKind::Try(try_stmt) => { - check_expr(ctx, hir, &try_stmt.expr, state); - - let baseline = state.clone(); - let mut merged = baseline.clone(); - for clause in try_stmt.clauses { - let mut clause_state = baseline.clone(); - check_block(ctx, hir, clause.block, &mut clause_state); - merged.merge_from(clause_state); - } - *state = merged; - } - StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { - check_block(ctx, hir, *block, state); - } - StmtKind::AssemblyBlock(block) => check_block(ctx, hir, *block, state), - StmtKind::Switch(switch) => { - check_expr(ctx, hir, switch.selector, state); - - let baseline = state.clone(); - let mut merged = baseline.clone(); - for case in switch.cases { - let mut case_state = baseline.clone(); - check_block(ctx, hir, case.body, &mut case_state); - merged.merge_from(case_state); - } - *state = merged; - } - StmtKind::Return(None) - | StmtKind::Break - | StmtKind::Continue - | StmtKind::Placeholder - | StmtKind::Err(_) => {} } } -fn check_expr<'hir>( - ctx: &LintContext, - hir: &'hir Hir<'hir>, - expr: &'hir Expr<'hir>, - state: &mut ReturnUseState, -) { - match &expr.peel_parens().kind { - ExprKind::Assign(lhs, op, rhs) => { - check_expr(ctx, hir, rhs, state); - if op.is_some() { - check_expr(ctx, hir, lhs, state); - overwrite_lhs(ctx, hir, lhs, state); - } else { - check_lhs_reads(ctx, hir, lhs, state); - update_assignment_capture(ctx, hir, lhs, rhs, state); - } - } - ExprKind::Call(callee, args, options) => { - check_expr(ctx, hir, callee, state); - if let Some(options) = options { - for arg in options.args { - check_expr(ctx, hir, &arg.value, state); - } - } - for arg in args.exprs() { - check_expr(ctx, hir, arg, state); - } - } - ExprKind::Binary(lhs, _, rhs) => { - check_expr(ctx, hir, lhs, state); - check_expr(ctx, hir, rhs, state); - } - ExprKind::Unary(_, inner) - | ExprKind::Delete(inner) - | ExprKind::Member(inner, _) - | ExprKind::Payable(inner) - | ExprKind::YulMember(inner, _) => check_expr(ctx, hir, inner, state), - ExprKind::Ternary(cond, then_expr, else_expr) => { - check_expr(ctx, hir, cond, state); - - let baseline = state.clone(); - let mut then_state = baseline.clone(); - check_expr(ctx, hir, then_expr, &mut then_state); - let mut else_state = baseline; - check_expr(ctx, hir, else_expr, &mut else_state); - - let mut merged = ReturnUseState::default(); - merged.merge_from(then_state); - merged.merge_from(else_state); - *state = merged; - } - ExprKind::Tuple(exprs) => { - for expr in exprs.iter().flatten() { - check_expr(ctx, hir, expr, state); - } - } - ExprKind::Array(exprs) => { - for expr in *exprs { - check_expr(ctx, hir, expr, state); - } - } - ExprKind::Index(base, index) => { - check_expr(ctx, hir, base, state); - if let Some(index) = index { - check_expr(ctx, hir, index, state); - } - } - ExprKind::Slice(base, start, end) => { - check_expr(ctx, hir, base, state); - if let Some(start) = start { - check_expr(ctx, hir, start, state); - } - if let Some(end) = end { - check_expr(ctx, hir, end, state); - } - } - ExprKind::Ident(resolutions) => { - for res in *resolutions { - if let Res::Item(ItemId::Variable(var_id)) = res { - state.mark_read(*var_id); - } - } - } - ExprKind::Lit(_) - | ExprKind::New(_) - | ExprKind::TypeCall(_) - | ExprKind::Type(_) - | ExprKind::Err(_) => {} - } -} - -fn update_multi_capture( - ctx: &LintContext, - hir: &Hir<'_>, - vars: &[Option], - expr: &Expr<'_>, - state: &mut ReturnUseState, -) { - if let ExprKind::Tuple(exprs) = &expr.peel_parens().kind - && exprs.len() == vars.len() - { - for (var_id, rhs) in vars.iter().zip(*exprs) { - if let Some(var_id) = var_id { - if let Some(rhs) = rhs - && is_unused_return_call(hir, rhs) - { - add_pending_var(ctx, hir, *var_id, rhs.span, state); - } else { - state.mark_overwritten(ctx, *var_id); - } - } - } - return; - } - - if is_unused_return_call(hir, expr) { - for var_id in vars.iter().flatten() { - add_pending_var(ctx, hir, *var_id, expr.span, state); - } - } else { - for var_id in vars.iter().flatten() { - state.mark_overwritten(ctx, *var_id); - } - } -} - -fn update_assignment_capture( - ctx: &LintContext, - hir: &Hir<'_>, - lhs: &Expr<'_>, - rhs: &Expr<'_>, - state: &mut ReturnUseState, -) { - if let (ExprKind::Tuple(lhs_exprs), ExprKind::Tuple(rhs_exprs)) = - (&lhs.peel_parens().kind, &rhs.peel_parens().kind) - && lhs_exprs.len() == rhs_exprs.len() - { - for (lhs, rhs) in lhs_exprs.iter().zip(*rhs_exprs) { - if let Some(lhs) = lhs { - if let Some(rhs) = rhs - && is_unused_return_call(hir, rhs) - { - add_pending_lhs(ctx, hir, lhs, rhs.span, state); - } else { - overwrite_lhs(ctx, hir, lhs, state); - } - } - } - return; - } - - if is_unused_return_call(hir, rhs) { - add_pending_lhs(ctx, hir, lhs, rhs.span, state); - } else { - overwrite_lhs(ctx, hir, lhs, state); - } -} - -fn add_pending_lhs( - ctx: &LintContext, - hir: &Hir<'_>, - lhs: &Expr<'_>, - span: Span, - state: &mut ReturnUseState, -) { - let mut locals = Vec::new(); - collect_lhs_locals(hir, lhs, &mut locals); - for var_id in locals { - add_pending_var(ctx, hir, var_id, span, state); - } -} - -fn add_pending_var( - ctx: &LintContext, - hir: &Hir<'_>, - var_id: VariableId, - span: Span, - state: &mut ReturnUseState, -) { - if hir.variable(var_id).is_local_or_return() { - state.add_pending(ctx, var_id, span); - } -} - -fn overwrite_lhs(ctx: &LintContext, hir: &Hir<'_>, lhs: &Expr<'_>, state: &mut ReturnUseState) { - let mut locals = Vec::new(); - collect_lhs_locals(hir, lhs, &mut locals); - for var_id in locals { - state.mark_overwritten(ctx, var_id); - } -} - -fn collect_lhs_locals(hir: &Hir<'_>, lhs: &Expr<'_>, out: &mut Vec) { - match &lhs.peel_parens().kind { - ExprKind::Ident(resolutions) => { - for res in *resolutions { - if let Res::Item(ItemId::Variable(var_id)) = res - && hir.variable(*var_id).is_local_or_return() - { - out.push(*var_id); - } - } - } - ExprKind::Tuple(exprs) => { - for expr in exprs.iter().flatten() { - collect_lhs_locals(hir, expr, out); - } - } - _ => {} - } -} - -fn check_lhs_reads<'hir>( - ctx: &LintContext, - hir: &'hir Hir<'hir>, - lhs: &'hir Expr<'hir>, - state: &mut ReturnUseState, -) { - match &lhs.peel_parens().kind { - ExprKind::Ident(_) => {} - ExprKind::Tuple(exprs) => { - for expr in exprs.iter().flatten() { - check_lhs_reads(ctx, hir, expr, state); - } - } - ExprKind::Member(base, _) | ExprKind::YulMember(base, _) => { - check_expr(ctx, hir, base, state); - } - ExprKind::Index(base, index) => { - check_expr(ctx, hir, base, state); - if let Some(index) = index { - check_expr(ctx, hir, index, state); - } - } - ExprKind::Slice(base, start, end) => { - check_expr(ctx, hir, base, state); - if let Some(start) = start { - check_expr(ctx, hir, start, state); - } - if let Some(end) = end { - check_expr(ctx, hir, end, state); - } - } - _ => check_expr(ctx, hir, lhs, state), - } +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 @@ -464,36 +65,32 @@ fn is_unused_return_call(hir: &Hir<'_>, expr: &Expr<'_>) -> bool { 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/UnusedReturn.sol b/crates/lint/testdata/UnusedReturn.sol index 7fd8162cdad14..59e6aeae0000d 100644 --- a/crates/lint/testdata/UnusedReturn.sol +++ b/crates/lint/testdata/UnusedReturn.sol @@ -94,35 +94,42 @@ contract UnusedReturn { (IOracle(oracleAddr)).getPrice(t); //~WARN: Return value of an external call is not used } - // SHOULD FAIL: return value captured but never read + // SHOULD FAIL: tuple return has an ignored slot function bad7(address t) external { - uint256 price = oracle.getPrice(t); //~WARN: Return value of an external call is not used - price = 1; + (uint256 price, ) = oracle.latest(t); //~WARN: Return value of an external call is not used + price = price + 1; } - // SHOULD FAIL: bool return captured but never read - function bad8() external { - bool ok = oracle.update(); //~WARN: Return value of an external call is not used - ok = true; + // 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 FAIL: tuple return values captured but never read - function bad9(address t) external { - (uint256 price, bool ok) = oracle.latest(t); //~WARN: Return value of an external call is not used + // SHOULD PASS: captured return alone is considered used + function good7(address t) external { + uint256 price = oracle.getPrice(t); price = 1; - ok = true; } // SHOULD PASS: captured return is read before overwrite - function good7(address t) external returns (uint256) { + 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 good8(address t) external returns (uint256) { + function good10(address t) external returns (uint256) { (uint256 price, bool ok) = oracle.latest(t); uint256 out = price; bool ready = ok; @@ -131,7 +138,7 @@ contract UnusedReturn { } // SHOULD PASS: captured ERC20 transfer remains excluded - function good9(address to, uint256 amt) external { + 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 76bc180578bb7..a98499c6b0e72 100644 --- a/crates/lint/testdata/UnusedReturn.stderr +++ b/crates/lint/testdata/UnusedReturn.stderr @@ -49,24 +49,16 @@ LL │ (IOracle(oracleAddr)).getPrice(t); warning[unused-return]: Return value of an external call is not used ╭▸ ROOT/testdata/UnusedReturn.sol:LL:CC │ -LL │ uint256 price = oracle.getPrice(t); - │ ━━━━━━━━━━━━━━━━━━ +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 │ bool ok = oracle.update(); - │ ━━━━━━━━━━━━━━━ - │ - ╰ 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, bool ok) = oracle.latest(t); - │ ━━━━━━━━━━━━━━━━ +LL │ (price, ) = oracle.latest(t); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━ │ ╰ help: https://getfoundry.sh/forge/linting/unused-return From 3a4fe7ddfadaf4df19ecf09bfc067aebca6b9d17 Mon Sep 17 00:00:00 2001 From: Gustavo Figueiredo Date: Sun, 7 Jun 2026 10:40:19 +0100 Subject: [PATCH 7/7] fix(lint): avoid stale branch taint false positives --- crates/lint/src/sol/low/block_timestamp.rs | 80 ++++++++---- crates/lint/src/sol/med/div_mul.rs | 114 +++++++++++++----- crates/lint/testdata/BlockTimestamp.sol | 18 +++ crates/lint/testdata/DivideBeforeMultiply.sol | 36 ++++++ 4 files changed, 197 insertions(+), 51 deletions(-) diff --git a/crates/lint/src/sol/low/block_timestamp.rs b/crates/lint/src/sol/low/block_timestamp.rs index 572fdb7225540..3833a4051b8bd 100644 --- a/crates/lint/src/sol/low/block_timestamp.rs +++ b/crates/lint/src/sol/low/block_timestamp.rs @@ -46,10 +46,13 @@ fn check_block<'hir>( helpers: &HashSet, block: Block<'hir>, aliases: &mut HashSet, -) { +) -> bool { for stmt in block.stmts { - check_stmt(ctx, hir, helpers, stmt, aliases); + if !check_stmt(ctx, hir, helpers, stmt, aliases) { + return false; + } } + true } fn check_stmt<'hir>( @@ -58,7 +61,7 @@ fn check_stmt<'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 { @@ -70,35 +73,63 @@ fn check_stmt<'hir>( 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), - StmtKind::Emit(expr) | StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { + 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(); - check_stmt(ctx, hir, helpers, then_stmt, &mut then_aliases); + 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; - check_stmt(ctx, hir, helpers, else_stmt, &mut else_aliases); - *aliases = then_aliases.union(&else_aliases).copied().collect(); + if check_stmt(ctx, hir, helpers, else_stmt, &mut else_aliases) { + merged.extend(else_aliases); + falls_through = true; + } } else { - *aliases = baseline.union(&then_aliases).copied().collect(); + 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(); - check_block(ctx, hir, helpers, *block, &mut loop_aliases); - *aliases = baseline.union(&loop_aliases).copied().collect(); + *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); @@ -107,13 +138,15 @@ fn check_stmt<'hir>( let mut merged = baseline.clone(); for clause in try_stmt.clauses { let mut clause_aliases = baseline.clone(); - check_block(ctx, hir, helpers, clause.block, &mut clause_aliases); - merged.extend(clause_aliases); + 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); + check_block(ctx, hir, helpers, *block, aliases) } StmtKind::AssemblyBlock(block) => check_block(ctx, hir, helpers, *block, aliases), StmtKind::Switch(switch) => { @@ -123,16 +156,15 @@ fn check_stmt<'hir>( let mut merged = baseline.clone(); for case in switch.cases { let mut case_aliases = baseline.clone(); - check_block(ctx, hir, helpers, case.body, &mut case_aliases); - merged.extend(case_aliases); + if check_block(ctx, hir, helpers, case.body, &mut case_aliases) { + merged.extend(case_aliases); + } } *aliases = merged; + true } - StmtKind::Return(None) - | StmtKind::Break - | StmtKind::Continue - | StmtKind::Placeholder - | StmtKind::Err(_) => {} + StmtKind::Return(None) => false, + StmtKind::Break | StmtKind::Continue | StmtKind::Placeholder | StmtKind::Err(_) => true, } } @@ -462,6 +494,12 @@ fn is_timestamp_helper_call(helpers: &HashSet, expr: &Expr<'_>) -> b 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 diff --git a/crates/lint/src/sol/med/div_mul.rs b/crates/lint/src/sol/med/div_mul.rs index c4a742889861d..21e6ea3cd9f42 100644 --- a/crates/lint/src/sol/med/div_mul.rs +++ b/crates/lint/src/sol/med/div_mul.rs @@ -3,10 +3,15 @@ use crate::{ linter::{LateLintPass, LintContext}, sol::{Severity, SolLint}, }; -use solar::sema::{ - Gcx, Hir, - builtins::Builtin, - hir::{BinOpKind, Block, Expr, ExprKind, Function, ItemId, Res, Stmt, StmtKind, VariableId}, +use solar::{ + ast::UnOpKind, + sema::{ + Gcx, Hir, + builtins::Builtin, + hir::{ + BinOpKind, Block, Expr, ExprKind, Function, ItemId, Res, Stmt, StmtKind, VariableId, + }, + }, }; use std::collections::HashSet; @@ -37,10 +42,13 @@ fn check_block<'hir>( hir: &'hir Hir<'hir>, block: Block<'hir>, tainted: &mut HashSet, -) { +) -> bool { for stmt in block.stmts { - check_stmt(ctx, hir, stmt, tainted); + if !check_stmt(ctx, hir, stmt, tainted) { + return false; + } } + true } fn check_stmt<'hir>( @@ -48,7 +56,7 @@ fn check_stmt<'hir>( 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 { @@ -60,43 +68,75 @@ fn check_stmt<'hir>( 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::Expr(expr) => check_expr(ctx, hir, expr, tainted), - StmtKind::Emit(expr) | StmtKind::Revert(expr) | StmtKind::Return(Some(expr)) => { - check_expr(ctx, hir, expr, tainted) + 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 mut then_tainted = tainted.clone(); - check_stmt(ctx, hir, then_stmt, &mut then_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 = tainted.clone(); - check_stmt(ctx, hir, else_stmt, &mut else_tainted); - *tainted = union_taints(&then_tainted, &else_tainted); + 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 { - *tainted = union_taints(tainted, &then_tainted); + merged_taint = union_taints(&merged_taint, &baseline); + falls_through = true; } + + if falls_through { + *tainted = merged_taint; + } + falls_through } StmtKind::Loop(block, _) => { - let mut loop_tainted = tainted.clone(); - check_block(ctx, hir, *block, &mut loop_tainted); - *tainted = union_taints(tainted, &loop_tainted); + 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(); - check_block(ctx, hir, clause.block, &mut clause_tainted); - merged_taint = union_taints(&merged_taint, &clause_tainted); + 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) @@ -107,16 +147,15 @@ fn check_stmt<'hir>( let mut merged_taint = tainted.clone(); for case in switch.cases { let mut case_tainted = tainted.clone(); - check_block(ctx, hir, case.body, &mut case_tainted); - merged_taint = union_taints(&merged_taint, &case_tainted); + 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) - | StmtKind::Break - | StmtKind::Continue - | StmtKind::Placeholder - | StmtKind::Err(_) => {} + StmtKind::Return(None) => false, + StmtKind::Break | StmtKind::Continue | StmtKind::Placeholder | StmtKind::Err(_) => true, } } @@ -146,7 +185,7 @@ fn check_expr<'hir>( Some(op) if op.kind == BinOpKind::Div => { update_lhs_taint(hir, lhs, true, tainted); } - Some(_) => {} + Some(_) => update_lhs_taint(hir, lhs, false, tainted), } } ExprKind::Binary(left, op, right) => { @@ -212,7 +251,12 @@ fn check_expr<'hir>( check_expr(ctx, hir, expr, tainted); } } - ExprKind::Unary(_, inner) => check_expr(ctx, hir, inner, 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(_) @@ -365,6 +409,16 @@ 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 { diff --git a/crates/lint/testdata/BlockTimestamp.sol b/crates/lint/testdata/BlockTimestamp.sol index ed23a7a8a1299..2245580f7047a 100644 --- a/crates/lint/testdata/BlockTimestamp.sol +++ b/crates/lint/testdata/BlockTimestamp.sol @@ -97,6 +97,24 @@ contract BlockTimestamp { 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; } diff --git a/crates/lint/testdata/DivideBeforeMultiply.sol b/crates/lint/testdata/DivideBeforeMultiply.sol index 4e169c3dbcd4f..aec789e8087f0 100644 --- a/crates/lint/testdata/DivideBeforeMultiply.sol +++ b/crates/lint/testdata/DivideBeforeMultiply.sol @@ -53,6 +53,30 @@ contract DivideBeforeMultiply { 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 @@ -65,6 +89,18 @@ contract DivideBeforeMultiply { 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