Summary
A user that sends more funds than the MartenitsaToken NFT price , will lose the surplus sent by mistake because there is no method to withdraw ether from the marketplace.
Vulnerability Details
A user wants to buy a MartenitsaToken NFT priced at 1 wei but sends 1 ether by mistake; the 0.999999999999999999 ether will be locked forever in the marketplace.
POC
Add this function to the contract and run the command
forge test --mt testLostFundBuyMartenitsa
function testLostFundBuyMartenitsa() public listMartenitsa {
vm.prank(chasy);
martenitsaToken.approve(address(marketplace), 0);
vm.prank(bob);
marketplace.buyMartenitsa{value: 1 ether}(0);
assert(martenitsaToken.ownerOf(0) == bob);
assert(martenitsaToken.getCountMartenitsaTokensOwner(bob) == 1);
assert(martenitsaToken.getCountMartenitsaTokensOwner(chasy) == 0);
uint256 bobBalance = address(bob).balance;
console.log("balance",bobBalance);
assert(bobBalance == 4 ether);
uint256 marketplaceBalance = 1 ether - 1 ;
assert(address(marketplace).balance == marketplaceBalance );
}
We get the output:
Ran 1 test for test/MartenitsaMarketplace.t.sol:MartenitsaMarketplace
[PASS] testLostFundBuyMartenitsa() (gas: 293463)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.62ms (232.08µs CPU time)
Ran 1 test suite in 147.63ms (1.62ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
Users who mistakenly send more than the MartenitsaToken price to buy the NFT will lose the rest of the funds they sent.
Tools Used
manual Review
Recommendations
Create a function withdraw that the user will use to withdraw the rest of their funds that weren't used to buy the NFT, or change the function buy.
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, "Insufficient funds");
address seller = listing.seller;
address buyer = msg.sender;
uint256 salePrice = listing.price;
martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");
// Clear the listing
delete tokenIdToListing[tokenId];
emit MartenitsaSold(tokenId, buyer, salePrice);
// Transfer funds to seller
(bool sent,) = seller.call{value: salePrice}("");
require(sent, "Failed to send Ether");
// Transfer the token to the buyer
martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
}
