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

MartenitsaToken's transferFrom/safeTransferFrom can bypass countMartenitsaTokensOwner's account

jennifersun

Summary

MartenitsaToken's transferFrom()/safeTransferFrom() can change people's tokens owned. However, we lack of updating the related variable countMartenitsaTokensOwner.

Vulnerability Details

In MartenitsaToken, there is one variable named countMartenitsaTokensOwner. This countMartenitsaTokensOwner aims to record how many NTF tokens this owner own. When people mint/sell/buy NFT Tokens, this variable can be changed correctly. However, NTF Token can be transferred by transfer()/transferFrom()/safeTransfer()/safeTransferFrom(). In these cases, the variable countMartenitsaTokensOwner does not change correctly.

Poc

function testTransferBypass() public {
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
console.log("Jack's token amount: ", martenitsaToken.getCountMartenitsaTokensOwner(jack));
address alice = makeAddr("Alice");
martenitsaToken.transferFrom(jack, alice, 0);
console.log("Jack's token amount after transfer to Alice: ", martenitsaToken.getCountMartenitsaTokensOwner(jack));
}

The result is as below:

Logs:
Jack's token amount: 1
Jack's token amount after transfer to Alice: 1

Jack has already transferred his NTF Token to Alice. However, variable countMartenitsaTokensOwner shows Jack still has one NTF Token.

Impact

The variable countMartenitsaTokensOwner stat is in a mess. Sometimes is totally wrong. This can be manipulated to earn more rewards.

Tools Used

Manual & Foundry

Recommendations

Override standard ERC721's transferFrom/safeTransferFrom() to update the variable countMartenitsaTokensOwner

+ function transferFrom(address from, address to, uint256 tokenId) public override {
+ super.transferFrom(from, to, tokenId);
+ countMartenitsaTokensOwner[from] -= 1;
+ countMartenitsaTokensOwner[to] += 1;
+ }
+
+ function safeTransferFrom(address from, address to, uint256 tokenId) public override {
+ super.safeTransferFrom(from, to, tokenId);
+ countMartenitsaTokensOwner[from] -= 1;
+ countMartenitsaTokensOwner += 1;
+ }
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.