Blur Exchange contest - bardamu'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 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L145 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L154-L161

Vulnerability details

Impact

When ERC1155 orders are matched using StandardPolicyERC1155 the order amount is ignored and the matching amount is returned as a single item. This means that if Alice and Bob agree to exchange X > 1 amount of tokens with ID Y for a certain price, the protocol will just transfer one token Y to Bob. Alice will keep the remaining and receive the full order price.

Proof of Concept

ERC1155 is a standard interface for contracts that manage multiple token types. A single deployed contract may include any combination of fungible tokens, non-fungible tokens or other configurations (e.g. semi-fungible tokens).

Blur Exchange attempts to implement support for ERC1155 tokens containing variable amounts, as can be observed in the main order matching function in BlurExchange.sol:

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L145

function execute(Input calldata sell, Input calldata buy)
        external
        payable
        reentrancyGuard
        whenOpen
    {
[...]
  (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType) = _canMatchOrders(sell.order, buy.order);

  _executeFundsTransfer(
        sell.order.trader,
        buy.order.trader,
        sell.order.paymentToken,
        sell.order.fees,
        price
  );
  _executeTokenTransfer(
        sell.order.collection,
        sell.order.trader,
        buy.order.trader,
        tokenId,
        amount,
        assetType
  );

As can be seen in the excerpt above, function _canMatchOrders will determine if a buy and sell orders can be matched. If they do match, funds to settle the order are sent to the seller and the informed amount of tokens with tokenId are transferred from the seller to the buyer in exchange.

However, the implemented matching policy for ERC1155 as outlined in StandardPolicyERC1155.sol assumes the order amount is always set to 1. This ignores the buyer/seller specified amounts, and moreover, even if they match, will result in a single token being transferred to the buyer regardless of the agreed exchange amount. This would result in the seller keeping all but one tokens with tokenId and receiving all funds sent by the buyer committed to buy a larger amount of tokens as per the order submitted.

The following test can be added as first test in tests/execution.test.ts to demonstrate this scenario.

/////// PoC
it('cannot transfer more than amount=1 when ERC1155', async () => {
  await mockERC1155.mint(alice.address, tokenId, 10);
  sell = generateOrder(alice, {
    side: Side.Sell,
    tokenId,
    amount: 10,
    collection: mockERC1155.address,
    matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
  });
  buy = generateOrder(bob, {
    side: Side.Buy,
    tokenId,
    amount: 10,
    collection: mockERC1155.address,
    matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
  });
  sellInput = await sell.pack();
  buyInput = await buy.pack();

  await waitForTx(exchange.execute(sellInput, buyInput));

  // Because only 1 token is transferred, alice still has 9 in balance even though she sold them
  expect(await mockERC1155.balanceOf(alice.address, tokenId)).to.be.equal(9);
      
  // bob's balance is only 1 even though he bought 10 tokens
  expect(await mockERC1155.balanceOf(bob.address, tokenId)).to.be.equal(1);
      
  // this is what should have happened instead, which will now fail
  expect(await mockERC1155.balanceOf(bob.address, tokenId)).to.be.equal(10);
      
  await checkBalances(
    aliceBalance,
    aliceBalanceWeth.add(priceMinusFee),
    bobBalance,
    bobBalanceWeth.sub(price),
    feeRecipientBalance,
    feeRecipientBalanceWeth.add(fee),
  );
});

Test will generate an order from Alice to sell 10 tokens with ID tokenId, which Bob will gladly want to buy in the same amount. However, after the orders are matched Alice will still be in possession of 9 out of her 10 initial tokens (expect(await mockERC1155.balanceOf(alice.address, tokenId)).to.be.equal(9);) and Bob will have received only 1 token even though he paid for 10 (expect(await mockERC1155.balanceOf(bob.address, tokenId)).to.be.equal(1);). The final expect statement expect(await mockERC1155.balanceOf(bob.address, tokenId)).to.be.equal(10); covers the intended result, which in this case makes the test fail.

1) Exchange
       execute
         cannot transfer more than amount=1 when ERC1155:
     AssertionError: Expected "1" to be equal 10

Tools Used

vim

Recommended Mitigation Steps

Any matching policy implementing ERC1155 should be sensitive to order provided token amounts.