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

Poor enumerable design in `KittyConnect` leads to incorrect account of kitties owned by accounts

pacelli

Summary

KittyConnect connect implements a enumerable system to keep track of the amount of kitties owned by addresses, but state variables are updated incorrectly after the ownership of a kitty is transferred, that leads to give the impression that one kitty is owned by multiple accounts.

Vulnerability Details

KittyConnect::_updateOwnershipInfo updates the ownership info of a kitty after the transfer of the token, the function correctly handles correctly the info of the new owner, but does not update the info of the previous owner.

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

Proof of Concept

The flow of the bug can be explained in the following steps:

  1. Shop partner mints kitty to user

  2. User approves new owner to transfer ownership

  3. Shop partner transfers kitty from user to new owner

  4. If we query KittyConnect::getCatsTokenIdOwnedBy with the address of the previous and new owner, the function will return the same token id.

This is confirmed in the test below.

PoC Paste the following test in `KittyTest.t.sol`.
function testSecurityReview__BadAccountingAfterKittyTransfer() 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.prank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, tokenId);
​
assertEq(kittyConnect.ownerOf(tokenId), newOwner);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner).length, 1);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner)[0], tokenId);
​
// incorrect internal accounting
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 1); // the length should be zero bc ownership was transferred
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user)[0], tokenId); // shouldn't return any tokenId bc ownership was transferred
}

Impact

A kitty can give the impression to be owned by multiple owners.

Tools Used

Manual review.

Recommendations

Keeping track and updating an enumerable after each transfer of ownership can be a difficult algorithm to to design. I recommend implementing a battle tested library like ERC721Enumerable from OpenZeppelin to track the amount of kitties owned by accounts.

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.