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 update storage variables associated to `currCatOwner`

feder

Summary

KittyConnect::_updateOwnershipInfo does not update KittyConnect::s_ownerToCatsTokenId and KittyConnect::s_catInfo storage variables associated with currCatOwner, resulting in KittyConnect::getCatsTokenIdOwnedBy and KittyConnect::getCatInfo returning inaccurate data.

Vulnerability Details

While KittyConnect::_updateOwnershipInfo does update KittyConnect::s_ownerToCatsTokenId and KittyConnect::s_catInfo storage variables associated with the newOwner, it fails to update said variables for the currCatOwner.

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

As a result, KittyTest::test_transferCatToNewOwner and KittyTest::test_safetransferCatToNewOwner tests fail, and KittyConnect::getCatsTokenIdOwnedBy and KittyConnect::getCatInfo return inaccurate data, as shown in the following test.

/**
* This test involves minting 2 cats for a given user and then transferring the **first** * cat to a new user
*/
function test_transferCatToNewOwnerMultiCat() public {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
uint256 firstCatTokenId = kittyConnect.getTokenCounter();
address newOwner = makeAddr("newOwner");
​
// Mint 2 cats for user
vm.startPrank(partnerA);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowdy_1", "Ragdoll_1", block.timestamp);
uint256 secondCatTokenId = kittyConnect.getTokenCounter();
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowdy_2", "Ragdoll_2", block.timestamp);
vm.stopPrank();
​
// Now user wants to transfer the first cat to a new owner
// first user approves the cat's token id to new owner
vm.prank(user);
kittyConnect.approve(newOwner, firstCatTokenId);
​
// then the shop owner checks up with the new owner and confirms the transfer
vm.expectEmit(false, false, false, true, address(kittyConnect));
emit CatTransferredToNewOwner(user, newOwner, firstCatTokenId);
vm.prank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, firstCatTokenId);
​
uint256[] memory oldOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(user);
uint256[] memory newOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(newOwner);
​
assert(kittyConnect.getCatInfo(firstCatTokenId).idx == 0);
assert(kittyConnect.getCatInfo(secondCatTokenId).idx == 0);
​
assert(kittyConnect.getCatsTokenIdOwnedBy(user).length == 1);
assert(kittyConnect.getCatsTokenIdOwnedBy(newOwner).length == 1);
​
assertEq(oldOwnerTokenIds[0], secondCatTokenId);
assertEq(newOwnerTokenIds[0], firstCatTokenId);
}

Impact

KittyConnect::getCatsTokenIdOwnedBy may return inaccurate data. In other words, elements of the returned array (token ids) may no longer belong to the provided user address.

KittyConnect::getCatInfo may return the wrong idx field value for the provided tokenId.

Tools Used

Manual review.

Recommended Mitigation

Remove the tokenId from the array s_ownerToCatsTokenId[currCatOwner]. To achieve this, we need to consider two scenarios:

Firstly, if the tokenId is located at the last element in the mentioned array, we simply remove the last element from the array.

Otherwise, we move the last element in the array, which we'll call lastTokenId, to the position (index) associated with tokenIn. Additionally, update the idx field in s_catInfo[lastTokenId] with the new index.

function _updateOwnershipInfo(address currCatOwner, address newOwner, uint256 tokenId) internal {
+ uint256[] memory userTokenIds = s_ownerToCatsTokenId[currCatOwner];
+ uint256 lastTokenId = userTokenIds[userTokenIds.length - 1];
+ CatInfo memory catInfo = s_catInfo[tokenId];
+ uint256 idx = catInfo.idx;
+ s_ownerToCatsTokenId[currCatOwner].pop();
+ if (idx < (userTokenIds.length - 1)) {
+ s_ownerToCatsTokenId[currCatOwner][idx] = lastTokenId;
+ s_catInfo[lastTokenId].idx = idx;
+ }
​
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.