Summary
The KittyConnect contract allows users to mint NFTs representing ownership of cats. The safeTransferFrom function is used to transfer the ownership of a given cat. But there is an issue in this function that leads to ownership inconsistencies.
Vulnerability Details
The KittyConnect::safeTransferFrom frunction transfers the cat ownership to a new owner:
function safeTransferFrom(address currCatOwner, address newOwner, uint256 tokenId, bytes memory data) public override onlyShopPartner {
require(_ownerOf(tokenId) == currCatOwner, "KittyConnect__NotKittyOwner");
require(getApproved(tokenId) == newOwner, "KittyConnect__NewOwnerNotApproved");
_updateOwnershipInfo(currCatOwner, newOwner, tokenId);
emit CatTransferredToNewOwner(currCatOwner, newOwner, tokenId);
_safeTransfer(currCatOwner, newOwner, tokenId, data);
}
The function calls the internal function _updateOwnershipInfo to update the information about the new and prevous owners:
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);
}
Actually, this function adds the current owner to the prevOwner array of the cat and adds the tokenId to the newOwner address. But the function doesn't remove the tokenId from the current owner's collection of tokenIds. That means, the current owner is still owner of the cat.
Impact
The safeTransferFrom function does not remove the token from the s_ownerToCatsTokenId mapping of the current owner after a successful transfer. This results in the previous owner still having a reference to a token they no longer own.
The test function test__safetransferCatToNewOwner function in KittyTest.t.sol file demontrates this issue. You can execute this test using the foundry command: forge test --match-test "test_safetransferCatToNewOwner" -vvvvv.
function test_safetransferCatToNewOwner() 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.expectEmit(false, false, false, true, address(kittyConnect));
emit CatTransferredToNewOwner(user, newOwner, tokenId);
vm.prank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, tokenId);
assertEq(kittyConnect.ownerOf(tokenId), newOwner);
console.log(kittyConnect.getCatsTokenIdOwnedBy(user).length);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 0);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner).length, 1);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner)[0], tokenId);
assertEq(kittyConnect.getCatInfo(tokenId).prevOwner[0], user);
assertEq(kittyConnect.getCatInfo(tokenId).prevOwner.length, 1);
assertEq(kittyConnect.getCatInfo(tokenId).idx, 0);
}
The test fails due to this line: assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 0);. The user is the prevous owner and should have 0 cats tokenId, but the protocol doesn't update the tokens owned by the user after the transfer ownership of the cat. Therefore, the prevous owner still owns 1 catsTokenId:
Ran 1 test for test/KittyTest.t.sol:KittyTest
[FAIL. Reason: assertion failed: 1 != 0] test_safetransferCatToNewOwner() (gas: 418363)
Logs:
1
Tools Used
Manual Review, Foundry
Recommendations
Implement a function to remove the token from the prevous owner:
+ function _removeTokenFromPreviousOwner(address prevOwner, uint256 tokenId) internal {
+ uint256 lastTokenIndex = s_ownerToCatsTokenId[prevOwner].length - 1;
+ uint256 tokenIndex = s_catInfo[tokenId].idx;
+ // Only need to swap if the token to remove is not the last one
+ if (tokenIndex != lastTokenIndex) {
+ uint256 lastTokenId = s_ownerToCatsTokenId[prevOwner][lastTokenIndex];
+ // Move the last token to the slot of the token to remove
+ s_ownerToCatsTokenId[prevOwner][tokenIndex] = lastTokenId;
+ // Update the moved token's index
+ s_catInfo[lastTokenId].idx = tokenIndex;
+ }
+ // Remove the last token (which is now the token to remove)
+ s_ownerToCatsTokenId[prevOwner].pop();
+ }
Then calls this function from the KittyConnect::_updateOwnershipInfo function:
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);
//@audit add a function to remove the tokenId from the currCatOwner
+ _removeTokenFromPreviousOwner(currCatOwner, tokenId);
}
