Code4renaCODE423N4:2022-10-BLUR-FINDINGS-ISSUES-734ERC1155's Amount Parameter Manipulation To Steal Buyers' Funds
Lines of code
<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).
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: }
The text was updated successfully, but these errors were encountered:
All reactions