feat(forge-lint): [claude] check for unwrapped modifiers

[0xClandestine]

Motivation

Coauthored by Claude 4 Opus Max

Modifiers in Solidity that contain logic directly in their body can lead to code duplication when used across multiple functions. Since modifiers are inlined at compile time, any logic within them is duplicated for each function that uses the modifier increasing contract size and deployment cost. A common best practice is to extract modifier logic into internal functions that can be called from within the modifier, reducing code duplication and contract size.

Solution

This PR adds a new gas optimization lint unwrapped-modifier-logic that detects when modifiers contain logic directly in their body instead of using internal/private/public functions.

The lint checks for modifiers that contain any statements other than:

• Direct calls to internal/private/public functions.
• The placeholder _;

Example of code that triggers the lint:

modifier onlyOwner() {
    require(msg.sender == owner, "Not owner"); // Unwrapped logic
    _;
}

Recommended pattern:

modifier onlyOwner() {
    _checkOwner(); // Wrapped in internal function
    _;
}

function _checkOwner() internal view {
    require(msg.sender == owner, "Not owner");
}

PR Checklist

• Added Tests - Created testdata/ModifierLogic.sol and testdata/ModifierLogic.stderr with comprehensive test cases
• Added Documentation - Need to add lint documentation to the Foundry book under the gas optimization section
• Breaking changes - No breaking changes, this is a new lint rule

[grandizzy]

thank you, makes sense. left some nits, pending @0xrusowsky @DaniPopes review

[0xrusowsky]

overall the PR looks good, however i'd like to hold it back until we merge:

• feat(forge-lint): new LateLintPass + support code snippets #10846

and then make it do a fix suggestion

[kaxhnet]

\

[grandizzy]

overall the PR looks good, however i'd like to hold it back until we merge:

* [feat(forge-lint): new `LateLintPass` + support code snippets #10846](https://github.com/foundry-rs/foundry/pull/10846)

and then make it do a fix suggestion

that's merged now so we can rebuild this PR on top of it

[0xrusowsky]

@0xClandestine if u want to see an example of how to generate snippets, you can check:

• https://github.com/foundry-rs/foundry/blob/rusowsky/keccak-gen-asm/crates/lint/src/sol/gas/keccak.rs

note that in that case, rather than using the AST, i used the HIR cause i needed semantic info (i think that for you case the AST should be enough)

[0xrusowsky]

please improve the code suggestions, and let's wait for others to voice their opinions on whether to use fn post_source_unit or not

[0xrusowsky]

while i expect this to be true most of the time, technically speaking the lint will only reduce codesize if the modifier is used more than once.

@grandizzy @DaniPopes should we use fn post_source_unit to also account for the modifier usage?

[grandizzy]

makes sense to warn only if used more than once

[0xClandestine]

You're right. However, I think it's still a good idea to emit the lint regardless if it the modifier is used more than once in the scope of the current contract. Abstract contracts like openzeppelin's ReentrancyGuard don't use their own modifiers directly; instead, they're intended for use by inheriting contracts.

Alternatively, could we simply note that the lint only results in bytecode size savings when the modifier it flags is used more than once?

[0xrusowsky]

sounds good, let's make sure to add it to the foundry book when documenting this lint

[0xrusowsky]

since we check that total_valid > 1, can we instead turn it into a bool and early exit when the 2nd valid is found?

Suggested change

	fn check_stmts(&self, stmts: &[Stmt<'_>]) -> bool {
	let mut total_valid = 0;
	for stmt in stmts {
	if !self.is_valid_stmt(stmt) {
	return true;
	}
	if let StmtKind::Expr(expr) = &stmt.kind
	&& self.is_valid_expr(expr)
	{
	total_valid += 1;
	}
	}
	total_valid > 1
	}
	fn check_stmts(&self, stmts: &[Stmt<'_>]) -> bool {
	let mut has_valid = false;
	for stmt in stmts {
	if !self.is_valid_stmt(stmt) {
	return true;
	}
	if let StmtKind::Expr(expr) = &stmt.kind
	&& self.is_valid_expr(expr)
	{
	if has_valid { return true; }
	has_valid = true;
	}
	}
	false
	}

[0xrusowsky]

note for other reviewers: this fn is weird but kinda needed cause the diff is produced against the code span (which is printed indented).

i'll do a follow-up PR to remove the indentation of the original span, so that fixes don't need to care about spacing:

• feat(lint): auto remove indentation of code to be removed when using ctx.emit_with_fix() #11038

once that is implemented, this fn would only need to return the whole fn span

[0xrusowsky]

why would it throw false positives @0xClandestine?

[0xClandestine]

There's not a good way to differentiate between library member calls (which are valid) and other member calls (e.g. external calls) thus we simply mark all member calls as invalid.

[0xrusowsky]

ok, i see.

if that's the case i'd encourage you to move your lint to LateLintPass, as there you'll have the semantic information required to know if the the contract being called is a library or not... i think it's best if we avoid throwing false positives now that we have the tools 🙂 (you'll see that using the HIR is quite similar to using the AST)

also if you need help or don't feel like doing the change yourself, i can give u a hand, just lmk!