feat(lint): add solmate-safe-transfer-lib detector

crates/lint/README.md

@@ -49,6 +49,7 @@ It helps enforce best practices and improve code quality within Foundry projects
   - `missing-zero-check`: Address parameter is used in a state write or value transfer without a zero-address check.
   - `reentrancy-events`: Events emitted after external calls can be reordered or fabricated by a reentrant callee and mislead off-chain consumers.
   - `return-bomb`: External calls with a gas limit should not consume unbounded return data.
+  - `solmate-safe-transfer-lib`: solmate's released `SafeTransferLib` does not check that the token has code, so token operations against a token-less address succeed silently.
 - **Informational / Style Guide:**
   - `boolean-equal`: Boolean comparisons to constants should be simplified.
   - `too-many-digits`: Numeric literals with 5+ consecutive zeros are error-prone.

crates/lint/docs/solmate-safe-transfer-lib.md

@@ -0,0 +1,48 @@
+# Solmate SafeTransferLib
+
+**Severity**: `Low`
+**ID**: `solmate-safe-transfer-lib`
+
+Flags token operations of solmate's `SafeTransferLib`, which does not check that the token has code in its released version.
+
+## What it does
+
+Reports a reference, called or used as a value, that resolves to `safeTransfer`, `safeTransferFrom` or `safeApprove` declared in a library named exactly `SafeTransferLib` whose source comes from a solmate package path (`lib/solmate`, `solmate/...`). Resolution goes through the type checker, so the `using for` method form, the library-qualified form and import aliases are all recognized, while same-name functions declared in other libraries (Uniswap's `TransferHelper` style) and same-name libraries from other packages (Solady's `SafeTransferLib` checks token code on the empty-return path) stay out of scope. `safeTransferETH` involves no token code and stays clean. A vendored solmate copy under a path that does not name solmate is not recognized.
+
+Aderyn's detector of the same name flags the import directive whose path contains `solmate` and `SafeTransferLib`; resolving the calls instead anchors the warning where the risk sits and skips files that import the library without using it.
+
+## Why is this bad?
+
+In the released solmate v6, a token call that returns no data is treated as a success without checking that the token has code (`success := 1` on the empty-return path), unlike OpenZeppelin's `SafeERC20`. A token operation against an address with no code, a wrong address, a not-yet-deployed or a self-destructed token, is therefore a silent no-op that looks like a successful transfer. The unreleased solmate main branch has since added a code check to the empty-return path; on a released version, the mitigation is to verify the token has code, or to use OpenZeppelin's `SafeERC20`.
+
+## Example
+
+### Bad
+
+```solidity
+using SafeTransferLib for ERC20;
+
+function pay(ERC20 token, address to, uint256 amount) internal {
+    token.safeTransfer(to, amount);
+}
+```
+
+### Good
+
+```solidity
+using SafeERC20 for IERC20;
+
+function pay(IERC20 token, address to, uint256 amount) internal {
+    token.safeTransfer(to, amount);
+}
+```
+
+The lint flags every resolved solmate reference and does not recognize a manual code check
+before the call. A call guarded by `require(address(token).code.length > 0, ...)` mitigates
+the pitfall but still reports; suppress it explicitly:
+
+```solidity
+require(address(token).code.length > 0, "token has no code");
+// forge-lint: disable-next-line(solmate-safe-transfer-lib)
+token.safeTransfer(to, amount);
+```

crates/lint/src/sol/low/mod.rs

@@ -41,6 +41,9 @@ use reentrancy_events::REENTRANCY_EVENTS;
 mod require_revert_in_loop;
 use require_revert_in_loop::REQUIRE_REVERT_IN_LOOP;
 
