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

0 stars 0 forks source link

User cannot buy NFT with ERC20 tokens in LooksRare, even with WETH #176

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/proxies/LooksRareProxy.sol#L115-#L132

Vulnerability details

Impact

User cannot buy NFT with ERC20 tokens with LooksRareProxy even with WETH.

Proof of Concept

This is the current implementation of matchAskWithTakerBidUsingETHAndWETH, this function only takes ETH or WETH as payment:

 function matchAskWithTakerBidUsingETHAndWETH(
        OrderTypes.TakerOrder calldata takerBid,
        OrderTypes.MakerOrder calldata makerAsk
    ) external payable override nonReentrant {
        require((makerAsk.isOrderAsk) && (!takerBid.isOrderAsk), "Order: Wrong sides");
        require(makerAsk.currency == WETH, "Order: Currency must be WETH");
        require(msg.sender == takerBid.taker, "Order: Taker must be the sender");

       // If not enough ETH to cover the price, use WETH
        if (takerBid.price > msg.value) {
            IERC20(WETH).safeTransferFrom(msg.sender, address(this), (takerBid.price - msg.value));
        } else {
            require(takerBid.price == msg.value, "Order: Msg.value too high");
        }
}

So although LooksRareProxy allows you to specify order.currency any ERC20 token, it will eventually fail with error Order: Currency must be WETH. Even if user use WETH, either the transaction will fail or WETH is ignored, since LooksRareProxy.sol will always transfer value=takerBid.price to LooksRareExchange:

 marketplace.matchAskWithTakerBidUsingETHAndWETH{value: takerBid.price}(takerBid, makerAsk);

Due to this code:

// If not enough ETH to cover the price, use WETH
        if (takerBid.price > msg.value) {
            IERC20(WETH).safeTransferFrom(msg.sender, address(this), (takerBid.price - msg.value));
        } else {
            require(takerBid.price == msg.value, "Order: Msg.value too high");
        }

LooksRareExchange will only take WETH if takerBid.price > msg.value; however, since LooksRareProxy always transfer value=takerBid.price, this will never happen.

Tools Used

Manual code review

Recommended Mitigation Steps

I recommend only transfer the amount of ETH that user send to aggregator.

0xhiroshi commented 1 year ago

Technically all LooksRare listings have WETH as the currency. We will be forcing the users in the frontend to provide ETH instead of WETH if it’s a LooksRare order.

c4-sponsor commented 1 year ago

0xhiroshi marked the issue as sponsor disputed

Picodes commented 1 year ago

Downgrading to QA, see https://github.com/code-423n4/2022-11-looksrare-findings/issues/72

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-11-looksrare-findings/issues/105