almantare - Invariant of FLOOR_MULTIPLE < 10 can be broken whether by negligence or on purpose

[sherlock-admin4]

almantare

Medium

Invariant of FLOOR_MULTIPLE < 10 can be broken whether by negligence or on purpose

Summary

This error allows breaking the protocol invariant that the commission paid by the user for a Listing is > 1 collection token. This invariant is not allowed in the Listings::createListings function, as a commission > 1 token will not pass due to a revert on this [line](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/Listings.sol#L151). However, when a user creates/modifies a listing not using the Listings::createListings function, but for example Listings::modifyListings, Listings::relist - there the commission for creating a collection can be paid using _payTaxWithEscrow, which will deduct the commission amount from the user's balance in TokenEscrow. In this deduction, there is no check that the commission being paid by the user does not exceed 1.

This error falls under the category from the Contest Readme.

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

Response

Responses such as expected fees and tax calculations should be correct for external protocols to utilise. It is also important that each NFT has a correct status. Having tokens that aren't held by Flayer listed as for sale, protected listings missing, etc. would be detrimental.

I will also provide a dialogue from the Private Thread

My Question

Hey, I have another question about invariants
Is it important invariant that user cannot create listing with floor multiple more than ~ 700
Because I find the way how user can create floor multiple as much as tx size he can pay

Team Answer

Oh really? I think we had a hard cap at 10x. I think the issue would be if the user had to pay more tax than the 1 token returned, it could lead to some really weird protocol interactionSo I'd say that should be considered a bug, though I'm unsure the extent of it

My answer

Yes, the thing is that you can't do it directly via create listing, but if you use modifyListings or relist function and specify _payTaxWithEscrow as true - then there is no check if user is less than 1 token in escrow payment.

Do you think this could be seen as broken protocol behaviour / invariant ?

Team answer

I would say that if the user can set a nutty floor multiple above the max we would want to set then yes this would be a broken behaviour

Vulnerability Detail

When calling functions that do not create a listing directly, but overwrite/modify an already created one, you can specify the bool parameter payTaxWithEscrow - which will pay the commission for you from your TokenEscrow balance. This payment occurs in the <i>Listings::payTaxWithEscrow</i> function and has no checks that the commission paid by the user will not be greater than 1. This means either the user will knowingly lose funds if they mistakenly set a large FloorMultiple (when creating a listing in CreateListings, any FloorMultiple > ~1000 will result in the commission being > 1 token and the transaction will revert), or intentionally set a high FloorMultiple and pay the commission from Escrow, aiming to break the protocol invariant.

Impact

This issue breaks one of the invariants and expected behaviors of the protocol.

Severity: Medium

Code Snippet

Tool used

Manual Review

Recommendation

Add check for listing_tx size in <i>Listings::payTaxWithEscrow</i>