Skip to content

The open-salt entry points have no test oracle independent of the library under test #43

Description

@thedavidmeister

What

CloneFactory's two open-salt entry points — cloneDeterministicOpenSalt and
predictDeterministicAddressOpenSalt — have no test oracle independent of the
library they delegate to.

Their only coverage is CloneFactoryLibEquivalence.t.sol, which states its
open-salt expectations in terms of LibICloneableFactoryV4.effectiveOpenSalt(salt, data):

bytes32 effectiveSalt = LibICloneableFactoryV4.effectiveOpenSalt(salt, data);
assertEq(
    I_CLONE_FACTORY.predictDeterministicAddressOpenSalt(implementation, data, salt),
    LibICloneableFactoryV4.predictCloneAddress(address(I_CLONE_FACTORY), implementation, effectiveSalt)
);

Both sides of that assertion are the code under test.

The namespaced pair does not have this problem

CloneFactoryCloneDeterministicTest.testCloneDeterministicSaltIsDomainTaggedHash
builds the effective salt from the interface constant and checks the
predicted address against OpenZeppelin Clones, a foreign EIP-1167
implementation. Its own NatSpec states the intent:

Pinned against OZ Clones — a foreign implementation of the same EIP-1167
standard — under an independently constructed salt, so the test does not
restate the library's arithmetic back to itself.

So the repo already holds the standard this issue asks for. It is applied to one
of the two derivations and not the other.

Why it matters

ICloneableFactoryV4 pins the open-salt derivation as a MUST, and spells the
preimage out twice in its own NatSpec:

keccak256(abi.encode(ICLONEABLE_FACTORY_V4_OPEN_SALT_DOMAIN, salt, keccak256(data)))

Nothing in this repo checks the code against that spec. A test that derives its
expectation from the implementation moves with the implementation: if
effectiveOpenSalt changed its domain separator, dropped keccak256(data) from
the preimage, or reordered the abi.encode operands, the equivalence assertions
would still pass, because both sides change together. The interface's stated
guarantee — that the open-salt and namespaced images cannot overlap, so no other
entry point on the factory can be aimed at an open-salt address — rests on that
first word being exactly this domain tag.

Scope — what this is NOT

This is not a live mutation gap, and the issue should not be read as claiming
one. A mutation probe over the concrete at 5c165a8 killed all 12 delegation
mutants with the behaviour-only suite. No surviving mutant motivates this.

The claim is narrower and is exactly this: the namespaced pair is checked against
an independent source of truth and the open-salt pair is not, so the open-salt
tests cannot detect a change to the derivation they are supposed to pin.

Proposed fix

Add the open-salt counterpart to the namespaced suite, built the way the
namespaced test is built:

  • the effective salt from ICLONEABLE_FACTORY_V4_OPEN_SALT_DOMAIN and the spec's
    preimage, predicted through OZ Clones — never through LibICloneableFactoryV4
  • deploy-matches-predict, through LibExtrospectERC1167Proxy and the initialized
    TestCloneable
  • the derivation's two defining properties, which the namespaced pair cannot
    have: the address is sender independent (that is what "open" means) and it
    commits to data

Revert paths and the NewClone event do not need restating; the equivalence
suite already holds them field for field.


Found by adversarial mutation testing (skill 0.35.0), whole-repo scan of
5c165a880ac105d6c038cee096500f76849a2190. Split out of #37, which originally
carried this alongside two unrelated concerns.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    auditFinding from an audit or mutation-test scan

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions