Blur Exchange contest - rokinot's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L24-L35 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L50-L61

Vulnerability details

Impact

Users may overpay for a single ERC1155 transaction.

Proof of Concept

When calling execute() with the intention of trading ERC1155 tokens, in order to acquire the number of tokens to transfer, execute() calls _canMatchOrders(), which then calls either canMatchMakerAsk() or canMatchMakerAsk(). However both the functions are hardcoded to return 1 as the amount to transfer. The rest of the transaction proceeds as intended. It's possible for users to systematically use this to steal from orders i.e. user A creates a sell order of 5 tokens, total price being 5 WETH (so 1 WETH per token) and user B fills the order with 5 WETH, only receiving one in exchange.

Submitting this finding as high as this behaviour seems unintended, since ERC1155 tokens are transfered with a custom amount in _executeTokenTransfer(), the Order struct has an amount variable and there's no mentions of the exchange not allowing multiple tokens to be traded at once.

Tools Used

Manual code reading

Recommended Mitigation Steps

Modify StandardPolicyERC1155.sol with the following:

return (
            (makerAsk.side != takerBid.side) &&
            (makerAsk.paymentToken == takerBid.paymentToken) &&
            (makerAsk.collection == takerBid.collection) &&
            (makerAsk.tokenId == takerBid.tokenId) &&
            (makerAsk.matchingPolicy == takerBid.matchingPolicy) &&
            (makerAsk.price == takerBid.price) &&
            (makerAsk.amount == takerBid.amount),
            makerAsk.price,
            makerAsk.tokenId,
            makerAsk.amount,
            AssetType.ERC1155
        );

...

return (
            (makerBid.side != takerAsk.side) &&
            (makerBid.paymentToken == takerAsk.paymentToken) &&
            (makerBid.collection == takerAsk.collection) &&
            (makerBid.tokenId == takerAsk.tokenId) &&
            (makerBid.matchingPolicy == takerAsk.matchingPolicy) &&
            (makerBid.price == takerAsk.price) &&
            (makerBid.amount == takerAsk.amount),
            makerBid.price,
            makerBid.tokenId,
            makerBid.amount,
            AssetType.ERC1155
        );

It is important to check the amount of tokens being traded are equal as this isn't verified elsewhere. This was written with the assumption orders can only be completely filled instead of maybe partially filled, as the protocol doesn't seem to support partial fill orders (discussed in the QA report).