Potential DoS in unbounded for loop

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/5a31c9db8ab32175dbd7264b05ce84931b6c0428/ethereum/contracts/zksync/facets/Getters.sol#L163

Vulnerability details

Proof of Concept

The function facets() in Getters.sol iterates over the DiamondStorage facets array. In Diamond.sol we can see that by calling diamondCut in a way that _saveFacetIfNew gets called, the diamond owner can add an indefinite amount of entries. If this is the case, the facets() function will be in a state of DoS because iterating over a very big array can cost more than the current block gas limit.

Impact

The impact is potential functionality getting into a DoS state which is a problem even though the method is a getter - it might be used in a front-end.

Recommendation

Add an offset parameter to the facets() function so the loop iterates the array starting from offset index

[GalloDaSballo]

Looks Low at best

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[miladpiri]

It seems like a self-DoS issue, why should a governor do so? A malicious governor can apply more severe attack instead of just DoS. Moreover, it is clear that the number of facets and the selectors will not be that high resulting in DoS issue. So it is an invalid issue!

[GalloDaSballo]

You'd need 2.4k facets to consider the risk of reverts

Am closing as inflated

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity

[GalloDaSballo]

12*10^6 / 5000 (2.1k per facet technically so being generous),