First Flight #13: Baba Marta
First Flight #13
Beginner FriendlyFoundryGameFi
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

`MartenitsaMarketplace::listMartenitsaForSale` does not return excess ethers

0xlouistsai

Summary

Users may send more ether than the listing price to the marketplace, but the contract does not refund the excess ethers, an it does not contain any withdrawal function either. Consequently, the surplus ether will be locked within the contract forever.

Vulnerability Details

For each Martenitsa token available for purchase, there is a listed price. Buyers must pay at least this price, and the marketplace will transfer the exact amount of ethers equal to the sale price to the seller. If a user sends more than the sale price, the excess ethers will be locked in the contract, as there is no withdrawal function. For instance, if Chasy sets a Martenitsa token's sale price at 1 wei, but a careless buyer like Bob mistakenly sends 1 ether through MartenitsaMarketplace::buyMartenitsa, the excess 1 ether minus 1 wei tokens will be permanently locked in the contract.

A simple proof-of-concept is as follows:

function testExcessEtherNotReturned() public createMartenitsa {
// Chasy owns ID 0 martenitsa token
address owner = martenitsaToken.ownerOf(0);
assertEq(owner, chasy);
​
// Chasy list the token for sale and approve the token
vm.prank(chasy);
martenitsaToken.approve(address(marketplace), 0);
​
vm.prank(chasy);
marketplace.listMartenitsaForSale(0, 1 wei);
​
vm.deal(bob, 1 ether);
​
// Bob sends 1 ether but the sale price is only 1 wei
vm.prank(bob);
marketplace.buyMartenitsa{value: 1 ether}(0);
​
assertEq(address(marketplace).balance, 1 ether - 1 wei);
}

Impact

The surplus ether remains indefinitely locked in the contract, and the negligent user will not receive a refund.

Tools Used

Manual Review, foundry testing

Recommendations

Send the excess ether back to the user, the transfer process should be careful and follow the check-effect-interaction pattern to prevent reentrancy issue.

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");
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);
+ if(msg.value>salePrice) {
+ uint256 amount = msg.value - salePrice
+ (bool success, ) = seller.call{value: msg.value-salePrice}("");
+ require(sucess, "Failed to send ether");
+ }
}
Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Excess ETH not refunded to the user

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.