Blur Exchange contest - Soosh's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L24 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L50

Vulnerability details

Loss of ERC1155 tokens by not checking for matching amounts

The StandardPolicyERC1155.sol matching policy does not check if the makerAsk.amount == takerBid.amount/makerBid.amount == takerAsk.amount. This means that it is possible to match any amount of ERC1155 token for an order, causing loss of funds for the user which intended to exchange less ERC1155 for the set price.

Another bug -> "Incorrect amount of ERC1155 received", issue=236, actually causes amount to always be 1. Say that the bug is fixed, then this will become an issue.

return (
		//@audit amount not matched
		(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.price,
		makerAsk.tokenId,
		1,
		AssetType.ERC1155
	);

Impact

Loss of ERC1155 tokens for the order maker.

Proof of Concept

• A user creates a makerBid of 10 ExampleToken ERC1155 tokens for price P.
• An attacker can match the order with amount 0 since amounts are not checked for equivalence in StandardPolicyERC1155.sol.
• The user gets 0 ExampleToken but the attacker gets the full price P of ETH.

Recommendations

Add a check for the order amounts to match. in canMatchMakerAsk(), add:

(makerAsk.amount == takerBid.amount)

in canMatchMakerBid(), add:

(makerBid.amount == takerAsk.amount)

Lines of code

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

Vulnerability details

Protocol can be easily rug-pulled by the owner

To use the protocol (buy/sell NFTs), users must approve the ExecutionDelegate to handle transfers for their ERC721, ERC1155, or ERC20 tokens.

The safety mechanisms mentioned by the protocol do not protect users at all if the project's owner decides to rugpull.

From the contest page, Safety Features:

• The calling contract must be approved on the ExecutionDelegate
• Users have the ability to revoke approval from the ExecutionDelegate without having to individually calling every token contract.

POC

function transferERC20(address token, address from, address to, uint256 amount)
        approvedContract
        external
        returns (bool)
    {
        require(revokedApproval[from] == false, "User has revoked approval");
        return IERC20(token).transferFrom(from, to, amount);
    }

The owner can set approvedContract to any address at any time with approveContract(address _contract), and revokeApproval() can be frontrun. As a result, all user funds approved to the ExecutionDelegate contract can be lost via rugpull.

Justification

While rug-pulling may not be the project's intention, I find that this is still an inherently dangerous design.

I am unsure about the validity of centralization risk findings on C4, but I argue this is a valid High risk issue as:

• It is too easy to steal all of user funds as a project owner. A single Bored Ape NFT traded on the exchange would mean roughly $200,000 can be stolen based on current floor price (75.6 ETH as of writing, Source: https://nftpricefloor.com/bored-ape-yacht-club). $200k because 75.6ETH for NFT seller and at least 75.6ETH approved by buyer.
• web3 security should not be based on "trust".
• Assuming the project owner is not malicious and will never rug-pull:
  • 1 successful phishing attack (private key compromise) against the project's owner is all it takes to wipe the protocol out.
  • the protocol is still affected as user's will not want to trade on a platfrom knowing such an attack is possible.

Recommendations

This is due to an insecure design of the protocol. So as far as recommendations go, the team should reconsider the protocol's design.

I do not think ExecutionDelegate should be used. It would be better if BlurExchange.sol is approved by users instead. The exchange should require that the buyer has received their NFT and the seller has received their ETH/WETH or revert.