Lack of event for blacklisted facets

[sujithsomraaj]

Context: EmergencyPauseFacet.sol#L138

Description: The unpauseDiamond function doesn't emit events for blacklisted facets, which could make it challenging to track which facets were not reinstated.

Recommendation: Add an event emission for each blacklisted facet that is not reinstated.

LI.FI: Fixed in 7fcf18641a45b9ecbdf744a014652e20b7092455 by using the diamondCut function that emits an event for every removed facet.

Researcher: Confirmed.

[0xDEnYO]

Not necessary in my opinion. We know what we are executing and we are the only ones that can execute this function (via multisig). I would close this without further action, pending a comment from Ed.

[maxklenk]

As we use internal functions of LibDiamond instead of diamondCut no diamond events are emitted. There is the unpause event, so someone could request the currently registered facets, but that requires custom work.

In my optinion: I think it would be good to stick with the standard flows and build one diamondCut call instead of accessing the internal functions directly. This way a event would be emitted that can automatically be parsed by tools that support diamonds.

[0xDEnYO]

That would emit one event for every facet we update. So on mainnet we would be emitting 40+ events. I honestly do not think this is necessary (and adds unnecessary complexity and gas consumption), especially for an admin function that is not part of our normal analytics.

How about we add the blacklist array as a parameter to the EmergencyUnpaused event instead? @maxklenk @sujithsomraaj

[ezynda3]

I would emit the normal diamond events as the EIP intends for them to be a sort of audit trail of upgrades for the diamond. This helps with transparency and is easier to track with standard tools. Gas consumption I don't think is really an issue here.

[0xDEnYO]

I just realized that we talk about the blacklisted facets only, so yes, events and gas consumption are not an issue. I like that suggestion.... Proposed solution:

    function unpauseDiamond(address[] calldata _blacklist) external {
        // make sure this function can only be called by the owner
        LibDiamond.enforceIsContractOwner();

        // get all facets from storage
        Storage storage s = getStorage();

        // iterate through all facets and reinstate the facet with its function selectors
        for (uint256 i; i < s.facets.length; ) {
            LibDiamond.replaceFunctions(
                s.facets[i].facetAddress,
                s.facets[i].functionSelectors
            );

            // gas-efficient way to increase loop counter
            unchecked {
                ++i;
            }
        }

        // go through blacklist and overwrite all function selectors with zero address
        // It would be easier to not reinstate these facets in the first place but
        //  a) that would leave their function selectors associated with address of EmergencyPauseFacet (=> throws 'DiamondIsPaused() error when called)
        //  b) it consumes a lot of gas to check every facet address if it's part of the blacklist
        bytes4[] memory currentSelectors;
        for (uint256 i; i < _blacklist.length; ) {
            currentSelectors = LibDiamondLoupe.facetFunctionSelectors(
                _blacklist[i]
            );

            // make sure that the DiamondCutFacet cannot be removed as this would make the diamond immutable
            if (currentSelectors[0] == DiamondCutFacet.diamondCut.selector)
                continue;

            // build FacetCut parameter
            IDiamondCut.FacetCut[]
                memory facetCut = new IDiamondCut.FacetCut[](1);
            facetCut[0] = IDiamondCut.FacetCut({
                facetAddress: address(0), // needs to be address(0) for removals
                action: IDiamondCut.FacetCutAction.Remove,
                functionSelectors: currentSelectors
            });

            // remove facet and its selectors from diamond
            LibDiamond.diamondCut(facetCut, address(0), "");

            // gas-efficient way to increase loop counter
            unchecked {
                ++i;
            }
        }

        // free storage
        delete s.facets;

        emit EmergencyUnpaused(msg.sender);
    }

@sujithsomraaj works?

[sujithsomraaj]

What's the resolution here? adding blacklist to the EmergencyUnpaused works.

[0xDEnYO]

The resolution is to use the diamondCut function that emits an event for every removed facet:

Fix: https://github.com/lifinance/contracts/commit/7fcf18641a45b9ecbdf744a014652e20b7092455