Possibly incorrect listing type calculation

Summary

A listing whose duration is dutchable (1 days <= duration <= 7 days) can never be LIQUID because of the getListingType() function.

Vulnerability Detail

A type of listing is determined by the getListingType() function.

    function getListingType(Listing memory _listing) public view returns (Enums.ListingType) {
        // If we cannot find a valid listing and get a null parameter value, then we know
        // that the listing does not exist and it is therefore just a base token.
        if (_listing.owner == address(0)) {
            return Enums.ListingType.NONE;
        }

        // If the listing was created as a dutch listing, or if the liquid listing has
        // expired, then this is a dutch listing.
        if (
            (_listing.duration >= MIN_DUTCH_DURATION && _listing.duration <= MAX_DUTCH_DURATION) ||
            _listing.created + _listing.duration < block.timestamp
        ) {
            return Enums.ListingType.DUTCH;
        }

        // For all other eventualities, we have a default liquid listing
        return Enums.ListingType.LIQUID;
    }

"Dutch" literally means a type of auction where the price of an item starts high and gradually decreases over time until a buyer accepts the current price. So only the period that the price decreases from high to low, should be DUTCH.

But, according to the above function, the type of a listing whose duration is dutchable (1 days <= duration <= 7 days), has the following listing type over time.

          DUTCH                             DUTCH
|-----------------------|---------------------------------------------->
0                    duration                                        + Inf.

On the other hand, for a listing whose duration is out-of-dutch, it has the following listing type over time.

       LIQUID          LIQ. DUTCH                DUTCH
|-------------------|--------------|----------------------------------->
0                duration   (duration + 4 days)                      + Inf.

The above diagram shows that a listing whose duration is dutchable can never be LIQUID.

Proof-Of-Concept

Here is a test case of the getListingType() function:

    // @audit-poc
    function test_IncorrectListingType() public {
        // Ensure that we don't get a token ID conflict
        uint256 _tokenId = 1;
        address _owner = address(0x101);
        uint16 _floorMultiple = 150;
        address _collection = address(erc721a);
        ICollectionToken token = locker.collectionToken(_collection);

        erc721a.mint(_owner, _tokenId);

        vm.startPrank(_owner);
        erc721a.approve(address(listings), _tokenId);
        _createListing({
            _listing: IListings.CreateListing({
                collection: address(erc721a),
                tokenIds: _tokenIdToArray(_tokenId),
                listing: IListings.Listing({
                    owner: payable(_owner),
                    created: uint40(block.timestamp),
                    duration: 5 days,
                    floorMultiple: _floorMultiple
                })
            })
        });
        vm.stopPrank();

        vm.warp(block.timestamp + 1 days);
        IListings.Listing memory listing = listings.listings(_collection, _tokenId);
        console.log("Listing Type after 1 day(s):", uint(listings.getListingType(listing)));

        vm.warp(block.timestamp + 5 days);
        listing = listings.listings(_collection, _tokenId);
        console.log("Listing Type after 6 day(s):", uint(listings.getListingType(listing)));

        vm.warp(block.timestamp + 94 days);
        listing = listings.listings(_collection, _tokenId);
        console.log("Listing Type after 100 day(s):", uint(listings.getListingType(listing)));
    }

Here are the logs:

$ forge test --match-test test_IncorrectListingType -vv
[⠒] Compiling...
[⠔] Compiling 1 files with Solc 0.8.26
[⠒] Solc 0.8.26 finished in 13.48s

Ran 1 test for test/Listings.t.sol:ListingsTest
[PASS] test_IncorrectListingType() (gas: 487880)
Logs:
  Listing Type after 1 day(s): 0
  Listing Type after 6 day(s): 0
  Listing Type after 100 day(s): 0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.25ms (506.20µs CPU time)

Ran 1 test suite in 9.96ms (5.25ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

As can be seen from the logs, the listing type is always DUTCH even after the dutch duration has expired.

Impact

As only LQUID type listings can be modifiable and cancellable, the listings with dutchable durations can never be modified or canceled.

Code Snippet

Listings.sol#L962-L980

Tool used

Manual Review

Recommendation

In my opinion, the intended timemap of a listing could be as follows:

For dutchable duration,

          DUTCH                             LIQUID
|-----------------------|---------------------------------------------->
0                    duration                                        + Inf.

For non-dutchable duration,

       LIQUID          LIQ. DUTCH                LIQUID
|-------------------|--------------|----------------------------------->
0                duration   (duration + 4 days)                      + Inf.

Based on this assumption, I'd suggest updating the getListingType() function as follows:

    function getListingType(Listing memory _listing) public view returns (Enums.ListingType) {
        if (_listing.owner == address(0)) {
            return Enums.ListingType.NONE;
        }

        if (_listing.duration >= MIN_DUTCH_DURATION && _listing.duration <= MAX_DUTCH_DURATION) {
            if (_listing.created + _listing.duration < block.timestamp) {
                return Enums.ListingType.LIQUID;
            }
            return Enums.ListingType.DUTCH;
        }

        if (_listing.created + _listing.duration < block.timestamp && _listing.created + _listing.duration + LIQUID_DUTCH_DURATION >= block.timestamp) {
            return Enums.ListingType.DUTCH;
        }

        return Enums.ListingType.LIQUID;
    }

sherlock-audit / 2024-08-flayer-judging

stuart_the_minion - Possibly incorrect listing type calculation #686