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 bypass `KittyConnect:.safeTransferFrom` and transfer Kitty NFTs themselves

paprikrumplikas

Summary

Using KittyConnect::transferFrom (a function KittyConnect inherits from ERC721.sol), users can bypass KittyConnect:.safeTransferFrom` and transfer Kitty NFTs themselves.

Vulnerability Details

Transfer of Kitty NFTs is supposed to require the facilitation of the partner shops, users are not supposed to be able to transfer Kitty NFTs themselves. This is signified by the onylShopOwner modifier in KittyConnect::safeTransferFrom:

function safeTransferFrom(address currCatOwner, address newOwner, uint256 tokenId, bytes memory data) public override onlyShopPartner {
...

However, users can bypass KittyConnect:safeTransferFrom and transfer Kitty NFTs themselves if they call KittyConnect::transferFrom (a function KittyConnect inherits from `ERC721.sol).

Proof of Code
function test_usersCanTransferNFTsThemselves() public {
address newOwner = makeAddr("newOwner");
uint256 tokenId = 0;
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
​
vm.prank(partnerA);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowdy", "Ragdoll", block.timestamp);
​
assert(kittyConnect.ownerOf(tokenId) == user);
​
vm.prank(user);
kittyConnect.approve(newOwner, tokenId);
​
vm.prank(user);
kittyConnect.transferFrom(user, newOwner, tokenId);
​
// true ownership transferred to newOwner
assert(kittyConnect.ownerOf(tokenId) == newOwner);
​
// internal ownership accounting is not updated
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 1);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(newOwner).length, 0);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user)[0], tokenId);
assertEq(kittyConnect.getCatInfo(tokenId).prevOwner.length, 0);
}

Impact

Internal ownership accounting in KittyConnect will be messed up and not reflect true ownership status. For those NFTs that are transferred via KittyConnect::transferFrom, the following variables will have incorrect values:

  • s_ownerToCatsTokenId

  • s_catInfo.

Note that this data can become even more entangled and cause more problems.
The idx within the CatInfo structure is used as an index to track the position of each NFT within an owner's array of token IDs (s_ownerToCatsTokenId[owner]). This design aims to facilitate efficient management and lookup of NFTs owned by a particular user, especially for operations that involve modifying the ownership or characteristics of these NFTs.
idx is relied on in KittyConnect::bridgeNftToAnotherChain as follows:

function bridgeNftToAnotherChain(uint64 destChainSelector, address destChainBridge, uint256 tokenId) external {
...
​
uint256[] memory userTokenIds = s_ownerToCatsTokenId[msg.sender];
uint256 lastItem = userTokenIds[userTokenIds.length - 1];
​
s_ownerToCatsTokenId[msg.sender].pop(); // e last one is removed
​
if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}
...
}

However, after calling KittyConnect::safeTransferFrom, idx will be incorrect and unreliable.

Tools Used

Manual review, Foundry.

Recommendations

Enforce that transfers can be made only via KittyConnect:safeTransferFrom by overwriting ERC721::transferFrom in KittyConnect as follows:

+ function transferFrom(address currCatOwner, address newOwner, uint256 tokenId) public override {
+ safeTransferFrom(currCatOwner, newOwner, tokenId);
+ }
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.