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

Internal accounting is not updated when NFTs are transferred via standard `ERC721` method `transferFrom`

paprikrumplikas

Summary

Using MartenitsaToken::transferFrom (a function MartentisaToken inherits from ERC721.sol) users can transfer NFTs between themselves. Since this function is not overriden, the logic responsible for updating internal accounting mapping(address => uint256) public countMartenitsaTokensOwner is not triggered.

Vulnerability Details

MartenitsaToken::updateCountMartenitsaTokensOwner is supposed to update mapping(address => uint256) public countMartenitsaTokensOwner whenever a Marenitsa NFT is minted or changes owners. The mapping countMartenitsaTokensOwner is used for internal accounting and is supposed to reflect how many Martenitsa NFTs are owned by each address.

However, this internal accounting is not updated when users transfer NFTs via the standard ERC721 method transferFrom.

Consider the following test that demonstrates this vulnerability:

Proof of Code
function testInternalAccountingNotUpdatedWhenNftIsTransferredViaStandardErc721Method() public {
address user = makeAddr("user");
​
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
vm.stopPrank();
​
uint256 userInternalBalance_before = martenitsaToken.getCountMartenitsaTokensOwner(user); // 0
uint256 jackInternalBalance_before = martenitsaToken.getCountMartenitsaTokensOwner(jack); // 1
​
vm.startPrank(jack);
assert(martenitsaToken.ownerOf(0) == jack);
martenitsaToken.approve(user, 0);
vm.stopPrank();
​
vm.prank(user);
martenitsaToken.transferFrom(jack, user, 0);
​
uint256 userInternalBalance_after = martenitsaToken.getCountMartenitsaTokensOwner(user); // 0
uint256 jackInternalBalance_after = martenitsaToken.getCountMartenitsaTokensOwner(jack); // 1
​
// internal accounting has not changed, has not tracked transfer via standard ERC721 function
assert(userInternalBalance_after == userInternalBalance_before); // 0 = 0
assert(jackInternalBalance_after == jackInternalBalance_before); // 1 = 1
​
// internal accounting does not match reality
assert(userInternalBalance_after != martenitsaToken.balanceOf(user)); // 0 != 1
assert(jackInternalBalance_after != martenitsaToken.balanceOf(jack)); // 1 != 0
}

Impact

  1. Internal accounting (martenitsaToken.countMartenitsaTokensOwner()) will not refelct true ownership status (martenitsaToken.balanceOf()).

  2. Users whose internal accounting data does not reflect true ownership status will be able to claim either less or more healthToken rewards from MartenitsaMarketplace::collectRewards than they are entitled to.

Tools Used

Manual review, Foundry.

Recommendations

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

  1. Replace the countMartenitsaTokensOwner with the the standard ERC721 balanceOf(), as the former appears to duplicate the functionality already provided by the latter (i.e. tracking the number of tokens owned by each address). In this case you can completely remove not only the mapping but also the updateCountMartenitsaTokensOwner function.
    (According to the protocol owner, this is the less favorable option, the protocol prefers to keep the internal accounting.)

  2. Override the standard ERC721 method transferFrom by modifying MartenitsaToken.sol as follows. Note that when doing so, other vulnerabilites need to be considered:

+ import {MartenitsaMarketplace} from "./MartenitsaMarketplace.sol";
​
+ address marketplaceAddress;
​
// part of a fix to another vulnerability
+ function setMarketplaceAddress(address _marketplace) external onlyOwner {
+ require(marketplaceAddress == address(0), "Marketplace already set"); // Optional: Ensure only set once
+ marketplaceAddress = _marketplace;
+ }
​
​
- function updateCountMartenitsaTokensOwner(address owner, string memory operation) external {
+ function updateCountMartenitsaTokensOwner(address owner, string memory operation) public {
// part of a fix to another vulnerability
+ require(msg.sender == marketplaceAddress || msg.sender == address(this), "Unauthorized call");
​
// existing logic
}
​
+ function transferFrom(address currentOwner, address newOwner, uint256 tokenId) public override {
// check that the NFT is not listed (fix for yet another vulnerability)
+ require(!MartenitsaMarketplace(marketplaceAddress).getListing(tokenId).forSale, "This NFT is currently listed on the marketplace.");
​
+ updateCountMartenitsaTokensOwner(newOwner, "add");
+ updateCountMartenitsaTokensOwner(currentOwner, "sub");
+ super.transferFrom(currentOwner, newOwner, tokenId); // Call the inherited method to handle the transfer
+ }
Updates

Lead Judging Commences

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

ERC721 `transferFrom` not overriden

Support

FAQs

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