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

## Users can transfer NFTs through `KittyConnect::transferFrom`, resulting in `s_catInfo` and `s_ownerToCatsTokenId` not being updated

0xlouistsai

Summary

Both the safeTransferFrom and transferFrom methods can transfer NFTs in the ERC721 token standard. In KittyConnect, while kittyConnect::safeTransferFrom restricts the transfer process to shop partners, normal users can still transfer through kittyConnect::transferFrom. However, certain crucial values may not be updated via kittyConnect::_updateOwnershipInfo.

Vulnerability Details

In the ERC-721 OpenZeppelin implementation, users can transfer NFTs using the transferFrom and safeTransferFrom methods. In the kittyConnect::safeTransferFrom method, the update of s_catInfo and s_ownerToCatsTokenId is processed by kittyConnect::_updateOwnershipInfo. However, when users execute kittyConnect::transferFrom, the core information may not be updated.

function test_transferCatByTransferFrom() public {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
​
vm.prank(partnerA);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowdy", "Ragdoll", block.timestamp);
​
uint256 tokenId = kittyConnect.getTokenCounter() - 1;
address newOwner = makeAddr("newOwner");
​
// User Execute transferFrom instead of safeTransferFrom
vm.prank(user);
kittyConnect.transferFrom(user, newOwner, tokenId);
​
KittyConnect.CatInfo memory catInfo = kittyConnect.getCatInfo(tokenId);
uint256[] memory ownerToCatsTokenId = kittyConnect.getCatsTokenIdOwnedBy(newOwner);
​
assertEq(catInfo.prevOwner.length, 0); // Should be 1
assertEq(ownerToCatsTokenId.length, 0); // Should be 1
}

Impact

This scenario could lead to confusion for both users and developers interacting with getCatInfo and getCatsTokenIdOwnedBy, as it may return incorrect data.

For instance, let's consider two users, Alice and Bob, who have never minted an NFT before. Alice mints an NFT and transfers it to Bob through kittyConnect::transferFrom, making Bob the new owner of the NFT.

However, when Bob attempts to transfer the NFT from the current chain to another chain via bridgeNftToAnotherChain, issues may arise. This could occur if the userTokenIds array is empty, and an attempt is made to access userTokenIds[userTokenIds.length - 1], which would involve accessing a negative array index.

This situation highlights the importance of ensuring that the transfer and ownership update mechanisms are properly implemented to avoid such confusion and errors.

function bridgeNftToAnotherChain(uint64 destChainSelector, address destChainBridge, uint256 tokenId) external {
address catOwner = _ownerOf(tokenId);
​
require(msg.sender == catOwner);
​
CatInfo memory catInfo = s_catInfo[tokenId];
uint256 idx = catInfo.idx;
bytes memory data = abi.encode(catOwner, catInfo.catName, catInfo.breed, catInfo.image, catInfo.dob, catInfo.shopPartner);
​
_burn(tokenId);
delete s_catInfo[tokenId];
​
uint256[] memory userTokenIds = s_ownerToCatsTokenId[msg.sender];
uint256 lastItem = userTokenIds[userTokenIds.length - 1]; // @audit-issue invalid array index access
​
s_ownerToCatsTokenId[msg.sender].pop();
​
if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}
​
emit NFTBridgeRequestSent(block.chainid, destChainSelector, destChainBridge, tokenId);
i_kittyBridge.bridgeNftWithData(destChainSelector, destChainBridge, data);
}

Tools Used

Manual Review

Recommendations

Developer should add onlyShopPartner modifier to transferFrom method, in order to restrict the access to the token transfer operation.

function transferFrom(address currCatOwner, address newOwner, uint256 tokenId) 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);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

ERC721 `transferFrom` not overriden

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.