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

kittyConnect::_updateOwnershipInfo fails to remove tokenId from previous owner

tobezzi

Summary

The kittyConnect::_updateOwnershipInfo function does not remove the tokenId from the currCatOwner. It only updates the prevOwner array, Idx, newOwner. And leaves room for the previous owner and new Owner to have the same token.

s_catInfo[tokenId].prevOwner.push(currCatOwner);
s_catInfo[tokenId].idx = s_ownerToCatsTokenId[newOwner].length;
s_ownerToCatsTokenId[newOwner].push(tokenId);

Vulnerability Details

When a cat owner wants to transfer ownership to a new owner, they have to approve it to the new owner address and the shopPartner will do the transfer using the safeTransferFrom function.

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);
}

The function does all checking and call the _updateOwnershipInfo which updates the prevOwner array, Idx, newOwner but doesn't remove the tokend from the previous owner, leaving it there will result in conflict.

Impact

The Cat Owner can transfer NFT to a new owner and still hold the same NFT but have lost approval right.

POC
​
function test_safetransferCatToNewOwner() public {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
uint256 tokenId = kittyConnect.getTokenCounter();
address newOwner = makeAddr("newOwner");
address newOwner2 = makeAddr("newOwner2");
​
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

Manual review

Recommendations

Here's how you can update the function to remove the token from the list of tokens owned by the previous owner:

​
function _updateOwnershipInfo(address currCatOwner, address newOwner, uint256 tokenId) internal {
// Push the token ID to the list of tokens owned by the new owner
s_ownerToCatsTokenId[newOwner].push(tokenId);
// Find the index of the token ID in the list of tokens owned by the current owner
uint256 indexToRemove;
for (uint256 i = 0; i < s_ownerToCatsTokenId[currCatOwner].length; i++) {
if (s_ownerToCatsTokenId[currCatOwner][i] == tokenId) {
indexToRemove = i;
break;
}
}
// Remove the token ID from the list of tokens owned by the current owner
if (indexToRemove < s_ownerToCatsTokenId[currCatOwner].length - 1) {
s_ownerToCatsTokenId[currCatOwner][indexToRemove] = s_ownerToCatsTokenId[currCatOwner][s_ownerToCatsTokenId[currCatOwner].length - 1];
}
s_ownerToCatsTokenId[currCatOwner].pop();
}

This updated function removes the tokenId from the list of tokens owned by the previous owner (currCatOwner). It searches for the index of the tokenId in the list and then removes it by replacing it with the last element in the list and then popping the last element. This ensures that the list remains contiguous and doesn't have any gaps after the removal.

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.