Listing is not deleted from mapping in reserve function
Summary
Listing::reserve creates a Protected Listing from a regular Listing. Within its execution, it has a branch where it checks if a regular listing with this NFT already exists. And if it exists, the NFT is bought out from the current listing. (see code snippet)
However, in this branch, the listing is not deleted from the listings mapping. Therefore, for the contract, this listing still exists. This means that all functions applicable to a regular listing can be applied to it. Consequently, the previous owner of the listing can at least cancel it by calling the cancel function, thereby retrieving the NFT that no longer belongs to them.
Vulnerability Detail
When creating a Protected Listing, it is assumed that the NFT will be returned to the user upon unlockProtectedListing. Therefore, it is critically important to remove the listing from the listings mapping. Otherwise, a situation arises where records of different listings for the same NFT exist in different contracts Listings and ProtectedListings. This implies full freedom to interact with the NFT in both contracts, which is unacceptable, since there is only one true owner of the NFT - msg.sender of reserve, not oldListing.owner.
This is simply a fundamental error in the protocol's code, contradicting its logic, which has multiple ways to break the protocol. One of these is the following method.
oldListing.owner will call cancelListings in the Listings contract. Since for the contract, they are still considered the owner of the listing, they will successfully retrieve the NFT from the contract, even though it no longer belongs to them.
Impact
Severity: High
One of the possible impacts was described above. Also, the most minimal contradiction to the logic of the contract, which can be seen even without calling other functions, is that without removing the listing from the structure, the following happens
unchecked { listingCount[_collection] -= 1; }
Thus the number of listings does not correspond to the real number.
Cuddly Ocean Gibbon
High
Listing is not deleted from mapping in reserve function
Summary
Listing::reservecreates a Protected Listing from a regular Listing. Within its execution, it has a branch where it checks if a regular listing with this NFT already exists. And if it exists, the NFT is bought out from the current listing. (see code snippet)However, in this branch, the listing is not deleted from the listings mapping. Therefore, for the contract, this listing still exists. This means that all functions applicable to a regular listing can be applied to it. Consequently, the previous owner of the listing can at least cancel it by calling the cancel function, thereby retrieving the NFT that no longer belongs to them.
Vulnerability Detail
When creating a Protected Listing, it is assumed that the NFT will be returned to the user upon unlockProtectedListing. Therefore, it is critically important to remove the listing from the listings mapping. Otherwise, a situation arises where records of different listings for the same NFT exist in different contracts
ListingsandProtectedListings. This implies full freedom to interact with the NFT in both contracts, which is unacceptable, since there is only one true owner of the NFT -msg.sender of reserve, notoldListing.owner.This is simply a fundamental error in the protocol's code, contradicting its logic, which has multiple ways to break the protocol. One of these is the following method.
oldListing.ownerwill callcancelListingsin the Listings contract. Since for the contract, they are still considered the owner of the listing, they will successfully retrieve the NFT from the contract, even though it no longer belongs to them.Impact
Severity: High
One of the possible impacts was described above. Also, the most minimal contradiction to the logic of the contract, which can be seen even without calling other functions, is that without removing the listing from the structure, the following happens
unchecked { listingCount[_collection] -= 1; }Thus the number of listings does not correspond to the real number.
Code Snippet
https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/Listings.sol#L690-L759
Tool used
Manual Review
Recommendation
Delete Listing from listings mapping