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

Incomplete Ownership Transfer in KittyConnect

yaioxy

Summary

The KittyConnect::_updateOwnershipInfo function does not properly remove the tokenId from the previous owner's token list, leading to a potential vulnerability where the previous owner can still interact with the NFT even after transferring it.

Vulnerability Details

The KittyConnect::_updateOwnershipInfo function is used to update the ownership mapping when a cat NFT is transferred to a new owner. However, it only adds the tokenId to the new owner's list and does not remove it from the previous owner's list.
This can allow the previous owner to still call onlyOwner functions on the NFT contract, effectively retaining control over it even after transferring it away

Impact

This vulnerability allows the previous owner of an NFT to retain control over it after transferring it away. They can call restricted onlyOwner functions, modify metadata, or transfer it again.

This can lead to loss of funds or assets for the new owner who assumes they have full control over the NFT after transfer.

POC

function test_previousOwnerStillHaveTokenId() 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);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner).length, 1);
}

Tools Used

  • Manual review

  • Foundry Test

Recommendations

Update KittyConnect::_updateOwnershipInfo to remove the tokenId from the previous owner:

function _updateOwnershipInfo(
address currCatOwner,
address newOwner,
uint256 tokenId
) internal {
​
// Remove cat from previous owner
+ uint256 lastTokenId = s_ownerToCatsTokenId[currCatOwner][s_catInfo[tokenId].idx];
+ s_ownerToCatsTokenId[currCatOwner][s_catInfo[tokenId].idx] = lastTokenId;
+ s_ownerToCatsTokenId[currCatOwner].pop();
​
// Update new owner info
s_catInfo[tokenId].prevOwner.push(currCatOwner);
s_catInfo[tokenId].idx = s_ownerToCatsTokenId[newOwner].length;
s_ownerToCatsTokenId[newOwner].push(tokenId);
}
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.