docs(LibCast): name the dirty-address hazard on asAddressesArray and pin its containment

[thedavidmeister]

Closes #20.

The ruling

Documentation, not masking. The issue offered three options; this takes (a)
and declines (b) and (c), on the oracle.

The oracle is the LibCast NatSpec ("the cast will merely retype the data in
place") and the README ("a cast moves between Solidity types without changing the
binary data"). Masking is excluded by both, and not only on a definitional
reading:

• asAddressesArray returns the caller's own buffer. Masking in place would
  silently truncate the caller's live uint256[], which is a strictly worse bug
  than the one reported.
• Masking would break the round trip asUint256Array(asAddressesArray(us)) == us
  that the suite pins today.
• A masking helper that copies is a conversion by this repo's own vocabulary,
  belongs in LibConvert, has no caller asking for it, and would not reach the
  consumer at risk here — that consumer does not know there is anything to opt
  into. Unused surface on an audited library is a cost, not a fix.

Closing as by design (option c) under-serves the finding. The finding is not "the
behaviour is wrong"; it is "the consequence is nowhere written down, and the
generic 'ensure the validity of the data' does not name it". That is fixed by
writing it down.

What changed

src/LibCast.sol. A library-level paragraph on the narrow-cast case: a cast
performs no writes, so it cannot establish Solidity's zero-extension memory
convention, and asAddressesArray is the only cast here that narrows (the
bytes32 casts are word for word and have nothing to observe).

asAddressesArray then names, specifically:

• the hazard — any input word >= 2 ** 160 yields an element that keeps its
  upper 96 bits, and the caller MUST ensure the words fit if the result is to be
  a conventional address[];
• the containment — comparison, abi.encode, abi.encodePacked, hashing,
  storage assignment, mapping keys, memory element writes and the external ABI
  boundary all clean to 160 bits, so a dirty element cannot corrupt state,
  cannot desync == from a hash, and cannot revert at an ABI boundary;
• the escape — an assembly mload of an element reads all 256 bits, on every
  pipeline, and reading address[] memory elements with assembly is the normal
  idiom across these libraries;
• the part a reader cannot derive — whether a plain addresses[i] in a stack
  slot is cleaned is pipeline dependent and must not be relied on in either
  direction. Legacy (what this repo compiles with) leaves it dirty; via-IR
  cleans it. That can flip under a compiler or pipeline change with no change to
  this library, so it is documented as unspecified rather than as either answer.

asUint256Array(address[]) gets the matching note: it widens, so the words it
yields are exactly the ones the buffer already held, which is what makes the
round trip an identity rather than a truncation.

README.md. One paragraph, because the README is where a consumer decides
whether the library is safe for their use, and the issue is explicitly about who
is in a position to notice.

test/LibCast.t.sol. Prose that makes a factual claim about compiler
behaviour rots silently, and the issue's own concern is that "the exposure could
change silently in either direction on a compiler or pipeline switch". So the
claim is pinned:

• testAsAddressesArrayKeepsUpperBitsForAssemblyReads — the element keeps all
  256 bits under an mload.
• testAsAddressesArrayDirtyElementCannotEscapeThroughSolidity — walks every
  containment leg the NatSpec lists, after first asserting that the element
  really is dirty, so that no leg can pass vacuously.

Both fuzz over the low 160 bits with the top bit forced set, so every run
exercises the dirty case without discarding runs to an assumption.

Deliberately not asserted: what addresses[0] holds once it is in a stack
slot. That is the pipeline-dependent row, and an assertion either way would be
false on one of the two pipelines. The omission is commented in the test so it
is not mistaken for an oversight.

Checks run locally

forge fmt --check clean, slither . 0 findings, forge test 17/17 passing on
the merged tree.

Notes for review

• No signature or return-style change: the diff is NatSpec and tests only, so the
  "no behaviour change" claim is checkable by inspection. The file's pre-existing
  named returns are the assembly assignment targets and are left alone as out of
  scope for this issue.
• test(LibCast): observe the intermediate and pin the in-place contract #18 landed on main while this was open and appended its tests to the same
  end-of-contract region. main is merged in here and both sides are kept in
  full — test(LibCast): observe the intermediate and pin the in-place contract #18's five in-place tests and its strengthened round trips, plus this
  branch's two containment tests and the echoFirstAddress ABI-boundary helper.
  Nothing from either side was dropped.

QA

• Discriminating tests:
  testAsAddressesArrayKeepsUpperBitsForAssemblyReads and
  testAsAddressesArrayDirtyElementCannotEscapeThroughSolidity. src/ carries no
  logic change here, so neither "fails on base" in the usual sense — they pass on
  main's source because main's source already is the documented behaviour.
  What they discriminate against is the change this issue rejects: both fail
  under either masking mutant below, measured on the merged tree.
  • At this branch's original base (main @ 9f725d1, before test(LibCast): observe the intermediate and pin the in-place contract #18 landed), M01
    SURVIVED the entire pre-existing suite — baseline: green (10 passed),
    then SURVIVED — because the round-trip tests there compared the input
    buffer against itself, so an in-place mask moved both sides together. These
    two tests were its only killers.
  • Re-measured after test(LibCast): observe the intermediate and pin the in-place contract #18 merged, that is no longer the exclusive claim: test(LibCast): observe the intermediate and pin the in-place contract #18's
    strengthened testAddressesArrayRound0 snapshots the expected words into an
    independent buffer, so it now kills M01 as well (baseline: green (15 passed), then KILLED). Stated plainly rather than left as the older,
    stronger-sounding result.
  • What these two still carry alone is the containment half. No other test
    exercises abi.encode, abi.encodePacked, hashing, a storage assignment, a
    mapping key or the external ABI boundary on a dirty element. That half is a
    guard against a compiler or pipeline change moving the boundary, which no
    mutant of this library can express, so it is deliberately not claimed as
    mutation-discriminated.
• Mutations applied
  (nix run github:rainlanguage/adversarial-mutation-test#mutation-probe -- mutants-issue20-libcast.toml,
  merged tree, baseline: green (17 passed), result
  2/2 killed; survived: 0; no-run: 0; harness errors: 0; killers below are from
  running the suite under each mutant directly, since the probe's fail-pattern
  over-matched forge's output):
  • src/LibCast.sol asAddressesArray body -> add an in-place 160-bit mask loop
    over the elements (M01) -> KILLED by
    testAsAddressesArrayKeepsUpperBitsForAssemblyReads,
    testAsAddressesArrayDirtyElementCannotEscapeThroughSolidity and
    testAddressesArrayRound0.
  • src/LibCast.sol asAddressesArray whole function -> allocate and return a
    masked copy (M02) -> KILLED by those three plus
    testAsAddressesArrayRetypesInPlace and testAsAddressesArrayWritesAreShared,
    which M02 also breaks by dropping the aliasing.
  • The NatSpec and README lines themselves: n/a — prose carries no logic to
    mutate.
• Oracle: the issue's stated intent oracle — the LibCast NatSpec ("It is the
  calling context that MUST ensure the validity of the data, the cast will merely
  retype the data in place") and the README ("a cast moves between Solidity types
  without changing the binary data") — plus the issue's measured consumer table
  and the follow-up comment's legacy/via-IR split. Expected values in the tests
  come from Solidity's documented memory convention (an address is held zero
  extended, so the clean element is uint256(uint160(word))) and from the fuzzed
  input word itself; nothing is read back out of the implementation to build an
  expectation.
• Category check: the issue asks (a) document the hazard on
  asAddressesArray, naming assembly consumers as the ones that see the dirty
  word; (b) additionally offer a checked or masking variant; (c) close as by
  design. Covered (a), including the follow-up comment's explicit ask for "a
  sentence in the NatSpec" about the pipeline dependence. (b) declined with
  reasons in the body above — it is a conversion rather than a cast, has no
  caller, and cannot reach a consumer who does not know to opt in. (c) declined —
  the finding is the undocumented consequence, and it is fixed by documenting it.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@thedavidmeister, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

Limit details: You’ve used the included review currently available.

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?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 357a8884-0efc-499e-ad2d-c848b38b3a4d

📥 Commits

Reviewing files that changed from the base of the PR and between 001817f and 00c85b7.

📒 Files selected for processing (3)

• README.md
• src/LibCast.sol
• test/LibCast.t.sol

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}