Summary
When cats transferred through KittyConnect::safeTransferFrom using _updateOwnershipInfo they are not removed from the original owners s_ownerToCatsTokenId field in storage.
Vulnerability Details
After a cat is transferred, the id of that cat still remains attached to the original owner through the kittyConnect.getCatsTokenIdOwnedBy(user); function, so as a cat is transferred around it will leave its Id on anyone who owns it.
POC test
function test_bridgeTest() public {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
vm.startPrank(partnerA);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Hehe", "Hehe", block.timestamp);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Hehe2", "Hehe2", block.timestamp);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Hehe3", "Hehe3", block.timestamp);
vm.stopPrank();
vm.startPrank(user);
uint256 tokenId = 2;
address newOwner = makeAddr("newOwner");
kittyConnect.approve(newOwner, tokenId);
vm.stopPrank();
vm.startPrank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, tokenId);
vm.stopPrank();
vm.startPrank(user);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 3);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner).length, 1);
assert(kittyConnect.ownerOf(tokenId) == newOwner);
uint256[] memory usercats = kittyConnect.getCatsTokenIdOwnedBy(user);
assert(usercats[2] == tokenId);
}
Impact
If another blockchain application is to look at this as confirmation of ownership it could cause problems, and confusion for users tracking their owned catIds
Tools Used
Manual Review, Solidity testing
Recommendations
Remove the id from s_ownerToCatsTokenId when transferring cats between users in _updateOwnershipInfo like we do when bridging our cats.
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);
+ // Remove the token id from the previous owner's list
+ uint256[] memory userTokenIds = s_ownerToCatsTokenId[currCatOwner];
+ uint256 lastItem = userTokenIds[userTokenIds.length - 1];
+ s_ownerToCatsTokenId[currCatOwner].pop();
+ uint256 idx = s_catInfo[tokenId].idx;
+ if (idx < (userTokenIds.length - 1)) {
+ s_ownerToCatsTokenId[currCatOwner][idx] = lastItem;
+ }
}
