From 7d79f997e576785446947c3055c2ee9fcb808ff9 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 19 Jun 2025 10:31:16 +0200 Subject: [PATCH 1/6] feat: add new lints for unchecked low-level calls, ERC20.transfer/transferFrom --- crates/lint/src/sol/high/mod.rs | 11 +- crates/lint/src/sol/high/unchecked_call.rs | 114 ++++++++++++++++++ .../src/sol/high/unchecked_transfer_erc20.rs | 69 +++++++++++ crates/lint/testdata/UncheckedCall.sol | 88 ++++++++++++++ crates/lint/testdata/UncheckedCall.stderr | 64 ++++++++++ .../lint/testdata/UncheckedTransferERC20.sol | 80 ++++++++++++ .../testdata/UncheckedTransferERC20.stderr | 40 ++++++ 7 files changed, 465 insertions(+), 1 deletion(-) create mode 100644 crates/lint/src/sol/high/unchecked_call.rs create mode 100644 crates/lint/src/sol/high/unchecked_transfer_erc20.rs create mode 100644 crates/lint/testdata/UncheckedCall.sol create mode 100644 crates/lint/testdata/UncheckedCall.stderr create mode 100644 crates/lint/testdata/UncheckedTransferERC20.sol create mode 100644 crates/lint/testdata/UncheckedTransferERC20.stderr diff --git a/crates/lint/src/sol/high/mod.rs b/crates/lint/src/sol/high/mod.rs index 720f172420e66..e00b088d5cee2 100644 --- a/crates/lint/src/sol/high/mod.rs +++ b/crates/lint/src/sol/high/mod.rs @@ -4,6 +4,15 @@ use crate::{ }; mod incorrect_shift; +mod unchecked_call; +mod unchecked_transfer_erc20; + use incorrect_shift::INCORRECT_SHIFT; +use unchecked_call::UNCHECKED_CALL; +use unchecked_transfer_erc20::UNCHECKED_TRANSFER_ERC20; -register_lints!((IncorrectShift, (INCORRECT_SHIFT))); +register_lints!( + (IncorrectShift, (INCORRECT_SHIFT)), + (UncheckedCall, (UNCHECKED_CALL)), + (UncheckedTransferERC20, (UNCHECKED_TRANSFER_ERC20)) +); diff --git a/crates/lint/src/sol/high/unchecked_call.rs b/crates/lint/src/sol/high/unchecked_call.rs new file mode 100644 index 0000000000000..0ce7c71525a69 --- /dev/null +++ b/crates/lint/src/sol/high/unchecked_call.rs @@ -0,0 +1,114 @@ +use super::UncheckedCall; +use crate::{ + declare_forge_lint, + linter::{EarlyLintPass, LintContext}, + sol::{Severity, SolLint}, +}; +use solar_ast::{visit::Visit, Expr, ExprKind, ItemFunction, Stmt, StmtKind, VariableDefinition}; +use std::ops::ControlFlow; + +declare_forge_lint!( + UNCHECKED_CALL, + Severity::High, + "unchecked-call", + "Low-level calls should check the success return value" +); + +impl<'ast> EarlyLintPass<'ast> for UncheckedCall { + fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { + if let Some(body) = &func.body { + let mut checker = UncheckedCallChecker { ctx }; + let _ = checker.visit_block(body); + } + } +} + +/// Visitor that detects unchecked low-level calls within function bodies. +/// +/// Similar to unchecked transfers, unchecked calls appear as standalone expression +/// statements. When the success value is checked (in require, if, etc.), the call +/// is part of a larger expression and won't be flagged. +struct UncheckedCallChecker<'a, 's> { + ctx: &'a LintContext<'s>, +} + +impl<'ast> Visit<'ast> for UncheckedCallChecker<'_, '_> { + type BreakValue = (); + + fn visit_stmt(&mut self, stmt: &'ast Stmt<'ast>) -> ControlFlow { + match &stmt.kind { + // Check standalone expression statements: `target.call(data);` + StmtKind::Expr(expr) => { + if is_low_level_call(expr) { + self.ctx.emit(&UNCHECKED_CALL, expr.span); + } else if let ExprKind::Assign(lhs, _, rhs) = &expr.kind { + // Check assignments to existing variables: `(, existingVar) = + // target.call(data);` + if is_low_level_call(rhs) && is_unchecked_tuple_assignment(lhs) { + self.ctx.emit(&UNCHECKED_CALL, expr.span); + } + } + } + // Check multi-variable declarations: `(bool success, ) = target.call(data);` + StmtKind::DeclMulti(vars, expr) => { + if is_low_level_call(expr) && is_unchecked_multi_declaration(vars) { + self.ctx.emit(&UNCHECKED_CALL, stmt.span); + } + } + _ => {} + } + self.walk_stmt(stmt) + } +} + +/// Checks if an expression is a low-level call that should be checked. +/// +/// Detects patterns like: +/// - `target.call(...)` +/// - `target.delegatecall(...)` +/// - `target.staticcall(...)` +/// - `target.call{value: x}(...)` +fn is_low_level_call(expr: &Expr<'_>) -> bool { + match &expr.kind { + ExprKind::Call(call_expr, _args) => { + // Check the callee expression + let callee = match &call_expr.kind { + // Handle call options like {value: x} + ExprKind::CallOptions(inner_expr, _) => inner_expr, + // Direct call without options + _ => call_expr, + }; + + // Must be a member access pattern: `target.call(...)` + if let ExprKind::Member(_, member) = &callee.kind { + let method_name = member.as_str(); + // Check for low-level call methods + matches!(method_name, "call" | "delegatecall" | "staticcall") + } else { + false + } + } + _ => false, + } +} + +/// Checks if a multi-variable declaration doesn't properly check the success value. +/// +/// Returns true if the first variable (success) is None: `(, bytes memory data) = target.call(...)` +fn is_unchecked_multi_declaration(vars: &[Option>]) -> bool { + vars.len() == 2 && vars.first().map_or(true, |v| v.is_none()) +} + +/// Checks if a tuple assignment doesn't properly check the success value. +/// +/// Returns true if the expression is a tuple where: +/// - All elements are empty/underscore +/// - The first element (success position) is empty/underscore +fn is_unchecked_tuple_assignment(expr: &Expr<'_>) -> bool { + if let ExprKind::Tuple(elements) = &expr.kind { + // Check if the tuple is empty or the first element is None (underscore) + elements.is_empty() || elements.first().map_or(true, |e| e.is_none()) + } else { + false + } +} diff --git a/crates/lint/src/sol/high/unchecked_transfer_erc20.rs b/crates/lint/src/sol/high/unchecked_transfer_erc20.rs new file mode 100644 index 0000000000000..5503d26db8a16 --- /dev/null +++ b/crates/lint/src/sol/high/unchecked_transfer_erc20.rs @@ -0,0 +1,69 @@ +use super::UncheckedTransferERC20; +use crate::{ + declare_forge_lint, + linter::{EarlyLintPass, LintContext}, + sol::{Severity, SolLint}, +}; +use solar_ast::{visit::Visit, Expr, ExprKind, ItemFunction, Stmt, StmtKind}; +use std::ops::ControlFlow; + +declare_forge_lint!( + UNCHECKED_TRANSFER_ERC20, + Severity::High, + "erc20-unchecked-transfer", + "ERC20 'transfer' and 'transferFrom' calls should check the return value" +); + +impl<'ast> EarlyLintPass<'ast> for UncheckedTransferERC20 { + fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { + if let Some(body) = &func.body { + let mut checker = UncheckedTransferERC20Checker { ctx }; + let _ = checker.visit_block(body); + } + } +} + +/// Visitor that detects unchecked ERC20 transfer calls within function bodies. +/// +/// Unchecked transfers appear as standalone expression statements. +/// When a transfer's return value is used (in require, assignment, etc.), it's part +/// of a larger expression and won't be flagged. +struct UncheckedTransferERC20Checker<'a, 's> { + ctx: &'a LintContext<'s>, +} + +impl<'ast> Visit<'ast> for UncheckedTransferERC20Checker<'_, '_> { + type BreakValue = (); + + fn visit_stmt(&mut self, stmt: &'ast Stmt<'ast>) -> ControlFlow { + // Only expression statements can contain unchecked transfers. + if let StmtKind::Expr(expr) = &stmt.kind { + if is_transfer_call(expr) { + self.ctx.emit(&UNCHECKED_TRANSFER_ERC20, expr.span); + } + } + self.walk_stmt(stmt) + } +} + +/// Checks if an expression is an ERC20 `transfer` or `transferFrom` call. +/// +/// Validates both the method name and argument count to avoid false positives +/// from other functions that happen to be named "transfer". +fn is_transfer_call(expr: &Expr<'_>) -> bool { + match &expr.kind { + ExprKind::Call(call_expr, args) => { + // Must be a member access pattern: `token.transfer(...)` + if let ExprKind::Member(_, member) = &call_expr.kind { + let method_name = member.as_str(); + // function ERC20.transfer(to, amount) + // function ERC20.transferFrom(from, to, amount) + (args.len() == 2 && method_name == "transfer") || + (args.len() == 3 && method_name == "transferFrom") + } else { + false + } + } + _ => false, + } +} diff --git a/crates/lint/testdata/UncheckedCall.sol b/crates/lint/testdata/UncheckedCall.sol new file mode 100644 index 0000000000000..c2f71637a7b33 --- /dev/null +++ b/crates/lint/testdata/UncheckedCall.sol @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +contract UncheckedCall { + + // SHOULD PASS: Properly checked low-level calls + function checkedCallWithTuple(address target, bytes memory data) public { + (bool success, bytes memory result) = target.call(data); + require(success, "Call failed"); + emit CallResult(success, result); + } + + function checkedCallWithIfStatement(address target, bytes memory data) public { + (bool success, ) = target.call(data); + if (!success) { + revert("Call failed"); + } + } + + function checkedDelegateCall(address target, bytes memory data) public returns (bool) { + (bool success, ) = target.delegatecall(data); + return success; + } + + function checkedStaticCall(address target, bytes memory data) public view returns (bytes memory) { + (bool success, bytes memory result) = target.staticcall(data); + require(success, "Static call failed"); + return result; + } + + function checkedCallInRequire(address target) public { + (bool success, ) = target.call(""); + require(success, "Call must succeed"); + } + + function checkedCallWithAssert(address target) public { + (bool success, ) = target.call(""); + assert(success); + } + + // Edge case: pre-existing variable assignment + bool success; + function checkWithExistingVar(address target) public { + (bool, ) = target.call(""); + (bool, existingData) = target.call(""); + } + + // Edge case: send and transfer are not low-level calls (they automatically revert on failure) + function sendEther(address payable target) public { + target.transfer(1 ether); // Should not trigger + bool sent = target.send(1 ether); // Should not trigger + require(sent, "Send failed"); + } + + + // SHOULD FAIL: Unchecked low-level calls + function uncheckedCall(address target, bytes memory data) public { + target.call(data); //~WARN: Low-level calls should check the success return value + } + + function uncheckedCallWithValue(address payable target, uint256 value) public { + target.call{value: value}(""); //~WARN: Low-level calls should check the success return value + } + + function uncheckedDelegateCall(address target, bytes memory data) public { + target.delegatecall(data); //~WARN: Low-level calls should check the success return value + } + + function uncheckedStaticCall(address target, bytes memory data) public { + target.staticcall(data); //~WARN: Low-level calls should check the success return value + } + + function multipleUncheckedCalls(address target1, address target2) public { + target1.call(""); //~WARN: Low-level calls should check the success return value + target2.delegatecall(""); //~WARN: Low-level calls should check the success return value + } + + function ignoredReturnWithPartialTuple(address target) public { + (, bytes memory data) = target.call(""); //~WARN: Low-level calls should check the success return value + // Only capturing data, not checking success + } + + bytes memory existingData; + function ignoredReturnExistingVar(address target) public { + (, existingData) = target.call(""); //~WARN: Low-level calls should check the success return value + } + +} diff --git a/crates/lint/testdata/UncheckedCall.stderr b/crates/lint/testdata/UncheckedCall.stderr new file mode 100644 index 0000000000000..a5a1010955f9b --- /dev/null +++ b/crates/lint/testdata/UncheckedCall.stderr @@ -0,0 +1,64 @@ +warning[unchecked-call]: Low-level calls should check the success return value + --> ROOT/testdata/UncheckedCall.sol:LL:CC + | +58 | target.call(data); + | ----------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call + +warning[unchecked-call]: Low-level calls should check the success return value + --> ROOT/testdata/UncheckedCall.sol:LL:CC + | +62 | target.call{value: value}(""); + | ----------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call + +warning[unchecked-call]: Low-level calls should check the success return value + --> ROOT/testdata/UncheckedCall.sol:LL:CC + | +66 | target.delegatecall(data); + | ------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call + +warning[unchecked-call]: Low-level calls should check the success return value + --> ROOT/testdata/UncheckedCall.sol:LL:CC + | +70 | target.staticcall(data); + | ----------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call + +warning[unchecked-call]: Low-level calls should check the success return value + --> ROOT/testdata/UncheckedCall.sol:LL:CC + | +74 | target1.call(""); + | ---------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call + +warning[unchecked-call]: Low-level calls should check the success return value + --> ROOT/testdata/UncheckedCall.sol:LL:CC + | +75 | target2.delegatecall(""); + | ------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call + +warning[unchecked-call]: Low-level calls should check the success return value + --> ROOT/testdata/UncheckedCall.sol:LL:CC + | +79 | (, bytes memory data) = target.call(""); + | ---------------------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call + +warning[unchecked-call]: Low-level calls should check the success return value + --> ROOT/testdata/UncheckedCall.sol:LL:CC + | +85 | (, existingData) = target.call(""); + | ---------------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call + diff --git a/crates/lint/testdata/UncheckedTransferERC20.sol b/crates/lint/testdata/UncheckedTransferERC20.sol new file mode 100644 index 0000000000000..5c3f53354078d --- /dev/null +++ b/crates/lint/testdata/UncheckedTransferERC20.sol @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +interface IERC20 { + function transfer(address to, uint256 amount) external returns (bool); + function transferFrom(address from, address to, uint256 amount) external returns (bool); + function approve(address spender, uint256 amount) external returns (bool); +} + +contract UncheckedTransfer { + IERC20 public token; + mapping(address => uint256) public balances; + + constructor(address _token) { + token = IERC20(_token); + } + + // SHOULD FAIL: Unchecked transfer calls + function uncheckedTransfer(address to, uint256 amount) public { + token.transfer(to, amount); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value + } + + function uncheckedTransferFrom(address from, address to, uint256 amount) public { + token.transferFrom(from, to, amount); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value + } + + function multipleUnchecked(address to, uint256 amount) public { + token.transfer(to, amount / 2); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value + token.transfer(to, amount / 2); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value + } + + function uncheckedInLoop(address[] memory recipients, uint256[] memory amounts) public { + for (uint i = 0; i < recipients.length; i++) { + token.transfer(recipients[i], amounts[i]); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value + } + } + + // SHOULD PASS: Properly checked transfer calls + function checkedTransferWithRequire(address to, uint256 amount) public { + require(token.transfer(to, amount), "Transfer failed"); + } + + function checkedTransferWithVariable(address to, uint256 amount) public { + bool success = token.transfer(to, amount); + require(success, "Transfer failed"); + } + + function checkedTransferFromWithIf(address from, address to, uint256 amount) public { + bool success = token.transferFrom(from, to, amount); + if (!success) { + revert("TransferFrom failed"); + } + } + + function checkedTransferWithAssert(address to, uint256 amount) public { + assert(token.transfer(to, amount)); + } + + function checkedTransferInReturn(address to, uint256 amount) public returns (bool) { + return token.transfer(to, amount); + } + + function checkedTransferInExpression(address to, uint256 amount) public { + if (token.transfer(to, amount)) { + balances[to] += amount; + } + } + + function checkedTransferInRequireWithLogic(address to, uint256 amount) public { + require( + amount > 0 && token.transfer(to, amount), + "Invalid amount or transfer failed" + ); + } + + // Edge case: approve is not a transfer function, should not be flagged + function uncheckedApprove(address spender, uint256 amount) public { + token.approve(spender, amount); + } +} diff --git a/crates/lint/testdata/UncheckedTransferERC20.stderr b/crates/lint/testdata/UncheckedTransferERC20.stderr new file mode 100644 index 0000000000000..c72ff19d1745c --- /dev/null +++ b/crates/lint/testdata/UncheckedTransferERC20.stderr @@ -0,0 +1,40 @@ +warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value + --> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC + | +20 | token.transfer(to, amount); + | -------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer + +warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value + --> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC + | +24 | token.transferFrom(from, to, amount); + | ------------------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer + +warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value + --> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC + | +28 | token.transfer(to, amount / 2); + | ------------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer + +warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value + --> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC + | +29 | token.transfer(to, amount / 2); + | ------------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer + +warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value + --> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC + | +34 | token.transfer(recipients[i], amounts[i]); + | ----------------------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer + From eeb26d6544e40d569f9974e73660a15423c569bb Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 19 Jun 2025 11:34:03 +0200 Subject: [PATCH 2/6] style: unify into unchecked_calls.rs file --- crates/lint/src/sol/high/mod.rs | 8 +- .../{unchecked_call.rs => unchecked_calls.rs} | 89 +++++++++++++++---- .../src/sol/high/unchecked_transfer_erc20.rs | 69 -------------- 3 files changed, 75 insertions(+), 91 deletions(-) rename crates/lint/src/sol/high/{unchecked_call.rs => unchecked_calls.rs} (54%) delete mode 100644 crates/lint/src/sol/high/unchecked_transfer_erc20.rs diff --git a/crates/lint/src/sol/high/mod.rs b/crates/lint/src/sol/high/mod.rs index e00b088d5cee2..8b02a59cc3ce1 100644 --- a/crates/lint/src/sol/high/mod.rs +++ b/crates/lint/src/sol/high/mod.rs @@ -4,15 +4,13 @@ use crate::{ }; mod incorrect_shift; -mod unchecked_call; -mod unchecked_transfer_erc20; +mod unchecked_calls; use incorrect_shift::INCORRECT_SHIFT; -use unchecked_call::UNCHECKED_CALL; -use unchecked_transfer_erc20::UNCHECKED_TRANSFER_ERC20; +use unchecked_calls::{UNCHECKED_CALL, ERC20_UNCHECKED_TRANSFER}; register_lints!( (IncorrectShift, (INCORRECT_SHIFT)), (UncheckedCall, (UNCHECKED_CALL)), - (UncheckedTransferERC20, (UNCHECKED_TRANSFER_ERC20)) + (UncheckedTransferERC20, (ERC20_UNCHECKED_TRANSFER)) ); diff --git a/crates/lint/src/sol/high/unchecked_call.rs b/crates/lint/src/sol/high/unchecked_calls.rs similarity index 54% rename from crates/lint/src/sol/high/unchecked_call.rs rename to crates/lint/src/sol/high/unchecked_calls.rs index 0ce7c71525a69..43d62adc7bb26 100644 --- a/crates/lint/src/sol/high/unchecked_call.rs +++ b/crates/lint/src/sol/high/unchecked_calls.rs @@ -1,10 +1,10 @@ -use super::UncheckedCall; +use super::{UncheckedCall, UncheckedTransferERC20}; use crate::{ declare_forge_lint, linter::{EarlyLintPass, LintContext}, sol::{Severity, SolLint}, }; -use solar_ast::{visit::Visit, Expr, ExprKind, ItemFunction, Stmt, StmtKind, VariableDefinition}; +use solar_ast::{visit::Visit, Expr, ExprKind, ItemFunction, Stmt, StmtKind}; use std::ops::ControlFlow; declare_forge_lint!( @@ -14,6 +14,71 @@ declare_forge_lint!( "Low-level calls should check the success return value" ); +declare_forge_lint!( + ERC20_UNCHECKED_TRANSFER, + Severity::High, + "erc20-unchecked-transfer", + "ERC20 'transfer' and 'transferFrom' calls should check the return value" +); + +// -- ERC20 UNCKECKED TRANSFERS ------------------------------------------------------------------- + +impl<'ast> EarlyLintPass<'ast> for UncheckedTransferERC20 { + fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { + if let Some(body) = &func.body { + let mut checker = UncheckedTransferERC20Checker { ctx }; + let _ = checker.visit_block(body); + } + } +} + +/// Visitor that detects unchecked ERC20 transfer calls within function bodies. +/// +/// Unchecked transfers appear as standalone expression statements. +/// When a transfer's return value is used (in require, assignment, etc.), it's part +/// of a larger expression and won't be flagged. +struct UncheckedTransferERC20Checker<'a, 's> { + ctx: &'a LintContext<'s>, +} + +impl<'ast> Visit<'ast> for UncheckedTransferERC20Checker<'_, '_> { + type BreakValue = (); + + fn visit_stmt(&mut self, stmt: &'ast Stmt<'ast>) -> ControlFlow { + // Only expression statements can contain unchecked transfers. + if let StmtKind::Expr(expr) = &stmt.kind { + if is_erc20_transfer_call(expr) { + self.ctx.emit(&ERC20_UNCHECKED_TRANSFER, expr.span); + } + } + self.walk_stmt(stmt) + } +} + +/// Checks if an expression is an ERC20 `transfer` or `transferFrom` call. +/// +/// Validates both the method name and argument count to avoid false positives +/// from other functions that happen to be named "transfer". +fn is_erc20_transfer_call(expr: &Expr<'_>) -> bool { + match &expr.kind { + ExprKind::Call(call_expr, args) => { + // Must be a member access pattern: `token.transfer(...)` + if let ExprKind::Member(_, member) = &call_expr.kind { + let method_name = member.as_str(); + // function ERC20.transfer(to, amount) + // function ERC20.transferFrom(from, to, amount) + (args.len() == 2 && method_name == "transfer") || + (args.len() == 3 && method_name == "transferFrom") + } else { + false + } + } + _ => false, + } +} + +// -- UNCKECKED LOW-LEVEL CALLS ------------------------------------------------------------------- + impl<'ast> EarlyLintPass<'ast> for UncheckedCall { fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { if let Some(body) = &func.body { @@ -42,8 +107,7 @@ impl<'ast> Visit<'ast> for UncheckedCallChecker<'_, '_> { if is_low_level_call(expr) { self.ctx.emit(&UNCHECKED_CALL, expr.span); } else if let ExprKind::Assign(lhs, _, rhs) = &expr.kind { - // Check assignments to existing variables: `(, existingVar) = - // target.call(data);` + // Check assignments to existing vars: `(, existingVar) = target.call(data);` if is_low_level_call(rhs) && is_unchecked_tuple_assignment(lhs) { self.ctx.emit(&UNCHECKED_CALL, expr.span); } @@ -51,7 +115,7 @@ impl<'ast> Visit<'ast> for UncheckedCallChecker<'_, '_> { } // Check multi-variable declarations: `(bool success, ) = target.call(data);` StmtKind::DeclMulti(vars, expr) => { - if is_low_level_call(expr) && is_unchecked_multi_declaration(vars) { + if is_low_level_call(expr) && vars.first().map_or(true, |v| v.is_none()) { self.ctx.emit(&UNCHECKED_CALL, stmt.span); } } @@ -92,22 +156,13 @@ fn is_low_level_call(expr: &Expr<'_>) -> bool { } } -/// Checks if a multi-variable declaration doesn't properly check the success value. -/// -/// Returns true if the first variable (success) is None: `(, bytes memory data) = target.call(...)` -fn is_unchecked_multi_declaration(vars: &[Option>]) -> bool { - vars.len() == 2 && vars.first().map_or(true, |v| v.is_none()) -} - /// Checks if a tuple assignment doesn't properly check the success value. /// -/// Returns true if the expression is a tuple where: -/// - All elements are empty/underscore -/// - The first element (success position) is empty/underscore +/// Returns true if the first variable (success) is None: `(, bytes memory data) = +/// target.call(...)` fn is_unchecked_tuple_assignment(expr: &Expr<'_>) -> bool { if let ExprKind::Tuple(elements) = &expr.kind { - // Check if the tuple is empty or the first element is None (underscore) - elements.is_empty() || elements.first().map_or(true, |e| e.is_none()) + elements.first().map_or(true, |e| e.is_none()) } else { false } diff --git a/crates/lint/src/sol/high/unchecked_transfer_erc20.rs b/crates/lint/src/sol/high/unchecked_transfer_erc20.rs deleted file mode 100644 index 5503d26db8a16..0000000000000 --- a/crates/lint/src/sol/high/unchecked_transfer_erc20.rs +++ /dev/null @@ -1,69 +0,0 @@ -use super::UncheckedTransferERC20; -use crate::{ - declare_forge_lint, - linter::{EarlyLintPass, LintContext}, - sol::{Severity, SolLint}, -}; -use solar_ast::{visit::Visit, Expr, ExprKind, ItemFunction, Stmt, StmtKind}; -use std::ops::ControlFlow; - -declare_forge_lint!( - UNCHECKED_TRANSFER_ERC20, - Severity::High, - "erc20-unchecked-transfer", - "ERC20 'transfer' and 'transferFrom' calls should check the return value" -); - -impl<'ast> EarlyLintPass<'ast> for UncheckedTransferERC20 { - fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { - if let Some(body) = &func.body { - let mut checker = UncheckedTransferERC20Checker { ctx }; - let _ = checker.visit_block(body); - } - } -} - -/// Visitor that detects unchecked ERC20 transfer calls within function bodies. -/// -/// Unchecked transfers appear as standalone expression statements. -/// When a transfer's return value is used (in require, assignment, etc.), it's part -/// of a larger expression and won't be flagged. -struct UncheckedTransferERC20Checker<'a, 's> { - ctx: &'a LintContext<'s>, -} - -impl<'ast> Visit<'ast> for UncheckedTransferERC20Checker<'_, '_> { - type BreakValue = (); - - fn visit_stmt(&mut self, stmt: &'ast Stmt<'ast>) -> ControlFlow { - // Only expression statements can contain unchecked transfers. - if let StmtKind::Expr(expr) = &stmt.kind { - if is_transfer_call(expr) { - self.ctx.emit(&UNCHECKED_TRANSFER_ERC20, expr.span); - } - } - self.walk_stmt(stmt) - } -} - -/// Checks if an expression is an ERC20 `transfer` or `transferFrom` call. -/// -/// Validates both the method name and argument count to avoid false positives -/// from other functions that happen to be named "transfer". -fn is_transfer_call(expr: &Expr<'_>) -> bool { - match &expr.kind { - ExprKind::Call(call_expr, args) => { - // Must be a member access pattern: `token.transfer(...)` - if let ExprKind::Member(_, member) = &call_expr.kind { - let method_name = member.as_str(); - // function ERC20.transfer(to, amount) - // function ERC20.transferFrom(from, to, amount) - (args.len() == 2 && method_name == "transfer") || - (args.len() == 3 && method_name == "transferFrom") - } else { - false - } - } - _ => false, - } -} From 1f0e1b9541419b577f2eced2a76a9e94397c3ef0 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 19 Jun 2025 11:38:31 +0200 Subject: [PATCH 3/6] style: clippy --- crates/lint/src/sol/high/unchecked_calls.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/lint/src/sol/high/unchecked_calls.rs b/crates/lint/src/sol/high/unchecked_calls.rs index 43d62adc7bb26..a54e39b597a32 100644 --- a/crates/lint/src/sol/high/unchecked_calls.rs +++ b/crates/lint/src/sol/high/unchecked_calls.rs @@ -115,7 +115,7 @@ impl<'ast> Visit<'ast> for UncheckedCallChecker<'_, '_> { } // Check multi-variable declarations: `(bool success, ) = target.call(data);` StmtKind::DeclMulti(vars, expr) => { - if is_low_level_call(expr) && vars.first().map_or(true, |v| v.is_none()) { + if is_low_level_call(expr) && vars.first().is_none_or(|v| v.is_none()) { self.ctx.emit(&UNCHECKED_CALL, stmt.span); } } @@ -162,7 +162,7 @@ fn is_low_level_call(expr: &Expr<'_>) -> bool { /// target.call(...)` fn is_unchecked_tuple_assignment(expr: &Expr<'_>) -> bool { if let ExprKind::Tuple(elements) = &expr.kind { - elements.first().map_or(true, |e| e.is_none()) + elements.first().is_none_or(|e| e.is_none()) } else { false } From fc71f7b3449756f6f56a18d0614ecf1f563b4982 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 19 Jun 2025 13:51:10 +0200 Subject: [PATCH 4/6] style: fmt --- crates/lint/src/sol/high/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/lint/src/sol/high/mod.rs b/crates/lint/src/sol/high/mod.rs index 8b02a59cc3ce1..7ef1100e65738 100644 --- a/crates/lint/src/sol/high/mod.rs +++ b/crates/lint/src/sol/high/mod.rs @@ -7,7 +7,7 @@ mod incorrect_shift; mod unchecked_calls; use incorrect_shift::INCORRECT_SHIFT; -use unchecked_calls::{UNCHECKED_CALL, ERC20_UNCHECKED_TRANSFER}; +use unchecked_calls::{ERC20_UNCHECKED_TRANSFER, UNCHECKED_CALL}; register_lints!( (IncorrectShift, (INCORRECT_SHIFT)), From cf1bd01969566eb9a41ccbd9d58050211c8fcbb9 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 19 Jun 2025 17:19:24 +0200 Subject: [PATCH 5/6] refactor: simplify unchecked calls lint detection Use cleaner pattern matching and `solar_interface::kw` keywords instead of string literals. Move function signatures to doc comments. --- crates/lint/src/sol/high/unchecked_calls.rs | 54 +++++++++------------ 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/crates/lint/src/sol/high/unchecked_calls.rs b/crates/lint/src/sol/high/unchecked_calls.rs index a54e39b597a32..104acd1e0e74d 100644 --- a/crates/lint/src/sol/high/unchecked_calls.rs +++ b/crates/lint/src/sol/high/unchecked_calls.rs @@ -5,6 +5,7 @@ use crate::{ sol::{Severity, SolLint}, }; use solar_ast::{visit::Visit, Expr, ExprKind, ItemFunction, Stmt, StmtKind}; +use solar_interface::kw; use std::ops::ControlFlow; declare_forge_lint!( @@ -56,25 +57,20 @@ impl<'ast> Visit<'ast> for UncheckedTransferERC20Checker<'_, '_> { } /// Checks if an expression is an ERC20 `transfer` or `transferFrom` call. +/// `function ERC20.transfer(to, amount)` +/// `function ERC20.transferFrom(from, to, amount)` /// /// Validates both the method name and argument count to avoid false positives /// from other functions that happen to be named "transfer". fn is_erc20_transfer_call(expr: &Expr<'_>) -> bool { - match &expr.kind { - ExprKind::Call(call_expr, args) => { - // Must be a member access pattern: `token.transfer(...)` - if let ExprKind::Member(_, member) = &call_expr.kind { - let method_name = member.as_str(); - // function ERC20.transfer(to, amount) - // function ERC20.transferFrom(from, to, amount) - (args.len() == 2 && method_name == "transfer") || - (args.len() == 3 && method_name == "transferFrom") - } else { - false - } + if let ExprKind::Call(call_expr, args) = &expr.kind { + // Must be a member access pattern: `token.transfer(...)` + if let ExprKind::Member(_, member) = &call_expr.kind { + return (args.len() == 2 && member.as_str() == "transfer") || + (args.len() == 3 && member.as_str() == "transferFrom") } - _ => false, } + false } // -- UNCKECKED LOW-LEVEL CALLS ------------------------------------------------------------------- @@ -133,27 +129,21 @@ impl<'ast> Visit<'ast> for UncheckedCallChecker<'_, '_> { /// - `target.staticcall(...)` /// - `target.call{value: x}(...)` fn is_low_level_call(expr: &Expr<'_>) -> bool { - match &expr.kind { - ExprKind::Call(call_expr, _args) => { - // Check the callee expression - let callee = match &call_expr.kind { - // Handle call options like {value: x} - ExprKind::CallOptions(inner_expr, _) => inner_expr, - // Direct call without options - _ => call_expr, - }; - - // Must be a member access pattern: `target.call(...)` - if let ExprKind::Member(_, member) = &callee.kind { - let method_name = member.as_str(); - // Check for low-level call methods - matches!(method_name, "call" | "delegatecall" | "staticcall") - } else { - false - } + if let ExprKind::Call(call_expr, _args) = &expr.kind { + // Check the callee expression + let callee = match &call_expr.kind { + // Handle call options like {value: x} + ExprKind::CallOptions(inner_expr, _) => inner_expr, + // Direct call without options + _ => call_expr, + }; + + if let ExprKind::Member(_, member) = &callee.kind { + // Check for low-level call methods + return matches!(member.name, kw::Call | kw::Delegatecall | kw::Staticcall) } - _ => false, } + false } /// Checks if a tuple assignment doesn't properly check the success value. From 4f4d7704d8ea97978817a0b582a02f676b4c0274 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Fri, 27 Jun 2025 15:36:10 +0200 Subject: [PATCH 6/6] style: clippy --- crates/lint/src/sol/high/unchecked_calls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/lint/src/sol/high/unchecked_calls.rs b/crates/lint/src/sol/high/unchecked_calls.rs index 104acd1e0e74d..e5007269338a4 100644 --- a/crates/lint/src/sol/high/unchecked_calls.rs +++ b/crates/lint/src/sol/high/unchecked_calls.rs @@ -1,6 +1,5 @@ use super::{UncheckedCall, UncheckedTransferERC20}; use crate::{ - declare_forge_lint, linter::{EarlyLintPass, LintContext}, sol::{Severity, SolLint}, }; @@ -24,6 +23,7 @@ declare_forge_lint!( // -- ERC20 UNCKECKED TRANSFERS ------------------------------------------------------------------- +/// WARN: can issue false positives. It does not check that the contract being called is an ERC20. impl<'ast> EarlyLintPass<'ast> for UncheckedTransferERC20 { fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { if let Some(body) = &func.body {