feat(forge-lint): [claude] unchecked calls

[0xrusowsky]

Motivation

• Continue expanding the supported lints supported by forge-lint
• Inspired by this ghostty PR and the prompts that Mitchell Hashimoto shared, we decided to showcase how to leverage LLMs and agentic —tools such as claude code— in our codebase, to encourage security researchers with limited rust knowledge to contribute.

Solution

Add 2 new lints to warn about unchecked:

• low-level calls
• ERC20 transfers

Setup:

The development of this PR was done by running Claude code and giving it access to both solar (so that it could check the AST impls when necessary) and foundry:

claude/
├── solar/
└── foundry/

IMPORTANT: link @foundry/docs/dev/lintrules.md in the prompts, so that claude is aware of the implementation and testing guidelines for forge-lint

Used Prompts:

Each individual prompt is separated by a blank line, to showcase the conversation with Claude code.

i want to extend forge-lint by adding support for new lints, as so far only the lints in @foundry/crates/lint/src/sol/ are supported.
let's implement a new lint for unchecked-transfer-erc20 which should validate whether ERC20 calls to transfer(address to, uint256 amount) and transferFrom(address from, address to, uint256 amount) check the return value. to do so, i think that the lint pass should use fn check_item_function to check all contract fns and flag calls that don't have its output assigned to a variable.
to understand how to add new lints, you can check the implementation guidelines defined in @foundry/docs/dev/lintrules.md.
for AST-related context, you can check @solar/crates/solar/ast/src/ast.

at this point we had a working lint! but it was quite ugly, so i made a follow-up prompt to improve it:

that's great! fn visit_stmt is too complex, you don't manually need to handle all cases. Instead
if the stmt is not an expression, simply do self.walk_stmt(stmt).
also, we don't need to cache whether we find unchecked calls or not.
finally can we improve the docs of @foundry/crates/lint/src/sol/high/unchecked_transfer_erc20.rs so that people can easily follow its implementation? don't make them excessively verbose, but expand the current docs

here we had a working lint for unchecked ERC20 transfers!
however, i wanted to push it a little bit more and also asked for a new lint that would do the same for low-level calls

excellent. can we add a similar lint rule that ensures that low-level calls check the success of the call?

it correctly implemented the logic to identify low-level calls.
although, as expected, it only checked for calls without variable assignments. Since low-level calls return a tuple, it is also necessary to handle the case where users do something with the returned bytes but not with the success boolean.

UncheckedCall is almost there, however, we need to ensure that the user at least uses (bool success, ) = target.call(""). note that checks should work regardless of the var names, and independently if the second var is used or not.

at this point we had a working implementation, which i tweaked a little bit manually. Finally i asked it to refactor the code and merge everything into a single file to keep "unchecked call" lints together:

great. let's refactor the code into a single file unchecked_calls.rs that contains different lints (UNCHECKED_CALL, UNCHECKED_TRANSFER_ERC20, etc).

finally i did a style-based refactor for code readability and consistency (and also had to run clippy and fmt)

[grandizzy]

nice, I think that's really great. Left couple of comments, pls check

[grandizzy]

nit: I suggest moving function names in fn description and simplify check from match to bellow

/// 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 {
            let method_name = member.as_str();
            return (args.len() == 2 && method_name == "transfer") ||
                (args.len() == 3 && method_name == "transferFrom")
        }
    }
    false
}

[grandizzy]

also, couldn't this give false warnings in case of a non ERC20 contract (Proxy) with transfer() or transferFrom() fns, e.g.

contract Proxy {
    function transfer(address to, uint256 amount) public {
        require(IERC20(token).transfer(to, amount), "Transfer failed");
    }
}

contract MyContract {
    Proxy public proxy;

    function refill() public {
        // this one fails lint
    	proxy.transfer(address(1), 1);
    }
}

so maybe we should check that fn returns a bool? (even so it can flag false warnings but could be more robust)

[0xrusowsky]

mmm that's true.

this check is more tricky, let me think alternatives do we have

[yash-atreya]

@0xrusowsky I think we can live with this, now that we have #10776. We can add a in docs regarding this false positive and address it in a followup?

[0xrusowsky]

i think this is pragmatic short-term solution if we want to move forward and support new lints. also it will keep the PR simple as an example on how to use LLMs effectively.

i think that once we merge #10662 we should be able to use the new fn post_source_unit to figure out if it really is an ERC20 or not.

[0xrusowsky]

later today i will link a PR with the docs, explaining that this lint can issue false positives

[0xrusowsky]

prompt to address @grandizzy's feedback:

can you incorporate the suggestions in the PR review [feat(forge-lint): [claude] unchecked calls](https://github.com/foundry-rs/foundry/pull/10810). I want you to address all the raised
comments except the one regarding potential false positives (this one we will ignore for now, as it
is more complex).
note the you can use the github CLI via gh

write a short commit msg

[yash-atreya]

lgtm!

Re: false positive #10810 (comment)

We can live with it for now and address it in a followup.

approved by Yash and Dani (pending to be improved)
users can now disable false positives thanks to inline config

[DaniPopes]

note that the default commit description when merging prs here is the aggregate of all the commits, so you should add the prompts to the commits themselves

[0xrusowsky]

note that the default commit description when merging prs here is the aggregate of all the commits, so you should add the prompts to the commits themselves

can i manually squash and use the PR description as the summary description of the squashed commit? shouldn't that work?

[DaniPopes]

sure, just noting that the prompts would not be currently included had i not overridden the message by pasting the description in

[0xrusowsky]

sure, just noting that the prompts would not be currently included had i not overridden the message by pasting the description in

makes sense, thanks for the heads up

[grandizzy]

lgtm