First Flight #12: Kitty Connect
First Flight #12: Kitty Connect
Beginner FriendlyFoundryNFTGameFi
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Improper update of user data in the safeTransferFrom function, leading to potential inconsistencies and ownership tracking issues.

josh4324

Summary

The safeTransferFrom function does not update the data of the current user properly, this can lead to inconsistencies and issues with ownership tracking.

Vulnerability Details

The safeTransferFrom function, which is responsible for transferring NFT ownership from one user to another, does not update the user data correctly. Specifically, it fails to remove the transferred token ID from the s_ownerToCatsTokenId mapping of the previous owner. This can lead to inconsistencies in the ownership tracking data and potential issues with future operations involving those tokens. The token data update happens inside the _updateOwnershipInfo function which is called inside the safeTransferFrom. The info of the previous user is not adequately removed.

function safeTransferFrom(address currCatOwner, address newOwner, uint256 tokenId, bytes memory data)
public
override
onlyShopPartner
{
require(_ownerOf(tokenId) == currCatOwner, "KittyConnect__NotKittyOwner");
​
require(getApproved(tokenId) == newOwner, "KittyConnect__NewOwnerNotApproved");
​
_updateOwnershipInfo(currCatOwner, newOwner, tokenId);
​
emit CatTransferredToNewOwner(currCatOwner, newOwner, tokenId);
_safeTransfer(currCatOwner, newOwner, tokenId, data);
}
function _updateOwnershipInfo(address currCatOwner, address newOwner, uint256 tokenId) internal {
s_catInfo[tokenId].prevOwner.push(currCatOwner);
s_catInfo[tokenId].idx = s_ownerToCatsTokenId[newOwner].length;
s_ownerToCatsTokenId[newOwner].push(tokenId);
}
​

Impact

  1. Ownership Tracking Issues: Failing to update the s_ownerToCatsTokenId mapping accurately can result in incorrect ownership information, making it difficult to track which tokens belong to each user.

  2. Data Integrity Concerns: The inconsistencies between the actual token ownership and the recorded data can lead to data integrity issues, potentially causing problems with future operations involving those tokens.

POC

In the POC below the previous user still has info linking to the transfered nft.

function test_safetransferCatToNewOwner() public {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
uint256 tokenId = kittyConnect.getTokenCounter();
address newOwner = makeAddr("newOwner");
​
vm.prank(partnerA);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowdy", "Ragdoll", block.timestamp);
​
vm.prank(user);
kittyConnect.approve(newOwner, tokenId);
​
vm.expectEmit(false, false, false, true, address(kittyConnect));
emit CatTransferredToNewOwner(user, newOwner, tokenId);
vm.prank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, tokenId);
​
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 1);
}

Tools Used

VS Code, Foundry and Manual Review

Recommendations

Modify the safeTransferFrom function to ensure that the s_ownerToCatsTokenId mapping is updated correctly during the transfer process.

Updates

Lead Judging Commences

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

Improper token ownership update in `_updateOwnershipInfo`

Support

FAQs

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