Skip to content

Library NatSpec claims a delegating concrete cannot misroute the msg.sender namespacing, but effectiveSalt + cloneAndInitialize compose to a factory that does #71

Description

@thedavidmeister

Unit

LibICloneableFactoryV4.effectiveSalt(address,bytes32) (src/lib/LibICloneableFactoryV4.sol:74-76) and the library-level safety claim made about it in the @title NatSpec (src/lib/LibICloneableFactoryV4.sol:58-63).

Intent oracle

src/lib/LibICloneableFactoryV4.sol:58-63:

msg.sender is read INSIDE this library — cloneDeterministic namespaces by it and NewClone reports it — and the internal functions execute in the factory's own call context, so a delegating concrete cannot get either wrong: there is no sender parameter to misroute tx.origin into. Likewise the predictions read address(this), the factory the library is inlined into.

and ICloneableFactoryV3:33-37, the property the namespacing exists to provide:

The factory MUST namespace salt by msg.sender before deriving the CREATE2 salt, so a caller's (implementation, salt) address cannot be squatted or front-run by another account.

Property that appears to be violated

The claim is that the library's shape makes the msg.sender namespacing unmisroutable by a delegating concrete. That holds for cloneDeterministic (:180-182), which reads msg.sender itself and exposes no sender parameter.

But the library also publishes effectiveSalt(address deployer, bytes32 salt) as an internal entry in its own right, and publishes cloneAndInitialize(implementation, derivedSalt, data, salt) (:150-170) which takes an already-derived salt. A concrete that composes those two instead of calling cloneDeterministic reaches a compiling, type-correct factory whose namespacing is whatever it passed:

function cloneDeterministic(address implementation, bytes calldata data, bytes32 salt)
    external
    returns (address)
{
    // Type-checks, matches ICloneableFactoryV3's signature, and namespaces by
    // tx.origin. The library's own primitives compose to it.
    return LibICloneableFactoryV4.cloneAndInitialize(
        implementation, LibICloneableFactoryV4.effectiveSalt(tx.origin, salt), data, salt
    );
}

That factory still emits NewClone with msg.sender and still lands in the namespaced derivation's image, so it looks conforming, while predictDeterministicAddress(implementation, salt, deployer) on the same factory keeps predicting from deployer — the two halves disagree, and the ICloneableFactoryV3 squat-resistance MUST is not met for any caller reached through an intermediate contract.

predictDeterministicAddress taking a deployer parameter is required by the ICloneableFactoryV3 signature and is not in question here. The question is only about the sentence that says a delegating concrete "cannot get either wrong", which reads as unconditional but is true only of the whole entry points, not of the primitives the library also exposes.

Verified repro

Static/compositional; no runtime counterexample is claimed. Verified by reading the library surface at c1c2afd: effectiveSalt is internal and takes an arbitrary deployer (:74), cloneAndInitialize is internal and takes an arbitrary derivedSalt (:150), and neither constrains the other. The snippet above is the composition; test/src/concrete/TestCloneFactory.sol is the intended composition it deviates from.

The repo's own convention is that the concrete lives elsewhere (rain.factory.deploy, see CLAUDE.md), so the concrete that must not make this mistake is not in this repository and is not covered by this repository's tests.

Triage framing

No defect in effectiveSalt itself — it reproduces the pinned formula exactly and every mutation of its derivation is killed by the existing suite. This is about whether the NatSpec's unconditional phrasing should be scoped to the whole entry points, and/or whether the composable primitives should carry a warning that they are the building blocks and not the guarantee. It is also possible this is considered adequately covered by "a delegating concrete", i.e. one that only delegates. Flagged for a maintainer decision rather than adjudicated.

Found during adversarial mutation testing of LibICloneableFactoryV4 at c1c2afd3d88405d3228cb3b21e55c9d63ba8f5be.

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

    adversarialFound by adversarial reviewauditAudit finding

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions