code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/AmountDeriver.sol#L57

Vulnerability details

Impact

Existing orders with moving prices (startAmount and endAmount) can match via matchAdvancedOrders. However the expression (startAmount * remaining) + (endAmount * elapsed) + extraCeiling in AmountDeriver.sol#L57 makes it very unlikely that totalBeforeDivision is divisible by duration when elapsed > 0, which is at any timestamp > startTime (basically all the time except the first tx). When two orders match, each side is considered the offerer when computing the offer and consideration amounts, which favors offerers by:

rounding down the offer (offerer has to give less) OrderCombiner.sol#L281
rounding up the consideration (offerer receives more) OrderCombiner.sol#L331

However, because the protocol tries to favor both, orders that could match do not match anymore due to the rounding above. Example:

Alice is selling ERC20 at a moving price [10, 20) for 10 ERC1155.
Bob is selling 10 ERC1155 at a moving price [10, 20) ERC20.

This looks like it should just match, and it does at startTime. However, at timestamp startTime + 1, the current sell price for Alice's ERC20 is 10, and the current buy price for the ERC20 that Bob wants is 11. This happens even though the moving prices are the same, and the orders' startTime and endTime are the same. For different cases, the problem is bigger and makes it almost impossible to match orders that use moving prices.

Opensea documentation mentions that the way an order can be matched is by creating a mirror image of an existing order. However, this technique becomes nearly impossible to match for moving price due to rounding issues. This is why the issue has a severity of Medium.

Proof of Concept

The patch below when used with forge test -m "testMatchOrdersMovingPrice" -vvvv reproduces the example above:

@@ -931,4 +1029,152 @@ contract MatchAdvancedOrder is BaseOrderTest {
             currentAmount.mul(2) / 10
         );
     }
+
+       function testMatchOrdersMovingPrice() public {
+           FuzzInputsAscendingDescending memory args = FuzzInputsAscendingDescending(
+               address(0),
+               uint(0),
+               bytes32(uint(0)),
+               uint(0),
+               uint128(10),
+               uint128(20),
+               uint120(1),
+               uint120(1),
+               false,
+               uint(10)
+           );
+
+           ContextAscendingDescending memory context = ContextAscendingDescending(consideration, args);
+
+           context.args.baseStart = 10;
+           context.args.baseEnd = 20;
+
+           bytes32 conduitKey = context.args.useConduit
+               ? conduitKeyOne
+               : bytes32(0);
+
+               test1155_1.mint(bob, context.args.id, 20);
+               token1.mint(
+                   alice,
+                   context.args.baseEnd > context.args.baseStart
+                       ? context.args.baseEnd
+                       : context.args.baseStart
+               );
+
+               _configureOfferItem(
+                   ItemType.ERC20,
+                   0,
+                   context.args.baseStart,
+                   context.args.baseEnd
+               );
+               _configureConsiderationItem(
+                   alice,
+                   ItemType.ERC1155,
+                   context.args.id,
+                   20
+               );
+
+               OrderParameters memory orderParameters = OrderParameters(
+                   address(alice),
+                   context.args.zone,
+                   offerItems,
+                   considerationItems,
+                   OrderType.PARTIAL_OPEN,
+                   block.timestamp,
+                   block.timestamp + 10 + 1,
+                   context.args.zoneHash,
+                   context.args.salt,
+                   conduitKey,
+                   considerationItems.length
+               );
+
+               OrderComponents memory orderComponents = getOrderComponents(
+                   orderParameters,
+                   context.consideration.getNonce(alice)
+               );
+
+               bytes memory signature = signOrder(
+                   context.consideration,
+                   alicePk,
+                   context.consideration.getOrderHash(orderComponents)
+               );
+
+               delete offerItems;
+               delete considerationItems;
+
+               _configureERC1155OfferItem(context.args.id, 20);
+               // create mirror consideration item with current amount
+               _configureConsiderationItem(
+                   ItemType.ERC20,
+                   address(token1),
+                   0,
+                   10,
+                   20,
+                   bob
+               );
+
+               OrderParameters memory mirrorOrderParameters = OrderParameters(
+                   address(bob),
+                   context.args.zone,
+                   offerItems,
+                   considerationItems,
+                   OrderType.PARTIAL_OPEN,
+                   block.timestamp,
+                   block.timestamp + 10 + 1,
+                   context.args.zoneHash,
+                   context.args.salt,
+                   conduitKey,
+                   considerationItems.length
+               );
+               OrderComponents memory mirrorOrderComponents = getOrderComponents(
+                   mirrorOrderParameters,
+                   context.consideration.getNonce(bob)
+               );
+
+               bytes memory mirrorSignature = signOrder(
+                   context.consideration,
+                   bobPk,
+                   context.consideration.getOrderHash(mirrorOrderComponents)
+               );
+
+               AdvancedOrder[] memory orders = new AdvancedOrder[](2);
+               orders[0] = AdvancedOrder(orderParameters, 1, 1, signature, "0x");
+               orders[1] = AdvancedOrder(
+                   mirrorOrderParameters,
+                   1,
+                   1,
+                   mirrorSignature,
+                   "0x"
+               );
+
+               fulfillmentComponent = FulfillmentComponent(0, 0);
+               fulfillmentComponents.push(fulfillmentComponent);
+               fulfillment.offerComponents = fulfillmentComponents;
+               delete fulfillmentComponents;
+               fulfillmentComponent = FulfillmentComponent(1, 0);
+               fulfillmentComponents.push(fulfillmentComponent);
+               fulfillment.considerationComponents = fulfillmentComponents;
+               fulfillments.push(fulfillment);
+               delete fulfillmentComponents;
+               delete fulfillment;
+
+               fulfillmentComponent = FulfillmentComponent(1, 0);
+               fulfillmentComponents.push(fulfillmentComponent);
+               fulfillment.offerComponents = fulfillmentComponents;
+               delete fulfillmentComponents;
+               fulfillmentComponent = FulfillmentComponent(0, 0);
+               fulfillmentComponents.push(fulfillmentComponent);
+               fulfillment.considerationComponents = fulfillmentComponents;
+               fulfillments.push(fulfillment);
+               delete fulfillmentComponents;
+               delete fulfillment;
+
+               vm.warp(block.timestamp + 1);
+
+               context.consideration.matchAdvancedOrders(
+                   orders,
+                   new CriteriaResolver[](0),
+                   fulfillments
+               );
+       }

Tools Used

Manual review

Recommended Mitigation Steps

Potentially reconsider the usability of moving prices.

0age commented 2 years ago

Orders with ascending/descending amount items can be easily mirrored using any method other than matchOrders; and even when using matchOrders they can still be fulfilled by simply adding 1 to the mirrored startAmount and endAmount for offer items fulfilling an ascending/descending consideration item, or subtracting 1 from the mirrored startAmount and endAmount for consideration items fulfilling an ascending/descending offer item.

0xleastwood commented 2 years ago

As the sponsor has noted, orders can be mirrored using any other method, however, to utilise matchOrders(), adding -1/+1 seems to be a plausible work around. Considering the outlined issue has no direct security risk, I think it would be fair to downgrade this to QA and merge with #203

code-423n4 / 2022-05-opensea-seaport-findings