Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buyers can be tricked into buying fewer ERC1155 tokens while paying full price #477

Closed
code423n4 opened this issue Oct 10, 2022 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L25-L30
https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L51-L56

Vulnerability details

Impact

Due to missing validation of sell and buy order amounts in StandardPolicyERC1155 buyers may not get the amount of token
the wish to buy. Sellers can sell fewer tokens than buyers agreed to buy.

Proof of Concept

Unlike ERC721, ERC1155 allows to mint multiple tokens with the same ID. Thus, when matching buy and sell orders it's
critical to verify that amounts in the orders are matching. An example exploit scenario is when seller sells fewer tokens
than buyer expects to buy:

// tests/execution.test.ts
it('transfers wrong ERC1155 amount [audit]', async () => {
  await mockERC1155.mint(alice.address, tokenId, 10);
  sell = generateOrder(alice, {
    side: Side.Sell,
    tokenId,
    amount: 1, // Alice sells 1 token
    collection: mockERC1155.address,
    matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
  });
  buy = generateOrder(bob, {
    side: Side.Buy,
    tokenId,
    amount: 10, // Bob expects to buy 10 tokens
    collection: mockERC1155.address,
    matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
  });
  sellInput = await sell.pack();
  buyInput = await buy.pack();

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

  // Bob receives only 1 token...
  expect(await mockERC1155.balanceOf(bob.address, tokenId)).to.be.equal(1);
  // but pays for 10 tokens.
  await checkBalances(
    aliceBalance,
    aliceBalanceWeth.add(priceMinusFee),
    bobBalance,
    bobBalanceWeth.sub(price),
    feeRecipientBalance,
    feeRecipientBalanceWeth.add(fee),
  );
});

Recommended Mitigation Steps

In StandardPolicyERC1155, verify that both sell order and buy order amounts are matching.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Oct 10, 2022
code423n4 added a commit that referenced this issue Oct 10, 2022
@GalloDaSballo
Copy link
Collaborator

Dup of #666

@GalloDaSballo GalloDaSballo added the duplicate This issue or pull request already exists label Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants