Skip to content

deployInclude selectors incorrectly marked Add when moving across lower-priority facets #13

Description

@Super-Genius

Bug

BaseDeploymentStrategy.updateFunctionSelectorRegistryTasks() — the "Inclusion Override Filter" for deployInclude selectors only upgrades to Replace when the selector is already registered under a higher-priority facet. If it exists under a lower-priority or equal-priority facet, it falls through to Add.

Where

dist/strategies/BaseDeploymentStrategy.js, updateFunctionSelectorRegistryTasks, Inclusion Override Filter (~line 245–275).

The registryHigherPrioritySplit filter uses entry.priority > priority (stricter-than). Selectors in lower-priority facets are excluded from this set, so the if (higherPriorityFacet) check fails, and the else branch inserts a spurious Add:

const higherPriorityFacet = Object.keys(registryHigherPrioritySplit).find((facetName) => {
    return registryHigherPrioritySplit[facetName].includes(includeFuncSelector);
});
if (higherPriorityFacet) {
    // → Replace ✅
} else {
    // → Add ❌  (should be no-op — selector already exists)
    registry.set(includeFuncSelector, {
        priority, address: currentFacetAddress,
        action: RegistryFacetCutAction.Add,
        facetName: newFacetName,
    });
}

The Add entry then gets spliced out of funcSelectors before the Priority Resolution Pass, so that pass never gets a chance to correct it to Replace.

Impact

When v2.5 of a config uses deployInclude to move selectors from one facet (e.g., GNUSNFTFactory, priority 40) to another (ERC1155ProxyOperator, priority 45), the selectors that already exist on-chain are marked Add. Executing the resulting diamondCut reverts with:

LibDiamondCut: Can not add function that already exists
→ GS013

Reproduction

  1. Deploy a diamond with two facets: FacetA (priority 40) owning selector S, and FacetB (priority 45) not owning S.
  2. Create an upgrade config where FacetB has deployInclude: [S] and FacetA has deployExclude: [S].
  3. Run getFacetCuts — the selector S is classified as Add onto FacetB instead of Replace.

Fix

The else branch should be a no-op — the selector already exists in the registry, just under a different facet. Remove the registry.set (and the subsequent splice from funcSelectors) so the Priority Resolution Pass handles it correctly:

} // else: selector already registered — nothing to do, Priority Resolution Pass will handle
const existing = newDeployedFacets[newFacetName];
// ... splice removed too

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions