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

`_updateOwnershipInfo` function in `KittyConnect.sol` contract does not update the `s_ownerToCatsTokenId` of the previous owner correctly

naman1729

Summary

  • _updateOwnershipInfo function in KittyConnect.sol contract does not update the s_ownerToCatsTokenId of the previous owner and leave the tokenId in the array of the previous owner does not erase it.

Vulnerability Details

  • _updateOwnershipInfo function in KittyConnect.sol contract does not update the s_ownerToCatsTokenId of the previous owner correctly. It pushes the tokenId to the array of the new owner and does not erase the tokenId from the array of the previous owner which create conflict that the tokenId is owned by two owners at the same time.

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

POC

  • this test shows that size size of the array remains the same after the transfer of the token to the new owner from previous owner.

  • paste this test in kittyTest.t.sol .

function test_transferCatToNewOwner() public {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
uint256 tokenId = kittyConnect.getTokenCounter();
address newOwner = makeAddr("newOwner");
​
// Shop Partner gives Cat to user
vm.prank(partnerA);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowdy", "Ragdoll", block.timestamp);
​
// Now user wants to transfer the cat to a new owner
// first user approves the cat's token id to new owner
vm.prank(user);
kittyConnect.approve(newOwner, tokenId);
​
// 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, tokenId);
vm.prank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, tokenId);
​
uint256[] memory newOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(newOwner);
string memory tokenUri = kittyConnect.tokenURI(tokenId);
console.log(tokenUri);
​
assert(newOwnerTokenIds.length == 1);
// the length of the array of the previous owner(i.e user) should be 0 but it is not
assert(kittyConnect.getCatsTokenIdOwnedBy(user).length == 1);
}

  • run this test by this command

forge test --mt test_transferCatToNewOwner -vvvv

Impact

  • the tokenId is owned by two owners at the same time which is not correct.

  • same tokenId present in the array of the previous owner and the new owner which we can see by this getCatsTokenIdOwnedBy function

  • Can create problem in bridging the token to other network.

Tools Used

  • Manual review

Recommendations

  • Here is the updated code which fix the s_ownerToCatsTokenId of the previous owner correctly. By erasing the tokenId from the array of the previous owner.

function _updateOwnershipInfo(address currCatOwner, address newOwner, uint256 tokenId) internal {
+ uint256 prevIdx = s_catInfo[tokenId].idx;
s_catInfo[tokenId].prevOwner.push(currCatOwner);
s_catInfo[tokenId].idx = s_ownerToCatsTokenId[newOwner].length;
s_ownerToCatsTokenId[newOwner].push(tokenId);
+ uint256[] storage currOwnerTokenArray = s_ownerToCatsTokenId[currCatOwner];
+ uint256 endIdxData = currOwnerTokenArray[currOwnerTokenArray.length - 1];
+ currOwnerTokenArray.pop();
+ if (prevIdx < currOwnerTokenArray.length) {
+ currOwnerTokenArray[prevIdx] = endIdxData;
+ }
}
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.