sherlock-audit / 2024-08-flayer-judging

0 stars 0 forks source link

Vast Umber Walrus - Failure to update global state when relisting floor items #734

Open sherlock-admin3 opened 4 days ago

sherlock-admin3 commented 4 days ago

Vast Umber Walrus

Medium

Failure to update global state when relisting floor items

Summary

The Listings::relist() allows users to relist items that are unlisted but deposited in the Locker at the floor price, effectively treating them as new listings. However, this process does not correctly update global state variables, such as listingCount, nor does it create necessary checkpoints via createCheckpoints().

Vulnerability Detail

The Listings::relist() function fails to update critical global state variables, such as listingCount, and does not create checkpoints using createCheckpoints() when relisting floor items deposited into the Locker but not currently listed in the Listing contract.

This issue arises under the assumption that it is intentional design to support such cases, as demonstrated in the test cases: test_CanRelistFloorItemAsLiquidListing() and test_CanRelistFloorItemAsDutchListing().

Impact

Inaccurate listing counts arise because the listingCount remains outdated, affecting how listings are managed and reported.

Additionally, the lack of checkpoints created when the listing amounts are changed results in missing checkpoints, especially when the utilization rate has changed.

Code Snippet

Listings::relist()

File: Listings.sol
625:     function relist(CreateListing calldata _listing, bool _payTaxWithEscrow) public nonReentrant lockerNotPaused {
626:         // Load our tokenId
627:         address _collection = _listing.collection;
628:         uint _tokenId = _listing.tokenIds[0];
629: 
630:         // Read the existing listing in a single read
631:         Listing memory oldListing = _listings[_collection][_tokenId];
632: 
633:         // Ensure the caller is not the owner of the listing
634:         if (oldListing.owner == msg.sender) revert CallerIsAlreadyOwner(); //@audit allow oldListing.owner == address(0) (not Listed)
---
661:         // Validate our new listing
662:         _validateCreateListing(_listing);
663: 
664:         // Store our listing into our Listing mappings
665:         _listings[_collection][_tokenId] = listing;
666: 
667:         // Pay our required taxes
668:         payTaxWithEscrow(address(collectionToken), getListingTaxRequired(listing, _collection), _payTaxWithEscrow);
669: 
670:         // Emit events
671:         emit ListingRelisted(_collection, _tokenId, listing);
672:     }

Tool used

Manual Review

Recommendation

Updates global state variables such as listingCount and creates necessary checkpoints via createCheckpoints() when relisting floor items.

function relist(CreateListing calldata _listing, bool _payTaxWithEscrow) public nonReentrant lockerNotPaused {
---
    payTaxWithEscrow(address(collectionToken), getListingTaxRequired(listing, _collection), _payTaxWithEscrow);

+    if(oldListing.owner == address(0)) {
+        // Increment our listings count
+        unchecked { listingCount[_collection] += 1; }
+
+        // Create our checkpoint as utilisation rates will change
+        protectedListings.createCheckpoint(_collection);
+    }

    // Emit events
    emit ListingRelisted(_collection, _tokenId, listing);
}