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

`KittyConnect::_updateOwnershipInfo` not updating correctly resulting `s_ownerToCatsTokenId` mapping have a `tokenId` that have multiple owners

farismaulana

Summary

when calling KittyConnect::safeTransferFrom the NFT is supposed to transfer between users but the function _updateOwnershipInfo not correctly removing previous owner NFT tokenId, thus making tokenId transferred have multiple owner: the one who send and the one who receive the tokenId.

Vulnerability Details

when calling safeTransferFrom function, _updateOwnershipInfo only update the mapping s_ownerToCatsTokenId with newOwner data, but forget to delete the currCatOwner from corresponding mappings.

KittyConnect.sol:

function safeTransferFrom(address currCatOwner, address newOwner, uint256 tokenId, bytes memory data)
public
override
onlyShopPartner
{
require(_ownerOf(tokenId) == currCatOwner, "KittyConnect__NotKittyOwner");
​
// answered where is the approval?
require(getApproved(tokenId) == newOwner, "KittyConnect__NewOwnerNotApproved");
​
@> _updateOwnershipInfo(currCatOwner, newOwner, tokenId);
​
emit CatTransferredToNewOwner(currCatOwner, newOwner, tokenId);
_safeTransfer(currCatOwner, newOwner, tokenId, data);
}
.
.
.
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);
}
proof of code

add the following code to KittyTest.t.sol:

function test_safetransferCatCorrectlyUpdateOwnerMappings() public {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
address newOwner = makeAddr("newOwner");
​
vm.startPrank(partnerA);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowdy", "Ragdoll", block.timestamp);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowdo", "Ragdoll", block.timestamp);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Meowda", "Ragdoll", block.timestamp);
vm.stopPrank();
​
// tokenId set to 2
uint256 tokenId = kittyConnect.getTokenCounter() - 1;
​
vm.prank(user);
kittyConnect.approve(newOwner, tokenId);
​
vm.expectEmit(false, false, false, true, address(kittyConnect));
emit CatTransferredToNewOwner(user, newOwner, tokenId);
​
uint256[] memory userTokenIds = kittyConnect.getCatsTokenIdOwnedBy(user);
uint256[] memory newOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(newOwner);
// before transfer
// user have 3 cats
// newOwner have 0 cats
assertEq(userTokenIds.length, 3);
assertEq(newOwnerTokenIds.length, 0);
​
vm.prank(partnerA);
kittyConnect.safeTransferFrom(user, newOwner, tokenId);
​
userTokenIds = kittyConnect.getCatsTokenIdOwnedBy(user);
newOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(newOwner);
// after transfer
// user have 2 cats
// newOwner have 1 cats
assertEq(userTokenIds.length, 2);
assertEq(newOwnerTokenIds.length, 1);
}

then run the following command forge test --mt test_safetransferCatCorrectlyUpdateOwnerMappings.
the result should FAIL:

[FAIL. Reason: assertion failed: 3 != 2] test_safetransferCatCorrectlyUpdateOwnerMappings() (gas: 909945)

Impact

it only affect mapping s_ownerToCatsTokenId and making confusion who own who.

Tools Used

manual review and foundry

Recommendations

we can implement the ERC721::ownerOf as a reliable way to get who own the tokenId.

or we can fix the code base and use mapping s_ownerToCatsTokenId as intended.

add this code to KittyConnect.sol:

function _updateOwnershipInfo(address currCatOwner, address newOwner, uint256 tokenId) internal {
+ uint256 idx = s_catInfo[tokenId].idx;
s_catInfo[tokenId].prevOwner.push(currCatOwner);
s_catInfo[tokenId].idx = s_ownerToCatsTokenId[newOwner].length;
+ delete s_ownerToCatsTokenId[currCatOwner][tokenId];
+ uint256[] memory userTokenIds = s_ownerToCatsTokenId[currCatOwner];
+ uint256 lastItem = userTokenIds[userTokenIds.length - 1];
+ s_ownerToCatsTokenId[currCatOwner].pop();
​
+ if (idx < (userTokenIds.length - 1)) {
+ s_ownerToCatsTokenId[currCatOwner][idx] = lastItem;
+ }
​
s_ownerToCatsTokenId[newOwner].push(tokenId);
}

to verify this fix, run the following forge test --mt test_safetransferCatCorrectlyUpdateOwnerMappings the result should PASS:

[PASS] test_safetransferCatCorrectlyUpdateOwnerMappings() (gas: 852582)
Updates

Lead Judging Commences

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

Improper token ownership update in `_updateOwnershipInfo`

Support

FAQs

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