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

`MartenitsaMarketplace::makePresent` allows the transfer of listed NFTs (without clearing the listing)

paprikrumplikas

Summary

MartenitsaMarketplace::makePresent allows users to transfer their NFTs to another user, but it fails to ensure that either

  • listed NFTs cannot be given away as a present, or

  • if a listed NFT is given away as a present, the listing is cleared.

Vulnerability Details

MartenitsaMarketplace::makePresent allows users to give away their NFTs to another user for free, as a present. Importantly, however, listed NFTs are not supposed to be able to be transferred between users unless bought. MartenitsaMarketplace::makePresent does not check whether an NFT is listed and, accordingly, a user can give away its listed NFT as a present to another user.

Consider the following test that demonstrates this vulnerability:

Proof of Code
function testListedNftCanBeGivenAwayAsPresent() public {
address user = makeAddr("user");
uint256 price = 1e18;
​
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
uint256 tokenId = 0;
assert(martenitsaToken.ownerOf(tokenId) == jack);
assert(martenitsaToken.isProducer(jack) == true);
marketplace.listMartenitsaForSale(tokenId, price);
assert(marketplace.getListing(tokenId).forSale == true);
martenitsaToken.approve(address(marketplace), tokenId);
marketplace.makePresent(user, tokenId);
vm.stopPrank();
​
// listed nft changed owners
assert(martenitsaToken.ownerOf(tokenId) == user);
assert(martenitsaToken.balanceOf(jack) == 0);
​
// nft is still listed
assert(marketplace.getListing(tokenId).forSale == true);
}

Impact

If a listed NFT is given away to another user via MartenitsaMarketplace::makePresent, the NFT will remian incorrectly listed even after the ownership change. As a consequence:

  • mapping(uint256 => Listing) public tokenIdToListing; will be incorrect for the affected NFT,

  • MartenitsaMarketplace::getListing will return the incorrect listing data for the affected NFT,

  • although the affected NFT will continue to appear to be listed, it cannot be bought (as safeTransferFrom in MartenitsaMarketplace::buyMartenitsa will fail since it will try to move the NFT from the previous owner who made the present). Unsuspecting buyers will waste gas trying to buy the NFT.

(Note that all these issues will become void if the new owner properly lists the NFT it got as a present, since previous listing details will be overriden and reflect reality and true ownership status once again.)

Tools Used

Manual review, Foundry.

Recommendations

Consider the following options and select whichever matches your current and future requirements more appropriately:

  1. Implement a check in MartenitsaMarketplace::makePresent to ensure that listed tokens cannot be given away:

function makePresent(address presentReceiver, uint256 tokenId) external {
require(msg.sender == martenitsaToken.ownerOf(tokenId), "You do not own this token");
+ require(!getListing(tokenId).forSale, "Token is currently listed on the marketplace, you cannot give it away");
martenitsaToken.updateCountMartenitsaTokensOwner(presentReceiver, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(msg.sender, "sub");
martenitsaToken.safeTransferFrom(msg.sender, presentReceiver, tokenId);
}

Note that additionally, you should overwrite the standard ERC721 method transferFrom in the MartenitsaToken contract and include a similar check therein.

  1. Implement a logic that automatically cancels the listing when someone tries to give away thier listed NFTs as present:

​
+ event ListingDeleted(uint256 indexed tokenId);
​
function makePresent(address presentReceiver, uint256 tokenId) external {
require(msg.sender == martenitsaToken.ownerOf(tokenId), "You do not own this token");
+ if(getListing(tokenId).forSale) {
+ delete tokenIdToListing[tokenId];
+ emit ListingDeleted(tokenId);
+ }
martenitsaToken.updateCountMartenitsaTokensOwner(presentReceiver, "add");
martenitsaToken.updateCountMartenitsaTokensOwner(msg.sender, "sub");
martenitsaToken.safeTransferFrom(msg.sender, presentReceiver, tokenId);
}
​
Updates

Lead Judging Commences

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

Listed MartenitsaToken can be transferred before the sale

Support

FAQs

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