should FractionData'snumerator and denominator be uint120 as it's in other places?
Reference: Severe bug where one can aggregate items that are not the same. The _checkMatchingConsideration function does not revert on false, instead the result is assigned to the potentialCandidate.invalidFulfillment field. But this field can be overwritten with a true value by aggregating another item that matches in the next loop iteration. This allows trading the offerer's items for useless items by falsely aggregating high-value consideration items. Their amount is set to zero and one does not have to transfer these consideration items. The bug is not present in the YUL version where the _checkMatchingConsiderationalways reverts on false.
Reference: Wrong comment in ReferenceOrderFulfiller.sol: Should be "Create Received Item from OfferConsideration item."
_cancel and _validate always return true. It's misleading as returning a boolean indicates that this function can also return false on a failed cancelation/validation (instead of reverting). The return value is useless here as it's always set to true. Consider removing the return value to make the semantics of these functions more clear and save gas.
_assertValidSignature only verifies that v == 27 || v == 28 in the signature.length == 65 case, but not in the signature.length == 64 case. This is inconsistent behavior. For reference, the OZ implementation checks v in both cases. In practice, this does not seem to be an issue as ecrecover should revert anyway on invalid v. However, the BadSignature error in Seaport is thrown if v == 27 || v == 28 only in one of the two cases. Consider adding the same check to the signature.length == 64 case.
The _performERC721Transfer function performs a transferFrom call instead of a safeTransferFrom call. It could be that to is a receiver contract which does not accept ERC721 tokens signaled by not implementing onERC721Received as demanded by the spec. The tokens can get stuck in these contracts. This is generally not a problem if msg.sender is the to receiver or when the offerer chose their offer items but it can happen as arbitrary ERC721 transfers can be added as additional consideration items.
HardlyDifficult
commented
2 years ago
QA
FractionData'snumeratoranddenominatorbeuint120as it's in other places?_checkMatchingConsiderationfunction does not revert onfalse, instead the result is assigned to thepotentialCandidate.invalidFulfillmentfield. But this field can be overwritten with atruevalue by aggregating another item that matches in the next loop iteration. This allows trading the offerer's items for useless items by falsely aggregating high-value consideration items. Their amount is set to zero and one does not have to transfer these consideration items. The bug is not present in the YUL version where the_checkMatchingConsiderationalways reverts onfalse.ReferenceOrderFulfiller.sol: Should be "Create Received Item fromOfferConsideration item."_canceland_validatealways returntrue. It's misleading as returning abooleanindicates that this function can also returnfalseon a failed cancelation/validation (instead of reverting). The return value is useless here as it's always set totrue. Consider removing the return value to make the semantics of these functions more clear and save gas._assertValidSignatureonly verifies thatv == 27 || v == 28in thesignature.length == 65case, but not in thesignature.length == 64case. This is inconsistent behavior. For reference, the OZ implementation checksvin both cases. In practice, this does not seem to be an issue asecrecovershould revert anyway on invalidv. However, theBadSignatureerror in Seaport is thrown ifv == 27 || v == 28only in one of the two cases. Consider adding the same check to thesignature.length == 64case._performERC721Transferfunction performs atransferFromcall instead of asafeTransferFromcall. It could be thattois a receiver contract which does not accept ERC721 tokens signaled by not implementingonERC721Receivedas demanded by the spec. The tokens can get stuck in these contracts. This is generally not a problem ifmsg.senderis thetoreceiver or when the offerer chose their offer items but it can happen as arbitrary ERC721 transfers can be added as additional consideration items.