code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/TokenTransferrer.sol#L21-L22

Vulnerability details

Impact

ERC20 tokens don't throw a error when failed in transfer. A malicious user can pretend to be sending an ERC721 token while it is something else. Orders are getting executed inside LooksRareProxy.sol, an attacker as a maker can make an BasicOrder that has defined collectionType as a ERC721 but collection address (or token address) of ERC20 which is not ERC721 type. So if contract try executing order it will transfer the mentioned price in the order to the attacker and failed to transfer that ERC20 (pretended to ERC721) to the buyer.

struct BasicOrder {
  address signer; // The order's maker
  address collection; // The address of the ERC721/ERC1155 token to be purchased
  CollectionType collectionType; // 0 for ERC721, 1 for ERC1155
  uint256[] tokenIds; // The IDs of the tokens to be purchased
  uint256[] amounts; // Always 1 when ERC721, can be > 1 if ERC1155
  uint256 price; // The *taker bid* price to pay for the order
  address currency; // The order's currency, address(0) for ETH
  uint256 startTime; // The timestamp when the order starts becoming valid
  uint256 endTime; // The timestamp when the order stops becoming valid
  bytes signature; // split to v,r,s for LooksRare
}

Proof of Concept

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/LooksRareProxy.sol#L50

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/LooksRareProxy.sol#L125-L131

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/TokenTransferrer.sol#L14-L26

execute function inside LooksRareProxy.sol will call the _executeSingleOrder(takerBid, makerAsk, recipient, order.collectionType, isAtomic); . Making sure as an attacker to send the isAtomic value to be false so that it end up transfering token with _transferTokenToRecipient function.

Tools Used

manual

Recommended Mitigation Steps

use safeTransferFrom instead of transferFrom for ERC721 transfers.

Picodes commented 1 year ago

Unclear, no PoC

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Insufficient quality

code-423n4 / 2022-11-looksrare-findings