Vast Umber Walrus - Failure to delete the listing when it is reserved

[sherlock-admin4]

Vast Umber Walrus

High

Failure to delete the listing when it is reserved

Summary

The Listings::reserve() function fails to delete listings after they are moved to the ProtectedListing contract, resulting in outdated data remaining in the Listing contract.

This failure to delete listings can lead to the misuse of listing data, causing issues in functions such as modifyListings(), cancelListings(), fillListings() and others that interact with listings.

Vulnerability Detail

The Listings::reserve() function does not properly remove the listing from the Listing contract once the token listing is transferred to the ProtectedListing contract.

As a result, the outdated listing data persists and can be inadvertently used in various functions within the Listing contract. This can cause incorrect behavior and inaccuracies in processing, as the reserved/protected listing should not be available for further interactions unless it is liquidated from the ProtectedListing contract.

Impact

Incorrect usage of non-existent listing data leads to several issues, including incorrect tax accounting, incorrect reserve adjustments, and the misuse of outdated listing data in various critical functions.

Code Snippet

Listings::reserve()

File: Listings.sol
690:     function reserve(address _collection, uint _tokenId, uint _collateral) public nonReentrant lockerNotPaused {
---
704:         // Check if the listing is a floor item and process additional logic if there
705:         // was an owner (meaning it was not floor, so liquid or dutch).
706:         if (oldListing.owner != address(0)) {
707:             // We can process a tax refund for the existing listing if it isn't a liquidation
708:             if (!_isLiquidation[_collection][_tokenId]) {
709:                 (uint _fees,) = _resolveListingTax(oldListing, _collection, true);
710:                 if (_fees != 0) {
711:                     emit ListingFeeCaptured(_collection, _tokenId, _fees);
712:                 }
713:             }
714: 
715:             // If the floor multiple of the original listings is different, then this needs
716:             // to be paid to the original owner of the listing.
717:             uint listingFloorPrice = 1 ether * 10 ** collectionToken.denomination();
718:             if (listingPrice > listingFloorPrice) {
719:                 unchecked {
720:                     collectionToken.transferFrom(msg.sender, oldListing.owner, listingPrice - listingFloorPrice);
721:                 }
722:             }
723: 
724:             // Reduce the amount of listings
725:@>           unchecked { listingCount[_collection] -= 1; }
726:         }
---
750:         // Create our listing, receiving the ERC20 into this contract
751:@>       protectedListings.createListings(createProtectedListing);
---
759:     }

Tool used

Manual Review

Recommendation

Delete the listing after it is reserved and moved to the ProtectedListings contract. Additionally, for the reservation of liquidation listings, the liquidation status should be reset.

function reserve(address _collection, uint _tokenId, uint _collateral) public nonReentrant lockerNotPaused {
---
    if (oldListing.owner != address(0)) {
        // We can process a tax refund for the existing listing if it isn't a liquidation
        if (!_isLiquidation[_collection][_tokenId]) {
            (uint _fees,) = _resolveListingTax(oldListing, _collection, true);
            if (_fees != 0) {
                emit ListingFeeCaptured(_collection, _tokenId, _fees);
            }
-        }
+        } else {
+           delete _isLiquidation[_collection][_tokenId];
+        }

        // If the floor multiple of the original listings is different, then this needs
        // to be paid to the original owner of the listing.
        uint listingFloorPrice = 1 ether * 10 ** collectionToken.denomination();
        if (listingPrice > listingFloorPrice) {
            unchecked {
                collectionToken.transferFrom(msg.sender, oldListing.owner, listingPrice - listingFloorPrice);
            }
        }

        // Reduce the amount of listings
        unchecked { listingCount[_collection] -= 1; }

+       // Delete the token listing
+        delete _listings[_collection][_tokenId];
    }
---
}