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

Excess ETH send to the `MartenitsaMarketplace::buyMartenitsa` function is not refunded to the buyer, causing funds stuck in the `MartenitsaMarketplace` contract.

bluedragon

Description

If a user sends a msg.value (the amount of ETH sent with the transaction) that is greater than the sales price of the MartenitsaToken listed by a seller, the excess ETH gets stuck in the marketplace contract. This is because the contract does not have a mechanism to refund the excess ETH to the user.

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");
}

Impact

The excess ETH becomes locked in the MartenitsaMarketplace contract, and the user is unable to retrieve it. This can lead to a loss of funds for the user and may result in a poor user experience. Additionally, the accumulation of excess ETH in the contract could potentially lead to a denial-of-service attack by filling up the contract's balance.

Tools Used

Manual Review

Proof of Concept

  1. seller creates a martenitsaToken and lists it for sale.

  2. buyer buys the martenitsaToken accidentally with more than the sales price of the martenitsaToken.

  3. buyer gets the martenitsaToken and the seller gets the salesprice of the martenitsa.

  4. excess amount eth of the buyer is stuck in the contract and not refunded to the buyer.

Proof Of Code
function test_BuyerSendsGreaterValueThanSalesPrice() public {
// create martenitsa and list for sale
vm.startPrank(chasy);
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(0, 1 ether);
vm.stopPrank();
​
// approve marketplace to spend martenitsa token
vm.prank(chasy);
martenitsaToken.approve(address(marketplace), 0);
address user = makeAddr("user");
vm.deal(user, 2 ether);
console.log("User balance before buying martenitsa: ", user.balance);
console.log("Marketplace balance before buying martenitsa: ", address(marketplace).balance);
console.log("Seller balance before buying martenitsa: ", chasy.balance);
​
// buy martenitsa with greater value than sales price
vm.startPrank(user);
marketplace.buyMartenitsa{value: 2 ether}(0);
vm.stopPrank();
​
console.log("User balance after buying martenitsa: ", user.balance);
console.log("Marketplace balance after buying martenitsa: ", address(marketplace).balance);
console.log("Seller balance after buying martenitsa: ", chasy.balance);
console.log("Owner of the token after buying martenitsa: ", martenitsaToken.ownerOf(0));
}

Here is the foundry output

Logs:
User balance before buying martenitsa: 2000000000000000000
Marketplace balance before buying martenitsa: 0
Seller balance before buying martenitsa: 0
User balance after buying martenitsa: 0
Marketplace balance after buying martenitsa: 1000000000000000000
Seller balance after buying martenitsa: 1000000000000000000

Recommendations

To mitigate this issue, the marketplace contract should include a refund mechanism to send the excess ETH back to the user. One possible solution is to modify the MartenitsaMarketplace::buyMartenitsa function to refund the excess ETH:

function buyMartenitsa(uint256 tokenId) external payable {
.
.
.
.
// Refund excess ETH to buyer
if (msg.value > salePrice) {
(bool refunded,) = msg.sender.call{value: msg.value - salePrice}("");
require(refunded, "Failed to refund excess Ether");
}
​
+ // 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);
}

By adding the refund logic, the marketplace contract ensures that any excess ETH sent by the user is returned to them and not getting stuck in the contract.

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.