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

Manipulating `MartenitsaToken::countMartenitsaTokensOwner` causes `MartenitsaMarketplace::buyMartenitsa` to revert.

feder

Summary

MartenitsaMarketplace::buyMartenitsa is intended to allow users to buy tokens from producers.

Internally, the function calls MartenitsaToken::updateCountMartenitsaTokensOwner to update the number of tokens held by each party
(seller and buyer) prior to transferring the token's ownership via the safeTransferFrom function.

The function can be exploited by forcing the MartenitsaToken::countMartenitsaTokensOwner to be zero for the seller thus
forcing the MartenitsaMarketplace::buyMartenitsa to revert when trying to decrease the amount of tokens held by the seller
due to an overflow.

Vulnerability Details

Given a seller account (producer), MartenitsaMarketplace::buyMartenitsa could be exploited by repeatedly calling
MartenitsaToken::updateCountMartenitsaTokensOwner(seller, "sub") until countMartenitsaTokensOwner[seller] reaches zero.
Then martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub"); will revert due to overflow.

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");
// <@ the line below will revert if `countMartenitsaTokensOwner[targetAccount]` is set to zero
// <@ prior to calling this function due to overflow.
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}(""); // @-check what if the seller is a contract?
require(sent, "Failed to send Ether");
​
// Transfer the token to the buyer
martenitsaToken.safeTransferFrom(seller, buyer, tokenId); // @-audit does this revert if the transfer isn't successful?
}

Proof of concept

  1. A producer mints an NFT with tokenId.

  2. Attacker calls MartenitsaToken::updateCountMartenitsaTokensOwner() passing the producer address and the operation "sub".

  3. Calling MartenitsaToken::getCountMartenitsaTokensOwner() for the producer address should return zero.

  4. Trying to call buyMartenitsa for the tokenId should revert due to overflow.

PoC

Place the following code in MartenitsaMarketplace.t.sol.

function testDoSBuyMartenitsa() public listMartenitsa {
vm.prank(chasy);
martenitsaToken.approve(address(marketplace), 0);
​
address attacker = makeAddr("attacker");
vm.prank(attacker);
martenitsaToken.updateCountMartenitsaTokensOwner(address(chasy), "sub");
​
uint256 chasyCount = martenitsaToken.getCountMartenitsaTokensOwner(address(chasy));
assert(chasyCount == 0);
​
vm.expectRevert(stdError.arithmeticError);
vm.prank(bob);
marketplace.buyMartenitsa{value: 1 wei}(0);
}

Impact

Producers are unable to sell tokens.

Tools Used

Manual review.

Recommended Mitigation

No need to implementcountMartenistaTokensOwnerto keep track of the number of tokens held by each user. Use ERC721 keeps track of it internally and the amount of tokens can be fetch viaERC721::balanceOf

- mapping(address => uint256) public countMartenitsaTokensOwner;
​
- function updateCountMartenitsaTokensOwner(address owner, string memory operation) public {
- if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("add"))) {
- countMartenitsaTokensOwner[owner] += 1;
- } else if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("sub"))) {
- countMartenitsaTokensOwner[owner] -= 1;
- } else {
- revert("Wrong operation");
- }
- }

Finally, remove the calls to martenitsaToken.updateCountMartenitsaTokensOwner inside the buyMartenitsa functtion.

// MartenitsaMarketplace.sol
​
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);
}
Updates

Lead Judging Commences

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

Missing access control

Support

FAQs

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