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` function doesn't update the ownership info of the kitty's previous owner, leads to confusion in managing and querying the ownership of NFTs.

0xShiki

Vulnerability Details

When KittyConnect::safeTransferFrom function is called, it updates the ownership information of the NFT by calling KittyConnect::_updateOwnershipInfo. However, the function does not update the KittyConnect::s_ownerToCatsTokenId mapping, where the owner of the NFT is stored.

Impact

This could lead to confusion or inefficiencies in managing and querying the ownership of NFTs.

Tools Used

Manual Review

Proof of Code

The proof of concept is this test, which was already written:

PoC
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.ownerOf(tokenId), newOwner);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 0);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner).length, 1);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner)[0], tokenId);
assertEq(kittyConnect.getCatInfo(tokenId).prevOwner[0], user);
assertEq(kittyConnect.getCatInfo(tokenId).prevOwner.length, 1);
assertEq(kittyConnect.getCatInfo(tokenId).idx, 0);
}
​

By running this test, we can see that this assertion fails:

assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 0);

Recommendations

To prevent this, we can remove the tokenId from the array of the previous owner in KittyConnect::s_ownerToCatsTokenId mapping. This can be done by adding the following line of code in the KittyConnect::_updateOwnershipInfo function:

function _updateOwnershipInfo(
address currCatOwner,
address newOwner,
uint256 tokenId
) internal {
+ // Get the index of the token ID in the array
+ uint256 tokenIndex = s_ownerToCatsTokenId[currCatOwner].length;
+ for (uint256 i = 0; i < tokenIndex; i++) {
+ if (s_ownerToCatsTokenId[currCatOwner][i] == tokenId) {
+ tokenIndex = i;
+ break;
+ }
+ }
+
+ // Swap the token ID to be removed with the last element in the array
+ s_ownerToCatsTokenId[currCatOwner][tokenIndex] = s_ownerToCatsTokenId[
+ currCatOwner
+ ][s_ownerToCatsTokenId[currCatOwner].length - 1];
​
+ // Pop the last element from the array
+ s_ownerToCatsTokenId[currCatOwner].pop();
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.