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

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions