Blur Exchange contest - arcoun's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L33 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/matchingPolicies/StandardPolicyERC1155.sol#L59 https://github.com/code-423n4/2022-10-blur/blob/main/contracts/lib/OrderStructs.sol#L19

Vulnerability details

Impact

Blur Exchange supports buying or selling several ERC1155 tokens by specifying the amount value in an Order structure. This is correctly implemented in the BlurExchange contract but not in the StandardPolicyERC1155 contract which fails to validate the amount and will always return 1 as the amount of the token.

A buyer wanted to buy several tokens will pay the full price and only get one token.

Proof of Concept

• A buyer wants to buy 10 ERC1155-Famous-Token for 10 ether
• He create and sign an order with amount = 10 and price = 10 ether
• A seller with only one ERC1155-Famous-Token create and sign a related order (amount can be of any value in the order)
• Someone calls execute() with the two orders
• The transaction is executed. The seller get 10 ether, but the buyer only get 1 ERC1155-Famous-Token instead of 10

Recommended Mitigation Steps

The amount value must be checked (both orders must have the same amount) and returns inside canMatchMakerAsk and canMatchMakerBid.

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/ExecutionDelegate.sol#L36-L39

Vulnerability details

Impact

The owner of the ExecutionDelegate contract can call approveContract() to add a new address allowed to transfer ERC20/ERC721/ERC1155 tokens.

As the BlurEchange contract is already upgradeable, there is no need to add any other addresses after the contract has been created/initialized.

If there are very valuable approvals on ExecutionDelegate or if the private key of the owner is compromised, this is a potential scenario.

Proof of Concept

• A user wants to make a 10000 WETH offer on some ERC721/ERC1155
• He call approve() on the WETH contract to approve the transfer by the ExecutionDelegate contract
• The owner adds himself as an approved contract using approveContract()) and steal the 10000 WETH (and all other approved tokens) by calling transferERC20() (or the other transfer functions).

Recommended Mitigation Steps

Remove Ownable, contracts, approveContract() and denyContract(). Add an imutable exchangeAddress address as the only allowed address to transfer tokens, and only set it in the constructor or initializer.

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L451-L453

Vulnerability details

Impact

The execute() function is payable to allow a buyer to directly provide Ether as payment. If a user does a mistake and send Ether while this is not expected, the Ether will be locked in the contract and not refunded.

Proof of Concept

• A user wants to buy an ERC721 token for 1 ether, the related seller order has used paymentToken=WETH
• The user approve the ExecutionDelegate contract for WETH or already has an old approval
• He creates an order and execute it, mistakenly adding 1 Ether as payment
• The trade is executed using 1 WETH but the additional 1 Ether is lost and locked in the contract

Recommended Mitigation Steps

The _executeFundsTransfer function must ensure that msg.value is zero when a direct payment is not expected.