Blur Exchange contest - trustindistrust's results

Lines of code

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

Vulnerability details

Description

The contract StandardPolicyERC1155 provides logic for BlurExchange that determines if a given Sell Order and buy Order are a match across a set of parameters. canMatchMakerAsk() and canMatchMakerBid() will return a bool to indicate the match is valid, along with price, tokenID and (critically), quantity of ERC1155 tokens.

However, due to hardcoded values in StandardPolicyERC1155, quantity of tokens will always be 1. As a result, a buyer will pay for N tokens that the seller intended to sell, but will only receive 1.

Proof of concept

Modify the test can transfer ERC1155 in execution.test.ts to change the quantities from 1 to 2. This will mint 2 tokens, create a sell Order and matching buy Order with 2 tokens as the amount, and execute the trade. However, the assert will fail with 1

it('can transfer ERC1155', async () => {
    await mockERC1155.mint(alice.address, tokenId, 2);
    sell = generateOrder(alice, {
    side: Side.Sell,
    tokenId,
    amount: 2,
    collection: mockERC1155.address,
    matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
    });
    buy = generateOrder(bob, {
    side: Side.Buy,
    tokenId,
    amount: 2,
    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(2);
    await checkBalances(
    aliceBalance,
    aliceBalanceWeth.add(priceMinusFee),
    bobBalance,
    bobBalanceWeth.sub(price),
    feeRecipientBalance,
    feeRecipientBalanceWeth.add(fee),
    );
});

  92 passing (15s)
  1 failing

  1) Exchange
       execute
         can transfer ERC1155:
     AssertionError: Expected "1" to be equal 2
      at /home/warden/investigations/blur/tests/execution.test.ts:122:71
      at Generator.next (<anonymous>)
      at fulfilled (tests/execution.test.ts:5:58)

Suggested course of action

Modify StandardPolicyERC1155 to change 1 in the return statement to reference the ask's/bid's Order.amount. Additionally, there should be a check in the bool logic that the quantity of tokens being sold and bought is a match, unless there is a business case for allowing "partial" fulfillment of ERC1155 orders. If so, additional logic will be required in BlurExchange to support only sending the token quantity requested by the buyer.

Tooling required - Hardhat testing scaffold provided by sponsor