Blur Exchange contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/matchingPolicies/StandardPolicyERC1155.sol#L33 https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L145

Vulnerability details

Impact

While BlurExchange retrieves the amount parameter from _canMatchOrders (which retrieves it from the policies), the amount is always 1, even for ERC1155 trades. Therefore, the parameter in the order struct is currently completely unnecessary and an order with an amount of 1, 12, 44 (or even 0) is treated identical. Moreover, the price within an order struct is for the whole order (otherwise, the logic in execute would be wrong, as _executeFundsTransfer would need to be called with price * amount). Therefore, a user thinks he gets X tokens for the provided price, but only gets 1.

Proof Of Concept

Bob wants to sell 42 ERC1155 tokens and creates a corresponding sell order for it (with the amount set to 42). Alice wants to buy 42 of these tokens and thinks the price (for all 42 tokens) is fair, so she submits a buy order. However, she receives only 1 token, but has paid for 42 tokens.

Recommended Mitigation Steps

Incorporate the amount correctly (and optionally allow for partial fulfillment).

Signature scheme does not support smart contracts

Blur does not support EIP 1271 and therefore no signatures that are validated by smart contracts. This limits the applicability for protocols that want to build on top of it and persons that use smart contract wallets. Consider implementing support for it

Fee recipient may be address(0)

The recipient of a fee (https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L478) may be address(0), leading to lost ETH.

Very restrictive policies

The provided policies enforce that the prices match exactly (https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/matchingPolicies/StandardPolicyERC721.sol#L56), which is very restrictive in practice. If the seller has a lower sale price than the price that is provided by the buyer, this should ideally also match (either at the lower price or in between), because both parties would be happy with that.

BlurExchange.execute buy.order.side not validated

In BlurExchange.execute, it is not validated that buy.order.side == Side.Buy (only the sell side is validated). With the current system, all policies ensure that, but it would also make sense to validate it in execute IMO. Future policies may not validate that and such a basic check should also not be the responsibility of a policy in my opinion.

• Instead of using a boolean for the reentrancy lock, an integer would be more gas-efficient. This is described in detail in OZ's reentrancy guard (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol):

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

    // The values being non-zero value makes deployment a bit more expensive,
    // but in exchange the refund on every call to nonReentrant will be lower in
    // amount. Since refunds are capped to a percentage of the total
    // transaction's gas, it is best to keep them low in cases like this one, to
    // increase the likelihood of the full refund coming into effect.

• In multiple for loops, the loop iteration can be marked as unchecked because an overflow is not possible (as the iterator is bounded):

contracts/PolicyManager.sol:77:        for (uint256 i = 0; i < length; i++) {
contracts/lib/MerkleVerifier.sol:38:        for (uint256 i = 0; i < proof.length; i++) {
contracts/lib/EIP712.sol:77:        for (uint256 i = 0; i < fees.length; i++) {
contracts/BlurExchange.sol:199:        for (uint8 i = 0; i < orders.length; i++) {
contracts/BlurExchange.sol:476:        for (uint8 i = 0; i < fees.length; i++) {