+mod solmate_safe_transfer_lib;
+use solmate_safe_transfer_lib::SOLMATE_SAFE_TRANSFER_LIB;
+
 register_lints!(
     (BlockTimestamp, late, (BLOCK_TIMESTAMP)),
     (CallsLoop, late, (CALLS_LOOP)),
@@ -55,4 +58,5 @@ register_lints!(
     (ReturnBomb, late, (RETURN_BOMB)),
     (ReentrancyEvents, late, (REENTRANCY_EVENTS)),
     (RequireRevertInLoop, late, (REQUIRE_REVERT_IN_LOOP)),
+    (SolmateSafeTransferLib, late, (SOLMATE_SAFE_TRANSFER_LIB)),
 );

crates/lint/src/sol/low/solmate_safe_transfer_lib.rs

@@ -0,0 +1,114 @@
+use super::SolmateSafeTransferLib;
+use crate::{
+    linter::{LateLintPass, LintContext},
+    sol::{Severity, SolLint},
+};
+use solar::{
+    interface::source_map::FileName,
+    sema::{
+        Gcx,
+        hir::{self, Expr, ExprKind, FunctionId, Hir, Visit},
+        ty::TyKind,
+    },
+};
+use std::{convert::Infallible, ops::ControlFlow};
+
+declare_forge_lint!(
+    SOLMATE_SAFE_TRANSFER_LIB,
+    Severity::Low,
+    "solmate-safe-transfer-lib",
+    "Solmate's `SafeTransferLib` does not check that the token has code, so a transfer to a token-less address succeeds silently"
+);
+
+impl<'hir> LateLintPass<'hir> for SolmateSafeTransferLib {
+    fn check_function(
+        &mut self,
+        ctx: &LintContext,
+        gcx: Gcx<'hir>,
+        hir: &'hir Hir<'hir>,
+        func: &'hir hir::Function<'hir>,
+    ) {
+        if let Some(body) = &func.body {
+            let mut finder = TokenOpFinder { gcx, hir, ctx };
+            for stmt in body.stmts {
+                let _ = finder.visit_stmt(stmt);
+            }
+        }
+    }
+}
+
+/// Looks for references to `SafeTransferLib`'s token operations, called or used as values.
+struct TokenOpFinder<'ctx, 's, 'c, 'hir> {
+    gcx: Gcx<'hir>,
+    hir: &'hir Hir<'hir>,
+    ctx: &'ctx LintContext<'s, 'c>,
+}
+
+impl<'hir> Visit<'hir> for TokenOpFinder<'_, '_, '_, 'hir> {
+    type BreakValue = Infallible;
+
+    fn hir(&self) -> &'hir Hir<'hir> {
+        self.hir
+    }
+
+    fn visit_expr(&mut self, expr: &'hir Expr<'hir>) -> ControlFlow<Self::BreakValue> {
+        // A name or member expression typed as a function is a resolved reference, called or
+        // used as a value: judge the single declaration the type checker selected. The callee
+        // of a call is visited by the default walk, so calls need no dedicated arm.
+        if matches!(expr.kind, ExprKind::Ident(..) | ExprKind::Member(..))
+            && let Some(function_id) = self.resolved_function(expr)
+            && self.is_unchecked_token_op(function_id)
+        {
+            self.ctx.emit(&SOLMATE_SAFE_TRANSFER_LIB, expr.span);
+        }
+        self.walk_expr(expr)
+    }
+}
+
+impl TokenOpFinder<'_, '_, '_, '_> {
+    /// The single function an expression resolves to, for a callee or a reference used as a
+    /// value. `type_of_expr` is the function the type checker resolved, so overload selection,
+    /// override shadowing, the qualified and `using for` forms and import aliases are already
+    /// accounted for.
+    fn resolved_function(&self, expr: &Expr<'_>) -> Option<FunctionId> {
+        let ty = self.gcx.type_of_expr(expr.peel_parens().id)?;
+        match ty.kind {
+            TyKind::Fn(function_ty) => function_ty.function_id,
+            _ => None,
+        }
+    }
+
+    /// Whether `function_id` is one of the token operations of solmate's `SafeTransferLib`.
+    /// `safeTransferETH` stays out: sending ETH involves no token code, so the missing-code
+    /// concern does not apply to it. A same-name function of another library (Uniswap's
+    /// `TransferHelper` style) stays out through the resolution, and so does a same-name
+    /// library from another package (Solady's `SafeTransferLib` checks token code on the
+    /// empty-return path), which fails the provenance check.
+    fn is_unchecked_token_op(&self, function_id: FunctionId) -> bool {
+        let function = self.hir.function(function_id);
+        let Some(name) = function.name else { return false };
+        let Some(contract_id) = function.contract else { return false };
+        // The name alone does not prove the declaration is solmate's: the declaring source
+        // must come from a solmate package path (`lib/solmate`, `solmate/...`).
+        if !self.is_solmate_source(function.source) {
+            return false;
+        }
+        let contract = self.hir.contract(contract_id);
+        matches!(name.as_str(), "safeTransfer" | "safeTransferFrom" | "safeApprove")
+            && contract.kind.is_library()
+            && contract.name.as_str() == "SafeTransferLib"
+    }
+
+    /// Whether a source file belongs to the solmate package, judged by a full path component.
+    /// Matching a whole component rather than a substring keeps a vendored or patched copy under
+    /// a misleading path such as `vendor/solmate-fixed/` from being recognized.
+    fn is_solmate_source(&self, source_id: hir::SourceId) -> bool {
+        match &self.hir.source(source_id).file.name {
+            FileName::Real(path) => path.components().any(|component| {
+                matches!(component, std::path::Component::Normal(name)
+                    if name.eq_ignore_ascii_case("solmate"))
+            }),
+            _ => false,
+        }
+    }
+}

crates/lint/testdata/SolmateSafeTransferLib.sol

@@ -0,0 +1,152 @@
+//@compile-flags: --only-lint solmate-safe-transfer-lib
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.18;
+
+import {IToken, SafeTransferLib} from "./auxiliary/solmate/SolmateMocks.sol";
+import {SafeTransferLib as STL, ITokenAux} from "./auxiliary/solmate/SolmateSafeTransferLib.sol";
+import {SafeTransferLib as SoladySTL} from "./auxiliary/solady/SafeTransferLib.sol";
+import {SafeTransferLib as FixedSTL, IToken as ITokenFixed} from "./auxiliary/solmate-fixed/SafeTransferLib.sol";
+
+// Tests for `solmate-safe-transfer-lib`: the released solmate v6 `SafeTransferLib` treats a
+// call that returns no data as a success without checking that the token has code, so a token
+// operation against a token-less address is a silent no-op. A reference is flagged when it
+// resolves to `safeTransfer` / `safeTransferFrom` / `safeApprove` declared in a library named
+// exactly `SafeTransferLib` whose source comes from a solmate package path, whatever the call
+// form. `safeTransferETH` involves no token code and stays clean, as do same-name functions
+// declared in other libraries and same-name libraries from other packages (Solady).
+// Note: this file's own path names solmate, so the canonical mirror and the out-of-package
+// probes live in auxiliary files whose paths decide their provenance.
+
+// Same function names in another library (Uniswap's TransferHelper style): out of scope.
+library TransferHelper {
+    function safeTransfer(IToken token, address to, uint256 amount) internal {
+        token.transfer(to, amount);
+    }
+
+    function safeTransferFrom(IToken token, address from, address to, uint256 amount) internal {
+        token.transferFrom(from, to, amount);
+    }
+}
+
+contract UsesSolmate {
+    using SafeTransferLib for IToken;
+
+    IToken internal token;
+
+    function viaUsingFor(address to, uint256 amount) internal {
+        token.safeTransfer(to, amount); //~WARN: Solmate's `SafeTransferLib` does not check
+    }
+
+    function viaQualified(address from, address to, uint256 amount) internal {
+        SafeTransferLib.safeTransferFrom(token, from, to, amount); //~WARN: Solmate's `SafeTransferLib` does not check
+    }
+
+    function viaApprove(address spender, uint256 amount) internal {
+        token.safeApprove(spender, amount); //~WARN: Solmate's `SafeTransferLib` does not check
+    }
+
+    function ethTransferIsClean(address to, uint256 amount) internal {
+        SafeTransferLib.safeTransferETH(to, amount);
+    }
+}
+
+contract UsesAliasedImport {
+    ITokenAux internal token;
+
+    // The import alias renames the call site, not the declared library the call resolves to.
+    function viaAlias(address to, uint256 amount) internal {
+        STL.safeTransfer(token, to, amount); //~WARN: Solmate's `SafeTransferLib` does not check
+    }
+}
+
+// The same operations through Uniswap-style TransferHelper: out of scope.
+contract UsesTransferHelper {
+    using TransferHelper for IToken;
+
+    IToken internal token;
+
+    function viaHelper(address to, uint256 amount) internal {
+        token.safeTransfer(to, amount);
+    }
+
+    function viaHelperQualified(address from, address to, uint256 amount) internal {
+        TransferHelper.safeTransferFrom(token, from, to, amount);
+    }
+}
+
+// The granular form binds a single function: it resolves to the same declaration.
+contract GranularUsing {
+    using {SafeTransferLib.safeTransfer} for IToken;
+
+    IToken internal token;
+
+    function viaGranular(address to, uint256 amount) internal {
+        token.safeTransfer(to, amount); //~WARN: Solmate's `SafeTransferLib` does not check
+    }
+}
+
+// Calls in a constructor or a modifier are calls like any other.
+contract EagerPayer {
+    using SafeTransferLib for IToken;
+
+    IToken internal token;
+
+    constructor(address to) {
+        token.safeTransfer(to, 1); //~WARN: Solmate's `SafeTransferLib` does not check
+    }
+
+    modifier paying(address to, uint256 amount) {
+        token.safeTransfer(to, amount); //~WARN: Solmate's `SafeTransferLib` does not check
+        _;
+    }
+
+    function noop(address to, uint256 amount) external paying(to, amount) {}
+}
+
+// A free function is analyzed like a contract function.
+function freePay(IToken token, address to, uint256 amount) {
+    SafeTransferLib.safeTransfer(token, to, amount); //~WARN: Solmate's `SafeTransferLib` does not check
+}
+
+// A reference used as a value is a use of the unchecked operation too.
+contract RefUser {
+    function pick() internal pure returns (function(IToken, address, uint256) internal) {
+        return SafeTransferLib.safeTransfer; //~WARN: Solmate's `SafeTransferLib` does not check
+    }
+}
+
+// A contract's own `safeTransfer` is not the solmate library: out of scope.
+contract NotALibrary {
+    function safeTransfer(IToken token, address to, uint256 amount) internal {
+        token.transfer(to, amount);
+    }
+
+    function viaContract(IToken token, address to, uint256 amount) internal {
+        safeTransfer(token, to, amount);
+    }
+}
+
+// Solady's SafeTransferLib shares the library and function names but checks token code, and
+// its path does not name solmate: the provenance check keeps it out of scope.
+contract UsesSolady {
+    using SoladySTL for IToken;
+
+    IToken internal token;
+
+    function viaSolady(address to, uint256 amount) internal {
+        token.safeTransfer(to, amount);
+    }
+}
+
+// A same-name `SafeTransferLib` under a `solmate-fixed/` path: the string "solmate" is a
+// substring of that path but not one of its components, so the provenance check must treat
+// it as unrelated code and keep it out of scope.
+contract UsesFixedSolmate {
+    using FixedSTL for ITokenFixed;
+
+    ITokenFixed internal token;
+
+    function viaFixed(address to, uint256 amount) internal {
+        token.safeTransfer(to, amount);
+    }
+}