Restrict eth offer items #464
Conversation
Add tests for InvalidNativeOfferItem and modify existing tests with an eth offer item
|
Working on getting coverage back to 100% |
|
MagicModulus = 69 ftw 🔥 |
| @@ -260,6 +264,10 @@ contract ReferenceOrderCombiner is | |||
| // Retrieve the offer item. | |||
| OfferItem memory offerItem = offer[j]; | |||
|
|
|||
| anyNativeOfferItems = | |||
| anyNativeOfferItems || | |||
There was a problem hiding this comment.
This line can be removed since it hasn't been assigned before anyway.
There was a problem hiding this comment.
I don't think it can be removed. It's in a loop, you'd overwrite the result every iteration - only the last iteration would be deciding. You want to check if any of the offer items is native.
There was a problem hiding this comment.
oh yea my bad, didn't notice the loop in the GH diff
| @@ -348,6 +356,10 @@ contract ReferenceOrderCombiner is | |||
| } | |||
| } | |||
|
|
|||
| if (anyNativeOfferItems && nonMatchFn) { | |||
There was a problem hiding this comment.
Would recommend moving the assignment of nonMatchFn here.
| gt( | ||
| // Take the remainder of the selector modulo a magic value. | ||
| mod( | ||
| shr(NumBitsAfterSelector, calldataload(0)), |
There was a problem hiding this comment.
Is this always true? Wouldn't it be safer to just shift by 28 bytes and mask?
| // of the two match selectors modulo the magic value. | ||
| NonMatchSelector_MagicRemainder | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The external functions that reach this point are:
fb4c2af9: fulfillAvailableAdvancedOrders(((address,address,(uint8,address,uint256,uint256,uint256)[],(uint8,address,uint256,uint256,uint256,address)
[],uint8,uint256,uint256,bytes32,uint256,bytes32,uint256),uint120,uint120,bytes,bytes)[],(uint256,uint8,uint256,uint256,bytes32[])[],(uint256,uint256)[][],(uint256,uint256)[][],bytes32,uint256)
ed98a574: fulfillAvailableOrders(((address,address,(uint8,address,uint256,uint256,uint256)[],(uint8,address,uint256,uint256,uint256,address)[],uint8,uint256,uint256,bytes32,uint256,bytes32,uint256),bytes)[],(uint256,uint256)[][],(uint256,uint256)[][],bytes32,uint256)
55944a42: matchAdvancedOrders(((address,address,(uint8,address,uint256,uint256,uint256)[],(uint8,address,uint256,uint256,uint256,address)[],uint8,uint256,uint256,bytes32,uint256,bytes32,uint256),uint120,uint120,bytes,bytes)[],(uint256,uint8,uint256,uint256,bytes32[])[],((uint256,uint256)[],(uint256,uint256)[])[])
a8174404: matchOrders(((address,address,(uint8,address,uint256,uint256,uint256)[],(uint8,address,uint256,uint256,uint256,address)[],uint8,uint256,uint256,bytes32,uint256,bytes32,uint256),bytes)[],((uint256,uint256)[],(uint256,uint256)[])[])
The sigs mod 69 are:
39, 52, 28, 29 (0x1d)
so this works
There was a problem hiding this comment.
(just a mental note: 0b10 if fulfill*, 0b00 otherwise so far after execution)
| @@ -359,6 +390,13 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { | |||
| } | |||
| } | |||
|
|
|||
| // If the second bit is set in the error buffer, we are not in a match function. | |||
| // If the first bit is set, a native offer item was encountered. | |||
| // If the value is greater than two, both the first and second bits were set. | |||
There was a problem hiding this comment.
This comment is a bit misleading since it's true here because > 3 is not possible but the statement isn't true in general.
There was a problem hiding this comment.
For reference, this has been fixed in main. comment now states:
// If the first bit is set, a native offer item was encountered. If the
// second bit is set in the error buffer, the current function is not
// matchOrders or matchAdvancedOrders. If the value is three, both the
// first and second bits were set; in that case, revert with an error.
| @@ -194,6 +194,10 @@ contract ReferenceOrderCombiner is | |||
| // Track the order hash for each order being fulfilled. | |||
| bytes32[] memory orderHashes = new bytes32[](totalOrders); | |||
|
|
|||
| // Check if we are in a match function | |||
| bool nonMatchFn = msg.sig != 0x55944a42 && msg.sig != 0xa8174404; | |||
There was a problem hiding this comment.
could use SeaportInterface.matchOrders/matchAdvancedOrders.selector?
Disallow ETH offer items except in matchOrders and matchAdvancedOrders.