diff --git a/crates/lint/src/sol/high/mod.rs b/crates/lint/src/sol/high/mod.rs index 30d3dc8a02d4e..110b60bfe6ae7 100644 --- a/crates/lint/src/sol/high/mod.rs +++ b/crates/lint/src/sol/high/mod.rs @@ -1,6 +1,13 @@ use crate::sol::{EarlyLintPass, SolLint}; mod incorrect_shift; +mod unchecked_calls; + use incorrect_shift::INCORRECT_SHIFT; +use unchecked_calls::{ERC20_UNCHECKED_TRANSFER, UNCHECKED_CALL}; -register_lints!((IncorrectShift, (INCORRECT_SHIFT))); +register_lints!( + (IncorrectShift, (INCORRECT_SHIFT)), + (UncheckedCall, (UNCHECKED_CALL)), + (UncheckedTransferERC20, (ERC20_UNCHECKED_TRANSFER)) +); diff --git a/crates/lint/src/sol/high/unchecked_calls.rs b/crates/lint/src/sol/high/unchecked_calls.rs new file mode 100644 index 0000000000000..e5007269338a4 --- /dev/null +++ b/crates/lint/src/sol/high/unchecked_calls.rs @@ -0,0 +1,159 @@ +use super::{UncheckedCall, UncheckedTransferERC20}; +use crate::{ + linter::{EarlyLintPass, LintContext}, + sol::{Severity, SolLint}, +}; +use solar_ast::{visit::Visit, Expr, ExprKind, ItemFunction, Stmt, StmtKind}; +use solar_interface::kw; +use std::ops::ControlFlow; + +declare_forge_lint!( + UNCHECKED_CALL, + Severity::High, + "unchecked-call", + "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 ------------------------------------------------------------------- + +/// 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 { + 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. +/// `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 { + 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 +} + +// -- 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 { + 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 vars: `(, 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) && vars.first().is_none_or(|v| v.is_none()) { + 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 { + 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 +} + +/// Checks if a tuple assignment doesn't properly check the success value. +/// +/// 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 { + elements.first().is_none_or(|e| e.is_none()) + } else { + 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 +