Blur Exchange contest - serial-coder's results

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 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L425 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L429 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L159

Vulnerability details

Vulnerability Details

We discovered that a rogue seller (i.e., attacker) can place an order for selling N amount (where N > 1) of a specific token id of an ERC-1155 NFT collection. However, when the sell order is fulfilled by a buyer, the attacker would spend only 1 (not N) amount of that NFT and grab a huge fund from the buyer.

For example, the attacker places an order to sell 10 amount of a specific token id of the ERC-1155 NFT collection. the attacker quotes the price for that order for 10 WETH (1 WETH per 1 token). But, after the order is fulfilled, the attacker gets 10 WETH by exchanging only 1 token.

Consider the following explanation to understand the vulnerability.

In the StandardPolicyERC1155 contract (code snippet 1), the canMatchMakerAsk function (L33) and canMatchMakerBid function (L59) would return a fixed value of 1 to the return parameter amount.

SNIPPET: 1
FILE: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol

12:    function canMatchMakerAsk(Order calldata makerAsk, Order calldata takerBid)
13:        external
14:        pure
15:        override
16:        returns (
17:            bool,
18:            uint256,
19:            uint256,
20:            uint256,
21:            AssetType
22:        )
23:    {
24:        return (
25:            (makerAsk.side != takerBid.side) &&
26:            (makerAsk.paymentToken == takerBid.paymentToken) &&
27:            (makerAsk.collection == takerBid.collection) &&
28:            (makerAsk.tokenId == takerBid.tokenId) &&
29:            (makerAsk.matchingPolicy == takerBid.matchingPolicy) &&
30:            (makerAsk.price == takerBid.price),
31:            makerAsk.price,
32:            makerAsk.tokenId,
33:            1,
34:            AssetType.ERC1155
35:        );
36:    }
37:
38:    function canMatchMakerBid(Order calldata makerBid, Order calldata takerAsk)
39:        external
40:        pure
41:        override
42:        returns (
43:            bool,
44:            uint256,
45:            uint256,
46:            uint256,
47:            AssetType
48:        )
49:    {
50:        return (
51:            (makerBid.side != takerAsk.side) &&
52:            (makerBid.paymentToken == takerAsk.paymentToken) &&
53:            (makerBid.collection == takerAsk.collection) &&
54:            (makerBid.tokenId == takerAsk.tokenId) &&
55:            (makerBid.matchingPolicy == takerAsk.matchingPolicy) &&
56:            (makerBid.price == takerAsk.price),
57:            makerBid.price,
58:            makerBid.tokenId,
59:            1,
60:            AssetType.ERC1155
61:        );
62:    }

Upon invoking the canMatchMakerAsk function (L425 in code snippet 2) or the canMatchMakerBid function (L429), the _canMatchOrders function would always get the return parameter amount of value 1.

SNIPPET: 2
FILE: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol

416:    function _canMatchOrders(Order calldata sell, Order calldata buy)
417:        internal
418:        view
419:        returns (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType)
420:    {
421:        bool canMatch;
422:        if (sell.listingTime <= buy.listingTime) {
423:            /* Seller is maker. */
424:            require(policyManager.isPolicyWhitelisted(sell.matchingPolicy), "Policy is not whitelisted");
425:            (canMatch, price, tokenId, amount, assetType) = IMatchingPolicy(sell.matchingPolicy).canMatchMakerAsk(sell, buy);
426:        } else {
427:            /* Buyer is maker. */
428:            require(policyManager.isPolicyWhitelisted(buy.matchingPolicy), "Policy is not whitelisted");
429:            (canMatch, price, tokenId, amount, assetType) = IMatchingPolicy(buy.matchingPolicy).canMatchMakerBid(buy, sell);
430:        }
431:        require(canMatch, "Orders cannot be matched");
432:    
433:        return (price, tokenId, amount, assetType);
434:    }

Then, the return parameter amount (containing 1) would then be passed into the _executeTokenTransfer function (L159 in code snippet 3). Subsequently, the buyer would receive only 1 token regardless of how many amount the seller was specified.

SNIPPET: 3
FILE: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol

