refactor(lint): improve selected Slither-parity lints (#15086)

* Improve lint parity with Slither

* Improve Slither lint parity

* test(forge): update lint parity snapshots

Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e9872-f047-7033-acf4-1f985641d25e

* fix(lint): tighten divide-before-multiply taint

Amp-Thread-ID: https://ampcode.com/threads/T-019e9872-f047-7033-acf4-1f985641d25e
Co-authored-by: Amp <amp@ampcode.com>

* fix(lint): improve Slither parity for return and timestamp

* fix(lint): avoid unused-return branch false positives

* fix(lint): avoid stale branch taint false positives

---------

Co-authored-by: Amp <amp@ampcode.com>

Author: figtracer

crates/forge/tests/cli/lint.rs

@@ -18,9 +18,9 @@ uint256 constant screaming_snake_case_info = 0;
 contract ContractWithLints {
     uint256 VARIABLE_MIXED_CASE_INFO;
 
-    function incorrectShiftHigh() public {
-        uint256 localValue = 50;
-        uint256 result = 8 >> localValue;
+    function incorrectShiftHigh() public pure {
+        uint256 result = 8;
+        assembly { result := shr(result, 8) }
     }
     function divideBeforeMultiplyMedium() public {
         (1 / 2) * 3;
@@ -666,10 +666,10 @@ forgetest!(can_override_config_lint, |prj, cmd| {
     cmd.arg("lint").args(["--only-lint", "incorrect-shift"]).assert_success().stderr_eq(str![[
         r#"
 warning[incorrect-shift]: the order of args in a shift operation is incorrect
-   [FILE]:13:26
+   [FILE]:13:30
    │
-13 │         uint256 result = 8 >> localValue;
-   │                          ━━━━━━━━━━━━━━━
+13 │         assembly { result := shr(result, 8) }
+   │                              ━━━━━━━━━━━━━━
    │
    ╰ help: https://getfoundry.sh/forge/linting/incorrect-shift
 
@@ -707,24 +707,12 @@ warning[divide-before-multiply]: multiplication should occur before division to
 [COMPILING_FILES] with [SOLC_VERSION]
 [SOLC_VERSION] [ELAPSED]
 Compiler run successful with warnings:
-Warning (2072): Unused local variable.
-  [FILE]:13:9:
-   |
-13 |         uint256 result = 8 >> localValue;
-   |         ^^^^^^^^^^^^^^
-
 Warning (6133): Statement has no effect.
   [FILE]:16:9:
    |
 16 |         (1 / 2) * 3;
    |         ^^^^^^^^^^^
 
-Warning (2018): Function state mutability can be restricted to pure
-  [FILE]:11:5:
-   |
-11 |     function incorrectShiftHigh() public {
-   |     ^ (Relevant source part starts here and spans across multiple lines).
-
 Warning (2018): Function state mutability can be restricted to pure
   [FILE]:15:5:
    |
@@ -806,24 +794,12 @@ forgetest!(build_respects_lint_on_build_false, |prj, cmd| {
 [COMPILING_FILES] with [SOLC_VERSION]
 [SOLC_VERSION] [ELAPSED]
 Compiler run successful with warnings:
-Warning (2072): Unused local variable.
-  [FILE]:13:9:
-   |
-13 |         uint256 result = 8 >> localValue;
-   |         ^^^^^^^^^^^^^^
-
 Warning (6133): Statement has no effect.
   [FILE]:16:9:
    |
 16 |         (1 / 2) * 3;
    |         ^^^^^^^^^^^
 
-Warning (2018): Function state mutability can be restricted to pure
-  [FILE]:11:5:
-   |
-11 |     function incorrectShiftHigh() public {
-   |     ^ (Relevant source part starts here and spans across multiple lines).
-
 Warning (2018): Function state mutability can be restricted to pure
   [FILE]:15:5:
    |
@@ -882,24 +858,12 @@ forgetest!(build_no_lint_flag_skips_lint, |prj, cmd| {
 [COMPILING_FILES] with [SOLC_VERSION]
 [SOLC_VERSION] [ELAPSED]
 Compiler run successful with warnings:
-Warning (2072): Unused local variable.
-  [FILE]:13:9:
-   |
-13 |         uint256 result = 8 >> localValue;
-   |         ^^^^^^^^^^^^^^
-
 Warning (6133): Statement has no effect.
   [FILE]:16:9:
    |
 16 |         (1 / 2) * 3;
    |         ^^^^^^^^^^^
 
-Warning (2018): Function state mutability can be restricted to pure
-  [FILE]:11:5:
-   |
-11 |     function incorrectShiftHigh() public {
-   |     ^ (Relevant source part starts here and spans across multiple lines).
-
 Warning (2018): Function state mutability can be restricted to pure
   [FILE]:15:5:
    |
@@ -1427,22 +1391,14 @@ 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;
   │ ━━━━━━━━━━━━━━━━━━━━━━━━
   │
   ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent
 
-note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20
-  [FILE]:3:1
-  │
-3 │ pragma solidity 0.8.20;
-  │ ━━━━━━━━━━━━━━━━━━━━━━━
-  │
-  ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent
-
 
 "#]
     ]);
@@ -1538,30 +1494,14 @@ forgetest!(pragma_inconsistent_duplicates_among_conflict, |prj, cmd| {
 
     cmd.arg("lint").args(["--only-lint", "pragma-inconsistent"]).assert_success().stderr_eq(str![
         [r#"
-note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20
+note[pragma-inconsistent]: 2 different Solidity pragma version requirements are used: 0.8.20, ^0.8.20
   [FILE]:3:1
   │
 3 │ pragma solidity 0.8.20;
   │ ━━━━━━━━━━━━━━━━━━━━━━━
   │
   ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent
 
-note[pragma-inconsistent]: 'pragma solidity 0.8.20;' conflicts with other version requirements in the project: ^0.8.20
-  [FILE]:3:1
-  │
-3 │ pragma solidity 0.8.20;
-  │ ━━━━━━━━━━━━━━━━━━━━━━━
-  │
-  ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent
-
-note[pragma-inconsistent]: 'pragma solidity ^0.8.20;' conflicts with other version requirements in the project: 0.8.20
-  [FILE]:3:1
-  │
-3 │ pragma solidity ^0.8.20;
-  │ ━━━━━━━━━━━━━━━━━━━━━━━━
-  │
-  ╰ help: https://getfoundry.sh/forge/linting/pragma-inconsistent
-
 
 "#]
     ]);
@@ -1578,22 +1518,14 @@ 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;
   │ ━━━━━━━━━━━━━━━━━━━━━━━
   │
   ╰ 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
-
 
 "#]
     ]);

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
+}
 ```

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?
 

crates/lint/src/sol/analysis/interface.rs

@@ -10,18 +10,22 @@ 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<ContractId> {
-    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)
             } else {
                 None
             }
         }
-        ExprKind::Call(
-            Expr { kind: ExprKind::Ident([Res::Item(ItemId::Contract(cid))]), .. },
-            ..,
-        ) => Some(*cid),
+        ExprKind::Call(callee, ..) => {
+            if let ExprKind::Ident([Res::Item(ItemId::Contract(cid))]) = &callee.peel_parens().kind
+            {
+                Some(*cid)
+            } else {
+                None
+            }
+        }
         _ => None,
     }
 }

crates/lint/src/sol/high/incorrect_shift.rs

@@ -3,7 +3,10 @@ use crate::{
     linter::{EarlyLintPass, LintContext},
     sol::{Severity, SolLint},
 };
-use solar::ast::{BinOp, BinOpKind, Expr, ExprKind};
+use solar::{
+    ast::{Stmt, StmtKind, yul},
+    interface::kw,
+};
 
 declare_forge_lint!(
     INCORRECT_SHIFT,
@@ -13,28 +16,62 @@ declare_forge_lint!(
 );
 
 impl<'ast> EarlyLintPass<'ast> for IncorrectShift {
-    fn check_expr(&mut self, ctx: &LintContext, expr: &'ast Expr<'ast>) {
-        if let ExprKind::Binary(
-            left_expr,
-            BinOp { kind: BinOpKind::Shl | BinOpKind::Shr, .. },
-            right_expr,
-        ) = &expr.kind
-            && contains_incorrect_shift(left_expr, right_expr)
-        {
-            ctx.emit(&INCORRECT_SHIFT, expr.span);
+    fn check_stmt(&mut self, ctx: &LintContext, stmt: &'ast Stmt<'ast>) {
+        if let StmtKind::Assembly(assembly) = &stmt.kind {
+            check_yul_block(ctx, &assembly.block);
         }
     }
 }
 
-// TODO: come up with a better heuristic. Treat initial impl as a PoC.
-// Checks if the left operand is a literal and the right operand is not, indicating a potential
-// reversed shift operation.
-const fn contains_incorrect_shift<'ast>(
-    left_expr: &'ast Expr<'ast>,
-    right_expr: &'ast Expr<'ast>,
-) -> bool {
-    let is_left_literal = matches!(left_expr.kind, ExprKind::Lit(..));
-    let is_right_not_literal = !matches!(right_expr.kind, ExprKind::Lit(..));
+fn check_yul_block(ctx: &LintContext, block: &yul::Block<'_>) {
+    for stmt in block.stmts.iter() {
+        check_yul_stmt(ctx, stmt);
+    }
+}
 
-    is_left_literal && is_right_not_literal
+fn check_yul_stmt(ctx: &LintContext, stmt: &yul::Stmt<'_>) {
+    match &stmt.kind {
+        yul::StmtKind::Block(block) => check_yul_block(ctx, block),
+        yul::StmtKind::AssignSingle(_, expr)
+        | yul::StmtKind::AssignMulti(_, expr)
+        | yul::StmtKind::Expr(expr) => check_yul_expr(ctx, expr),
+        yul::StmtKind::If(cond, block) => {
+            check_yul_expr(ctx, cond);
+            check_yul_block(ctx, block);
+        }
+        yul::StmtKind::For(for_stmt) => {
+            check_yul_block(ctx, &for_stmt.init);
+            check_yul_expr(ctx, &for_stmt.cond);
+            check_yul_block(ctx, &for_stmt.step);
+            check_yul_block(ctx, &for_stmt.body);
+        }
+        yul::StmtKind::Switch(switch) => {
+            check_yul_expr(ctx, &switch.selector);
+            for case in switch.cases.iter() {
+                check_yul_block(ctx, &case.body);
+            }
+        }
+        yul::StmtKind::FunctionDef(func) => check_yul_block(ctx, &func.body),
+        yul::StmtKind::VarDecl(_, Some(init)) => check_yul_expr(ctx, init),
+        yul::StmtKind::Leave
+        | yul::StmtKind::Break
+        | yul::StmtKind::Continue
+        | yul::StmtKind::VarDecl(_, None) => {}
+    }
+}
+
+fn check_yul_expr(ctx: &LintContext, expr: &yul::Expr<'_>) {
+    let yul::ExprKind::Call(call) = &expr.kind else { return };
+
+    if matches!(call.name.name, kw::Shl | kw::Shr | kw::Sar)
+        && let [left, right] = call.arguments.as_ref()
+        && !matches!(left.kind, yul::ExprKind::Lit(_))
+        && matches!(right.kind, yul::ExprKind::Lit(_))
+    {
+        ctx.emit(&INCORRECT_SHIFT, expr.span);
+    }
+
+    for arg in call.arguments.iter() {
+        check_yul_expr(ctx, arg);
+    }
 }

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::<Vec<_>>()
-                .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);
     }
 }