-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(lint): add UnsafeTypecast lint #11046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 16 commits
e8aafe0
0455b31
befa78a
34e6a68
45f5bda
dff52fa
2768187
47e80e4
3614115
01210d8
3d4f355
e7febc7
7b541f9
a541299
c140190
403792d
df675c8
8816572
bcf6760
4883cba
f20127b
30ee511
84f8f1b
6fc6866
114b609
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
use super::UnsafeTypecast; | ||
use crate::{ | ||
linter::{LateLintPass, LintContext, Snippet}, | ||
sol::{Severity, SolLint}, | ||
}; | ||
use solar_sema::hir::{self, ExprKind, TypeKind}; | ||
|
||
declare_forge_lint!( | ||
UNSAFE_TYPECAST, | ||
Severity::Med, | ||
"unsafe-typecast", | ||
"typecasts that can truncate values should be checked" | ||
); | ||
|
||
impl<'hir> LateLintPass<'hir> for UnsafeTypecast { | ||
fn check_expr( | ||
&mut self, | ||
ctx: &LintContext<'_>, | ||
hir: &'hir hir::Hir<'hir>, | ||
expr: &'hir hir::Expr<'hir>, | ||
) { | ||
// Check for type cast expressions: Type(value) | ||
if let ExprKind::Call(call_expr, args, _) = &expr.kind | ||
&& let ExprKind::Type(target_type) = &call_expr.kind | ||
&& args.len() == 1 | ||
&& let Some(first_arg) = args.exprs().next() | ||
&& is_unsafe_typecast_hir(hir, first_arg, target_type) | ||
{ | ||
let suggestion = get_suggestion(); | ||
ctx.emit_with_fix(&UNSAFE_TYPECAST, expr.span, suggestion); | ||
TropicalDog17 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} | ||
|
||
// Returns the suggested fix based on the unsafe typecast expression | ||
fn get_suggestion() -> Snippet { | ||
Snippet::Block { | ||
desc: Some("Consider disabling this lint if you're certain the cast is safe:"), | ||
code: "// Cast is safe because [explain why]\n// forge-lint: disable-next-line(unsafe-typecast)".to_string(), | ||
} | ||
} | ||
|
||
/// Checks if a typecast from the source expression to target type is unsafe. | ||
fn is_unsafe_typecast_hir( | ||
hir: &hir::Hir<'_>, | ||
source_expr: &hir::Expr<'_>, | ||
target_type: &hir::Type<'_>, | ||
) -> bool { | ||
// Get target elementary type | ||
let TypeKind::Elementary(target_elem_type) = &target_type.kind else { | ||
return false; | ||
}; | ||
|
||
// Determine source type from the expression | ||
let Some(source_elem_type) = infer_source_type(hir, source_expr) else { | ||
return false; | ||
}; | ||
|
||
is_unsafe_elementary_typecast(&source_elem_type, target_elem_type) | ||
} | ||
|
||
/// Infers the elementary type of a source expression. | ||
/// For cast chains, returns the ultimate source type, not intermediate cast results. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we check intermediate casts for cast chains? I think that's why this isn't throwing: function _addInt128(uint64 a, int128 b) internal pure returns (uint64) {
return uint64(uint128(int128(uint128(a)) + b));
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would suggest adding a test, you can also reproduce using the following commands: foundryup --pr 11046
git clone https://github.com/Layr-Labs/eigenlayer-contracts.git
git checkout 44ece6d
forge lint src/contracts/core/AllocationManager.sol --only-lint unsafe-typecast There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good call, fixed to support cast chains. |
||
fn infer_source_type(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option<hir::ElementaryType> { | ||
use hir::{ElementaryType, ItemId, Lit as HirLit, Res}; | ||
use solar_ast::LitKind; | ||
|
||
match &expr.kind { | ||
// Recursive cast: Type(val) | ||
ExprKind::Call(call_expr, args, _) => { | ||
if let ExprKind::Type(_ty) = &call_expr.kind | ||
&& args.len() == 1 | ||
&& let Some(first_arg) = args.exprs().next() | ||
{ | ||
return infer_source_type(hir, first_arg); | ||
} | ||
None | ||
} | ||
|
||
// Identifiers (variables) | ||
ExprKind::Ident(resolutions) => { | ||
if let Some(Res::Item(ItemId::Variable(var_id))) = resolutions.first() { | ||
let variable = hir.variable(*var_id); | ||
if let TypeKind::Elementary(elem_type) = &variable.ty.kind { | ||
return Some(*elem_type); | ||
} | ||
} | ||
None | ||
} | ||
|
||
// Handle literal strings/hex | ||
ExprKind::Lit(HirLit { kind, .. }) => match kind { | ||
LitKind::Str(_, bytes, _) => { | ||
let byte_len = bytes.len().try_into().unwrap(); | ||
Some(ElementaryType::FixedBytes(solar_ast::TypeSize::new_fb_bytes(byte_len))) | ||
} | ||
|
||
LitKind::Address(_) => Some(ElementaryType::Address(false)), | ||
|
||
LitKind::Bool(_) => Some(ElementaryType::Bool), | ||
|
||
// Treat number literals as wide unsigned ints (won’t trigger linter) | ||
TropicalDog17 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
LitKind::Number(_) => None, | ||
LitKind::Rational(_) => None, | ||
LitKind::Err(_) => None, | ||
}, | ||
|
||
// Unary operations (e.g. -x) | ||
ExprKind::Unary(op, inner_expr) => match op.kind { | ||
solar_ast::UnOpKind::Neg => match infer_source_type(hir, inner_expr) { | ||
Some(ElementaryType::UInt(size)) => Some(ElementaryType::Int(size)), | ||
Some(signed_type @ ElementaryType::Int(_)) => Some(signed_type), | ||
_ => Some(ElementaryType::Int(solar_ast::TypeSize::ZERO)), | ||
}, | ||
_ => infer_source_type(hir, inner_expr), | ||
}, | ||
|
||
_ => None, | ||
} | ||
} | ||
|
||
/// Checks if a type cast from source_type to target_type is unsafe. | ||
fn is_unsafe_elementary_typecast( | ||
source_type: &hir::ElementaryType, | ||
target_type: &hir::ElementaryType, | ||
) -> bool { | ||
use hir::ElementaryType; | ||
|
||
match (source_type, target_type) { | ||
// Numeric downcasts (smaller target size) | ||
(ElementaryType::UInt(source_size), ElementaryType::UInt(target_size)) | ||
| (ElementaryType::Int(source_size), ElementaryType::Int(target_size)) => { | ||
source_size.bits() > target_size.bits() | ||
} | ||
|
||
// Signed to unsigned conversion (potential loss of sign) | ||
(ElementaryType::Int(_), ElementaryType::UInt(_)) => true, | ||
|
||
// Unsigned to signed conversion with same or smaller size | ||
(ElementaryType::UInt(source_size), ElementaryType::Int(target_size)) => { | ||
source_size.bits() >= target_size.bits() | ||
} | ||
|
||
// Fixed bytes to smaller fixed bytes | ||
(ElementaryType::FixedBytes(source_size), ElementaryType::FixedBytes(target_size)) => { | ||
source_size.bytes() > target_size.bytes() | ||
} | ||
|
||
// Dynamic bytes to fixed bytes (potential truncation) | ||
(ElementaryType::Bytes, ElementaryType::FixedBytes(_)) | ||
| (ElementaryType::String, ElementaryType::FixedBytes(_)) => true, | ||
|
||
// Address to smaller uint (truncation) - address is 160 bits | ||
(ElementaryType::Address(_), ElementaryType::UInt(target_size)) => target_size.bits() < 160, | ||
|
||
// Address to int (sign issues) | ||
(ElementaryType::Address(_), ElementaryType::Int(_)) => true, | ||
|
||
_ => false, | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.