Blur Exchange contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L540 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L12 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L38 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L145

Vulnerability details

Impact

Detailed description of the impact of this finding.

the ERC1155 policy failed to validate if the two order's amount when handling the ERC1155 order matching.

note the linke

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

the _canMatchOrders eventually call the function below.

 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.price,
            makerBid.tokenId,
            1,
            AssetType.ERC1155
        );

the returned amount would always return 1 no matter what the original amount in the order specified.

then later when ERC1155 is transferred, the transferred amount would always be 1 if order matched.

executionDelegate.transferERC1155(collection, from, to, tokenId, amount);

This critical bug can leads to invalid ERC1155 order settlement at the cost of user's fund and NFT.

Proof of Concept

1. Alice create a order, she wants to buy 5 ERC1155 NFT for NFT id 1 at the price of 1 ETH.

2. Bob create a order, he wants to sell 10 ERC1155 NFT for NFT id 1 at the price of 1 ETH.

3. Two order is matched.

4. Bob discovered that only 1 of the ERC1155 NFT is transferred out to Alice and he receives 1 ETH, he is happy and write a bot to exploit his bug.

5. Alice discovered that she paid 1 ETH for 1 ERC1155 NFT but she wants to buy 5, she is very sad and upset.

Tools Used

Recommended Mitigation Steps

We recommend the project validate the order amount for both order in ERC1155 matching policy

 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 &&  // amount check is added here
            ),
            makerBid.price,
            makerBid.tokenId,
            markerBid.amount, // amount of nft is returned here.
            AssetType.ERC1155
        );

The Code in BlurExchange#CancelOrder overwrites the reserved "hash" keyword

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

  bytes32 hash = _hashOrder(order, nonces[order.trader]);

require statement does not have reason of failure

User will not know which steps goes wrong when transaction reverted.

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

        require(sell.order.side == Side.Sell);

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

        require(msg.sender == order.trader);

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

            require(msg.value == price);

Ownership can be transferred to address.

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

the Policy Manager ownership can be transferred to 0, then no one can set matching policy.

Address(0) policy can be added

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

address(0) can be added as policy

EnumerableSet.AddressSet add and remove return value not handled in PolicyManager.sol

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

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

According to the implementation of

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableSet.sol,

the function

function add(AddressSet storage set, address value) internal returns (bool) {

and

function remove(AddressSet storage set, address value) internal returns (bool) {

returns value, but when adding and removing policy, the return value are not properly handled.

Openzepplein package is outdated.

the package used is

    "@openzeppelin/contracts": "4.4.1",
    "@openzeppelin/contracts-upgradeable": "^4.6.0",

we recommend the project upgrade to the newest implementation of the openzepplin to avoid any known vulnerability in the openzepplin package

https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts

Use deprecated payable(address).transfer instead of (payable).call

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