AMT coverage: g1-libicloneablefactoryv4-effec - #75
Open
thedavidmeister wants to merge 8 commits into
Open
Conversation
… code `vm.etch` rejects any code whose first bytes are `0xef01` unless the code is exactly the 23-byte EIP-7702 delegation designator, so the fuzzer eventually produces a `code` the cheatcode cannot write and the test fails on unmutated source (counterexample `code = 0xef0150ab5cd0906843c517`). EIP-3541 forbids deployed code beginning `0xEF` on a real chain in the first place, so the input is excluded as unrepresentable rather than by weakening what the code-size guard is asserted to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mutation probing found two derivation behaviours the suite covered only by fuzz luck. A mutant that special-cases `bytes32(type(uint256).max)` survived a full run of the suite in both `effectiveOpenSalt` and `predictCloneAddress`: 2048 fuzz runs per test reach `bytes32(0)` reliably but do not reach the maximum word reliably, so the top of the salt domain was not pinned anywhere. Both boundaries are now asserted deliberately, in the style of the existing `testEffectiveOpenSaltEmptyData`, which pins the empty-data boundary for the same reason. The prediction boundary is checked against OZ `Clones` rather than against this library's own arithmetic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe test suite adds boundary checks for zero and maximum salts, and excludes ChangesLibrary test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR only adds boundary coverage and adjusts test inputs without changing production behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
`test/src/**` mirrors `src/**` and holds the `.t.sol` suites. This repo is the library half of the split — there is no `src/concrete/`, the concrete lives in rain.factory.deploy — so `test/src/concrete/` mirrored nothing. Test SUPPORT code (harnesses, mocks, fixtures) belongs outside the mirror, in `test/concrete/`, `test/lib/`, `test/abstract/`, as in rain.deploy and rain.math.float. Pure move plus the import paths that follow it. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntable code Three parallel AMT branches (#76, #77, #78) each grew their own variant of `TestCloneable` because it satisfies neither of `ICloneableV2`'s normative MUSTs and moves in lockstep with the constant the library compares against. Fix it once, here, so the branches converge on one fixture instead of four: - `initialize` can NOT be called more than once. That is the interface's first MUST and no fixture honoured it. - The RECOMMENDED typed overload is present and reverts `InitializeSignatureFn` always, as the interface requires. - The success sentinel is written out from the LITERAL string the interface names, not imported from `ICLONEABLE_V2_SUCCESS`. Importing it put both sides of the library's comparison in lockstep: the constant could drift and every flow test would still pass, because the fixture drifted with it. A third party hard-codes `keccak256("ICloneableV2.initialize")`, so the fixture does too, and every existing flow test now discriminates a drift. Separately, `testCheckImplementationCodeEtched` could fail for a harness reason: `vm.etch` parses a `0xef01` prefix as an EIP-7702 delegation designator and rejects anything that is not exactly the 23-byte designator. EIP-3541 forbids deploying any `0xef`-leading code at all, so such code cannot exist at an implementation address on any chain and the guard is not specified over it; the fuzz domain is narrowed to code that could actually exist. The guard only ever reads code LENGTH, so nothing about the property changes. Gas snapshot regenerated for the extra `SSTORE` the initialization guard costs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # test/src/lib/LibICloneableFactoryV4.checkImplementationCode.t.sol
The merge from `2026-08-24-test-fixtures-out-of-src-mirror` gives `TestCloneable` a one-shot initialization guard, so every clone in the suite now costs an extra `SSTORE`, and this branch's two new boundary-salt tests had no entries at all. The `0xef` fuzz-domain narrowing this branch carried as its own commit is resolved onto the base branch's version of the same line, which now holds it once for all four AMT branches instead of four times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per CodeRabbit on this PR. The finding is correct and the comment on
`vm.assume(code[0] != 0xef)` was overclaiming.
The comment said no implementation on any chain can have `0xef`-leading code.
EIP-3541 does forbid DEPLOYING it, so no CREATE or CREATE2 can produce it — but
EIP-7702 leaves exactly one way an account can hold it anyway: a delegation
designator, `0xef0100 || address`, exactly 23 bytes. `EXTCODESIZE` on a
delegated EOA returns 23, not zero. The assume was silently excluding a case
that is real, behind a comment saying it was not.
`testCheckImplementationCodeEip7702Designator` pins it as a fixed case, since
the fuzz test cannot reach it. It PASSES the guard, which is the part worth
having on the record: a code-SIZE check cannot tell an implementation contract
from an EOA that has delegated, and a delegation is REVOCABLE by the account
holder where deployed code is not. Callers wanting an immutable implementation
do not get that from this guard.
Mutation-checked, not assumed: with the guard mutated to
`code.length == 0 || code[0] == 0xef`, this is the ONLY test in the suite that
fails (42 pass, 1 fail). The fuzz test cannot kill that mutant by construction,
because its assume excludes the input that would.
Scoped honestly in the NatSpec: `foundry.toml` pins `evm_version = "cancun"`,
which predates EIP-7702, so the test asserts that the 23-byte designator is
storable and passes the SIZE check. It does not exercise, and does not claim,
the execution semantics of delegation.
The rest of the assume stands: `0xef`-leading blobs of any OTHER length cannot
exist on any chain, and `vm.etch` rejects them outright ("Eip7702 is not 23
bytes long"), which is what was breaking the fuzz test 8 runs in 8.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # .gas-snapshot
Contributor
Author
|
@coderabbitai review Queuing a review — the original attempt hit the OSS rate limit before reading the diff. Orientation: nothing under |
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adversarial mutation-testing pass over group
g1-libicloneablefactoryv4-effec: the two effective-salt derivations, the EIP-1167 creation code and the CREATE2 prediction — 29 behaviours across four units insrc/lib/LibICloneableFactoryV4.sol(plus the two domain-tag constants it imports fromsrc/interface/ICloneableFactoryV4.sol).58 mutants probed against the pre-existing suite before a single new test was written, so every kill below credits a named pre-existing test. 52 died to the existing suite, 3 could not be expressed as compiling code, 1 is genuinely equivalent, and 2 survived — both the same defect, in two different functions.
Do not merge; this is an audit deliverable.
What survived, and why it matters
A mutation that changes the derivation only when the salt is
bytes32(type(uint256).max)survived a full run of the suite ineffectiveOpenSaltand again inpredictCloneAddress. The corresponding zero-salt mutants died, and the same max-salt mutation died ineffectiveSalt— not because that function is tested differently, but because it is reached from more fuzz tests and therefore gets more draws. In other words the top of the salt domain was pinned nowhere, and the boundary kills the suite did score were fuzz luck rather than coverage. Both boundaries are now asserted deliberately, in the style of the existingtestEffectiveOpenSaltEmptyData, and both mutants now die on the first case (runs: 0).The seed-dependence itself is filed as #70 rather than papered over here: this PR pins the two boundaries whose mutants actually survived and deliberately does not pin the ones that happened to die, so the ledger stays honest about which is which.
The branch also carries a fix for a pre-existing test that fails on unmutated source (#68 / #64):
testCheckImplementationCodeEtchedfeeds fuzzedbytestovm.etch, which rejects any0xef01-prefixed code that is not the 23-byte EIP-7702 designator. EIP-3541 forbids0xEF-leading deployed code on chain in the first place, so the input is excluded as unrepresentable rather than by weakening the assertion. A red baseline blocks every probe in the repository, so this had to land first.Behaviour matrix
KILLED_PREEXISTING= the mutant died to the suite as it stood atc1c2afd.KILLED_NEW= it survived that pass and dies to a test added here.effectiveSalt(address,bytes32)— namespaced effective-CREATE2-salt derivationkeccak256over theabi.encodepreimagekeccak256→sha256testEffectiveSaltMatchesFormulaICLONEABLE_FACTORY_V4_NAMESPACED_DOMAINtestEffectiveSaltMatchesFormula,testEffectiveSaltPreimageShape,testDomainTagsPinned,testDomainTagsDistincttestEffectiveSaltMatchesFormula,testEffectiveSaltPreimageShapeabi.encode, 96-byte preimage, notabi.encodePackedabi.encode→abi.encodePacked; R04 tag word dropped (64-byte preimage)testEffectiveSaltPreimageShape(length 96),testEffectiveSaltMatchesFormulaabi.encodedeployer→bytes20(deployer)(right-padded)testEffectiveSaltPreimageShape,testCloneDeterministicOpenSaltDisjointTagsCloseTheSquatmsg.sender— deployer is a parameterdeployerparameter replaced bymsg.sender(function relaxed toview)testEffectiveSaltDeployerSensitive,testEffectiveSaltMatchesFormulatestEffectiveSaltSaltSensitive,testEffectiveSaltMatchesFormulaeffectiveOpenSalt(bytes32,bytes)— open-salt effective-CREATE2-salt derivationkeccak256over theabi.encodepreimagekeccak256→sha256testEffectiveOpenSaltMatchesFormulaICLONEABLE_FACTORY_V4_OPEN_SALT_DOMAINtestEffectiveOpenSaltMatchesFormula,testEffectiveOpenSaltPreimageShape,testDomainTagsPinnedkeccak256(data)word 2testEffectiveOpenSaltMatchesFormula,testEffectiveOpenSaltPreimageShapetestEffectiveOpenSaltPreimageShape,testEffectiveOpenSaltMatchesFormulaabi.encode, notabi.encodePackedabi.encode→abi.encodePackedbytes32— see the equivalence noteskeccak256("")rather than short-circuitingdata.length == 0short-circuits to the zero wordtestEffectiveOpenSaltEmptyDatamsg.sendermixed into the hashed data (function relaxed toview)testEffectiveOpenSaltDataSensitivetestEffectiveOpenSaltSaltSensitivetestEffectiveOpenSaltBoundarySalts(added here)cloneCreationCode(address)+EIP1167_CREATION_CODE_PREFIX/_SUFFIX3d602d80600a3d3981f3363d3d373d3d3d363d7373→72)testCloneCreationCodeIsEIP11675af43d82803e903d91602b57fd5bf3(15 bytes)602b→602c; P05 trailing byte droppedtestCloneCreationCodeIsEIP1167testCloneCreationCodeIsEIP1167abi.encodePacked— total length exactly 55 bytesabi.encodePacked→abi.encodetestCloneCreationCodeIsEIP1167(length 55)testCloneCreationCodeIsEIP1167,testCloneCreationCodeDeploysEIP1167Runtime602d→602c; P03 codecopy offset600a→600btestCloneCreationCodeDeploysEIP1167Runtime,testCloneCreationCodeIsEIP1167child.code)testCloneCreationCodeDeploysEIP1167Runtime,testCloneDeterministicMatchesPredict,testCloneDeterministicOpenSaltMatchesPredictimplementationparameter replaced byaddress(this)(function relaxed toview)predictCloneAddress(address,address,bytes32)— CREATE2 address formulakeccak256(0xff ++ factory ++ derivedSalt ++ keccak256(creationCode)), low 20 byteskeccak256→sha256; Q06 low 20 bytes → bytes 12..31 of the hashtestPredictCloneAddressMatchesOZ,testPredictCloneAddressIsCreate2Formula,testPredictCloneAddressMatchesRealDeploy0xffprefix byte0xff→0xfetestPredictCloneAddressMatchesOZ,testPredictCloneAddressIsCreate2Formula0xff, factory, salt, codehash (85-byte packed preimage)abi.encodePacked→abi.encode; R12 implementation hashed in place of factory; S05/S06 max/zero factory special-casedtestPredictCloneAddressMatchesOZ,testPredictCloneAddressIsCreate2Formula,testPredictCloneAddressMatchesRealDeploykeccak256of the CREATION code, not the runtime codetestPredictCloneAddressMatchesOZ,testPredictCloneAddressMatchesRealDeploytestPredictCloneAddressMatchesOZtestPredictCloneAddressBoundarySalts(added here)Equivalence notes
Three behaviours have no expressible mutant, and one mutant is genuinely equivalent. None is forced.
N06
abi.encode→abi.encodePackedineffectiveOpenSaltis EQUIVALENT. All three operands (ICLONEABLE_FACTORY_V4_OPEN_SALT_DOMAIN,salt,keccak256(data)) arebytes32, a static 32-byte type, so packed and non-packed encoding are byte-identical 96-byte preimages. The suite itself is the proof: under the mutant,testEffectiveOpenSaltMatchesFormula— which compares againstkeccak256(abi.encode(...))— passed all 2048 fuzz runs. The behaviour is not unobservable in principle, only in this function: the same mutation oneffectiveSalt(M05) is killed twice over, becausedeployeris anaddressthere and packs to 20 bytes instead of a padded word.M07 / N08 / P09 (purity) are compiler-enforced; no compiling mutant exists. Each was probed and returned NO-RUN with
solcError 2527 — "Function declared as pure, but this expression (potentially) reads from the environment or state and thus requiresview" — raised inside the pre-existing test files, at theexternal purecall sites inLibICloneableFactoryV4.t.solandLibICloneableFactoryV4.cloneCreationCode.t.sol. Thosepuretest functions are what makes the library'spuremutability load-bearing rather than decorative: relaxing it toviewdoes not compile the suite.This was checked rather than assumed. Inline assembly does not escape the rule — a standalone probe of
caller()inside apurelibrary function was compiled atsolc 0.8.25and rejected with the same Error 2527. No test is added for these, because a test nothing can make fail is not a discriminating test.Adversarial pass
Filed as issues, neutrally framed, none adjudicated here:
ICloneableFactoryV3NatSpec conditions cross-chain clone-address identity on the factory address alone, whichICloneableFactoryV4states is not sufficient. Verified with acastrepro: one factory address, one caller, one salt, two implementation addresses, two different clone addresses. (Independently filed as ICloneableFactoryV3 cross-network determinism claim omits the implementation address, contradicting ICloneableFactoryV4 #63 from the sibling slice; cross-linked.)testCheckImplementationCodeEtchedfails on unmutated source when the fuzzer draws0xEF-prefixed code. Fixed on this branch. (Independently filed as testCheckImplementationCodeEtched fails on unmutated main: vm.etch rejects fuzzed 0xEF-leading code #64; cross-linked.)@titleNatSpec says a delegating concrete "cannot get [themsg.sendernamespacing] wrong … there is no sender parameter to misroutetx.origininto", buteffectiveSalt(deployer, salt)andcloneAndInitialize(implementation, derivedSalt, data, salt)are both publishedinternalprimitives and compose to a type-correct factory that namespaces bytx.origin.Refuted rather than filed, recorded so the reasoning is auditable:
cloneCreationCode's NatSpec claim that "the tests pin it byte for byte against OZClones". Checked againstdependencies/@openzeppelin-contracts-5.6.1/proxy/Clones.sol:119-128: OZ assembles the same 55 bytes in memory and hashes them (keccak256(add(ptr, 0x0c), 0x37)), so address equality under fuzz is byte equality up to a keccak preimage collision. The claim stands.3d 602d 80 600a 3d 39 81 f3leaves[0, 45]on the stack andRETURNsmem[0:45]afterCODECOPY(dest=0, offset=10, length=45); the runtime's602bJUMPItarget is index 43, which is the5b JUMPDEST. Both match the implementation exactly.testDomainTagsDistinctandtestDomainTagsPinned, withtestCloneDeterministicOpenSaltDisjointTagsCloseTheSquatproving the reachable squat that the distinctness closes.Unprobed
None of the 29 behaviours is unprobed. Three (the purity behaviours) are probed only to the point of proving no compiling mutant exists; that limit is stated above rather than scored as coverage.
QA
testEffectiveOpenSaltBoundarySalts(bytes),testPredictCloneAddressBoundarySalts(address,address)— each fails on base under its mutant on the very first case (runs: 0), verified two ways: by hand-applying the mutation and reading the failing test name out offorge test, and bymutation-probe --only R08/--only S04returning1/1 killedwhere the full pass had scored both SURVIVED. AlsotestCheckImplementationCodeEtched(address,bytes), repaired rather than added: it fails on unmutated base whenever the fuzzer draws0xEF-leading code.src/, never test code.src/lib/LibICloneableFactoryV4.sol:88→salt == bytes32(type(uint256).max) ? bytes32(0) : salt→ killed bytestEffectiveOpenSaltBoundarySalts.src/lib/LibICloneableFactoryV4.sol:120→derivedSalt == bytes32(type(uint256).max) ? bytes32(0) : derivedSalt→ killed bytestPredictCloneAddressBoundarySalts. The other 56 (lines 31, 35, 75, 88, 99, 116-124, plussrc/interface/ICloneableFactoryV4.sol:15,24) are listed with their killing tests in the behaviour matrix above: 52 killed by the pre-existing suite, 1 equivalent, 3 non-compiling.testEffectiveOpenSaltBoundarySaltsrecomputeskeccak256(abi.encode(TAG, salt, keccak256(data)))inline from the interface constant, matching the file's existing convention;testPredictCloneAddressBoundarySaltsuses OpenZeppelinClones.predictDeterministicAddress, a foreign implementation of the same EIP-1167 CREATE2 prediction. The intent oracle for the whole pass is theICloneableFactoryV4NatSpec (src/interface/ICloneableFactoryV4.sol:34-48,:96-100,:172-184) together with EIP-1167 and EIP-1014, re-derived opcode by opcode rather than read off the implementation.audit+adversarial). Nothing in the group is uncovered; the three purity behaviours are probed to the limit of what compiles and that limit is stated rather than scored as coverage.Evidence
Harness is the CI's own, per
.github/workflows/rainix-sol.yaml→rainix/.github/workflows/rainix-sol-test.yaml:forge soldeer installthenforge testinsidenix develop .#sol-shell. Probe tool isnix run github:rainlanguage/adversarial-mutation-test#mutation-probe. The mutants files live outside the clone and are not committed.Pass 1 — 34 mutants, entire behaviour list, zero new tests present, baseline
42 passed:Pass 2 — 18 further mutants (tag literals, preimage shape, extremes, data-length special cases), baseline
42 passed:Pass 3 — 6 symmetric extremes, baseline
42 passed:Re-probe of the two survivors with the new tests in place,
--only:Attribution of those two kills, from a hand-applied mutation so the failing test names are visible:
Both fail on the first case, not on a lucky draw. In those same runs other fuzz tests also failed — which is exactly #70's point: the mutant a full pass scored SURVIVED is reachable by the existing tests, just not reliably.
Branch head:
forge fmt --checkclean,Ran 6 test suites: 44 tests passed, 0 failed, 0 skipped.🤖 Generated with Claude Code
Summary by CodeRabbit