zarkk01 - Anyone can redeem the ```canWithdrawAsset``` token by burning ```1``` ```CollectionToken``` while it is non-floor item leading to stealing it from lister of ```ProtectedListings```.

[sherlock-admin2]

zarkk01

High

Anyone can redeem the canWithdrawAsset token by burning 1 CollectionToken while it is non-floor item leading to stealing it from lister of ProtectedListings.

Summary

Malicious can redeem his 1 CollectionToken and steal the non-floor item of someone who had it listed as ProtectedListing and unlocked it without withdrawing it yet.

Root Cause

The check Locker::isListing() does not check if the token to be redeemed in Locker::redeem() is on the ProtectedListings::canWithdrawAsset category, but only if there is an active listing on it. We can see the Locker::redeem() here :

    function redeem(address _collection, uint[] calldata _tokenIds, address _recipient) public nonReentrant whenNotPaused collectionExists(_collection) {
        uint tokenIdsLength = _tokenIds.length;
        if (tokenIdsLength == 0) revert NoTokenIds();

        // Burn the ERC20 tokens from the caller
        ICollectionToken collectionToken_ = _collectionToken[_collection];
        collectionToken_.burnFrom(msg.sender, tokenIdsLength * 1 ether * 10 ** collectionToken_.denomination());

        // Define our collection token outside the loop
        IERC721 collection = IERC721(_collection);

        // Loop through the tokenIds and redeem them
        for (uint i; i < tokenIdsLength; ++i) {
            // Ensure that the token requested is not a listing
@>            if (isListing(_collection, _tokenIds[i])) revert TokenIsListing(_tokenIds[i]);

            // Transfer the collection token to the caller
            collection.transferFrom(address(this), _recipient, _tokenIds[i]);
        }

        emit TokenRedeem(_collection, _tokenIds, msg.sender, _recipient);
    }

Link to code

We can see the highlighted Locker::isListing check here :

    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)) {
            return true;
        }

        return false;
    }

Link to code

As we can see, it only checks if the token to be redeemed has an active listing on either Listing or ProtectedListing. However, this is not the only case where a token must not be redeemed by anyone. After the ProtectedListing, the lister has the option to let the token in the Locker and enable the canWithdrawAsset. We can see it here :

    function unlockProtectedListing(address _collection, uint _tokenId, bool _withdraw) public lockerNotPaused {
        // ...

        // Delete the listing objects
@>        delete _protectedListings[_collection][_tokenId];

        // Transfer the listing ERC721 back to the user
        if (_withdraw) {
            locker.withdrawToken(_collection, _tokenId, msg.sender);
            emit ListingAssetWithdraw(_collection, _tokenId);
        } else {
@>            canWithdrawAsset[_collection][_tokenId] = msg.sender;
        }
        // ...
    }

Link to code

As we can see, the related ProtectedListing is deleted and it is just saved in the canWithdrawMapping. However, this is not taken into consideration in the Locker::redeem(), so anyone can steal it by burning 1 CollectionToken.

Internal pre-conditions

1. User had created a ProtectedListing.
2. User unlocked his ProtectedListing and selected to not withdraw immediately his asset.

External pre-conditions

1. Attacker calling Locker::redeem() and steal the user's token by burning only 1 CollectionToken.

Attack Path

1. User list a token for ProtectedListing.
2. User unlock the token but doesn't withdraw it.
3. And, now, everyone can just redeem it and take it from the Locker.

Impact

The impact of this critical vulnerability is the theft of a token that worths > 1 CollectionToken and, nevertheless , it is not for sale. Also, this will totally messes up the accountings of NFTs <-> CollectionsTokens since only one collection token burned while it shouldn't be the case. At the end, someone can steal a token that the owner of it does not want to give it.

PoC

No PoC needed.

Mitigation

Ensure that Locker::isListing() check, also checks if the asset is in canWithdrawAsset mode from ProtectedListings, since technically it still is listing.