MartenitsaMarketplace::listMartenitsaForSale allows owners to list their NFTs without providing approval for the marketplace to handle the token

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2024-04-Baba-Marta/blob/5eaab7b51774d1083b926bf5ef116732c5a35cfd/src/MartenitsaMarketplace.sol#L39

Summary

MartenitsaMarketplace::listMartenitsaForSale lets Martenitsa NFT owners to list their NFTs on the marketplace, but it does not require the existance of owner approval for the marketplace to actually handle the NFT. If the approval is not made, the NFT will be impossible to buy despite the apparent listing.

Vulnerability Details

MartenitsaMarketplace::listMartenitsaForSale is supposed to allow users to list their Martenitsa NFTs for sale on the marketplace. After successful execution of the call to listMartenitsaForSale, the NFT appears as listed and as such, is supposed to be able to be bought by another user who calls MartenitsaMarketplace::buyMartenitsa. However, this is not always the case.

A preprequisite for successfully purchasing a listed NFT is that the seller approves the marketplace to move its listed token when it is purchased. Since MartenitsaMarketplace::listMartenitsaForSale does not check the existence of such an approval, unapproved NFTs can appear as listed but cannot be actually bought.

    function testNftListedWithoutApprovalCantBeBought() public {
        address user = makeAddr("user");
        uint256 price = 1 ether;

        // give user ETH
        vm.deal(user, 5 ether);

        // jack mints and NFT and then successfully lists it without approval
        vm.startPrank(jack);
        martenitsaToken.createMartenitsa("bracelet");
        assert(martenitsaToken.ownerOf(0) == jack);
        assert(martenitsaToken.isProducer(jack) == true);
        marketplace.listMartenitsaForSale(0, price);
        assert(marketplace.getListing(0).forSale == true);
        vm.stopPrank();

        // @note
        bytes memory expectedRevertReason =
            abi.encodeWithSignature("ERC721InsufficientApproval(address,uint256)", address(marketplace), 0);

        // user sees the listing, attempts to buy the NFT but the trx will fail
        // because Jack did not do the approval
        vm.expectRevert(expectedRevertReason);
        vm.prank(user);
        marketplace.buyMartenitsa{value: 1 ether}(0);
    }

Impact

NFTs that are listed by their owners without them providing their approval for the marketplace to move the listed token during a purchase attempt will appear as listed but will not be able to be actually bought. The implications are as follows:

• An owner who genuinly forgets to provide the approval will think that the listing is successful. Seeing that nobody buys the apparently listed NFT, the owner might list the NFT at a lower price:

-- If the owner provides the approval this time, the NFT might sell at a lower price than it actually would had.

-- If the owner does not provide the approval this time either, the NFT will not be sold, so the owner might keep lowering the price further, eventually tanking the floor price of the whole collection.

• An owner might intentionally not provide the approval to

-- mislead other users with the apparent listing

-- apparently tank the FP of the collection

-- to acquire and keep eligibility for getting votes on the NFT from users calling MartenitsaVoting::voteForMartenitsa, without a real intention to sell it. In this scenario the final goal is of course to win the vote and get the rewards for the win.

• Unsuspecting users trying to buy the apparently listed but not actually purchaseable NFTs will waste gas on failed transactions.

Tools Used

Manual review, Foundry.

Recommendations

Check whether the approval for the marketplace to manage to token being listed actually exists. Make the following modifications in MartenitsaMarketplace:

    function listMartenitsaForSale(uint256 tokenId, uint256 price) external {
        require(msg.sender == martenitsaToken.ownerOf(tokenId), "You do not own this token");
+      require(martenitsaToken.getApproved(tokenId) == address(this), "Provide approval for the marketplace to manage your token");
        require(martenitsaToken.isProducer(msg.sender), "You are not a producer!");
        require(price > 0, "Price must be greater than zero");

        Listing memory newListing = Listing({
            tokenId: tokenId,
            seller: msg.sender,
            price: price,
            design: martenitsaToken.getDesign(tokenId),
            forSale: true
        });

        tokenIdToListing[tokenId] = newListing;
        emit MartenitsaListed(tokenId, msg.sender, price);
    }