Closed code423n4 closed 2 years ago
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.
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
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.
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 than1
, 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 of1
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.
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 of2
. To fully fulfill that item we would need to provide 2 criteria resolvers. However, when looping through the criteria resolvers inCriteriaResolution
, the first resolver will change the item type in memory toERC721
(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 of1
, it does not take into consideration that such an item can have an amount other than1
.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 withInvalidERC721TransferAmount
.