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

Reentrance: An Attacker can `updateCountMartenitsaTokensOwner` multiple times before it is deleted from the tokenListing

tobezzi

Summary

When a user wants to buy a Maritenitsa, the function martenitsaToken::updateCountMartenitsaTokensOwner can be called multiple times before it is deleted delete tokenIdToListing[tokenId];. Since the updateCountMartenitsaTokensOwner function is called before deleting the token from the listing. This creates a vulnerability where an attacker could potentially manipulate the token count multiple times before the state is updated correctly, leading to inaccurate counts of Martenitsa tokens owned by buyers and sellers.

Vulnerability Details

Bob wants to get more Martenitsa token
But Bob can't wait for the producers
Bob create a contract to attack the buyMartenitsa function and quickly call the updateCountMartenitsaTokensOwner many times to get more token
Use it for healthToken

Impact

If an attacker can add multiple token to his address, he can get more healthToken and possibly leading to draining the contract

function testBuyMartenitsaMany() public listMartenitsa {
// Ensure the seller has listed a Martenitsa
vm.prank(chasy);
​
// Approve the marketplace contract to transfer the token
martenitsaToken.approve(address(marketplace), 0);
​
// Perform multiple buy transactions within the same block
for (uint256 i = 0; i < 5; i++) {
// Perform the buy transaction
martenitsaToken.updateCountMartenitsaTokensOwner(bob, "add");
}
vm.prank(bob);
marketplace.buyMartenitsa{value: 1 wei}(0);
​
// Check the final state and see bob has 6 of those token
assert(martenitsaToken.ownerOf(0) == bob);
assert(martenitsaToken.getCountMartenitsaTokensOwner(bob) == 6);
assert(martenitsaToken.getCountMartenitsaTokensOwner(chasy) == 0);
}

Bob ends with 6 tokens instead of 1.

Tools Used

slither, manual review

Recommendations

To mitigate this vulnerability, it's important to ensure that state modifications are done in the correct order, especially when interacting with external contracts. In this case, it would be safer to delete the tokenId from the tokenIdToListing mapping before making the external call to update the token counts.

Here's how you could adjust the order of operations:

​
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;
​
// Transfer the token to the buyer
martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
​
// Clear the listing
delete tokenIdToListing[tokenId];
​
// Update token counts after state changes
martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");
​
emit MartenitsaSold(tokenId, buyer, salePrice);
​
// Transfer funds to seller
(bool sent, ) = seller.call{value: salePrice}("");
require(sent, "Failed to send Ether");
}
Updates

Lead Judging Commences

bube Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Reentrancy

tobezzi Submitter
about 1 year ago
bube Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Reentrancy

Support

FAQs

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