feat(lint): detect external calls in loops#14778
Conversation
figtracer
force-pushed
the
feat/lint-unprotected-selfdestruct
branch
from
ad3d193 to
d1394c2
Compare
figtracer
changed the title
figtracer
force-pushed
the
feat/lint-unprotected-selfdestruct
branch
from
d1394c2 to
fa25ecd
Compare
figtracer
changed the title
figtracer
requested review from
0xrusowsky,
DaniPopes,
grandizzy,
mablr,
mattsse,
stevencartavia and
zerosnacks
as code owners
| 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
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
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.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
4 participants
Summary
calls-looplintsend/transfer, contract/interface calls,this.foo(), and internal helpers called from a loop