KittyConnect.sol::safeTransferFrom didn't update the owner registry, accumulating all the cats that he has, and the ones he already transferred.

• Description:

  • KittyConnect.sol allows owners to transfer their cats to a third party. The KittyConnect.sol::safeTransferFrom function was implemented and is operational to enable it. However, the storage is not updated correctly. Although the owner transfers his cat, his registers will always store the registry about it.

• Impact:

  • The previous owner will always have all the registers of cats that he had once. It will also keep increasing his registers. For example:

    • If he bought four cats and, for some reason, he sold these four cats.

    • The next buy will be number five. And not the number one, as it should be, considering that he is no longer the owner of the first four.

• Proof of Concept:

  Add the following code to `KittyTest.t.sol`

  ​

  function testPoCPreviousOwnerInfoItsNotUpdated() 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 firstOwner = kittyConnect.getCatsTokenIdOwnedBy(user);

  uint256[] memory newOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(newOwner);

  ​

  KittyConnect.CatInfo memory catInfo = kittyConnect.getCatInfo(tokenId);

  string memory tokenUri = kittyConnect.tokenURI(tokenId);

  ​

  assertEq(firstOwner.length, 1);

  }

  ​

• Recommendation:

  • Always remember to update all storage dependencies related to the element being utilized.

    Implement the following adjustments

    ​

    function _updateOwnershipInfo(address currCatOwner, address newOwner, uint256 tokenId) internal {

    //Push the current owner as a previous one.

    s_catInfo[tokenId].prevOwner.push(currCatOwner);

    //Update cat info, ading the number os cats of the next owner.

    s_catInfo[tokenId].idx = s_ownerToCatsTokenId[newOwner].length;

    //Push the cat into the newOwner ownership track.

    s_ownerToCatsTokenId[newOwner].push(tokenId);

    ​

    + _removeCatFromOwner(currCatOwner, tokenId);

    }

    ​

    + function _removeCatFromOwner(address owner, uint256 tokenId) internal {

    + uint256 lastCatIndex = s_ownerToCatsTokenId[owner].length - 1;

    + uint256 catIndex;

    + for(uint256 i = 0; i <= lastCatIndex; i++) {

    + if(s_ownerToCatsTokenId[owner][i] == tokenId) {

    + catIndex = i;

    + break;

    + }

    + }

    ​

    + if (catIndex < lastCatIndex) {

    + s_ownerToCatsTokenId[owner][catIndex] = s_ownerToCatsTokenId[owner][lastCatIndex];

    + }

    + s_ownerToCatsTokenId[owner].pop();

    + }

    ​