fix(lint): detect unsafe casts on function returns and ternaries#13939
fix(lint): detect unsafe casts on function returns and ternaries#13939ArshLabs wants to merge 4 commits intofoundry-rs:masterfrom
Conversation
Handle two previously-missed expression kinds in the unsafe-typecast lint rule: - Function return values: resolve the callee's return type from the HIR so uint128(getUint256()) is now flagged. - Ternary expressions: recurse on both branches so uint128(cond ? x : y) is now flagged. Closes foundry-rs#13921
grandizzy
left a comment
There was a problem hiding this comment.
ty, lgtm! @0xrusowsky @zerosnacks mind to have one more check?
zerosnacks
left a comment
There was a problem hiding this comment.
Hi @ArshLabs
With the help of our internal AI reviewing I've identified several findings I would like to see addressed prior to merging.
Findings
1. Early Return in Overload Resolution — Medium
File: crates/lint/src/sol/med/unsafe_typecast.rs#L166-L180
resolve_function_return iterates through function resolutions and returns the first matching single-return elementary type. For overloaded functions, resolutions can contain multiple functions with different return types. Returning early on the first match can lead to false negatives if the actual invoked overload returns a type that makes the cast unsafe, but an earlier overload in the list returns a safe type.
Recommendation: Collect and return all possible return types from all matching overloads. Flag the cast if any of the possible return types are unsafe.
2. Missing Member Access Calls — Medium
File: crates/lint/src/sol/med/unsafe_typecast.rs#L154-L163
resolve_call_return_type only matches ExprKind::Ident for direct function calls (e.g., foo()). It ignores ExprKind::Member, used for external calls and member accesses (e.g., contract.getUint256()). This means unsafe casts on member function returns are silently ignored.
Recommendation: Add a match arm for ExprKind::Member to resolve the member function's return type.
3. Unnecessary API Complexity — Low
File: crates/lint/src/sol/med/unsafe_typecast.rs#L70-L148
infer_source_types accepts mut output: Option<&mut Vec<ElementaryType>> and returns an Option<ElementaryType>. The root caller always passes Some and ignores the return value, forcing unnecessary branching and a track closure.
Recommendation: Simplify signature to fn infer_source_types(output: &mut Vec<ElementaryType>, hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) (returning ()).
4. Incomplete Test Coverage — Low
File: crates/lint/testdata/UnsafeTypecast.sol#L459-L474
Tests cover basic happy paths but miss edge cases:
- Overloaded functions with different return types
- Member access calls (e.g.,
uint128(this.getUint256())) - Mixed-safety ternary expressions (e.g.,
uint128(cond ? uint128(x) : uint256(y)))
Thanks!
- Simplify infer_source_types to take &mut Vec instead of Option wrapper - Collect all overload return types instead of returning first match - Add member function call resolution (contract.foo() pattern) - Add test cases for mixed ternary and member calls
|
Thanks for the thorough review @zerosnacks, pushed the fixes:
|
|
@0xrusowsky @zerosnacks would appreciate a re-review when you get a chance |
Closes #13921
Problem
The
unsafe-typecastlint misses unsafe casts when the source expression is a function return value, ternary, or other complex expression.infer_source_typesonly handlesIdent,Lit, type-castCall,Unary, andBinary-- everything else falls through_ => None, which makessource_typesempty and the lint assumes the cast is safe.Fix
Function.returns) so casts likeuint128(getUint256())are now flagged.Binarypattern) so casts likeuint128(cond ? x : y)is now flagged.Test cases added
uint128(getUint256())-- function return value narrowing castuint128(cond ? x : y)-- ternary with uint256 branches narrowed to uint128