Vast Umber Walrus - Failure to account for delayed withdrawals in listing checks leads to incorrect listing validation and asset loss

[sherlock-admin2]

Vast Umber Walrus

Medium

Failure to account for delayed withdrawals in listing checks leads to incorrect listing validation and asset loss

Summary

The Lockers::isListing() and CollectionShutdown::_hasListings() fail to account for listings that have been unlocked with a delayed withdrawal. As a result, the system incorrectly validates and processes listings that are no longer protected, exposing users to the risk of losing assets.

Vulnerability Detail

When a protected listing is unlocked without immediate withdrawal using ProtectedListings::unlockProtectedListing(address _collection, uint _tokenId, bool _withdraw: FALSE), the canWithdrawAsset\[_collection\]\[_tokenId\] is updated, and the listing is deleted with the listing count adjusted.

However, the checks within Lockers::isListing() (which checks the listing info for the specified token IDs) and CollectionShutdown::_hasListings() (which checks the listingCount) do not account for this case.

As a result, when the function tries to determine if the token is still listed, it incorrectly passes, allowing the processing of a listing that is no longer protected.

Consequently, users who attempt to unlock an asset from a protected listing without triggering an immediate withdrawal risk losing their asset.

Impact

This vulnerability exposes users to the risk of losing assets when attempting to unlock a protected listing without triggering an immediate withdrawal, as other processes can fulfill the listing due to incorrect checks.

The processes that use this checks are listed below:

• Lockers::redeem()
• Lockers::swap()
• Lockers::swapBatch()
• CollectionShutdown::execute()

Code Snippet

ProtectedListings::unlockProtectedListing()

File: ProtectedListings.sol
287:     function unlockProtectedListing(address _collection, uint _tokenId, bool _withdraw) public lockerNotPaused {
---
310:         // Remove our listing type
311:@>       unchecked { --listingCount[_collection]; }
312: 
313:         // Delete the listing objects
314:@>       delete _protectedListings[_collection][_tokenId];
315: 
316:         // Transfer the listing ERC721 back to the user
317:         if (_withdraw) {
318:             locker.withdrawToken(_collection, _tokenId, msg.sender);
319:             emit ListingAssetWithdraw(_collection, _tokenId);
320:         } else {
321:@>           canWithdrawAsset[_collection][_tokenId] = msg.sender;
322:         }
---
324:         // Update our checkpoint to reflect that listings have been removed
325:         _createCheckpoint(_collection);
326: 
327:         // Emit an event
328:         emit ListingUnlocked(_collection, _tokenId, fee);
329:     }

Locker::isListing()

File: Locker.sol
438:     function isListing(address _collection, uint _tokenId) public view returns (bool) {
439:         IListings _listings = listings;
440: 
441:         // Check if we have a liquid or dutch listing
442:         if (_listings.listings(_collection, _tokenId).owner != address(0)) {
443:             return true;
444:         }
445: 
446:         // Check if we have a protected listing
447:         if (_listings.protectedListings().listings(_collection, _tokenId).owner != address(0)) {
448:             return true;
449:         }
450: 
451:         return false;
452:     }

CollectionShutdown::_hasListings()

File: CollectionShutdown.sol
497:     function _hasListings(address _collection) internal view returns (bool) {
498:         IListings listings = locker.listings();
499:         if (address(listings) != address(0)) {
500:             if (listings.listingCount(_collection) != 0) {
501:                 return true;
502:             }
503: 
504:             // Check that no protected listings currently exist
505:             IProtectedListings protectedListings = listings.protectedListings();
506:             if (address(protectedListings) != address(0)) {
507:                 if (protectedListings.listingCount(_collection) != 0) {
508:                     return true;
509:                 }
510:             }
511:         }
512: 
513:         return false;
514:     }

Tool used

Manual Review

Recommendation

• For Lockers::isListing(): Update the function to include a check against the canWithdrawAsset[_collection][_tokenId] state.

function isListing(address _collection, uint _tokenId) public view returns (bool) {
    IListings _listings = listings;

    // Check if we have a liquid or dutch listing
    if (_listings.listings(_collection, _tokenId).owner != address(0)) {
        return true;
    }

    // Check if we have a protected listing
+    if (_listings.protectedListings().listings(_collection, _tokenId).owner != address(0) || _listings.protectedListings().canWithdrawAsset(_collection, _tokenId)) {
        return true;
    }

    return false;
}

• For CollectionShutdown::_hasListings(): Since this function is used in high-level execution roles (CollectionShutdown::execute()), a potential mitigation could involve applying a trust assumption. Specifically, avoid adding listings with pending withdrawals to the execution shutdown list to ensure that only active and protected listings are processed.