First Flight #12: Kitty Connect

First Flight #12: Kitty Connect

First Flight #12: Kitty Connect

Beginner FriendlyFoundryNFTGameFi
100 EXP
Ended
Submission Details
Severity: high
Valid

`KittyConnect::_updateOwnershipInfo` Does Not Update `s_ownerToCatsTokenId` of the Previous Owner

https://github.com/Cyfrin/2024-03-kitty-connect/blob/main/src/KittyConnect.sol

[L-1] KittyConnect::_updateOwnershipInfo Does Not Update s_ownerToCatsTokenId of the Previous Owner

Description: The KittyConnect::_updateOwnershipInfo function is intended to update the ownership information of an NFT within the NFT bridge protocol. However, it appears that this function does not update the s_ownerToCatsTokenId mapping for the previous owner of the NFT. This mapping is crucial for tracking the ownership of NFTs across chains, as it links an owner's address to the token ID of their NFT.

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);
@> //s_ownerToCatsTokenId[currCatOwner] is not removed
}

Impact: While this issue might not have a major impact on the overall functionality of the bridge, as other functions use multiple checks before bridging or transferring an NFT, it is still important to ensure that the s_ownerToCatsTokenId mapping does not contain incorrect data. Incorrect data in this mapping could cause confusion for users, potentially leading to misinterpretations of ownership and incorrect actions, such as attempting to transfer an NFT to the wrong owner or failing to recognize the current owner of an NFT.

Proof of Concept: There is no need to do any proof of concept because the test suit already has a test function which fails because of this issue.

function test_transferCatToNewOwner() public {
string
memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
uint256 tokenId = kittyConnect.getTokenCounter();
address newOwner = makeAddr("newOwner");

// Shop Partner gives Cat to user
vm.prank(partnerA);
kittyConnect.mintCatToNewOwner(
user,
catImageIpfsHash,
"Meowdy",
"Ragdoll",
block.timestamp
);

// Now user wants to transfer the cat to a new owner
// first user approves the cat's token id to new owner
vm.prank(user);
kittyConnect.approve(newOwner, tokenId);

// then the shop owner checks up with the new owner and confirms the transfer
vm.expectEmit(false, false, false, true, address(kittyConnect));
emit CatTransferredToNewOwner(user, newOwner, tokenId);
vm.prank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, tokenId);

uint256[] memory newOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(
newOwner
);
KittyConnect.CatInfo memory catInfo = kittyConnect.getCatInfo(tokenId);
string memory tokenUri = kittyConnect.tokenURI(tokenId);
console.log(tokenUri);
@> assert(kittyConnect.getCatsTokenIdOwnedBy(user).length == 0); // this fails because the length will stay 1 after transfer,
assert(newOwnerTokenIds.length == 1);
assert(newOwnerTokenIds[0] == tokenId);

Recommended Mitigation: Add the following line to _updateOwnershipInfo function:

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);
+ s_ownerToCatsTokenId[currCatOwner].pop();
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Improper token ownership update in `_updateOwnershipInfo`

Support

FAQs

Can’t find an answer? Join our Discord or follow us on Twitter.

Cyfrin
Updraft
CodeHawks
Solodit
Resources