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

`idx` can be bigger than `(userTokenIds.length - 1)` and `bridgeNftToAnotherChain` will not work as expected

zark

Summary

idx can be bigger than (userTokenIds.length - 1) and bridgeNftToAnotherChain will not work as expected

Vulnerability Details

In the bridgeNftToAnotherChain function, the contract bridges the NFT from a user to another chain. The idx variable in catInfo basically means how many NFTs has this user, taking in consideration the ones that he has not anymore. This happens because when he transfers an NFT to someone else, his s_ownerToCatsTokenId mapping is not updated and the tokenId is not removed from it. So consider this scenario :

  • User A has 5 NFTS [0, 1, 2, 3, 4].

  • He want to bridge the NFT with the ID of 1 to another chain and the array will result like this : [0, 4, 2, 3]

  • Now he wants to bridge the NFT with the id of 4 to another chain. Here the bug it is. The idx of 4. In this if statement:

if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}

the condition will not be satisfied since 4 (idx) = 4 (userTokenIds.length - 1). This means that the array of tokenIds of user will result like this [0, 4, 3] but this is not the expected scenario since he lost NFT with the ID of 2, instead of this one with ID of 4, which he wanted to transfer.

Impact

Basically messes up the functionallity of the protocol and can lead to a loss of NFTs since the s_ownerToCatsTokenId mapping will be not right.

Tools Used

Manual review
##Proof of Concept
Add this test in your test suite after commenting out the last line of the bridgeNftToAnotherChain function for simplicity's sake:

+ // i_kittyBridge.bridgeNftWithData(destChainSelector, destChainBridge, data);
function test_messedUpTokenIDsAfter2Bridges() public {
address newOwner = makeAddr("newOwner");
// User will have 5 NFTS.
for (uint256 i = 0; i < 5; i++) {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
vm.prank(partnerA);
kittyConnect.mintCatToNewOwner(newOwner, catImageIpfsHash, "Meowdy", "Ragdoll", block.timestamp);
} // Now IDs is like this : [0,1,2,3,4]
​
// Now he will bridge the NFT with ID 1
vm.prank(newOwner);
kittyConnect.bridgeNftToAnotherChain(14767482510784806043 , msg.sender, 1);
uint256[] memory newOwnerTokenIdsAfterBridge = kittyConnect.getCatsTokenIdOwnedBy(newOwner);
for (uint256 i = 0; i < newOwnerTokenIdsAfterBridge.length; i++) {
console.log("After bridging ID 1-", newOwnerTokenIdsAfterBridge[i]);
} // Now IDs is like this : [0,4,2,3]
​
// Now he will bridge the NFT with ID 4, will it be bridged properly?
vm.prank(newOwner);
kittyConnect.bridgeNftToAnotherChain(14767482510784806043 , msg.sender, 4);
uint256[] memory newOwnerTokenIdsAfterBridge2 = kittyConnect.getCatsTokenIdOwnedBy(newOwner);
for (uint256 i = 0; i < newOwnerTokenIdsAfterBridge2.length; i++) {
console.log("After bridging ID 4-", newOwnerTokenIdsAfterBridge2[i]);
} // Now IDs is like this : [0,4,2]
​
// As you can see, 4 didn't "bridged" properly for our tokenId array in user's mapping.
}

Recommendations

Consider updating index of an NFT when making a transfer to another user or chain.

Updates

Lead Judging Commences

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

Consider updating index of an NFT when making a transfer to another user or chain

Support

FAQs

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

Give us feedback!