Aymen0909 - User can stop a shutdown execution by creating a new listing which will force all voters to wait a long period before getting their payout

[sherlock-admin4]

Aymen0909

Medium

User can stop a shutdown execution by creating a new listing which will force all voters to wait a long period before getting their payout

Summary

A malicious user can create a new listing just before the execution of a collection shutdown to delay the sell out of the collection NFTs for a long period (even definitively), thus forcing previous holders to wait a long time before receiving their payouts.

Root Cause

Any user can create a new listing for a collection by calling Listings::createListings even if the collection is going through a shutdown.

Internal pre-conditions

1. A collection shutdown has reach quorum and is ready to be executed.

External pre-conditions

1. A malicious user holding an ERC721 token of the shutdown collection, creates a new listing for the collection before the shutdown get executed.

Attack Path

1. A collection is going through a shutdown vote and it has reached the quorum for sell execution.
2. Malicious user notices that the admin is triggering the shutdown execution, and front run the tx by creating a new listing for that specific collection.
3. The admin execution call to CollectionShutdown::execute will then get run but it will revert because _hasListings(_collection) will return true
4. The collection sell execution will stay pending until the malicious user listing expires (which could take up to 180 days for a liquid listing)
5. All voters and token holders will have to wait to get their payout from the collection sell out, and they won't have access to the collection ERC20 token anymore because they transferred it to the CollectionShutdown contract when voting.

Impact

The collection shutdown sell out will be halted for a long period (until malicious user listing expires), thus users participating in the voting will have to wait that long period to get their funds and may even wait definitively if the malicious user keeps repeating the same tactic.

PoC

The CollectionShutdown::execute function will always revert if there are still listing for the collection being shutdown because of the _hasListings(_collection) check as shown below (see CollectionShutdown::L231-L241):

function execute(address _collection, uint[] calldata _tokenIds) public onlyOwner whenNotPaused {
    // Ensure that the vote count has reached quorum
    CollectionShutdownParams storage params = _collectionParams[_collection];
    if (!params.canExecute) revert ShutdownNotReachedQuorum();

    // Ensure we have specified token IDs
    uint _tokenIdsLength = _tokenIds.length;
    if (_tokenIdsLength == 0) revert NoNFTsSupplied();

    // Check that no listings currently exist
    if (_hasListings(_collection)) revert ListingsExist();

    // Refresh total supply here to ensure that any assets that were added during
    // ...
}

And when a collection is going through a vote, at any time any user can still create a new listing for that collection regardless of the shutdown vote state, as Listings::createListings never check the collection shutdown status (see Listings::L130-L166):

function createListings(CreateListing[] calldata _createListings) public nonReentrant lockerNotPaused {
    // Loop variables
    uint taxRequired;
    uint tokensIdsLength;
    uint tokensReceived;

    // Loop over the unique listing structures
    for (uint i; i < _createListings.length; ++i) {
        // Store our listing for cheaper access
        CreateListing calldata listing = _createListings[i];

        // Ensure our listing will be valid
        _validateCreateListing(listing);

        // Map our listings
        tokensIdsLength = listing.tokenIds.length;
        tokensReceived = _mapListings(listing, tokensIdsLength) * 10 ** locker.collectionToken(listing.collection).denomination();

        // Get the amount of tax required for the newly created listing
        taxRequired = getListingTaxRequired(listing.listing, listing.collection) * tokensIdsLength;
        if (taxRequired > tokensReceived) revert InsufficientTax(tokensReceived, taxRequired);
        unchecked { tokensReceived -= taxRequired; }

        // Increment our listings count
        unchecked {
            listingCount[listing.collection] += tokensIdsLength;
        }

        // Deposit the tokens into the locker and distribute ERC20 to user
        _depositNftsAndReceiveTokens(listing, tokensReceived);

        // Create our checkpoint as utilisation rates will change
        protectedListings.createCheckpoint(listing.collection);

        emit ListingsCreated(listing.collection, listing.tokenIds, listing.listing, getListingType(listing.listing), tokensReceived, taxRequired, msg.sender);
    }
}

Note that _validateCreateListing doesn't check for the collection shutdown status either.

Mitigation

To avoid this issue, the creation of new listings should be disallowed for collections going through a shutdown or that already reached the shutdown vote quorum.