zkSync Era - Jorgect's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/libraries/Diamond.sol#L278

Vulnerability details

Impact

When a facet has not more selectors to delete, the contract delete the facet this is made it in the _removeFacet function deleting address of the facet, but the function is missing delete the position of the facet, this is in a important variable and is used in the important functions of the contract leading to mistakes.

Proof of Concept

_removeFacet function:

file:code/contracts/ethereum/contracts/zksync/libraries/Diamond.sol
 function _removeFacet(address _facet) private {
        DiamondStorage storage ds = getDiamondStorage();

        // Get index of `DiamondStorage.facets` of the facet and last element of array
        uint256 facetPosition = ds.facetToSelectors[_facet].facetPosition;
        uint256 lastFacetPosition = ds.facets.length - 1;

        // If the facet is not at the end of the array then move the last element to the facet position
        if (facetPosition != lastFacetPosition) {
            address lastFacet = ds.facets[lastFacetPosition];

            ds.facets[facetPosition] = lastFacet;
            ds.facetToSelectors[lastFacet].facetPosition = facetPosition.toUint16();
        }

        // Remove last element from the facets array
        ds.facets.pop();
        @--->  ds.facetToSelectors[lastFacet].facetPosition is not deleted
    }

This can be see it in other implementation of the diamond proxy like diamond-3-hardhat

Tools Used

Manual review

Recommended Mitigation Steps

Delete the ds.facetToSelectors[lastFacet].facetPosition in the Diamond.sol

function _removeFacet(address _facet) private {
        DiamondStorage storage ds = getDiamondStorage();

        // Get index of `DiamondStorage.facets` of the facet and last element of array
        uint256 facetPosition = ds.facetToSelectors[_facet].facetPosition;
        uint256 lastFacetPosition = ds.facets.length - 1;

        // If the facet is not at the end of the array then move the last element to the facet position
        if (facetPosition != lastFacetPosition) {
            address lastFacet = ds.facets[lastFacetPosition];

            ds.facets[facetPosition] = lastFacet;
            ds.facetToSelectors[lastFacet].facetPosition = facetPosition.toUint16();
        }

        // Remove last element from the facets array
        ds.facets.pop();
        delete ds.facetToSelectors[lastFacet].facetPosition; @-------> deleting the position
    }

Assessed type

Other