# Lister is overpaying during the cancel of his listing on ```Listings::cancelListings()```. Crit/High · Sherlock · NFT liquidity · 2nd September 2024 Finding H-23 of the Flayer competition. - Protocol: https://audits.sherlock.xyz/contests/468 - Codebase: https://github.com/0xsimao/2024-08-flayer/tree/0ec252cf9ef0f3470191dcf8318f6835f5ef688c - Source: https://github.com/sherlock-audit/2024-08-flayer-judging/issues/705 --- ### Summary Lister unfairly double pays the ```tax used``` while he is cancelling his ```listing``` through ```Listings::cancelListings()```. ### Root Cause When a user is creating his ```listing``` through ```Listings::createListings()```, he is paying upfront the tax that is expected to be used for the whole duration of the ```listing```. However, if he decides to cancel the listing after some time calling ```Listings::cancelListings()```, he will find himself paying again the portion of the tax that has been used until this point. Let's see the ```Listings::cancelListings()``` : ```solidity function cancelListings(address _collection, uint[] memory _tokenIds, bool _payTaxWithEscrow) public lockerNotPaused { uint fees; uint refund; for (uint i; i < _tokenIds.length; ++i) { uint _tokenId = _tokenIds[i]; // Read the listing in a single read Listing memory listing = _listings[_collection][_tokenId]; // Ensure the caller is the owner of the listing if (listing.owner != msg.sender) revert CallerIsNotOwner(listing.owner); // We cannot allow a dutch listing to be cancelled. This will also check that a liquid listing has not // expired, as it will instantly change to a dutch listing type. Enums.ListingType listingType = getListingType(listing); if (listingType != Enums.ListingType.LIQUID) revert CannotCancelListingType(); // Find the amount of prepaid tax from current timestamp to prepaid timestamp // and refund unused gas to the user. (uint _fees, uint _refund) = _resolveListingTax(listing, _collection, false); emit ListingFeeCaptured(_collection, _tokenId, _fees); fees += _fees; refund += _refund; // Delete the listing objects delete _listings[_collection][_tokenId]; // Transfer the listing ERC721 back to the user locker.withdrawToken(_collection, _tokenId, msg.sender); } // cache ICollectionToken collectionToken = locker.collectionToken(_collection); // Burn the ERC20 token that would have been given to the user when it was initially created @> uint requiredAmount = ((1 ether * _tokenIds.length) * 10 ** collectionToken.denomination()) - refund; @> payTaxWithEscrow(address(collectionToken), requiredAmount, _payTaxWithEscrow); collectionToken.burn(requiredAmount + refund); // ... } ``` [Link to code](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/Listings.sol#L414C1-L470C6) As we can see, user is expected to return back the whole ```1e18 - refund```. It helps to remember that when he created the listing he "took" ```1e18 - TAX``` where, now, ```TAX = refund + fees```. So the user is expected to give back to the protocol ```1e18 - refund``` while he got ```1e18 - refund - fees```. The difference of what he got at the start and what he is expected to return now : ```md whatHeGot - whatHeMustReturn = (1e18 - refund - fees) - (1e18 - refund) = -fees ``` So, now the user has to get out of his pocket and pay again for the ```fees``` while, technically, he has paid for them in the start **by not ever taking them**. Furthermore, in this way, as we can see from [this line](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/Listings.sol#L453), the protocol burns the whole ```1e18``` without considering the ```tax``` that **got actually used** and shouldn't be burned as it will be deposited to the ```UniswapV4Implementation``` : ```solidity collectionToken.burn(requiredAmount + refund); ``` ### Internal pre-conditions 1. User creates a ```listing``` from ```Listings::createListings()```. ### External pre-conditions 1. User wants to cancel his ```listing``` by ```Listings:cancelListings()```. ### Attack Path 1. User creates a ```listing``` from ```Listings::createListings()``` and takes back as ```collectionTokens``` -> ```1e18 - prepaidTax``` . 2. Some time passes by. 3. User wants to cancel the ```listing``` by calling ```Listings::cancelListings()``` and he has to give back ```1e18 - unusedTax```. This mean that he has to give also the ```usedTax``` amount. ### Impact The impact of this serious vulnerability is that the user is forced to double pay the tax that has been used for the duration that his ```listing``` was up. He, firstly, paid for it by not taking it and now, when he cancels the ```listing```, he has to pay it again out of his own pocket. This results to unfair **loss of funds** for whoever tries to cancel his ```listing```. ### PoC No PoC needed. ### Mitigation To mitigate this vulnerability successfully, consider not requiring user to return the ```fee``` variable as well : ```diff function cancelListings(address _collection, uint[] memory _tokenIds, bool _payTaxWithEscrow) public lockerNotPaused { uint fees; uint refund; for (uint i; i < _tokenIds.length; ++i) { // ... } // cache ICollectionToken collectionToken = locker.collectionToken(_collection); // Burn the ERC20 token that would have been given to the user when it was initially created - uint requiredAmount = ((1 ether * _tokenIds.length) * 10 ** collectionToken.denomination()) - refund; + uint requiredAmount = ((1 ether * _tokenIds.length) * 10 ** collectionToken.denomination()) - refund - fees; payTaxWithEscrow(address(collectionToken), requiredAmount, _payTaxWithEscrow); collectionToken.burn(requiredAmount + refund); // ... } ```