Summary
The KittyConnect::bridgeNftToAnotherChain() function facilitates cross-chain token transfers. However, there is a flaw wherein,
after burning a specific token, the function erroneously counts its ID as still valid for transfer.
This issue arises from the utilization of mapping(address user => uint256[]) private s_ownerToCatsTokenId to monitor token IDs per user.
The problem occurs when bridgeNftToAnotherChain recalculates the IDs (using the idx variable in the mapping) post-token burn,
resulting in incorrect IDs being assigned to the tokens the user retains.
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();
if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}
...
}
Vulnerability Details
A test is being implemented to expose a logic flaw. In this scenario, a shop partner mints tokens with IDs 0, 1, and 2 for a specific user.
Subsequently, the user attempts to transfer tokens with IDs 0 and 2, but after the transfers, retains token ID 2 instead of 1, which was not transferred.
To facilitate testing, a mock of the bridgeNftToAnotherChain function is employed. Two lines pertaining to the bridge transfer are commented out.
function mock_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;
@>
_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);
@>
}
function test_bridgeNftToAnotherChain() public {
string memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
vm.startPrank(partnerA);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Cat1", "Ragdoll", block.timestamp);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Cat2", "Ragdoll", block.timestamp);
kittyConnect.mintCatToNewOwner(user, catImageIpfsHash, "Cat3", "Ragdoll", block.timestamp);
vm.stopPrank();
vm.startPrank(user);
console.log("::Tokens before all transfers");
uint256[] memory idsBefore = kittyConnect.getCatsTokenIdOwnedBy(user);
for (uint256 i = 0; i < idsBefore.length; i++) {
console.log("Token : ", idsBefore[i]);
}
kittyConnect.mock_bridgeNftToAnotherChain(uint64(0), address(0), 0);
console.log("::Tokens after transfering token 0");
uint256[] memory idsAfterTransferOne = kittyConnect.getCatsTokenIdOwnedBy(user);
for (uint256 i = 0; i < idsAfterTransferOne.length; i++) {
console.log("Token : ", idsAfterTransferOne[i]);
}
kittyConnect.mock_bridgeNftToAnotherChain(uint64(0), address(0), 2);
console.log("::Tokens after transfering token 2");
uint256[] memory idsAfterTransferTwo = kittyConnect.getCatsTokenIdOwnedBy(user);
for (uint256 i = 0; i < idsAfterTransferTwo.length; i++) {
console.log("Token : ", idsAfterTransferTwo[i]);
}
vm.stopPrank();
}
Output
Logs:
::Tokens before all transfers
Token : 0
Token : 1
Token : 2
::Tokens after transfering token 0
Token : 2
Token : 1
::Tokens after transfering token 2
Token : 2
Impact
In some cases, incorrect token IDs are attributed to the user after a transfer to another chain.
Tools Used
Foundry and manual review
Recommendations
Revise the token ID accounting mechanism (idx) to accurately reflect the transferred IDs.
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;
-- }
++ //Array Remove An Element By Replacing Last
++ uint256[] storage userTokenIds = s_ownerToCatsTokenId[msg.sender];
++ uint256 lastTokenId = userTokenIds[userTokenIds.length - 1];
++ userTokenIds[idx] = lastTokenId;
++ s_catInfo[lastTokenId].idx = idx;
++ userTokenIds.pop();
emit NFTBridgeRequestSent(block.chainid, destChainSelector, destChainBridge, tokenId);
i_kittyBridge.bridgeNftWithData(destChainSelector, destChainBridge, data);
}
