Skip to content

CloneDeploymentFailed's "the exact clone asked for is already at the address" is false on the namespaced derivation, which leaves data out #74

Description

@thedavidmeister

Found by an adversarial mutation-test pass over LibICloneableFactoryV4
(group g2-libicloneablefactoryv4-predi), scanned at
c1c2afd. Filed for triage, not adjudicated.

Unit

CloneDeploymentFailed, src/lib/LibICloneableFactoryV4.sol:17-23, as raised
by cloneAndInitialize (:150-170) on both entry points.

Intent oracle

CloneDeploymentFailed's own NatSpec:

Thrown when the CREATE2 deploy of the clone itself fails. With the tiny fixed
EIP-1167 initcode the only realistic cause is that the effective salt is
already taken: the exact clone asked for is already at the address, so the
caller can never mistake an already-initialized contract for their own fresh
deploy.

Compare ICloneableFactoryV4, which makes the same claim but only for the
open-salt derivation, and is careful about why it holds there: "Since nothing
else can be deployed there, what occupies it is the clone that was asked for,
initialized with the bytes that were asked for"
— true because keccak256(data)
is inside the open-salt effective salt.

Violated property

The error doc is shared by both entry points, but its "the exact clone asked for
is already at the address" clause is only true of the open-salt derivation.

effectiveSalt(deployer, salt) = keccak256(abi.encode(NAMESPACED_DOMAIN, deployer, salt))
does not hash data. So on the namespaced path, one caller's
(implementation, salt) maps to exactly one address regardless of what they
initialize with — which the suite already pins, deliberately, in
testCloneDeterministicDataNotInDerivation. A second cloneDeterministic at the
same (sender, salt) with different data therefore reverts
CloneDeploymentFailed, and what is at the address is the clone initialized with
the FIRST caller's data, not "the exact clone asked for".

The second half of the sentence — "the caller can never mistake an
already-initialized contract for their own fresh deploy" — does hold on both
paths: the caller gets a revert, not a silent handback. It is the first half
that over-claims, and the two halves read as one guarantee.

Why it matters beyond wording: a consumer reading this error doc could
reasonably conclude that catching CloneDeploymentFailed and then using the
address at predictDeterministicAddress(implementation, salt, deployer) is safe,
because "the exact clone asked for is already there". On the namespaced path that
contract may have been initialized with entirely different bytes by the same
account at an earlier time.

Verified repro

// SPDX-License-Identifier: LicenseRef-DCL-1.0
// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
pragma solidity =0.8.25;

import {Test} from "forge-std-1.16.1/src/Test.sol";
import {CloneDeploymentFailed} from "src/lib/LibICloneableFactoryV4.sol";
import {TestCloneFactory} from "test/src/concrete/TestCloneFactory.sol";
import {TestCloneable} from "test/src/concrete/TestCloneable.sol";

contract ProbeOccupantTest is Test {
    function testProbeOccupantIsNotWhatWasAskedFor() external {
        TestCloneFactory factory = new TestCloneFactory();
        TestCloneable implementation = new TestCloneable();

        address first = factory.cloneDeterministic(address(implementation), hex"aaaa", bytes32(uint256(7)));
        emit log_named_bytes("first sData", TestCloneable(first).sData());

        try factory.cloneDeterministic(address(implementation), hex"bbbb", bytes32(uint256(7))) returns (address second) {
            emit log_named_address("UNEXPECTED second", second);
        } catch (bytes memory reason) {
            emit log_named_bytes("second deploy revert", reason);
            emit log_named_bytes32("CloneDeploymentFailed selector", bytes32(CloneDeploymentFailed.selector));
        }
        emit log_named_bytes("occupant sData after asking for 0xbbbb", TestCloneable(first).sData());
    }
}

Observed:

  first sData: 0xaaaa
  second deploy revert: 0x04ddc24d
  CloneDeploymentFailed selector: 0x04ddc24d00000000000000000000000000000000000000000000000000000000
  occupant sData after asking for 0xbbbb: 0xaaaa

The caller asked for 0xbbbb; the occupant holds 0xaaaa.

Triage framing

Readings, none of them chosen here:

  1. Documentation only. The runtime behaviour is exactly what the two
    derivations are designed to do, and is already pinned by
    testCloneDeterministicDataNotInDerivation. The fix is to split the claim:
    CloneDeploymentFailed's NatSpec says the deploy failed and the caller gets a
    revert rather than a handback, and defers "and what is there is what you asked
    for" to ICloneableFactoryV4, where it is derivation-specific and true only
    for open-salt.
  2. Not a defect at all. "The exact clone asked for" can be read as "a clone
    of that implementation from that (deployer, salt)", which is exactly what is
    there — data was never part of what the namespaced path promises about an
    address, and the interface says so at length.
  3. Wider claim to check. The same doc's "the only realistic cause is that the
    effective salt is already taken" also silently covers a CREATE2 that ran out
    of gas under the 63/64 rule and a destination that has a nonce but no code;
    both surface as CloneDeploymentFailed too.

Flagged rather than fixed: which of these is a real problem depends on whether
the error doc is meant to be read as a guarantee about the occupant.

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