Ether handling flaw in MartenitsaMarketplace::buyMartenitsa allows for unrecoverable overpayments

Severity

High Risk

Relevant GitHub Links

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

Summary

MartenitsaMarketplace::buyMartenitsa lets users to send more ETH to the contract than the price of the Martenitsa they intend to buy. These extra funds are not returned to the buyer but are kept in the contract which has no method for withdrawing ETH.

Vulnerability Details

MartenitsaMarketplace::buyMartenitsa is designed to enable users to buy listed Martenitsa NFTs. When calling the function, users are supposed to send an ETH amount with the transaction to cover the price of the selected NFT.

MartenitsaMarketplace::buyMartenitsa accepts amounts higher than the NFT price, but is not prepared to handle the extra funds: nor does it refund the buyer, neither does it have a mechanism for withdrawing such funds.

The code below demonstrates that the contract accepts amounts higher than the NFT price.

    function testEtherStuckInContract() public {
        address user = makeAddr("user");
        uint256 userEthBalance = 2 ether;
        uint256 price = 1 ether;

        // jack produces an NFT, lists it
        vm.startPrank(jack);
        martenitsaToken.createMartenitsa("bracelet");
        martenitsaToken.approve(address(marketplace), 0);
        marketplace.listMartenitsaForSale(0, price);
        vm.stopPrank();

        // user sends twice as much ETH than the price of the NFT
        vm.deal(user, userEthBalance);
        vm.startPrank(user);
        healthToken.approve(address(marketplace), userEthBalance);
        marketplace.buyMartenitsa{value: userEthBalance}(0);
        vm.stopPrank();

        assert(user.balance == 0);
        assert(jack.balance == price);
        assert(address(marketplace).balance == userEthBalance - price);
    }

Impact

• Financial loss: Users who overpay for an NFT will lose the excess Ether they sent, as there is no mechanism to refund it. Note that sending more Ether than the price of the NFT is not necessarily a sign of the buyer's mistake or unawareness than can be easily avoided. Consider the following scenario

-- buyer submits the transaction to buy an NFT. At this point, the transaction value matches the NFT price.

-- seller submits a transaction to lower the price of the NFT. This happens about the same time when the buyer submits its transaction.

-- if the seller's transaction is added the blockchain earlier than the buyer's, the buyer will unavoidably overpay compared to the reduced price.

• Unrecoverable Ether: extra funds send to the contract are stuck in the contract forever.

Tools Used

Manual review, Foundry.

Recommendations

There are a few possible solutions, including:

• modify MartenitsaMarketplace::buyMartenitsa to reject payments that exceed the NFT price, or
• send back the extra funds to the buyer, or
• implement a withdraw function that enables the contract owner or any other predefined, trusted address to withdraw Ether from the contract.

For simplicity and fairness, consider implementing the first option by introducing the following modification:

    function buyMartenitsa(uint256 tokenId) external payable {
        Listing memory listing = tokenIdToListing[tokenId];
        require(listing.forSale, "Token is not listed for sale");
-        require(msg.value >= listing.price, "Insufficient funds");
+       require(msg.value == listing.price, "Payment must exactly match the listing price");

     // rest of the logic
     ...
     }