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

Improper token ownership update in `_updateOwnershipInfo` leads to double ownership of the same Kitty

irondevx

Summary

_updateOwnershipInfo properly updates new owner's ownership of the Kitty, but fails to remove old owner's ownership of it.

Vulnerability Details

Upon transferring a token from one owner to another via safeTransferFrom, _updateOwnershipInfo is called to update the contract's internal state to reflect this change. However, due to the lack of logic to remove the token ID from the s_ownerToCatsTokenId array of the previous owner, the contract's state inaccurately reflects the ownership of the token. This flaw can be exploited by the previous owner to perform actions as if they still possessed the token, including bridging it to another chain, thereby creating a discrepancy in ownership across different platforms and within the ecosystem.

Impact

This vulnerability undermines the security and integrity of the token ownership model within the KittyConnect ecosystem. Exploiting this flaw can lead to multiple parties claiming ownership of the same token, resulting in confusion, loss of trust, and potential financial implications for the involved parties.

Tools Used

Foundry

Proof of Code

Add the following code to the KittyTest.t.sol file:

function test_transferCatToNewOwner_DoesntUpdateOwnership() public {
// ******************** Mint kitty ********************
uint256 tokenId = kittyConnect.getTokenCounter();
​
vm.prank(partnerA);
kittyConnect.mintCatToNewOwner(
user,
"ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62",
"Meowdy",
"Ragdoll",
block.timestamp
);
// ****************************************************
​
// ******************** Transfer kitty to a new owner ********************
address newOwner = makeAddr("newOwner");
​
vm.prank(user);
kittyConnect.approve(newOwner, tokenId);
vm.expectEmit(false, false, false, true, address(kittyConnect));
emit CatTransferredToNewOwner(user, newOwner, tokenId);
​
// Shop partner transfers to the new owner
vm.prank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, tokenId);
​
uint256[] memory oldOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(
user
);
uint256[] memory newOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(
newOwner
);
​
// Confirm that they both own the same Kitty
assertEq(oldOwnerTokenIds.length, 1);
assertEq(newOwnerTokenIds.length, 1);
​
assertEq(newOwnerTokenIds[0], tokenId);
assertEq(oldOwnerTokenIds[0], tokenId);
// **********************************************************************
}

Recommendations

Implement logic within the _updateOwnershipInfo function to remove the token ID from the previous owner's s_ownerToCatsTokenId array. This update ensures the contract's state accurately reflects the current ownership of tokens and prevents the previous owner from interacting with tokens they no longer own.

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);
​
+ // Remove the token from the previous owner's array
+ uint256 tokenIndex = s_catInfo[tokenId].idx;
+ uint256 lastTokenIndex = s_ownerToCatsTokenId[currCatOwner].length - 1;
+ uint256 lastTokenId = s_ownerToCatsTokenId[currCatOwner][lastTokenIndex];
​
+ // Swap and pop
+ s_ownerToCatsTokenId[currCatOwner][tokenIndex] = lastTokenId;
+ s_catInfo[lastTokenId].idx = tokenIndex;
+ s_ownerToCatsTokenId[currCatOwner].pop();
}
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.