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

Ownership information is not completely updated during NFT transfers via `KittyConnect::safeTransferFrom``

paprikrumplikas

Summary

When a token is transferred, it is not removed from the original owner's list of tokens (s_ownerToCatsTokenId[currCatOwner]). Consequently, the internal ownership accounting will be incorrect.

Vulnerability Details

When a token is transferred via KittyConnect::safeTransferFrom, ownership info is supposed to be updated via a call to KittyConnect::_updateOwnershipInfo:

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

However, KittyConnect::_updateOwnershipInfo misses to remove the transferred token from the the original owner's list of tokens s_ownerToCatsTokenId[currCatOwner].

Proof of Code
function test_ownershipInfoNotFullyUpdatedDuringTransfer() public {
address newOwner = makeAddr("newOwner");
uint256 tokenId = 0;
​
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
​
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);
​
uint256 tokenIdOwnedByUser = kittyConnect.getCatsTokenIdOwnedBy(user)[tokenId]; // this does not aligns with real ownership
uint256 tokenIdOwnedByNewOwner = kittyConnect.getCatsTokenIdOwnedBy(newOwner)[tokenId];
​
console.log(tokenIdOwnedByUser);
console.log(tokenIdOwnedByNewOwner);
​
assert(tokenIdOwnedByNewOwner == tokenIdOwnedByUser);
assert(kittyConnect.ownerOf(0) == newOwner);
}

Impact

Onwership info (internal ownership) accounting will be incorrect and unreliable.

Note that this data can become even more entangled and cause more problems.
The idx within the CatInfo structure is used as an index to track the position of each NFT within an owner's array of token IDs (s_ownerToCatsTokenId[owner]). This design aims to facilitate efficient management and lookup of NFTs owned by a particular user, especially for operations that involve modifying the ownership or characteristics of these NFTs.
idx is relied on in KittyConnect::bridgeNftToAnotherChain as follows:

function bridgeNftToAnotherChain(uint64 destChainSelector, address destChainBridge, uint256 tokenId) external {
...
​
uint256[] memory userTokenIds = s_ownerToCatsTokenId[msg.sender];
uint256 lastItem = userTokenIds[userTokenIds.length - 1];
​
s_ownerToCatsTokenId[msg.sender].pop(); // e last one is removed
​
if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}
...
}

However, after calling KittyConnect::safeTransferFrom, idx will be incorrect and unreliable. Consider the following scenario:

Detailed Scenario

  1. mintCatToNewOwner() is called to mint an NFT to userA. For this, Idx=0, tokenId=0.

  2. safeTransferFrom() is called to transfer tokenId=0 to userB.

  3. mintCatToNewOwner() is called to mint a 2nd NFT to userA. For this, Idx=1, tokenId=1 (due to the bug).

  4. mintCatToNewOwner() is called to mint a 3rd NFT to userA. For this, Idx=2, tokenId=2 (due to the bug).

  5. bridgeNftToAnotherChain() is called by userA to bridge tokenId=1 to another chain.

As a result, the s_ownerToCatsTokenId[userA] array will incorrectly be [0, 1], while it really should be [2].

Even more detailed walk-through

  • First Mint: userA receives an NFT (tokenId=0). The idx within CatInfo for this token is 0. The array s_ownerToCatsTokenId[userA] now contains [0].

  • Transfer to userB: tokenId=0 is transferred to userB. Due to the bug, s_ownerToCatsTokenId[userA] mistakenly still contains [0], although it should be empty.

  • Second Mint: Another NFT is minted to userA (tokenId=1). Given the bug, idx is incorrectly set to 1 for this new token because s_ownerToCatsTokenId[userA] was not updated properly earlier and still shows one token. After minting, s_ownerToCatsTokenId[userA] incorrectly shows [0, 1].

  • Third Mint: A third NFT (tokenId=2) is minted to userA. The idx for this token is set to 2, following the same incorrect pattern. Now, s_ownerToCatsTokenId[userA] is [0, 1, 2], which is incorrect because tokenId=0 should not be there.

  • Bridging tokenId=1: When bridgeNftToAnotherChain() is called for tokenId=1 by userA, the function attempts to remove tokenId=1 from userA's list. Here's what happens, step by step::

delete s_catInfo[tokenId]; removes the CatInfo for tokenId=1.
It then attempts to remove tokenId=1 from userA's list of token IDs by popping the last item and replacing tokenId=1's position if it's not the last item.
Given the array state [0, 1, 2]:

The last item (2) is stored in lastItem.
The last item is popped, so before any replacement, the array temporarily looks like [0, 1].
Since idx for tokenId=1 is 1, and it's not less than the current last index (1), no swap occurs.

Values left in s_ownerToCatsTokenId[userA]:
The array will be [0, 1] immediately after popping 2, and since the removal logic does not correctly address the middle of the array without additional steps, it incorrectly remains [0, 1] (assuming no swap due to incorrect indexing logic). The intended outcome after correctly removing tokenId=1 should have been just [0] considering the original bug, but with proper management, it should actually end up being [2] after the transfer bug is fixed and transfers are properly managed.

Tools Used

Manual review, Foundry.

Recommendations

Modify the _updateOwnershipInfo function to include the removal of thettokenId from the currCatOwner's list.

function _updateOwnershipInfo(address currCatOwner, address newOwner, uint256 tokenId) internal {
+ _removeTokenFromOwnerTokens(currCatOwner, tokenId);
s_catInfo[tokenId].prevOwner.push(currCatOwner);
s_catInfo[tokenId].idx = s_ownerToCatsTokenId[newOwner].length;
s_ownerToCatsTokenId[newOwner].push(tokenId);
}
​
+ function _removeTokenFromOwnerTokens(address owner, uint256 tokenId) internal {
+ uint256 lastTokenIndex = s_ownerToCatsTokenId[owner].length - 1;
+ uint256 tokenIndex = s_catInfo[tokenId].idx; // Assumes idx is accurate up to this point
+
+ // If the token to remove isn't the last one, swap it with the last one
+ if (tokenIndex != lastTokenIndex) {
+ uint256 lastTokenId = s_ownerToCatsTokenId[owner][lastTokenIndex];
+ s_ownerToCatsTokenId[owner][tokenIndex] = lastTokenId;
+ s_catInfo[lastTokenId].idx = tokenIndex; // Update the moved token's idx
+ }
​
+ // Remove the last token (now either the one we're removing, or swapped)
+ s_ownerToCatsTokenId[owner].pop();
+}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge about 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.