safdie - `ReplaceFunctions` does not update selectors will lead to confusion or issues during retrieval or iteration in `LibDiamond.sol`

[sherlock-admin4]

safdie

High

ReplaceFunctions does not update selectors will lead to confusion or issues during retrieval or iteration in LibDiamond.sol

Summary

In replaceFunctions method, while it replaces the facet address for a selector, it doesn’t update the selector array. Although the address is changed, the selector is still associated with the old address in the array, which could lead to confusion or issues during retrieval or iteration.

Root Cause

The vulnerability is in the replaceFunctions function in the Diamond standard (EIP-2535) library. The purpose of the function is to replace existing functions in the diamond with new ones. However, in the provided code, the function selectors array (selectors) is not updated after a function is replaced. This can lead to inconsistencies between the actual facet addresses and the selectors array, which is critical for determining which function is called. https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/libraries/LibDiamond.sol#L94-L109

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

In the vulnerable replaceFunctions implementation below, the selectors array is not modified when a function is replaced. Let's create a PoC that demonstrates the faulty behavior. PoC explanation:

1. We first deploy a Diamond contract with some initial functions.

   
   // SPDX-License-Identifier: GPL-3.0-or-later
   pragma solidity >=0.8.18;

contract InitialFacet { function initialFunction() external pure returns (string memory) { return "Initial function"; } }

2. Then, we attempt to replace one of the functions in the Diamond.
```solidity
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.18;

contract ReplacementFacet {
    function initialFunction() external pure returns (string memory) {
        return "Replaced function";
    }
}

4. Since the selectors[] array is not updated when replacing functions, querying the contract using a DiamondLoupe function may return outdated or incorrect information. When we call replaceFunctions(), the initialFunction() should be replaced, but since the selectors[] array is not updated, querying the selectors could give us inconsistent results.

   
   pragma solidity >=0.8.18;

contract TestReplaceFunction { IDiamondCut diamond;

constructor(address _diamondAddress) {
    diamond = IDiamondCut(_diamondAddress);
}

function performReplacement() public {
    // Replace initialFunction with a new one from ReplacementFacet
    bytes4;
    functionSelectors[0] = InitialFacet.initialFunction.selector;
    IDiamondCut.FacetCut;

    cut[0] = IDiamondCut.FacetCut({
        facetAddress: address(new ReplacementFacet()),
        action: IDiamondCut.FacetCutAction.Replace,
        functionSelectors: functionSelectors
    });

    diamond.diamondCut(cut, address(0), "");
}

}

After calling the `diamondCut()` function, you can query the diamond selectors, and they will not reflect the replacement.
```solidity
function getSelectors() external view returns (bytes4[] memory) {
    return LibDiamond.diamondStorage().selectors;
}

Results: The output of selectors[] will still point to the old initialFunction() from the InitialFacet, even though the function has been replaced by ReplacementFacet. This mismatch can cause unexpected behavior when trying to call functions using the outdated selector list.

Impact

1. The system may call the wrong facet function if the selector is outdated. This can result in breaking the contract’s expected behavior, especially in a complex system with multiple facets.
2. Attackers could exploit this vulnerability by intentionally leaving the selectors[] array in an inconsistent state. This could lead to improper function dispatching, potentially bypassing function access controls or causing mis-execution of critical contract logic.
3. Future upgrades might fail if the selectors array is corrupted or left in an inconsistent state, leaving the diamond contract in a frozen or inoperable state.

PoC

function replaceFunctions(address _facetAddress, bytes4[] memory _functionSelectors) internal {
    require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
    DiamondStorage storage ds = diamondStorage();
    require(_facetAddress != address(0), "LibDiamondCut: Replace facet can't be address(0)");
    enforceHasContractCode(_facetAddress, "LibDiamondCut: Replace facet has no code");

    // The selectors are being replaced, but the `selectors` array isn't updated
    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
        bytes4 selector = _functionSelectors[selectorIndex];
        address oldFacetAddress = ds.facetAddressAndSelectorPosition[selector].facetAddress;

        // can't replace immutable functions -- functions defined directly in the diamond
        require(oldFacetAddress != address(this), "LibDiamondCut: Can't replace immutable function");
        require(oldFacetAddress != _facetAddress, "LibDiamondCut: Can't replace function with same function");
        require(oldFacetAddress != address(0), "LibDiamondCut: Can't replace function that doesn't exist");

        // Replace facet address but don't update the `selectors` array
        ds.facetAddressAndSelectorPosition[selector].facetAddress = _facetAddress;
    }
}

Mitigation

To resolve this, the selectors[] array must be updated correctly during the replacement process.

function replaceFunctions(address _facetAddress, bytes4[] memory _functionSelectors) internal {
    require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
    DiamondStorage storage ds = diamondStorage();
    require(_facetAddress != address(0), "LibDiamondCut: Replace facet can't be address(0)");
    enforceHasContractCode(_facetAddress, "LibDiamondCut: Replace facet has no code");

    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
        bytes4 selector = _functionSelectors[selectorIndex];
        address oldFacetAddress = ds.facetAddressAndSelectorPosition[selector].facetAddress;

        // Update the `selectors` array when replacing functions
        ds.facetAddressAndSelectorPosition[selector].facetAddress = _facetAddress;
        ds.selectors[ds.facetAddressAndSelectorPosition[selector].selectorPosition] = selector;
    }
}

Now, the selectors[] array is updated correctly during the replacement process, ensuring that the function selectors are always synchronized with the actual facet addresses.