Blur Exchange contest - aviggiano's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L33 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L59

Vulnerability details

Impact

The functions StandardPolicyERC1155:canMatchMakerAsk and StandardPolicyERC1155:canMatchMakerBid incorrectly return 1 as the order amount, which makes executeOrder transfer only one ERC1155 token from the seller to the buyer, while the buyer pays the full price of the order.

Proof of Concept

The test scenario below does not pass. The seller creates an order of 3 NFTs ERC1155 and the buyer executes that order with a matching order. Instead of receiving 3 NFTs, the buyer receives only 1 NFT.

diff --git a/tests/execution.test.ts b/tests/execution.test.ts
index 7b115b7..3b06a1b 100644
--- a/tests/execution.test.ts
+++ b/tests/execution.test.ts
@@ -127,6 +127,37 @@ export function runExecuteTests(setupTest: any) {
         feeRecipientBalanceWeth.add(fee),
       );
     });
+    it('should transfer order amount for ERC1155 tokens', async () => {
+      await mockERC1155.mint(alice.address, tokenId, 3);
+      sell = generateOrder(alice, {
+        side: Side.Sell,
+        tokenId,
+        amount: 3,
+        collection: mockERC1155.address,
+        matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
+      });
+      buy = generateOrder(bob, {
+        side: Side.Buy,
+        tokenId,
+        amount: 3,
+        collection: mockERC1155.address,
+        matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
+      });
+      sellInput = await sell.pack();
+      buyInput = await buy.pack();
+
+      await waitForTx(exchange.execute(sellInput, buyInput));
+
+      expect(await mockERC1155.balanceOf(bob.address, tokenId)).to.be.equal(3);
+      await checkBalances(
+        aliceBalance,
+        aliceBalanceWeth.add(priceMinusFee),
+        bobBalance,
+        bobBalanceWeth.sub(price),
+        feeRecipientBalance,
+        feeRecipientBalanceWeth.add(fee)
+      );
+    });
     it('should revert with ERC20 not WETH', async () => {
       sell.parameters.paymentToken = mockERC721.address;
       buy.parameters.paymentToken = mockERC721.address;

Tools Used

Hardhat

Recommended Mitigation Steps

Validate that both maker and taker order amounts are equal and return its value instead of 1

diff --git a/contracts/matchingPolicies/StandardPolicyERC1155.sol b/contracts/matchingPolicies/StandardPolicyERC1155.sol
index e17cc5e..ec4b982 100644
--- a/contracts/matchingPolicies/StandardPolicyERC1155.sol
+++ b/contracts/matchingPolicies/StandardPolicyERC1155.sol
@@ -27,10 +27,11 @@ contract StandardPolicyERC1155 is IMatchingPolicy {
             (makerAsk.collection == takerBid.collection) &&
             (makerAsk.tokenId == takerBid.tokenId) &&
             (makerAsk.matchingPolicy == takerBid.matchingPolicy) &&
-            (makerAsk.price == takerBid.price),
+            (makerAsk.price == takerBid.price) &&
+            (makerAsk.amount == takerBid.amount),
             makerAsk.price,
             makerAsk.tokenId,
-            1,
+            makerAsk.amount,
             AssetType.ERC1155
         );
     }
@@ -53,10 +54,11 @@ contract StandardPolicyERC1155 is IMatchingPolicy {
             (makerBid.collection == takerAsk.collection) &&
             (makerBid.tokenId == takerAsk.tokenId) &&
             (makerBid.matchingPolicy == takerAsk.matchingPolicy) &&
-            (makerBid.price == takerAsk.price),
+            (makerBid.price == takerAsk.price) &&
+            (makerBid.amount == takerAsk.amount),
             makerBid.price,
             makerBid.tokenId,
-            1,
+            makerBid.amount,
             AssetType.ERC1155
         );
     }

After applying these changes, the test scenario described in the POC will pass.