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

Cat information and `s_ownerToCatsTokenId` are not updated during transfer/bridge, leading to impossibility to bridge

n0kto

Description

Cat information is not updated during other transfer function (only safeTransferFrom is override), and s_ownerToCatsTokenId is never updated during any transfer. Consequently, getCatsTokenIdOwnedBy won't return the real list of tokens owned by a user.

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);
}

During a bridge operation (mintBridgedNFT) or any other transfer functions, _updateOwnershipInfo is not called, leading to:

  • Failure to add the transferred NFT to s_ownerToCatsTokenId of the new owner.

  • Failure to update s_catInfo[tokenId].idx of the token.

  • Failure to add the previous owner in the array of the cat.

As a result, it will be impossible to bridge this NFT if the new owner only owns this NFT because bridgeNftToAnotherChain will try to remove the tokenId from an empty array. In the worst case, if the owner already has an NFT and tries to bridge a transferred NFT, it will remove the first NFT from the array, making it impossible to bridge this one.

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];
​
@> s_ownerToCatsTokenId[msg.sender].pop();
...
}

Risk

Likelyhood: High

  • Occurs every time a transfer/bridge operation is executed.

Impact: High

  • getCatsTokenIdOwnedBy will return tokens not owned by the user or won’t return token owned by the user.

  • A bridge operation or any normal transfer can result in the loss of NFTs or the impossibility to bridge them.

Proof of Concept

Foundry PoC to add in `KittyTest.t.sol`
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.getCatsTokenIdOwnedBy(user).length, 0);
}

Recommended Mitigation

  • Override __afterTokenTransfer and add _updateOwnershipInfo in it to update ownership information for every transfer.

  • Add _updateOwnershipInfo to mintBridgedNFT.

  • In _updateOwnershipInfo, update s_ownerToCatsTokenId by accessing the array of the previous owner and erase the transferred tokenId.

Updates

Lead Judging Commences

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

owner's token ID array not updated in `mintBridgedNFT`

Improper token ownership update in `_updateOwnershipInfo`

n0kto Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

owner's token ID array not updated in `mintBridgedNFT`

Improper token ownership update in `_updateOwnershipInfo`

Support

FAQs

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