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

The `KittyConnect::bridgeNftToAnotherChain` function removes the wrong `tokenId` if the bridged `tokenId` is not the last element in the list

bube

Summary

The KittyConnect::bridgeNftToAnotherChain function allows the user to bridge Kitty NFT from one chain to another chain. This involves burning of the kitty NFT on the source chain and minting on the destination chain. But there is an issue when removing the tokenId from the owner's list.

Vulnerability Details

The KittyConnect::bridgeNftToAnotherChain function allows the user to bridge Kitty NFT from one chain to another chain:

​
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();
​
if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}
​
emit NFTBridgeRequestSent(block.chainid, destChainSelector, destChainBridge, tokenId);
i_kittyBridge.bridgeNftWithData(destChainSelector, destChainBridge, data);
}
​

The bridgeNftToAnotherChain function is designed to burn a token on the current chain and prepare it for bridging to another chain. The function attempts to remove the token ID from the owner's list of tokens by calling pop() on the array. However, this only works correctly if the token being bridged is the last one in the array. If it is not, the function will remove the wrong token ID, leading to an inaccurate representation of the owner's tokens.

Impact

If the token to be bridged is not the last one in the owner's list, the function should swap it with the last element before popping to ensure that the correct token ID is removed. This step is currently missing in the bridgeNftToAnotherChain function, and without it, the wrong token ID could be removed if the token being bridged is not at the end of the list. That leads to internal tracking of token ownership to be incorrect.

Tools Used

Manual Review

Recommendations

Modify the bridgeNftToAnotherChain function to correctly handle the removal of any token ID from the owner's list. This involves swapping the token to be bridged with the last token in the list and then using pop() to remove it:

​
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();
​
- if (idx < (userTokenIds.length - 1)) {
- s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
- }
​
// Swap the token to be bridged with the last token in the owner's list, if necessary
+ if (idx < userTokenIds.length - 1) {
+ uint256 lastTokenId = userTokenIds[userTokenIds.length - 1];
+ s_ownerToCatsTokenId[msg.sender][idx] = lastTokenId;
+ s_catInfo[lastTokenId].idx = idx;
+ }
​
// Remove the last token (which is now the token to be bridged)
+ s_ownerToCatsTokenId[msg.sender].pop();
​
​
emit NFTBridgeRequestSent(block.chainid, destChainSelector, destChainBridge, tokenId);
i_kittyBridge.bridgeNftWithData(destChainSelector, destChainBridge, data);
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge about 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.