Blur Exchange contest - TomJ's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/OrderStructs.sol#L19 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L145 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L422-L433 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

Vulnerability details

Impact

Seller can specify their Order.amount as any numbers as they want for their ERC1155 tokens. However this Order.amount is meaningless number since it is not used in the code at all. Actual amount that will be sent to the buyer is calculated from matchingPolicy contract. In case of StandardPolicyERC1155.sol, amount of token is hard-coded as 1. So for example malicious seller can set their Order.amount as 10 and set the Order.price as price worth of 10 tokens. Buyer agrees to this trade and execute the order but buyer will receive only 1 token and will lose funds worth of 9 tokens.

Proof of Concept

OrderStructs.sol:Order User can specify amount of the token to transfer but this number will not be used in actual contract logic. https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/lib/OrderStructs.sol#L19

19:    uint256 amount;

BlurExchange.sol:execute() Calling _canMatchOrders internal function to get amount https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L145

145:        (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType) = _canMatchOrders(sell.order, buy.order);

BlurExchange.sol:_canMatchOrders() External call to matchingPolicy contract's canMatchMakerAsk function to get amount https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L422-L433

425:            (canMatch, price, tokenId, amount, assetType) = IMatchingPolicy(sell.matchingPolicy).canMatchMakerAsk(sell, buy);
429:            (canMatch, price, tokenId, amount, assetType) = IMatchingPolicy(buy.matchingPolicy).canMatchMakerBid(buy, sell);

StandardPolicyERC1155.sol:canMatchMakerAsk() and canMatchMakerBid() amount is hard-coded as 1 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

33:            1,
59:            1,

Tools Used

Manual Analysis

Recommended Mitigation Steps

For StandardPolicyERC1155.sol, change the hard-coded 1 to makerAsk.amount and makerBid.amount. Add check of makerAsk.amount and makerBid.amount is equal. Also add check whether amount is equal to 1 when it is dealing with ERC721 token (ex. StandardPolicyERC721.sol) since it might cause an error for tokens that has both ERC721 and ERC1155 implementation.