feat(LibBytes): add address<->bytes32 conversion helpers (EXSC-618) [LibBytes v1.1.0]#2055
Conversation
Consolidate the address<->bytes32 cast pattern duplicated across ~10 facets into LibBytes (v1.1.0): - toBytes32(address): widening, lossless - toAddress(bytes32): checked narrowing, reverts NotAnAddress on dirty high bits - toAddressUnchecked(bytes32): raw truncation for intentional/guarded cases Also replace toHexString's require-string with a custom error. Add unit tests. Leave breadcrumb comments at the 21 existing call sites (checked/unchecked per audit); each is migrated the next time its facet is touched, so this PR makes no facet behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
requires-types
WalkthroughLibBytes now provides checked and unchecked bytes32/address conversions and custom errors. Tests cover the new behavior, while bridge facets and ReceiverOIF include TODO markers for future migration to these helpers. ChangesLibBytes conversion utilities
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
lifi-action-bot
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
Test Coverage ReportLine Coverage: 90.23% (3483 / 3860 lines) |
…elpers (EXSC-618) New convention so future facets use LibBytes.toBytes32 / toAddress (checked) / toAddressUnchecked instead of hand-rolling address<->bytes32 casts — which keeps call sites free of the Olympix unsafe-downcast finding (centralised in LibBytes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| } | ||
| // solhint-disable-next-line gas-custom-errors | ||
| require(value == 0, "Strings: hex length insufficient"); | ||
| if (value != 0) revert HexLengthInsufficient(); |
There was a problem hiding this comment.
toHexString change silently alters SquidFacet bytecode with no version bump.
LibBytes functions are internal pure and get inlined into consumers. SquidFacet.sol:187 is the sole caller of toHexString, so swapping the require-string for revert HexLengthInsufficient() changes SquidFacet's compiled bytecode and its revert data — yet SquidFacet stays at @custom:version 1.0.0 and isn't listed among the "call sites intentionally not migrated". The PR body implies zero facet behavior changes, but this one does change.
Per [CONV:VERSIONING], "new revert on existing function" is a MAJOR-type change, and this is a revert-encoding change on a shipped function. In practice the branch is an unreachable invariant (after the loop value is always 0 for a correctly-sized input), so behavioral/security risk is effectively nil — but it should be surfaced rather than folded silently into a "no facets touched" PR.
Suggestion: either split the toHexString refactor into its own PR acknowledging the SquidFacet impact, or note in the PR body that SquidFacet's bytecode changes and confirm with audit/deploy that an unbumped version is acceptable here.
There was a problem hiding this comment.
Not bumping SquidFacet's version here — but you're right that the transitive bytecode change should be surfaced, so I've added a note to the PR description.
Reasoning: [CONV:VERSIONING] says a library MAJOR bump should be evaluated against consumers, not that any library change bumps them. The toHexString edit isn't a "new revert on existing function" — the revert already existed (require(value == 0, …)); only the revert encoding changed (Error(string) → HexLengthInsufficient()). And that branch is unreachable for a correctly-sized length (after the loop value is always 0, and SquidFacet always passes a correct length), so there's zero reachable behavior change at the ABI level → PATCH-class, which doesn't cascade. So SquidFacet stays 1.0.0 and LibBytes' MINOR bump (the three new functions dominate) is correct.
The only real action item was your "surface it rather than fold it silently" point — done: the description now flags that SquidFacet's compiled bytecode/revert-encoding changes (dead-code only) so whoever next deploys it expects the 1.0.0 bytecode to differ.
| function _convertAddressToBytes32( | ||
| address _address | ||
| ) internal pure returns (bytes32) { | ||
| // TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0 |
There was a problem hiding this comment.
Bare // TODO breadcrumbs.
The 21 breadcrumbs reference "LibBytes v1.1.0" but not the tracking ticket (EXSC-618). Un-ticketed TODOs tend to become permanent — consider referencing the ticket ID so they're greppable and closable, e.g. // TODO(EXSC-618): migrate to LibBytes.toBytes32. Also worth confirming the repo's solhint config doesn't flag TODO, and noting this runs against the repo's "minimal comments" default (though the migration-breadcrumb rationale is defensible).
There was a problem hiding this comment.
Done in 0fbe4b5 — created a dedicated tracking ticket (EXSC-626) and rewrote all 21 breadcrumbs to // TODO(EXSC-626): migrate to LibBytes.… so they're greppable and closable.
Used a new ticket rather than EXSC-618 since EXSC-618 closes with this PR, whereas the migrations outlive it (each rides its facet's next version bump). .solhint.json has no max-line-length rule, so the longer comments are fine.
🔍 QA Review — EXSC-618
🧠 What this ticket does
📊 Resolution Status — Re-review
🔎 Item-by-item resolution1. [High] Developer chose not to bump Technical verification: The developer added an explicit note to the PR description: "Flagged so whoever next deploys SquidFacet knows its 1.0.0 bytecode will differ from the currently-deployed 1.0.0." This satisfies the requirement to document the decision. No further action required.
2. [Low] Breadcrumb TODOs — ✅ Fixed Commit Spot-checked files — correct format confirmed:
Note: Using ✅ Ticket Coverage — HighPR delivers exactly what EXSC-618 describes: three helpers in 🧪 Test CoverageNo new test files introduced in the post-review commit (comment-only change). Original assessment stands — test coverage is good; all three new helpers have round-trip, boundary, and revert-path tests with specific error selectors. No new gaps. 🔗 Downstream ImpactNo change from original assessment. QA Agent — 2026-07-16 |
![lifi-qa-agent[bot]](https://avatars.githubusercontent.com/in/3411599?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes on 2 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟠 High | Versioning / Audit | toHexString custom-error change silently alters SquidFacet bytecode — @custom:version not bumped on src/Facets/SquidFacet.sol |
| 2 | 🟢 Low | Process | Breadcrumb TODOs missing Linear ticket reference EXSC-618 across 21 call-site files |
1. [High] toHexString custom-error change silently alters SquidFacet bytecode — no version bump
LibBytes.toHexString is internal pure and is inlined by the Solidity compiler into every consumer. SquidFacet._callBridge (line 187) is the sole caller. Replacing require(value == 0, "Strings: hex length insufficient") with if (value != 0) revert HexLengthInsufficient() changes the revert encoding emitted by SquidFacet (different selector, no revert string) without a @custom:version bump on SquidFacet.sol.
The versionControlAndAuditCheck CI only scans files changed in the PR diff (git diff --diff-filter=AM). Since SquidFacet.sol is not in the diff, CI will not flag this — the gap is invisible to automated enforcement. After this PR merges, the deployed SquidFacet bytecode will diverge from its audit20250508 audit artifact.
Required action: Either (a) bump SquidFacet to @custom:version 1.0.1, add a waitForAudit entry, and update auditLog.json — or (b) leave an explicit comment in this PR documenting the team's position that internal pure inlining does not count as a consumer version bump for the purposes of [CONV:VERSIONING], and capture that policy in the convention rule so it is consistently applied going forward. Option (b) requires a human reviewer to sign off on the policy decision before merge.
2. [Low] Breadcrumb TODOs missing Linear ticket reference EXSC-618
All 21 breadcrumb comments use the format // TODO: migrate to LibBytes.<helper> — see LibBytes v1.1.0 without the Linear ticket ID. Without a ticket reference, git grep EXSC-618 will not surface these breadcrumbs, and they risk becoming permanent once "LibBytes v1.1.0" is no longer a memorable shorthand.
Required action: Update breadcrumbs to include the ticket ID, e.g. // TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0 (EXSC-618). This is a comment-only change in 15 files.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
…XSC-618) The 21 `// TODO: migrate to LibBytes` breadcrumbs now cite the dedicated tracking ticket (EXSC-626) so they are greppable and closable, per review feedback on #2055. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
QA AI Reviewing
There was a problem hiding this comment.
✅ QA Pass — both CHANGES_REQUESTED items resolved: (1) SquidFacet bytecode change accepted with documented justification (dead-code path, PATCH-class per CONV:VERSIONING, note in PR description); (2) all 21 breadcrumbs updated to TODO(EXSC-626) format in commit 0fbe4b5. LibBytes v1.1.0 implementation, tests, and NatDoc are correct. AuditRequired label correctly applied — waitForAudit flow owns merge gate.
4 participants
Which Linear task belongs to this PR?
EXSC-618
Why did I implement it this way?
The
bytes32(uint256(uint160(addr)))/address(uint160(uint256(b)))conversion pattern is copy-pasted across ~10 facets, which triggers Olympixunsafe-downcastfindings and risks divergent semantics. The only prior implementation lived in the vendoredOFTComposeMsgCodec(a LayerZero copy), so it was not a suitable shared dependency.LibBytes v1.1.0 now centralises it:
toBytes32(address)— widening, always lossless.toAddress(bytes32)— checked narrowing: revertsNotAnAddressif the top 96 bits are set. This is the default; a checked narrowing silences the detector honestly rather than hiding a truncation. New overload alongside the existingtoAddress(bytes memory, uint256).toAddressUnchecked(bytes32)— raw truncation, for intentional / pre-guarded cases.toHexString'srequire-string → custom errorHexLengthInsufficient(removes thegas-custom-errorssolhint-disable).Call sites are intentionally NOT migrated in this PR. Each of the 21 existing sites gets a breadcrumb comment only (
// TODO(EXSC-626): migrate to LibBytes.…, tracked in EXSC-626) — no logic change, no facet@custom:versionbump, no re-audit. Each is swapped to the helper the next time its facet is touched. Audit of the sites:toBytes32(zero behavior change).toAddress: all are validation/compare paths that already revert on a bad value, so checked only changes which revert fires — never turns a revert into success.toAddressUnchecked:CalldataVerificationFacet(aviewreturningbool— checked would turn a legitimatereturn falseinto a revert) and theAcrossFacetPackedV4packed hot paths (two of which already manually guard the high bits before branching toNON_EVM).Numeric downcasts (
uintN— the larger source of findings) are tracked separately in EXSC-617 (SafeCast + a[CONV:DOWNCAST]rule).Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)