diff --git a/crates/lint/src/linter/mod.rs b/crates/lint/src/linter/mod.rs index f6770038eecf3..dcd86d16e2c6a 100644 --- a/crates/lint/src/linter/mod.rs +++ b/crates/lint/src/linter/mod.rs @@ -26,7 +26,7 @@ use crate::inline_config::InlineConfig; /// /// # Required Methods /// -/// - `init`: Creates a new solar `Session` with the appropiate linter configuration. +/// - `init`: Creates a new solar `Session` with the appropriate linter configuration. /// - `early_lint`: Scans the source files (using the AST) emitting a diagnostic for lints found. /// - `late_lint`: Scans the source files (using the HIR) emitting a diagnostic for lints found. pub trait Linter: Send + Sync + Clone { diff --git a/crates/lint/src/sol/gas/mod.rs b/crates/lint/src/sol/gas/mod.rs index 9664f3baf6dcc..7b879d98ce8fc 100644 --- a/crates/lint/src/sol/gas/mod.rs +++ b/crates/lint/src/sol/gas/mod.rs @@ -3,4 +3,10 @@ use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; mod keccak; use keccak::ASM_KECCAK256; -register_lints!((AsmKeccak256, late, (ASM_KECCAK256))); +mod unwrapped_modifier_logic; +use unwrapped_modifier_logic::UNWRAPPED_MODIFIER_LOGIC; + +register_lints!( + (AsmKeccak256, late, (ASM_KECCAK256)), + (UnwrappedModifierLogic, early, (UNWRAPPED_MODIFIER_LOGIC)) +); diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs new file mode 100644 index 0000000000000..471db10a6b60d --- /dev/null +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -0,0 +1,203 @@ +use super::UnwrappedModifierLogic; +use crate::{ + linter::{EarlyLintPass, LintContext, Snippet}, + sol::{Severity, SolLint}, +}; +use solar_ast::{ExprKind, ItemFunction, Stmt, StmtKind}; +use solar_interface::BytePos; + +declare_forge_lint!( + UNWRAPPED_MODIFIER_LOGIC, + Severity::Gas, + "unwrapped-modifier-logic", + "wrap modifier logic to reduce code size" +); + +impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { + fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { + // Only check modifiers with a body and name. + if !func.kind.is_modifier() || func.body.is_none() || func.header.name.is_none() { + return; + } + + let name = func.header.name.unwrap(); + let stmts = &func.body.as_ref().unwrap().stmts[..]; + + // Split statements into before and after the placeholder `_`. + let (before, after) = stmts + .iter() + .position(|s| matches!(s.kind, StmtKind::Placeholder)) + .map_or((stmts, &[][..]), |idx| (&stmts[..idx], &stmts[idx + 1..])); + + // Generate a fix snippet if the modifier logic should be wrapped. + if let Some(snippet) = self.get_snippet(ctx, func, stmts, before, after) { + ctx.emit_with_fix(&UNWRAPPED_MODIFIER_LOGIC, name.span, snippet); + } + } +} + +impl UnwrappedModifierLogic { + // TODO: Support library member calls like `Lib.foo` (throws false positives). + fn is_valid_expr(&self, expr: &solar_ast::Expr<'_>) -> bool { + if let ExprKind::Call(func_expr, _) = &expr.kind + && let ExprKind::Ident(ident) = &func_expr.kind + { + return !matches!(ident.name.as_str(), "require" | "assert"); + } + false + } + + fn is_valid_stmt(&self, stmt: &Stmt<'_>) -> bool { + match &stmt.kind { + StmtKind::Expr(expr) => self.is_valid_expr(expr), + StmtKind::Placeholder => true, + _ => false, + } + } + + 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 get_indent_and_span( + &self, + ctx: &LintContext<'_>, + func: &ItemFunction<'_>, + full_body: &[Stmt<'_>], + ) -> (String, String, solar_ast::Span) { + let (first, _) = match (full_body.first(), full_body.last()) { + (Some(f), Some(l)) => (f, l), + _ => { + let name_span = func.header.name.unwrap().span; + return (" ".to_string(), " ".to_string(), name_span); + } + }; + + let source_map = ctx.session().source_map(); + let loc_info = source_map.lookup_char_pos(first.span.lo()); + let line_start = first.span.lo() - BytePos(loc_info.col.to_usize() as u32); + + let body_indent = source_map + .span_to_snippet(solar_ast::Span::new(line_start, first.span.lo())) + .unwrap_or_else(|_| " ".to_string()); + + // Get modifier indent + let name_span = func.header.name.unwrap().span; + let pos = source_map.lookup_char_pos(name_span.lo()); + let mod_line_start = name_span.lo() - BytePos(pos.col.to_usize() as u32); + + let mod_indent = source_map + .span_to_snippet(solar_ast::Span::new(mod_line_start, name_span.lo())) + .ok() + .and_then(|s| s.rfind("modifier").map(|p| mod_line_start + BytePos(p as u32))) + .and_then(|start| { + source_map.span_to_snippet(solar_ast::Span::new(mod_line_start, start)).ok() + }) + .unwrap_or_else(|| " ".to_string()); + + // Get full function span + let start = name_span.lo() + - BytePos(source_map.lookup_char_pos(name_span.lo()).col.to_usize() as u32); + let span = func + .body + .as_ref() + .map(|b| solar_ast::Span::new(start, b.span.hi())) + .unwrap_or(name_span); + + (body_indent, mod_indent, span) + } + + fn get_snippet<'a>( + &self, + ctx: &LintContext<'_>, + func: &ItemFunction<'_>, + full_body: &'a [Stmt<'a>], + before: &'a [Stmt<'a>], + after: &'a [Stmt<'a>], + ) -> Option { + let wrap_before = !before.is_empty() && self.check_stmts(before); + let wrap_after = !after.is_empty() && self.check_stmts(after); + + if !(wrap_before || wrap_after) { + return None; + } + + let header_name = func.header.name.unwrap(); + let modifier_name = header_name.name.as_str(); + let params = &func.header.parameters; + + let param_list = params + .vars + .iter() + .filter_map(|v| v.name.as_ref().map(|n| n.name.to_string())) + .collect::>() + .join(", "); + + let param_decls = params + .vars + .iter() + .map(|v| { + let name = v.name.as_ref().map(|n| n.name.as_str()).unwrap_or(""); + let ty = ctx + .span_to_snippet(v.ty.span) + .unwrap_or_else(|| "/* unknown type */".to_string()); + format!("{ty} {name}") + }) + .collect::>() + .join(", "); + + let (body_indent, mod_indent, span) = self.get_indent_and_span(ctx, func, full_body); + + let body = match (wrap_before, wrap_after) { + (true, true) => format!( + "{body_indent}_{modifier_name}Before({param_list});\n{body_indent}_;\n{body_indent}_{modifier_name}After({param_list});" + ), + (true, false) => { + format!("{body_indent}_{modifier_name}({param_list});\n{body_indent}_;") + } + (false, true) => { + format!("{body_indent}_;\n{body_indent}_{modifier_name}({param_list});") + } + _ => unreachable!(), + }; + + let mut replacement = format!( + "{mod_indent}modifier {modifier_name}({param_decls}) {{\n{body}\n{mod_indent}}}" + ); + + let build_func = |stmts: &[Stmt<'_>], suffix: &str| { + let body_stmts = stmts + .iter() + .filter_map(|s| ctx.span_to_snippet(s.span)) + .map(|code| format!("\n{body_indent}{code}")) + .collect::(); + format!( + "\n\n{mod_indent}function _{modifier_name}{suffix}({param_decls}) internal {{{body_stmts}\n{mod_indent}}}" + ) + }; + + if wrap_before { + replacement.push_str(&build_func(before, if wrap_after { "Before" } else { "" })); + } + if wrap_after { + replacement.push_str(&build_func(after, if wrap_before { "After" } else { "" })); + } + + Some(Snippet::Diff { + desc: Some("wrap modifier logic to reduce code size"), + span: Some(span), + add: replacement, + }) + } +} diff --git a/crates/lint/testdata/UnwrappedModifierLogic.sol b/crates/lint/testdata/UnwrappedModifierLogic.sol new file mode 100644 index 0000000000000..e233110bfaa34 --- /dev/null +++ b/crates/lint/testdata/UnwrappedModifierLogic.sol @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +/** + * @title UnwrappedModifierLogicTest + * @notice Test cases for the unwrapped-modifier-logic lint + * @dev This lint helps optimize gas by preventing modifier code duplication. + * Solidity inlines modifier code at each usage point instead of using jumps, + * so any logic in modifiers gets duplicated, increasing deployment costs. + */ +contract UnwrappedModifierLogicTest { + // Helpers + + event DidSomething(address who); + mapping(address => bool) isOwner; + mapping(address => mapping(bytes32 => bool)) hasRole; + + /// ----------------------------------------------------------------------- + /// Good patterns (only 1 valid statement before or after placeholder) + /// ----------------------------------------------------------------------- + + function checkPublic(address sender) public {} + function checkPrivate(address sender) private {} + function checkInternal(address sender) internal {} + + modifier onlyOwnerPublic() { + checkPublic(msg.sender); + _; + } + + modifier onlyOwnerPrivate() { + checkPrivate(msg.sender); + _; + } + + modifier onlyOwnerInternal() { + checkInternal(msg.sender); + _; + } + + modifier onlyOwnerBeforeAfter() { + checkPublic(msg.sender); + _; + checkPrivate(msg.sender); + } + + /// ----------------------------------------------------------------------- + /// Bad patterns (multiple valid statements before or after placeholder) + /// ----------------------------------------------------------------------- + + // Bad because there are multiple valid function calls before the placeholder + modifier multipleBeforePlaceholder() { //~NOTE: wrap modifier logic to reduce code size + checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() + checkPrivate(msg.sender); + checkInternal(msg.sender); + _; + } + + // Bad because there are multiple valid function calls after the placeholder + modifier multipleAfterPlaceholder() { //~NOTE: wrap modifier logic to reduce code size + _; + checkPublic(msg.sender); // These should become _multipleAfterPlaceholder() + checkPrivate(msg.sender); + checkInternal(msg.sender); + } + + // Bad because there are multiple valid statements both before and after + modifier multipleBeforeAfterPlaceholder(address sender) { //~NOTE: wrap modifier logic to reduce code size + checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) + checkPrivate(sender); + _; + checkInternal(sender); // These should become _multipleBeforeAfterPlaceholderAfter(sender) + checkPublic(sender); + } + + /// ----------------------------------------------------------------------- + /// Bad patterns (uses built-in control flow) + /// ----------------------------------------------------------------------- + + // Bad because `require` built-in is used. + modifier onlyOwner() { //~NOTE: wrap modifier logic to reduce code size + require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); + _; + } + + // Bad because `if/revert` is used. + modifier onlyRole(bytes32 role) { //~NOTE: wrap modifier logic to reduce code size + if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); + _; + } + + // Bad because `assert` built-in is used. + modifier onlyRoleOrOpenRole(bytes32 role) { //~NOTE: wrap modifier logic to reduce code size + assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); + _; + } + + // Bad because `assert` built-in is used (ensures we can parse multiple params). + modifier onlyRoleOrAdmin(bytes32 role, address admin) { //~NOTE: wrap modifier logic to reduce code size + assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); + _; + } + + /// ----------------------------------------------------------------------- + /// Bad patterns (other invalid expressions and statements) + /// ----------------------------------------------------------------------- + + // Only call expressions are allowed (public/private/internal functions). + modifier assign(address sender) { //~NOTE: wrap modifier logic to reduce code size + bool _isOwner = true; + isOwner[sender] = _isOwner; + _; + } + + // Only call expressions are allowed (public/private/internal functions). + modifier assemblyBlock(address sender) { //~NOTE: wrap modifier logic to reduce code size + assembly { + let x := sender + } + _; + } + + // Only call expressions are allowed (public/private/internal functions). + modifier uncheckedBlock(address sender) { //~NOTE: wrap modifier logic to reduce code size + unchecked { + sender; + } + _; + } + + // Only call expressions are allowed (public/private/internal functions). + modifier emitEvent(address sender) { //~NOTE: wrap modifier logic to reduce code size + emit DidSomething(sender); + _; + } +} \ No newline at end of file diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr new file mode 100644 index 0000000000000..fba9b80753397 --- /dev/null +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -0,0 +1,281 @@ +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +52 | modifier multipleBeforePlaceholder() { + | ------------------------- + | + = note: wrap modifier logic to reduce code size + + - modifier multipleBeforePlaceholder() { + - checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() + - checkPrivate(msg.sender); + - checkInternal(msg.sender); + - _; + - } + + modifier multipleBeforePlaceholder() { + + _multipleBeforePlaceholder(); + + _; + + } + + + + function _multipleBeforePlaceholder() internal { + + checkPublic(msg.sender); + + checkPrivate(msg.sender); + + checkInternal(msg.sender); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +60 | modifier multipleAfterPlaceholder() { + | ------------------------ + | + = note: wrap modifier logic to reduce code size + + - modifier multipleAfterPlaceholder() { + - _; + - checkPublic(msg.sender); // These should become _multipleAfterPlaceholder() + - checkPrivate(msg.sender); + - checkInternal(msg.sender); + - } + + modifier multipleAfterPlaceholder() { + + _; + + _multipleAfterPlaceholder(); + + } + + + + function _multipleAfterPlaceholder() internal { + + checkPublic(msg.sender); + + checkPrivate(msg.sender); + + checkInternal(msg.sender); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +68 | modifier multipleBeforeAfterPlaceholder(address sender) { + | ------------------------------ + | + = note: wrap modifier logic to reduce code size + + - modifier multipleBeforeAfterPlaceholder(address sender) { + - checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) + - checkPrivate(sender); + - _; + - checkInternal(sender); // These should become _multipleBeforeAfterPlaceholderAfter(sender) + - checkPublic(sender); + - } + + modifier multipleBeforeAfterPlaceholder(address sender) { + + _multipleBeforeAfterPlaceholderBefore(sender); + + _; + + _multipleBeforeAfterPlaceholderAfter(sender); + + } + + + + function _multipleBeforeAfterPlaceholderBefore(address sender) internal { + + checkPublic(sender); + + checkPrivate(sender); + + } + + + + function _multipleBeforeAfterPlaceholderAfter(address sender) internal { + + checkInternal(sender); + + checkPublic(sender); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +81 | modifier onlyOwner() { + | --------- + | + = note: wrap modifier logic to reduce code size + + - modifier onlyOwner() { + - require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); + - _; + - } + + modifier onlyOwner() { + + _onlyOwner(); + + _; + + } + + + + function _onlyOwner() internal { + + require(isOwner[msg.sender], "Not owner"); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +87 | modifier onlyRole(bytes32 role) { + | -------- + | + = note: wrap modifier logic to reduce code size + + - modifier onlyRole(bytes32 role) { + - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); + - _; + - } + + modifier onlyRole(bytes32 role) { + + _onlyRole(role); + + _; + + } + + + + function _onlyRole(bytes32 role) internal { + + if(!hasRole[msg.sender][role]) revert("Not authorized"); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +93 | modifier onlyRoleOrOpenRole(bytes32 role) { + | ------------------ + | + = note: wrap modifier logic to reduce code size + + - modifier onlyRoleOrOpenRole(bytes32 role) { + - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); + - _; + - } + + modifier onlyRoleOrOpenRole(bytes32 role) { + + _onlyRoleOrOpenRole(role); + + _; + + } + + + + function _onlyRoleOrOpenRole(bytes32 role) internal { + + assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +99 | modifier onlyRoleOrAdmin(bytes32 role, address admin) { + | --------------- + | + = note: wrap modifier logic to reduce code size + + - modifier onlyRoleOrAdmin(bytes32 role, address admin) { + - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); + - _; + - } + + modifier onlyRoleOrAdmin(bytes32 role, address admin) { + + _onlyRoleOrAdmin(role, admin); + + _; + + } + + + + function _onlyRoleOrAdmin(bytes32 role, address admin) internal { + + assert(hasRole[msg.sender][role] || msg.sender == admin); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +109 | modifier assign(address sender) { + | ------ + | + = note: wrap modifier logic to reduce code size + + - modifier assign(address sender) { + - bool _isOwner = true; + - isOwner[sender] = _isOwner; + - _; + - } + + modifier assign(address sender) { + + _assign(sender); + + _; + + } + + + + function _assign(address sender) internal { + + bool _isOwner = true; + + isOwner[sender] = _isOwner; + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +116 | modifier assemblyBlock(address sender) { + | ------------- + | + = note: wrap modifier logic to reduce code size + + - modifier assemblyBlock(address sender) { + - assembly { + - let x := sender + - } + - _; + - } + + modifier assemblyBlock(address sender) { + + _assemblyBlock(sender); + + _; + + } + + + + function _assemblyBlock(address sender) internal { + + assembly { + + let x := sender + + } + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +124 | modifier uncheckedBlock(address sender) { + | -------------- + | + = note: wrap modifier logic to reduce code size + + - modifier uncheckedBlock(address sender) { + - unchecked { + - sender; + - } + - _; + - } + + modifier uncheckedBlock(address sender) { + + _uncheckedBlock(sender); + + _; + + } + + + + function _uncheckedBlock(address sender) internal { + + unchecked { + + sender; + + } + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +132 | modifier emitEvent(address sender) { + | --------- + | + = note: wrap modifier logic to reduce code size + + - modifier emitEvent(address sender) { + - emit DidSomething(sender); + - _; + - } + + modifier emitEvent(address sender) { + + _emitEvent(sender); + + _; + + } + + + + function _emitEvent(address sender) internal { + + emit DidSomething(sender); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic +