The execution of an order transfers 1 token regardless of Order.amount

[code423n4]

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

Impact

An order can be placed for an arbitrary amount, which is relevant for ERC1155. But when matched and executed only 1 token is transferred. This can lead to problems with accounting for the user, expecting a transfer of Order.amount tokens, potentially with a loss of funds as a consequence.

Proof of Concept

StandardPolicyERC1155.sol hardcodes a return value of 1, which is passed to BlurExchange as the amount used in the transferERC1155() function.

Tools Used

Code inspection

Recommended Mitigation Steps

Consider amending StandardPolicyERC1155.sol to make use of the Order.amount for ERC1155, either by allowing for the transfer of more than one token or by returning false in canMatchMakerAsk() and canMatchMakerBid for a match where Order.amount != 1 (e.g. by adding ... && makerAsk.amount == 1 && takerBid.amount == 1 to the bool return).

[blur-io-toad]

This was an oversight on my part for not putting this contract as out-of-scope. Our marketplace does not handle ERC1155 yet and so we haven't concluded what the matching critieria for those orders will be. This contract was mainly created to test ERC1155 transfers through the rest of the exchange, but shouldn't be deployed initially. When we are prepared to handle ERC1155 orders we will have to develop a new matching policy that determines the amount from the order parameters. Acknowledging that it's incorrect, but won't be making any changes as the contract won't be deployed.

[GalloDaSballo]

Dup of #666