Skip to content
Request an audit

‹ All findings

Lister is overpaying during the cancel of his listing on Listings::cancelListings().

Crit/HighFlayer·Sherlock · NFT liquidity · 2nd September 2024CodebaseH-23

Source

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

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, 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);

        // ...
    }