sherlock-audit / 2024-08-flayer-judging

2 stars 0 forks source link

almantare - ProtectedListings::withdrawProtectedListing functionality can be broken due to incorrect validation when redeeming NFT from locker #765

Open sherlock-admin4 opened 1 month ago

sherlock-admin4 commented 1 month ago

almantare

High

ProtectedListings::withdrawProtectedListing functionality can be broken due to incorrect validation when redeeming NFT from locker

Summary

When protectedListing.owner calls unlockProtectedListing with bool _withdraw = false - they receive permission to withdraw the NFT in a separate transaction at any time they want (see code snippet below).

It's worth noting that NFTs that interact with the protocol are stored in the Locker. However, the functions for withdrawing NFTs from the Locker do not have sufficient validation to prevent a user from withdrawing an NFT that is intended for withdrawal in withdrawProtectedListing ⇒ the withdrawProtectedListing functionality may have several undesirable usage scenarios.

  1. DOS, as their NFT can be withdrawn by another person through the functions Locker::redeem Locker::swap Locker::swapBatch
  2. In case their NFT has already been withdrawn from the contract and then put up for sale, they can withdraw the NFT themselves, thereby organizing a DoS of the Listings and Protected Listings functionality, breaking one of the main invariants of the protocol: an NFT put up for sale should be in the Locker.

Vulnerability Detail

Let's show that the functions in Locker intended for withdrawing NFTs indeed do not have sufficient validation.

Let's consider Locker::redeem (The batch and swapBatch functions have an identical structure)

From the code (see below in the code snippet), it's clear that the only validation for withdrawal availability is the isListing check

The isListing function only checks cases where the NFT is listed for a Listing, but it doesn't consider the case where the NFT is locked for withdrawal in a Protected Listing.

Impact

The root cause is this: the protocol allows withdrawing NFTs that are intended for withdrawal of unlocked ProtectedListings. This alone breaks the functionality of withdrawProtectedListing, but this error can also be used by attackers to violate other invariants of the protocol, let's consider the most significant case:

  1. An Unlocked User NFT was accidentally withdrawn using the redeem function not by the user (B) who should have done it in withdrawProtectedListing. Let's say by user A (the one who withdrew)
  2. Then user A puts the NFT up for sale.
  3. Afterwards, user B can use the previously obtained opportunity to withdraw and withdraw the NFT that is already recorded as a listing. To do this, they only need to call the withdrawProtectedListing function. It calls locker.withdrawToken which does not perform any checks during withdrawal.

Severity: High

Code Snippet

https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/ProtectedListings.sol#L287-L329

https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/ProtectedListings.sol#L341-L352

https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/Locker.sol#L209-L230

https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/Locker.sol#L438-L452

Tool used

Manual Review

Recommendation

Fix nft withdraw validation in locker