feat(lint): detect external calls in loops#14778
Conversation
ad3d193 to
d1394c2
Compare
d1394c2 to
fa25ecd
Compare
| res.as_variable().is_some_and(|var_id| { | ||
| matches!( | ||
| hir.variable(var_id).ty.kind, | ||
| TypeKind::Elementary(ty) if ty.to_abi_str() == "address" |
There was a problem hiding this comment.
ElementaryType::Address(_) pattern match is cleaner imo than to_abi_str() == "address"
| ) | ||
| } | ||
|
|
||
| fn is_contract_like(hir: &Hir<'_>, expr: &Expr<'_>) -> bool { |
There was a problem hiding this comment.
It seems that receivers derived from returned values, mappings, or struct fields are missed: getRecipient().foo(), m[addr].foo(), s.target.foo()
mattsse
left a comment
There was a problem hiding this comment.
I found two false-negative cases worth addressing before this lands:
-
crates/lint/src/sol/low/calls_loop.rsignoresStmtKind::Placeholder, so calls executed through a modifier placeholder inside a loop are missed. For example,modifier twice() { for (...) { _; } }applied tofunction f() external twice { receiver.ping(0); }executes the external call in a loop, but the modifier pass sees only_at loop depth > 0 and the function pass sees the call at loop depth 0. The reentrancy lint expands modifier placeholders; this lint likely needs similar handling. -
is_internal_callableskipspureinternal/private helpers, but pure helpers can still makeexternal pureinterface calls. I verifiedsolc 0.8.30accepts an internalpurehelper that callsIReceiver.ping(...) external pure; if that helper is called from a loop, this lint won’t descend into it. This should either descend into pure helpers too or add coverage showing a deliberately narrower behavior.
mattsse
left a comment
There was a problem hiding this comment.
I found a couple of remaining edge cases that should be addressed before this lands.
|
|
||
| fn is_contract_like(hir: &Hir<'_>, expr: &Expr<'_>) -> bool { | ||
| match &expr.peel_parens().kind { | ||
| ExprKind::Call(callee, _, _) |
There was a problem hiding this comment.
Explicit interface casts still appear to be missed here. is_contract_like only treats cast callees shaped as ExprKind::Type(...), but the existing ERC20 unchecked-transfer matcher handles interface casts as ExprKind::Ident([Res::Item(ItemId::Contract(_))]); IReceiver(target).ping(i) inside a loop would not pass this branch and will be skipped. Can we add that HIR shape plus a regression test? This is in the documented contract/interface-call scope.
| fn is_external_call(hir: &Hir<'_>, callee: &Expr<'_>) -> bool { | ||
| let ExprKind::Member(base, member) = &callee.peel_parens().kind else { return false }; | ||
|
|
||
| if matches!(member.name, kw::Call | kw::Delegatecall | kw::Staticcall) { |
There was a problem hiding this comment.
Low-level call names are emitted solely by member name here. Solidity allows library extension methods named call, delegatecall, or staticcall on non-address values; box.call() in a loop would be reported as an external low-level call even though the docs say library-style member calls on non-contract values are ignored. Please gate these names on an address-like base or add coverage for the intended behavior.
Summary
calls-looplintsend/transfer, contract/interface calls,this.foo(), and internal helpers called from a loop