Skip to content

Commit a131c31

Browse files
0xrusowsky0xrusowsky
authored
feat(lint): impl erc20 transfer check using HIR (#11552)
1 parent bcca6c7 commit a131c31

File tree

4 files changed

+148
-55
lines changed

4 files changed

+148
-55
lines changed

‎crates/lint/src/sol/high/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,5 +9,5 @@ use unchecked_calls::{ERC20_UNCHECKED_TRANSFER, UNCHECKED_CALL};
99
register_lints!(
1010
(IncorrectShift, early, (INCORRECT_SHIFT)),
1111
(UncheckedCall, early, (UNCHECKED_CALL)),
12-
(UncheckedTransferERC20, early, (ERC20_UNCHECKED_TRANSFER))
12+
(UncheckedTransferERC20, late, (ERC20_UNCHECKED_TRANSFER))
1313
);

‎crates/lint/src/sol/high/unchecked_calls.rs‎

Lines changed: 77 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
use super::{UncheckedCall, UncheckedTransferERC20};
22
use crate::{
3-
linter::{EarlyLintPass, LintContext},
3+
linter::{EarlyLintPass, LateLintPass, LintContext},
44
sol::{Severity, SolLint},
55
};
66
use solar::{
77
ast::{Expr, ExprKind, ItemFunction, Stmt, StmtKind, visit::Visit},
88
interface::kw,
9+
sema::hir::{self},
910
};
1011
use std::ops::ControlFlow;
1112

@@ -25,55 +26,91 @@ declare_forge_lint!(
2526

2627
// -- ERC20 UNCKECKED TRANSFERS -------------------------------------------------------------------
2728

28-
/// WARN: can issue false positives. It does not check that the contract being called is an ERC20.
29-
/// TODO: re-implement using `LateLintPass` so that it can't issue false positives.
30-
impl<'ast> EarlyLintPass<'ast> for UncheckedTransferERC20 {
31-
fn check_item_function(&mut self, ctx: &LintContext, func: &'ast ItemFunction<'ast>) {
32-
if let Some(body) = &func.body {
33-
let mut checker = UncheckedTransferERC20Checker { ctx };
34-
let _ = checker.visit_block(body);
35-
}
36-
}
37-
}
38-
39-
/// Visitor that detects unchecked ERC20 transfer calls within function bodies.
29+
/// Checks that calls to functions with the same signature as the ERC20 transfer methods, and which
30+
/// return a boolean are not ignored.
4031
///
41-
/// Unchecked transfers appear as standalone expression statements.
42-
/// When a transfer's return value is used (in require, assignment, etc.), it's part
43-
/// of a larger expression and won't be flagged.
44-
struct UncheckedTransferERC20Checker<'a, 's> {
45-
ctx: &'a LintContext<'s, 'a>,
46-
}
47-
48-
impl<'ast> Visit<'ast> for UncheckedTransferERC20Checker<'_, '_> {
49-
type BreakValue = ();
50-
51-
fn visit_stmt(&mut self, stmt: &'ast Stmt<'ast>) -> ControlFlow<Self::BreakValue> {
32+
/// WARN: can issue false positives, as it doesn't check that the contract being called sticks to
33+
/// the full ERC20 specification.
34+
impl<'hir> LateLintPass<'hir> for UncheckedTransferERC20 {
35+
fn check_stmt(
36+
&mut self,
37+
ctx: &LintContext,
38+
hir: &'hir hir::Hir<'hir>,
39+
stmt: &'hir hir::Stmt<'hir>,
40+
) {
5241
// Only expression statements can contain unchecked transfers.
53-
if let StmtKind::Expr(expr) = &stmt.kind
54-
&& is_erc20_transfer_call(expr)
42+
if let hir::StmtKind::Expr(expr) = &stmt.kind
43+
&& is_erc20_transfer_call(hir, expr)
5544
{
56-
self.ctx.emit(&ERC20_UNCHECKED_TRANSFER, expr.span);
45+
ctx.emit(&ERC20_UNCHECKED_TRANSFER, expr.span);
5746
}
58-
self.walk_stmt(stmt)
5947
}
6048
}
6149

6250
/// Checks if an expression is an ERC20 `transfer` or `transferFrom` call.
63-
/// `function ERC20.transfer(to, amount)`
64-
/// `function ERC20.transferFrom(from, to, amount)`
51+
/// * `function transfer(address to, uint256 amount) external returns bool;`
52+
/// * `function transferFrom(address from, address to, uint256 amount) external returns bool;`
6553
///
66-
/// Validates both the method name and argument count to avoid false positives
67-
/// from other functions that happen to be named "transfer".
68-
fn is_erc20_transfer_call(expr: &Expr<'_>) -> bool {
69-
if let ExprKind::Call(call_expr, args) = &expr.kind {
70-
// Must be a member access pattern: `token.transfer(...)`
71-
if let ExprKind::Member(_, member) = &call_expr.kind {
72-
return (args.len() == 2 && member.as_str() == "transfer")
73-
|| (args.len() == 3 && member.as_str() == "transferFrom");
54+
/// Validates the method name, the params (count + types), and the returns (count + types).
55+
fn is_erc20_transfer_call(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> bool {
56+
let is_type = |var_id: hir::VariableId, type_str: &str| {
57+
matches!(
58+
&hir.variable(var_id).ty.kind,
59+
hir::TypeKind::Elementary(ty) if ty.to_abi_str() == type_str
60+
)
61+
};
62+
63+
// Ensure the expression is a call to a contract member function.
64+
let hir::ExprKind::Call(
65+
hir::Expr { kind: hir::ExprKind::Member(contract_expr, func_ident), .. },
66+
hir::CallArgs { kind: hir::CallArgsKind::Unnamed(args), .. },
67+
..,
68+
) = &expr.kind
69+
else {
70+
return false;
71+
};
72+
73+
// Determine the expected ERC20 signature from the call
74+
let (expected_params, expected_returns): (&[&str], &[&str]) = match func_ident.as_str() {
75+
"transferFrom" if args.len() == 3 => (&["address", "address", "uint256"], &["bool"]),
76+
"transfer" if args.len() == 2 => (&["address", "uint256"], &["bool"]),
77+
_ => return false,
78+
};
79+
80+
let Some(cid) = (match &contract_expr.kind {
81+
// Call to pre-instantiated contract variable
82+
hir::ExprKind::Ident([hir::Res::Item(hir::ItemId::Variable(id)), ..]) => {
83+
if let hir::TypeKind::Custom(hir::ItemId::Contract(cid)) = hir.variable(*id).ty.kind {
84+
Some(cid)
85+
} else {
86+
None
87+
}
7488
}
75-
}
76-
false
89+
// Call to address wrapped by the contract interface
90+
hir::ExprKind::Call(
91+
hir::Expr {
92+
kind: hir::ExprKind::Ident([hir::Res::Item(hir::ItemId::Contract(cid))]),
93+
..
94+
},
95+
..,
96+
) => Some(*cid),
97+
_ => None,
98+
}) else {
99+
return false;
100+
};
101+
102+
// Try to find a function in the contract that matches the expected signature.
103+
hir.contract_item_ids(cid).any(|item| {
104+
let Some(fid) = item.as_function() else { return false };
105+
let func = hir.function(fid);
106+
func.name.is_some_and(|name| name.as_str() == func_ident.as_str())
107+
&& func.kind.is_function()
108+
&& func.mutates_state()
109+
&& func.parameters.len() == expected_params.len()
110+
&& func.returns.len() == expected_returns.len()
111+
&& func.parameters.iter().zip(expected_params).all(|(id, &ty)| is_type(*id, ty))
112+
&& func.returns.iter().zip(expected_returns).all(|(id, &ty)| is_type(*id, ty))
113+
})
77114
}
78115

79116
// -- UNCKECKED LOW-LEVEL CALLS -------------------------------------------------------------------

‎crates/lint/testdata/UncheckedTransferERC20.sol‎

Lines changed: 54 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,34 +7,50 @@ interface IERC20 {
77
function approve(address spender, uint256 amount) external returns (bool);
88
}
99

10+
interface IERC20Wrapper {
11+
function transfer(address to, uint256 amount) external;
12+
function transferFrom(address from, address to, uint256 amount) external;
13+
}
14+
1015
contract UncheckedTransfer {
1116
IERC20 public token;
17+
IERC20Wrapper public tokenWrapper;
1218
mapping(address => uint256) public balances;
1319

1420
constructor(address _token) {
1521
token = IERC20(_token);
22+
tokenWrapper = IERC20Wrapper(_token);
1623
}
1724

1825
// SHOULD FAIL: Unchecked transfer calls
1926
function uncheckedTransfer(address to, uint256 amount) public {
27+
IERC20(address(token)).transfer(to, amount); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value
2028
token.transfer(to, amount); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value
2129
}
2230

2331
function uncheckedTransferFrom(address from, address to, uint256 amount) public {
32+
IERC20(address(token)).transferFrom(from, to, amount); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value
2433
token.transferFrom(from, to, amount); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value
2534
}
2635

27-
function multipleUnchecked(address to, uint256 amount) public {
28-
token.transfer(to, amount / 2); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value
29-
token.transfer(to, amount / 2); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value
30-
}
31-
3236
function uncheckedInLoop(address[] memory recipients, uint256[] memory amounts) public {
3337
for (uint i = 0; i < recipients.length; i++) {
38+
IERC20(address(token)).transfer(recipients[i], amounts[i]); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value
3439
token.transfer(recipients[i], amounts[i]); //~WARN: ERC20 'transfer' and 'transferFrom' calls should check the return value
3540
}
3641
}
3742

43+
// SHOULD PASS: Function with same params but NO boolean return
44+
function proxyCheckedTransfer(address to, uint256 amount) public {
45+
IERC20Wrapper(address(token)).transfer(to, amount);
46+
tokenWrapper.transfer(to, amount);
47+
}
48+
49+
function proxyCheckedTransferFrom(address from, address to, uint256 amount) public {
50+
IERC20Wrapper(address(token)).transferFrom(from, to, amount);
51+
tokenWrapper.transferFrom(from, to, amount);
52+
}
53+
3854
// SHOULD PASS: Properly checked transfer calls
3955
function checkedTransferWithRequire(address to, uint256 amount) public {
4056
require(token.transfer(to, amount), "Transfer failed");
@@ -63,6 +79,8 @@ contract UncheckedTransfer {
6379
function checkedTransferInExpression(address to, uint256 amount) public {
6480
if (token.transfer(to, amount)) {
6581
balances[to] += amount;
82+
} else {
83+
revert("Transfer failed");
6684
}
6785
}
6886

@@ -73,8 +91,38 @@ contract UncheckedTransfer {
7391
);
7492
}
7593

76-
// Edge case: approve is not a transfer function, should not be flagged
7794
function uncheckedApprove(address spender, uint256 amount) public {
7895
token.approve(spender, amount);
7996
}
8097
}
98+
99+
library Currency {
100+
function transfer(Currency currency, address to, uint256 amount) internal {
101+
// transfer and check output internally
102+
}
103+
function transferFrom(Currency currency, address from, address to, uint256 amount) internal {
104+
// transfer and check output internally
105+
}
106+
}
107+
108+
contract UncheckedTransferUsingCurrencyLib {
109+
using Currency for address;
110+
111+
Currency public token;
112+
mapping(address => uint256) public balances;
113+
114+
constructor(Currency _token) {
115+
token = _token;
116+
}
117+
118+
// SHOULD PASS: Function with same params but NO boolean return
119+
function currencyTransfer(address to, uint256 amount) public {
120+
token.transfer(to, amount);
121+
token.transfer(to, amount);
122+
}
123+
124+
function currencyTransferFrom(address from, address to, uint256 amount) public {
125+
token.transferFrom(from, to, amount);
126+
token.transferFrom(from, to, amount);
127+
}
128+
}

‎crates/lint/testdata/UncheckedTransferERC20.stderr‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,47 @@
11
warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value
22
--> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC
33
|
4-
20 | token.transfer(to, amount);
4+
27 | IERC20(address(token)).transfer(to, amount);
5+
| -------------------------------------------
6+
|
7+
= help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer
8+
9+
warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value
10+
--> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC
11+
|
12+
28 | token.transfer(to, amount);
513
| --------------------------
614
|
715
= help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer
816

917
warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value
1018
--> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC
1119
|
12-
24 | token.transferFrom(from, to, amount);
13-
| ------------------------------------
20+
32 | ... IERC20(address(token)).transferFrom(from, to, amount);
21+
| -----------------------------------------------------
1422
|
1523
= help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer
1624

1725
warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value
1826
--> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC
1927
|
20-
28 | token.transfer(to, amount / 2);
21-
| ------------------------------
28+
33 | token.transferFrom(from, to, amount);
29+
| ------------------------------------
2230
|
2331
= help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer
2432

2533
warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value
2634
--> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC
2735
|
28-
29 | token.transfer(to, amount / 2);
29-
| ------------------------------
36+
38 | ... IERC20(address(token)).transfer(recipients[i], amounts[i]);
37+
| ----------------------------------------------------------
3038
|
3139
= help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer
3240

3341
warning[erc20-unchecked-transfer]: ERC20 'transfer' and 'transferFrom' calls should check the return value
3442
--> ROOT/testdata/UncheckedTransferERC20.sol:LL:CC
3543
|
36-
34 | token.transfer(recipients[i], amounts[i]);
44+
39 | token.transfer(recipients[i], amounts[i]);
3745
| -----------------------------------------
3846
|
3947
= help: https://book.getfoundry.sh/reference/forge/forge-lint#erc20-unchecked-transfer

0 commit comments

Comments
 (0)