128:    function execute(Input calldata sell, Input calldata buy)
129:        external
130:        payable
131:        reentrancyGuard
132:        whenOpen
133:    {
134:        require(sell.order.side == Side.Sell);
135:
136:        bytes32 sellHash = _hashOrder(sell.order, nonces[sell.order.trader]);
137:        bytes32 buyHash = _hashOrder(buy.order, nonces[buy.order.trader]);
138:
139:        require(_validateOrderParameters(sell.order, sellHash), "Sell has invalid parameters");
140:        require(_validateOrderParameters(buy.order, buyHash), "Buy has invalid parameters");
141:
142:        require(_validateSignatures(sell, sellHash), "Sell failed authorization");
143:        require(_validateSignatures(buy, buyHash), "Buy failed authorization");
144:
145:        (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType) = _canMatchOrders(sell.order, buy.order);
146:
147:        _executeFundsTransfer(
148:            sell.order.trader,
149:            buy.order.trader,
150:            sell.order.paymentToken,
151:            sell.order.fees,
152:            price
153:        );
154:        _executeTokenTransfer(
155:            sell.order.collection,
156:            sell.order.trader,
157:            buy.order.trader,
158:            tokenId,
159:            amount,
160:            assetType
161:        );
162:
163:        /* Mark orders as filled. */
164:        cancelledOrFilled[sellHash] = true;
165:        cancelledOrFilled[buyHash] = true;
166:
167:        emit OrdersMatched(
168:            sell.order.listingTime <= buy.order.listingTime ? sell.order.trader : buy.order.trader,
169:            sell.order.listingTime > buy.order.listingTime ? sell.order.trader : buy.order.trader,
170:            sell.order,
171:            sellHash,
172:            buy.order,
173:            buyHash
174:        );
175:    }

Impact

This issue is considered high because it can be occurred by both a rogue seller and even a regular seller.

Furthermore, the seller can be both the maker who places a sell order and waits for a buyer (i.e., taker) to fulfills the order, and can be the taker who fulfills a buy order opened by a buyer (i.e., maker).

Proof of Concept

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

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L425

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L429

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L159

Tools Used

VSCode (Manual Review)

Recommended Mitigation Steps

We recommend updating the following functions: canMatchMakerAsk and canMatchMakerBid of the StandardPolicyERC1155 contract as shown in the code snippet 4 below.

Specifically, we added the statement (makerAsk.amount == takerBid.amount) && (L29) into the canMatchMakerAsk function and changed L34 from 1 to makerAsk.amount.

For the canMatchMakerBid function, the statement (makerBid.amount == takerAsk.amount) && was added in L56. And, the return parameter amount was changed from 1 to makerBid.amount (L61).

SNIPPET: 4
FILE: https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol

12:    function canMatchMakerAsk(Order calldata makerAsk, Order calldata takerBid)
13:        external
14:        pure
15:        override
16:        returns (
17:            bool,
18:            uint256,
19:            uint256,
20:            uint256,
21:            AssetType
22:        )
23:    {
24:        return (
25:            (makerAsk.side != takerBid.side) &&
26:            (makerAsk.paymentToken == takerBid.paymentToken) &&
27:            (makerAsk.collection == takerBid.collection) &&
28:            (makerAsk.tokenId == takerBid.tokenId) &&
29:            (makerAsk.amount == takerBid.amount) &&
30:            (makerAsk.matchingPolicy == takerBid.matchingPolicy) &&
31:            (makerAsk.price == takerBid.price),
32:            makerAsk.price,
33:            makerAsk.tokenId,
34:            makerAsk.amount,
35:            AssetType.ERC1155
36:        );
37:    }
38:
39:    function canMatchMakerBid(Order calldata makerBid, Order calldata takerAsk)
40:        external
41:        pure
42:        override
43:        returns (
44:            bool,
45:            uint256,
46:            uint256,
47:            uint256,
48:            AssetType
49:        )
50:    {
51:        return (
52:            (makerBid.side != takerAsk.side) &&
53:            (makerBid.paymentToken == takerAsk.paymentToken) &&
54:            (makerBid.collection == takerAsk.collection) &&
55:            (makerBid.tokenId == takerAsk.tokenId) &&
56:            (makerBid.amount == takerAsk.amount) &&
57:            (makerBid.matchingPolicy == takerAsk.matchingPolicy) &&
58:            (makerBid.price == takerAsk.price),
59:            makerBid.price,
60:            makerBid.tokenId,
61:            makerBid.amount,
62:            AssetType.ERC1155
63:        );
64:    }