OlaHamid - [M-1] DOS (denial of service) vulnerability in the nested loop within the `DiamondLoupFacet.sol:facets` function.

[sherlock-admin3]

OlaHamid

Medium

[M-1] DOS (denial of service) vulnerability in the nested loop within the DiamondLoupFacet.sol:facets function.

Summary

Description: A Denial of Service (DoS) vulnerability was identified within the DiamondLoupFacet.sol:facets function. This is rooted in the inefficient handling of nested loops, leading to excessive gas consumption and potential transaction failures when processing function selectors.

Root Cause

The vulnerability is due to the nested loop structure in the facets function. The outer loop iterates over selectors, and the inner loop matches each selector to the corresponding facet address.

Internal pre-conditions

Internal Pre-conditions for DoS Vulnerability in DiamondLoupFacet.sol:facets Developer needs to implement nested loops in the facets function. Function relies on numFacets for loop execution. Large number of selectors need to be processed by the function. No gas limit checks are implemented within the function. Example: Developer needs to implement nested loops in the facets function. Function relies on numFacets for loop execution. Large number of selectors need to be processed by the function. No gas limit checks are implemented within the function.

External pre-conditions

No response

Attack Path

Attack Path for DoS Vulnerability in DiamondLoupFacet.sol:facets

1. Developer implements nested loops in the facets function.
2. Function relies on numFacets for loop execution.
3. Attacker identifies the nested loop structure and the lack of gas limit checks.
4. Attacker deploys a contract that calls the facets function with a large number of selectors.
5. Attacker calls the facets function with the large number of selectors, causing excessive gas consumption.
6. Transaction fails due to excessive gas usage, leading to a Denial of Service (DoS) condition.

Example:

1. Developer implements nested loops in the facets function.
2. Function relies on numFacets for loop execution.
3. Attacker identifies the nested loop structure and the lack of gas limit checks.
4. Attacker deploys a contract that calls the facets function with a large number of selectors.
5. Attacker calls the facets function with the large number of selectors, causing excessive

Impact

Impact: A Bad actor can exploit this vulnerability by submitting a large number of selectors, causing the function to consume excessive gas. This can lead to failed transactions, potentially disrupting the availability of the smart contract and affecting all users of the system.

PoC

Proof of Concept:

    function testDoSInFacets() public {
        // Initialize a large number of selectors to simulate a potential DoS attack
        uint256 selectorCount = 1000;
        bytes4[] memory selectors = new bytes4[](selectorCount);
        address[] memory facetAddresses = new address[](selectorCount);

        for (uint256 i = 0; i < selectorCount; i++) {
            selectors[i] = bytes4(keccak256(abi.encodePacked(i)));
            facetAddresses[i] = address(this);
        }

        // Deploy facets and set selectors
        // (assuming you have a function to do this in your contract)
        yourContract.setFacets(facetAddresses, selectors);

        // Gas usage before executing the function
        uint256 gasBefore = gasleft();

        // Call the vulnerable function
        yourContract.facets();

        // Gas usage after executing the function
        uint256 gasAfter = gasleft();

        // Calculate gas used
        uint256 gasUsed = gasBefore - gasAfter;

        // Log gas usage
        emit log_named_uint("Gas used for facets function", gasUsed);

        // Assert to ensure function does not fail due to excessive gas usage
        assert(gasUsed < block.gaslimit);
    }

### Mitigation

**Recommended Mitigation:** 
1. use mapping selectors directing to facet indexed. 
2. gas limit check.
3. optimize the overall architecture (optional).
4. batch processing

```solidity 
+   //Mapping for direct access
+   mapping(address => uint256) facetIndexMap; 
///code faucet////
function facets() external view override returns (Facet[] memory facets_) {
    LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
    uint256 selectorCount = ds.selectors.length;
    facets_ = new Facet[](selectorCount);
    uint8[] memory numFacetSelectors = new uint8[](selectorCount);
    uint256 numFacets;

    for (uint256 selectorIndex = 0; selectorIndex < selectorCount; selectorIndex++) {
        bytes4 selector = ds.selectors[selectorIndex];
        address facetAddress_ = ds.facetAddressAndSelectorPosition[selector].facetAddress;
+       // Gas limit check
+       if (gasleft() <> MIN_GAS_LIMIT){
    revert gasLimitError();
    };

        // + Direct mapping lookup to avoid nested loop
        uint256 facetIndex = facetIndexMap[facetAddress_];
        if (facetIndex == 0 && facetAddress_ != address(0)) {
            facetIndex = ++numFacets;
            facetIndexMap[facetAddress_] = facetIndex;
            facets_[facetIndex - 1].facetAddress = facetAddress_;
            facets_[facetIndex - 1].functionSelectors = new bytes4[](selectorCount);
        }

        facets_[facetIndex - 1].functionSelectors[numFacetSelectors[facetIndex - 1]++] = selector;
        require(numFacetSelectors[facetIndex - 1] < 255, "Too many functions from one facet");
-       bool continueLoop = false; // - Removed as it's no longer needed
-       // - Removed the nested loop and replaced with direct mapping lookup
-       for (uint256 facetIndex = 0; facetIndex < numFacets; facetIndex++) {
-           if (facets_[facetIndex].facetAddress == facetAddress_) {
-               facets_[facetIndex].functionSelectors[numFacetSelectors[facetIndex]] = selector;
-               require(numFacetSelectors[facetIndex] < 255);
-               numFacetSelectors[facetIndex]++;
-               continueLoop = true;
-               break;
-           }
-       }
-       if (continueLoop) {
-           continueLoop = false;
-           continue;
-       }

-       facets_[numFacets].facetAddress = facetAddress_;
-       facets_[numFacets].functionSelectors = new bytes4[](selectorCount);
-       facets_[numFacets].functionSelectors[0] = selector;
-       numFacetSelectors[numFacets] = 1;
-       numFacets++;
    }

    for (uint256 facetIndex = 0; facetIndex < numFacets; facetIndex++) {
        uint256 numSelectors = numFacetSelectors[facetIndex];
        bytes4[] memory selectors = facets_[facetIndex].functionSelectors;
        assembly {
            mstore(selectors, numSelectors)
        }
    }

    assembly {
        mstore(facets_, numFacets)
    }
}