Suppress unsafe-typecast lint on the range-checked uint160 cast - #111
Conversation
`checkResolvedAddresses` range checks the decoded word against `type(uint160).max` and reverts `ResolvedAddressReadFailed` before casting, so the cast cannot truncate — but forge-lint has no way to see the guard and warned on every build of every consumer of this library. Suppressed at the site with the reason, matching the house style already used by the `slither-disable-next-line low-level-calls` eighteen lines above and by `LibRainDeploySnapshot.sol:157`, so an unchecked cast added anywhere else is still reported. Closes #49 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Reviewed cbb5e9f: ready — Closes #49. The suppression is justified by the line directly above it: CI green, 0 unresolved threads — vacuous, CodeRabbit reports |
Closes #49
What
checkResolvedAddressesdecodes a static-call answer as auint256word andrange checks it against
type(uint160).max— revertingResolvedAddressReadFailedon a word with dirty upper 96 bits — before casting it to an address. forge-lint
cannot see that guard, so it warned
unsafe-typecaston the cast on every buildof every consumer of this library.
The cast is safe. This suppresses the lint at the site with the reason, which is
the house style already in this same file eighteen lines above:
src/lib/LibRainDeploy.sol:282-286—slither-disable-next-line low-level-calls,with a comment saying it is excluded at the site "rather than repo-wide so a
low-level call added anywhere else is still reported"
src/lib/LibRainDeploySnapshot.sol:157— this exact lint, same directivesrc/concrete/MigrationRegistry.sol:117-119—block-timestamp, same shapeLine 304 was the one suppressible site in
src/,script/ortest/that hadskipped it.
Comment-only. No behaviour changes.
Verification
Reproduced and then confirmed fixed with the repo's own toolchain
(
nix develop -c forge build --force, forge 1.7.2-nightly from the flake).Before, on
mainat86f8d96:After: no
warning[unsafe-typecast]anywhere in the build.forge fmt --checkpasses.forge test: identical results on this branch and onmain— 168 passing and47 failing on both, and the failing test-name sets are identical between the
two runs. Every one of those 47 is
vm.createSelectFork: environment variable <NETWORK>_RPC_URL not found, i.e. the fork tests this sandbox has no.envfor, exactly as
CLAUDE.mddocuments. Pre-existing and unrelated; CI suppliesthe endpoints.
The pinned deploy snapshots cannot move:
bytecode_hash = "none"andcbor_metadata = false, so comments are not in the creation code, andLibRainDeploy.solis aVm-importing script library that is not in the pinnedcontracts' compile graph at all.
GeneratedSnapshotShapeTest,LibRainDeploySnapshotTest,RainDeployVerifySnapshotTestandRegistryDeploySnapshotTestall pass unchanged.QA
Discriminating tests:
testCheckResolvedAddressesDirtyWordReverts(bytes32)in
test/src/lib/LibRainDeploy.t.sol— not a test this PR adds, but the testthat makes the suppression honest. A
forge-lint: disableis only ever as goodas the claim in the comment beside it, and the claim here is that the range
check rejects every word with dirty upper 96 bits. That fuzz test drives
exactly those words and asserts
ResolvedAddressReadFailed, so thejustification is enforced rather than merely asserted. Its own docstring
already names the failure mode a wrong suppression would hide: "truncating the
word silently PASSES this check against an address the read never gave".
For the comment itself the discriminating check is the build, run both ways on
this exact toolchain — mutation 1 below.
Mutations applied:
src/lib/LibRainDeploy.sol:304-> deleted(
git checkout main -- src/lib/LibRainDeploy.sol) ->forge build --forceemits
warning[unsafe-typecast]again (count 1); restored -> count 0. Sothis diff is what removes the warning, not something incidental to it.
src/lib/LibRainDeploy.sol:301-303if (word > type(uint160).max) { revert ResolvedAddressReadFailed(...); }-> deleted entirely, which makes the cast genuinely truncating and the new
suppression comment a lie -> killed by
testCheckResolvedAddressesDirtyWordReverts:[FAIL: next call did not revert as expected].Harness honesty for mutation 2, because a "killed" verdict is worthless if the
run was fake: the unmutated baseline
[PASS]ed atruns: 256, so the testreally executes; the mutant reported
Compiler run successful!, so its failureis an assertion failure and not a build error masquerading as one; the guard
was confirmed absent in the mutant (
grep -c= 0) and present again afterrestore (= 1); and
git statusis clean afterwards.Oracle: forge-lint's own output, independent of both the source comment and
of me — it either emits
warning[unsafe-typecast]for that span or it doesnot. forge-lint also printed the exact remedy it wanted
(
// forge-lint: disable-next-line(unsafe-typecast)), and the in-repoconvention sites listed above fix the comment's wording shape, so neither the
directive nor the style is my invention. For mutation 2 the oracle is the
interface contract — a read answering a non-address word must surface as
ResolvedAddressReadFailednaming network, target and index — taken from thaterror's declared purpose, not from the implementation.
Category check: the issue carries two findings (
Q4-04,LIBDEPLOY-05)proposing the same single fix — suppress this lint at this one site with a
justification. Covered. Treating the category as "suppressible lint sites
lacking their justified suppression" rather than only the one cited line, I
checked it is a category of one: after this change
forge build --forceemitsno
warning[...]forge-lint diagnostic of any kind acrosssrc/,script/and
test/. The one remaining build warning is a solcWarning (2018), adifferent category, deliberately out of scope — see below.
One correction to the issue
Both findings state this was the repo's only unsuppressed warning. It was the
only unsuppressed forge-lint warning, which is what this fixes, but the same
build also emits an unsuppressed solc warning:
LIBDEPLOY-05's own verification block already noted this and used it to arguethe severity down. It is a different category — a real code change to a test
helper, not a missing suppression — and neither finding proposed fixing it, so it
is out of scope here and left alone. Worth its own issue if a zero-warning build
is the actual goal, since this PR alone does not achieve one.
🤖 Generated with Claude Code