ERC721_WITH_CRITERIA items with an endAmount greater than 1 are problematic

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/CriteriaResolution.sol#L149-L152
https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/CriteriaResolution.sol#L136-L141

Vulnerability details

Impact

If an ERC721_WITH_CRITERIA item has an amount (endAmount) greater than
1, its order cannot be fulfilled using a single fulfillment call.

In the case that such an order is a full order (as opposed to partial order),
it is not possible to fulfill it at all. So, this requires having
multiple ERC721_WITH_CRITERIA items with the same token address and identifier.

In the case that such an order is a partial order, only a partial fulfillment which
makes all ERC721_WITH_CRITERIA items have an amount of 1 can be executed.
So if an item has 10 NFT item A, only 1/10th of the order
can be filled in a single fulfillment call. If an order has 10 NFT item A and 20 NFT item B,
that order cannot be fulfilled at all. It requires the order to be recreated
to instead have 10 NFT item A, 10 NFT item B, and 10 NFT item B. Even though this
order is fillable now, it is not possible to fulfill 2/10th of the order in
a single fulfillment call.

The following example given in the README file is also not fulfillable.

A straightforward heuristic is to start with a "unit" bundle (e.g. 1 NFT item
A, 3 NFT item B, and 5 NFT item C for 2 ETH) then applying a multiple to that
unit bundle (e.g. 7 of those units results in a partial order for 7 NFT item
A, 21 NFT item B, and 35 NFT item C for 14 ETH).

This limitation hurts the composability of the protocol, decreases
efficiency, and requires extra care when creating orders to ensure that they
are fulfillable.

Proof of Concept

Let’s say there is an ERC721_WITH_CRITERIA item with an amount of 2.
To fully fulfill that item we would need to provide 2 criteria resolvers.
However, when looping through the criteria resolvers in CriteriaResolution, the first resolver
will change the item type in memory to ERC721 (without criteria).
In the second pass with the second criteria resolver, the transaction
will throw because the function
does not let applying criteria resolver to an item type without criteria.

Although this is appropriate behaviour for ERC721_WITH_CRITERIA items
with amount of 1, it does not take into consideration that such
an item can have an amount other than 1.

Tools Used

Manual review

Recommended Mitigation Steps

To resolve this limitation, new code needs to be introduced at least to CriteriaResolution.
The item amount in memory should also be decremented to 1 if sufficient resolvers are provided,
so that the Executor will not throw with InvalidERC721TransferAmount.

[0age]

This is a mild limitation of the protocol in its current form, but also has workarounds (you can use matchOrders and give the same order multiple times, for instance). It should be documented but does not put users at risk.

[HardlyDifficult]

This limitation is an inefficiency, but does not prevent usage. As the warden noted "1/10th of the order can be filled in a single fulfillment call" which suggests that it could be filled with multiple calls as the sponsor had noted.

Lowering to a Low severity and grouping with the warden's QA report #113

[Shungy]

I just want to add here that although one of the examples I gave had a workaround, the other example has none.

So if an order has 10 of NFT A, it can be fulfilled with multiple matchOrders. But if an order has 10 NFT A and 20 NFT B, it can never be fullfilled, because partial filling a tenth of the order would require 2 NFT B to be transferred out, which would throw during criteria resolution.