Malicious users could force swap nft owned by other users in locker
Summary
In Locker::swap() , there is missing a check if targeted nft is already owned by someone else. In the case when users reserve nfts and obtain them through unlocking, but not yet withdraw, these nfts will be still in the locker . In this scenario, we should not allow swapping to happen.
Root Cause
In ProtectedListings::unlockProtectedListing, when unlocking a protected listing, the listing struct will be deleted
In Locker::swap(), before swapping, it only checks if the nfts is a listing by checking the owner field in the listing struct; however, because the listing struct is being removed earlier when unlocking, now these nfts are considered tradeable. This is unfair for users who use the reserving feature , since they may pay alot to get these nfts (Filling listing price + Floor price to unlock the protected listing)
Consider this scenario:
User A really like an nft X, which is priced at 2 ETH worth of collection token, so he reserve it; he pays 1ETH worth of collection token and creates a protected listing to pay the rest (floor price) later.
After fully paying, now User A can unlock it but decides to keep the nft in the locker.
Attacker B sees this opportunity; they will try to take the nft owned by user A by swapping it with a floor nft and profit.
Internal pre-conditions
Users use reserve() to create a protected listing for their desired nfts.
Users unlock protected listing without withdrawing these nfts.
External pre-conditions
N/A
Attack Path
Malicious users call Locker::swap() contract to swap the nfts owned by other users but not yet being withdrawn from the locker
Impact
Malicious users could steal valuable nfts by swapping them with floor nfts.
Polite Macaroon Parakeet
High
Malicious users could force swap nft owned by other users in locker
Summary
In
Locker::swap(), there is missing a check if targeted nft is already owned by someone else. In the case when users reserve nfts and obtain them through unlocking, but not yet withdraw, these nfts will be still in the locker . In this scenario, we should not allow swapping to happen.Root Cause
ProtectedListings::unlockProtectedListing, when unlocking a protected listing, the listing struct will be deletedLocker::swap(), before swapping, it only checks if the nfts is a listing by checking the owner field in the listing struct; however, because the listing struct is being removed earlier when unlocking, now these nfts are considered tradeable. This is unfair for users who use the reserving feature , since they may pay alot to get these nfts (Filling listing price + Floor price to unlock the protected listing)Consider this scenario:
Internal pre-conditions
External pre-conditions
N/A
Attack Path
Locker::swap()contract to swap the nfts owned by other users but not yet being withdrawn from the lockerImpact
Malicious users could steal valuable nfts by swapping them with floor nfts.
Code snippet
https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/ProtectedListings.sol#L314-L322
https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/Locker.sol#L246
PoC
No response
Mitigation
An easy way is to remove the option to keep the nft in the locker after unlocking it.