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

User can transfer `MartenitsaToken` NFT by calling `ERC721::transferFrom` function, causing the internal logic of user's martenitsa count to break

0xShiki

Vulnerability Details

The MartenitsaToken contract inherits the ERC721 standard to manage the ownership of NFTs. By using the ERC721::transferFrom function, a user can transfer an NFT to another user. However, in this way, the user's martenitsa count is not updated correctly in the contract, leading to inconsistencies in the contract state.

Impact

As a result of transfering NFTs using the transferFrom function, the user's martenitsa count is not updated correctly, leading to inconsistencies in the contract state.

Tools Used

Manual Review

Proof of Code

Add this test to MartenitsaToken.t.sol:

PoC
function testUserCanTransferToAnotherUserUsingTransferFrom() public {
// Chasy lists a martenitsa for sale
vm.startPrank(chasy);
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(0, 1 wei);
vm.stopPrank();
​
// Bob buys the martenitsa
vm.prank(chasy);
martenitsaToken.approve(address(marketplace), 0);
vm.prank(bob);
marketplace.buyMartenitsa{value: 1 wei}(0);
​
assert(martenitsaToken.ownerOf(0) == bob);
console.log("Bob's count of token Ids before transfer: ", martenitsaToken.getCountMartenitsaTokensOwner(bob));
console.log("Jack's count of token Ids before transfer: ", martenitsaToken.getCountMartenitsaTokensOwner(jack));
​
// Bob transfer his NFT to Jack
vm.startPrank(bob);
martenitsaToken.transferFrom(bob, jack, 0);
vm.stopPrank();
​
console.log("-----------------------------");
​
assert(martenitsaToken.ownerOf(0) == jack);
console.log("Bob's count of token Ids after transfer: ", martenitsaToken.getCountMartenitsaTokensOwner(bob));
console.log("Jack's count of token Ids after transfer: ", martenitsaToken.getCountMartenitsaTokensOwner(jack));
}

From the logs of this test we can see that Bob can transfer his NFT to Jack. The ownership of the NFT is correct, but the MartenitsaToken::getCountMartenitsaTokensOwner mapping is not updated correctly.

Recommendations

Consider overriding the transferFrom function in the MartenitsaToken contract:

+ function transferFrom(address from, address to, uint256 tokenId) public override {
+ // Revert the transaction with an error message.
+ revert("KittyConnect: use safeTransferFrom for transfers");
+ }
​
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.