Blur Exchange contest - polymorphism's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L425 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/BlurExchange.sol#L429 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L59

Vulnerability details

Impact

For NFT token of type ERC1155, there may be multiple tokens with the same tokenId. Therefore, when processing orders of type ERC1155, it is necessary to check not only whether the tokenId of the NFT for both buyers and sellers are matched, but also the amounts that should be matched.

The canMatchMakerAsk() function and the canMatchMakerBid() function don't check whether the amount parameters in both orders are equal. It could lead to attacks where a buyer can obtain 2 or more tokens but pay for only 1 token.

Furthermore, the _canMatchOrders() function returns a constant value of 1 w.r.t. the amount return-value, which means that only one token under the specified tokenId can be transferred at a time, which is most likely different from what the function was designed to do. It could lead to attacks where a seller can sell only 1 token to a victim buyer but with a price equivalent to 2 or more tokens.

Proof of Concept

Tools Used

Recommended Mitigation Steps

The function of canMatchMakerAsk() and canMatchMakerBid() should add amount checking and return the correct values.

The recommended changes are as follows.

function canMatchMakerAsk(Order calldata makerAsk, Order calldata takerBid)
        external
        pure
        override
        returns (
            bool,
            uint256,
            uint256,
            uint256,
            AssetType
        )
    {
        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), //@audit add checking
            makerAsk.price,
            makerAsk.tokenId,
            makerAsk.amount,   //@audit correct return value
            AssetType.ERC1155
        );
    }

function canMatchMakerBid(Order calldata makerBid, Order calldata takerAsk)
        external
        pure
        override
        returns (
            bool,
            uint256,
            uint256,
            uint256,
            AssetType
        )
    {
        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),  //@audit add checking
            makerBid.price,
            makerBid.tokenId,
            makerBid.amount,    //@audit correct return value
            AssetType.ERC1155
        );
    }