safdie - Large array allocation without trimming lead to a large waste of gas, or in the worst-case scenario, a denial-of-service (DoS) attack if the arrays become too large in `DiamondLoupFacet.sol`

[sherlock-admin4]

safdie

Medium

Large array allocation without trimming lead to a large waste of gas, or in the worst-case scenario, a denial-of-service (DoS) attack if the arrays become too large in DiamondLoupFacet.sol

Summary

Large array allocation without trimming in DiamondLoupFacet.sol lead to a large waste of gas, or in the worst-case scenario, a denial-of-service (DoS) attack if the arrays become too large.

Root Cause

The facets(), https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/DiamondLoup/DiamondLoupFacet.sol#L26-L60 facetFunctionSelectors(), https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/DiamondLoup/DiamondLoupFacet.sol#L76-L83 and facetAddresses() https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/DiamondLoup/DiamondLoupFacet.sol#L99-L118 functions in the DiamondLoupeFacet contract allocate arrays to hold the function selectors and facet addresses but do not properly trim these arrays. This could lead to a large waste of gas, or in the worst-case scenario, a denial-of-service (DoS) attack if the arrays become too large. Specifically, the code allocates arrays with a size equal to the total number of function selectors, but it doesn't trim the arrays until after they've been populated. This inefficient allocation can lead to performance issues and make the contract vulnerable to gas limit exhaustion, especially with large sets of selectors or repeated calls.

Internal pre-conditions

Let's simulate the impact of inefficient memory allocation on gas usage by creating a scenario where an attacker can exploit the array over-allocation to trigger out-of-gas errors.

1. Suppose the contract has hundreds or thousands of function selectors, each of which causes the facets(), facetFunctionSelectors(), or facetAddresses() functions to allocate large arrays (without trimming) based on the total number of selectors.
2. An attacker could call one of these functions repeatedly, forcing the contract to allocate large arrays without trimming, potentially causing out-of-gas errors due to memory bloat or high gas costs.

External pre-conditions

No response

Attack Path

PoC below can be used to spam the facets(), facetFunctionSelectors(), or facetAddresses() functions, each causing large arrays to be allocated without proper trimming. This significantly increases gas costs, and when scaled, can lead to out-of-gas errors, resulting in DoS attacks.

Impact

1. Each time a large array is allocated but not properly trimmed, unnecessary gas is consumed. With contracts holding many facets or selectors, this can become highly inefficient, wasting gas on unused memory.
2. Legitimate users calling these functions may be forced to pay disproportionately high gas costs due to the untrimmed arrays, making the contract more expensive to interact with.
3. As the contract grows (i.e., as more facets are added), the inefficiency in memory allocation grows, further increasing gas costs and making it harder to interact with the contract efficiently.

PoC

contract DiamondLoupeAttacker {
    DiamondLoupeFacet diamond;

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

    function spamFacets() public {
        // Continuously calling facets() to waste gas and bloat memory allocation
        for (uint256 i = 0; i < 1000; i++) {
            diamond.facets();
        }
    }

    function spamFacetFunctionSelectors(address _facet) public {
        // Continuously calling facetFunctionSelectors() to bloat memory allocation
        for (uint256 i = 0; i < 1000; i++) {
            diamond.facetFunctionSelectors(_facet);
        }
    }

    function spamFacetAddresses() public {
        // Continuously calling facetAddresses() to waste gas
        for (uint256 i = 0; i < 1000; i++) {
            diamond.facetAddresses();
        }
    }
}

Mitigation

To mitigate this issue, the contract should trim the arrays immediately after determining their final size, instead of after allocating the maximum possible size.

Here’s how you can fix the array allocations:

function facets() external view override returns (Facet[] memory facets_) {
    LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
    uint256 selectorCount = ds.selectors.length;
    uint8[] memory numFacetSelectors = new uint8[](selectorCount);
    uint256 numFacets;

    // Dynamically allocate based on the actual number of facets
    facets_ = new Facet[](numFacets); // Allocating based on actual size
    ...
}

function facetFunctionSelectors(address _facet) external view override returns (bytes4[] memory _facetFunctionSelectors) {
    LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
    uint256 numSelectors;

    // Dynamically allocate based on actual number of selectors for the facet
    _facetFunctionSelectors = new bytes4[](numSelectors);
    ...
}

function facetAddresses() external view override returns (address[] memory facetAddresses_) {
    LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
    uint256 numFacets;

    // Dynamically allocate based on actual number of facets
    facetAddresses_ = new address[](numFacets);
    ...
}