diff --git a/crates/lint/README.md b/crates/lint/README.md index e2bb610f12f2a..2940c8030e841 100644 --- a/crates/lint/README.md +++ b/crates/lint/README.md @@ -13,6 +13,7 @@ It helps enforce best practices and improve code quality within Foundry projects - `erc20-unchecked-transfer`: ERC20 `transfer` and `transferFrom` calls should check the return value. - **Medium Severity:** - `divide-before-multiply`: Warns against performing division before multiplication in the same expression, which can cause precision loss. + - `incorrect-erc20-interface`: Flags ERC20 interfaces and implementations with non-compliant function signatures. - `incorrect-erc721-interface`: Flags ERC721 interfaces and implementations with non-compliant function signatures. - `unsafe-typecast`: Typecasts that can truncate values should be checked. - **Informational / Style Guide:** diff --git a/crates/lint/src/sol/med/incorrect_erc20_interface.rs b/crates/lint/src/sol/med/incorrect_erc20_interface.rs new file mode 100644 index 0000000000000..b2f81e4503200 --- /dev/null +++ b/crates/lint/src/sol/med/incorrect_erc20_interface.rs @@ -0,0 +1,112 @@ +use super::IncorrectERC20Interface; +use crate::{ + linter::{LateLintPass, LintContext}, + sol::{Severity, SolLint}, +}; +use solar::sema::hir; + +declare_forge_lint!( + INCORRECT_ERC20_INTERFACE, + Severity::Med, + "incorrect-erc20-interface", + "incorrect ERC20 function interface" +); + +impl<'hir> LateLintPass<'hir> for IncorrectERC20Interface { + fn check_contract( + &mut self, + ctx: &LintContext, + hir: &'hir hir::Hir<'hir>, + contract: &'hir hir::Contract<'hir>, + ) { + // Check if the contract is a possible ERC20 by name or inheritance. + let is_erc20 = contract.linearized_bases.iter().any(|base_id| { + let name = hir.contract(*base_id).name.as_str(); + name == "ERC20" || name == "IERC20" + }); + + if !is_erc20 { + return; + } + + // If this contract implements a function from ERC721, we can assume it is an ERC721 token. + // These tokens offer functions which are similar to ERC20, but are not compatible. + let is_erc721 = contract.linearized_bases.iter().any(|base_id| { + let name = hir.contract(*base_id).name.as_str(); + name == "ERC721" || name == "IERC721" + }); + + if is_erc721 { + return; + } + + // Check each function in the contract for incorrect ERC20 signatures. + for item_id in contract.items { + let Some(fid) = item_id.as_function() else { continue }; + let func = hir.function(fid); + + if !func.kind.is_function() { + continue; + } + + let Some(name) = func.name else { continue }; + + if has_incorrect_erc20_signature(hir, name.as_str(), func.parameters, func.returns) { + ctx.emit(&INCORRECT_ERC20_INTERFACE, func.span); + } + } + } +} + +/// Checks if a function signature does not match the expected ERC20 specification. +/// +/// Returns `true` if the function name and parameter types match an ERC20 function but the return +/// types are incorrect. +fn has_incorrect_erc20_signature( + hir: &hir::Hir<'_>, + name: &str, + parameters: &[hir::VariableId], + returns: &[hir::VariableId], +) -> bool { + let is_type = |var_id: hir::VariableId, type_str: &str| { + matches!( + &hir.variable(var_id).ty.kind, + hir::TypeKind::Elementary(ty) if ty.to_abi_str() == type_str + ) + }; + + let params_match = |params: &[hir::VariableId], expected: &[&str]| -> bool { + params.len() == expected.len() + && params.iter().zip(expected).all(|(&id, &ty)| is_type(id, ty)) + }; + + let returns_match = |rets: &[hir::VariableId], expected: &[&str]| -> bool { + rets.len() == expected.len() && rets.iter().zip(expected).all(|(&id, &ty)| is_type(id, ty)) + }; + + match name { + // function transfer(address,uint256) external returns (bool) + "transfer" if params_match(parameters, &["address", "uint256"]) => { + !returns_match(returns, &["bool"]) + } + // function transferFrom(address,address,uint256) external returns (bool) + "transferFrom" if params_match(parameters, &["address", "address", "uint256"]) => { + !returns_match(returns, &["bool"]) + } + // function approve(address,uint256) external returns (bool) + "approve" if params_match(parameters, &["address", "uint256"]) => { + !returns_match(returns, &["bool"]) + } + // function allowance(address,address) external view returns (uint256) + "allowance" if params_match(parameters, &["address", "address"]) => { + !returns_match(returns, &["uint256"]) + } + // function balanceOf(address) external view returns (uint256) + "balanceOf" if params_match(parameters, &["address"]) => { + !returns_match(returns, &["uint256"]) + } + // function totalSupply() external view returns (uint256) + "totalSupply" if params_match(parameters, &[]) => !returns_match(returns, &["uint256"]), + _ => false, + } +} diff --git a/crates/lint/src/sol/med/mod.rs b/crates/lint/src/sol/med/mod.rs index 92a38529e0c09..ba7a09b0e9bac 100644 --- a/crates/lint/src/sol/med/mod.rs +++ b/crates/lint/src/sol/med/mod.rs @@ -3,6 +3,9 @@ use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; mod div_mul; use div_mul::DIVIDE_BEFORE_MULTIPLY; +mod incorrect_erc20_interface; +use incorrect_erc20_interface::INCORRECT_ERC20_INTERFACE; + mod incorrect_erc721_interface; use incorrect_erc721_interface::INCORRECT_ERC721_INTERFACE; @@ -11,6 +14,7 @@ use unsafe_typecast::UNSAFE_TYPECAST; register_lints!( (DivideBeforeMultiply, early, (DIVIDE_BEFORE_MULTIPLY)), + (IncorrectERC20Interface, late, (INCORRECT_ERC20_INTERFACE)), (IncorrectERC721Interface, late, (INCORRECT_ERC721_INTERFACE)), (UnsafeTypecast, late, (UNSAFE_TYPECAST)) ); diff --git a/crates/lint/testdata/IncorrectERC20Interface.sol b/crates/lint/testdata/IncorrectERC20Interface.sol new file mode 100644 index 0000000000000..bfc571ec0a474 --- /dev/null +++ b/crates/lint/testdata/IncorrectERC20Interface.sol @@ -0,0 +1,44 @@ +//@compile-flags: --severity high med low info + +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.18; + +interface IERC20 {} + +// SHOULD FAIL: Interface named ERC20 with incorrect function signatures +interface ERC20 { + function transfer(address to, uint256 value) external returns (uint256); //~WARN: incorrect ERC20 function interface + function approve(address spender, uint256 value) external returns (uint256); //~WARN: incorrect ERC20 function interface +} + +// SHOULD FAIL: Interface inheriting from IERC20 with incorrect function signatures +interface IERC20Incorrect is IERC20 { + function transfer(address to, uint256 value) external returns (uint256); //~WARN: incorrect ERC20 function interface + function transferFrom(address from, address to, uint256 value) external returns (uint256); //~WARN: incorrect ERC20 function interface + function approve(address spender, uint256 value) external returns (uint256); //~WARN: incorrect ERC20 function interface + function allowance(address owner, address spender) external view returns (bool); //~WARN: incorrect ERC20 function interface + function balanceOf(address account) external view returns (bool); //~WARN: incorrect ERC20 function interface + function totalSupply() external view returns (bool); //~WARN: incorrect ERC20 function interface +} + +// SHOULD PASS: Correct ERC20 interface inheriting from IERC20 +interface IERC20Correct is IERC20 { + function transfer(address to, uint256 value) external returns (bool); + function transferFrom(address from, address to, uint256 value) external returns (bool); + function approve(address spender, uint256 value) external returns (bool); + function allowance(address owner, address spender) external view returns (uint256); + function balanceOf(address account) external view returns (uint256); + function totalSupply() external view returns (uint256); +} + +// SHOULD PASS: Interface named IERC20 with correct function signatures +interface IERC20NamedCorrect { + function transfer(address to, uint256 value) external returns (bool); + function balanceOf(address account) external view returns (uint256); +} + +// SHOULD PASS: Contract that is NOT named ERC20 and does not inherit from one +interface INotERC20 { + function transfer(address to, uint256 value) external returns (uint256); + function balanceOf(address account) external view returns (bool); +} diff --git a/crates/lint/testdata/IncorrectERC20Interface.stderr b/crates/lint/testdata/IncorrectERC20Interface.stderr new file mode 100644 index 0000000000000..3bb60ecce8320 --- /dev/null +++ b/crates/lint/testdata/IncorrectERC20Interface.stderr @@ -0,0 +1,120 @@ +note[interface-naming]: interface names should be prefixed with 'I' + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ interface ERC20 { + │ ━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#interface-naming + +note[multi-contract-file]: prefer having only one contract, interface or library per file + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ interface IERC20 {} + │ ━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#multi-contract-file + +note[multi-contract-file]: prefer having only one contract, interface or library per file + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ interface ERC20 { + │ ━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#multi-contract-file + +note[multi-contract-file]: prefer having only one contract, interface or library per file + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ interface IERC20Incorrect is IERC20 { + │ ━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#multi-contract-file + +note[multi-contract-file]: prefer having only one contract, interface or library per file + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ interface IERC20Correct is IERC20 { + │ ━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#multi-contract-file + +note[multi-contract-file]: prefer having only one contract, interface or library per file + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ interface IERC20NamedCorrect { + │ ━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#multi-contract-file + +note[multi-contract-file]: prefer having only one contract, interface or library per file + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ interface INotERC20 { + │ ━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#multi-contract-file + +warning[incorrect-erc20-interface]: incorrect ERC20 function interface + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ function transfer(address to, uint256 value) external returns (uint256); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#incorrect-erc20-interface + +warning[incorrect-erc20-interface]: incorrect ERC20 function interface + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ function approve(address spender, uint256 value) external returns (uint256); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#incorrect-erc20-interface + +warning[incorrect-erc20-interface]: incorrect ERC20 function interface + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ function transfer(address to, uint256 value) external returns (uint256); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#incorrect-erc20-interface + +warning[incorrect-erc20-interface]: incorrect ERC20 function interface + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ function transferFrom(address from, address to, uint256 value) external returns (uint256); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#incorrect-erc20-interface + +warning[incorrect-erc20-interface]: incorrect ERC20 function interface + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ function approve(address spender, uint256 value) external returns (uint256); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#incorrect-erc20-interface + +warning[incorrect-erc20-interface]: incorrect ERC20 function interface + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ function allowance(address owner, address spender) external view returns (bool); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#incorrect-erc20-interface + +warning[incorrect-erc20-interface]: incorrect ERC20 function interface + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ function balanceOf(address account) external view returns (bool); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#incorrect-erc20-interface + +warning[incorrect-erc20-interface]: incorrect ERC20 function interface + ╭▸ ROOT/testdata/IncorrectERC20Interface.sol:LL:CC + │ +LL │ function totalSupply() external view returns (bool); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://book.getfoundry.sh/reference/forge/forge-lint#incorrect-erc20-interface +