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

Amount of Owned Tokens is not Updated on Transfers Resulting in Incorrect Accounting

pacelli

Summary

MartenitsaToken::countMartenitsaTokensOwner is a mapping that keeps tracks of the number of Martenitsa owned by accounts, this state variable is correctly updated when a token is created and when a token is sold, but when an owner transfer the token
to a new account the state variable is not updated resulting in the previous account is owning the token.

Vulnerability Details

If a user calls the MartenitsaToken::transferFrom and MartenitsaToken::safeTransferFrom methods to transfer the tokens, MartenitsaToken::countMartenitsaTokensOwner will still account for the transferred token.

Proof of Concept

To verify the vulnerability paste the following test in MartenitsaToken.t.sol:

PoC
function testSecurityReview__TokenCounterIsNoUpdatedOnTransfer() public {
vm.startPrank(jack, jack);
martenitsaToken.createMartenitsa("bracelet");
// jack transfer token
martenitsaToken.transferFrom(jack, chasy, 0);
​
vm.stopPrank();
​
// incorrect accounting after jack transferred token to chasy
assertEq(martenitsaToken.countMartenitsaTokensOwner(chasy), 0); // should return 1
assertEq(martenitsaToken.countMartenitsaTokensOwner(jack), 1); // should return 0
​
vm.prank(chasy, chasy);
// now chasy transfer token to bob
martenitsaToken.safeTransferFrom(chasy, bob, 0);
​
// incorrent accounting after chasy transfer token to bob
assertEq(martenitsaToken.countMartenitsaTokensOwner(bob), 0); // should return 1
}

Impact

Invariant broken, protocol still accounts for transferred tokens after owner renounced the ownership.

Tools Used

Manual review, Foundry

Recommendations

Override the transferFrom and safeTransferFrom and add logic to update MartenitsaToken::updateCountMartenitsaTokensOwner counter for the sender and recipient.

Updates

Lead Judging Commences

bube Lead Judge about 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.