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

`KittyConnect::mintBridgedNFT` Does Not Update `KittyConnect::s_ownerToCatsTokenId`, Preventing Owners from Bridging NFTs Back to the Original Chain

4rdiii

[H-3] KittyConnect::mintBridgedNFT Does Not Update KittyConnect::s_ownerToCatsTokenId, Preventing Owners from Bridging NFTs Back to the Original Chain

Description: The mintBridgedNFT function within the NFT bridge protocol is designed to mint a new NFT on the target blockchain, effectively bridging an NFT from the original chain to the target chain. However, it appears that this function does not update the s_ownerToCatsTokenId mapping, which is crucial for tracking the ownership of NFTs across chains. This mapping is used in the KittyConnect::bridgeNftToAnotherChain function, and if it's empty, this function will revert if the user tries to send the NFT back to the original or any other chain.

function mintBridgedNFT(bytes memory data) external onlyKittyBridge {
(
address catOwner,
string memory catName,
string memory breed,
string memory imageIpfsHash,
uint256 dob,
address shopPartner
) = abi.decode(data, (address, string, string, string, uint256, address));
​
uint256 tokenId = kittyTokenCounter;
kittyTokenCounter++;
​
s_catInfo[tokenId] = CatInfo({
catName: catName,
breed: breed,
image: imageIpfsHash,
dob: dob,
prevOwner: new address[](0),
shopPartner: shopPartner,
idx: s_ownerToCatsTokenId[catOwner].length
});
@> // s_ownerToCatsTokenId should be updated here
emit NFTBridged(block.chainid, tokenId);
_safeMint(catOwner, tokenId);
}
​
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(); // this will revert if user has no other nfts in this chain or worth will even remove the wrong nft from the mapping
​
if (idx < (userTokenIds.length - 1)) {
s_ownerToCatsTokenId[msg.sender][idx] = lastItem;
}
​
emit NFTBridgeRequestSent(block.chainid, destChainSelector, destChainBridge, tokenId);
i_kittyBridge.bridgeNftWithData(destChainSelector, destChainBridge, data);
}

Impact: This issue can significantly impact the functionality of the NFT bridge, as it prevents owners from transferring their NFTs back to the original chain. This could lead to a loss of access to NFTs for users who have transferred them to another chain and wish to return them.

Proof of Concept: To demonstrate this issue, one can attempt to bridge an NFT back to the original chain after it has been minted on the target chain. If the mintBridgedNFT function does not update the s_ownerToCatsTokenId mapping, the system will not recognize the owner of the NFT, and the bridge operation will fail. This can be verified by attempting to call the KittyConnect::bridgeNftToAnotherChain function and observing that the operation reverts due to the lack of an updated s_ownerToCatsTokenId entry.
To test this add following test function to protocol test suit, and run it using a forked speolia testnet url.

PoC
function test_bridgeToOriginalChainWillFail()
public
partnerGivesCatToOwner
{
////fund the kittyBridge with link
uint256 InitialLink = 10_000_000_000_000_000_000;
address linkHolder = 0x61E5E1ea8fF9Dc840e0A549c752FA7BDe9224e99;
vm.prank(linkHolder);
linkToken.approve(address(kittyBridge), InitialLink + 1);
vm.prank(address(kittyBridge));
linkToken.transferFrom(linkHolder, address(kittyBridge), InitialLink);
​
////user bridges the nft to target chain ->
vm.startPrank(user);
uint64 destChainSelector = 14767482510784806043;
bytes32 messageId;
bytes memory _data;
// i slightly changed the function to return messageId and _data to help me mock router message
(messageId, _data) = kittyConnect.bridgeNftToAnotherChain(
destChainSelector,
address(kittyBridge),
0 //tokenId 0
);
assertEq(kittyConnect.balanceOf(user), 0); // assert balanceOf user == 1 to make sure nft is indeed bridged and burned
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 0); // assert length of mapping == 0 to make sure mapping is updated when burend
vm.stopPrank();
​
//@notice note that in test we do not really bridge the nft and we will just manually call the receive function by pranking ccip router
////router calls the ccipReceive function on the kittyBridge in dstChain
Client.Any2EVMMessage memory message;
message = Client.Any2EVMMessage({
messageId: messageId,
sourceChainSelector: 14767482510784806043,
sender: abi.encode(address(kittyBridge)),
data: _data,
destTokenAmounts: new Client.EVMTokenAmount[](0)
});
vm.prank(kittyBridge.getRouter()); //calling ccipReceive as router
kittyBridge.ccipReceive(message);
assertEq(kittyConnect.balanceOf(user), 1); // assert balanceOf user ==1 to make sure NFT is received by target contract
​
//trying to bridge the nft agian
vm.startPrank(user);
vm.expectRevert();
kittyConnect.bridgeNftToAnotherChain(
destChainSelector,
address(kittyBridge),
1
);
assertEq(kittyConnect.getCatsTokenIdOwnedBy(user).length, 0); // assert length of mapping == 0 to make sure mapping is NOT updated when bridged and minted
}
forge test --fork-url $SEPOLIA_RPC_URL3 --mt test_bridgeToOriginalChainWillFail

Recommended Mitigation: o mitigate this issue, the mintBridgedNFT function should be modified to include a step where it updates the s_ownerToCatsTokenId mapping with the new owner's address and the token ID of the newly minted NFT. Here's a simplified example of how the mintBridgedNFT function might be modified to include the necessary update to the s_ownerToCatsTokenId mapping:

function mintBridgedNFT(bytes memory data) external onlyKittyBridge {
(
address catOwner,
string memory catName,
string memory breed,
string memory imageIpfsHash,
uint256 dob,
address shopPartner
) = abi.decode(data, (address, string, string, string, uint256, address));
​
uint256 tokenId = kittyTokenCounter;
kittyTokenCounter++;
​
s_catInfo[tokenId] = CatInfo({
catName: catName,
breed: breed,
image: imageIpfsHash,
dob: dob,
prevOwner: new address[](0),
shopPartner: shopPartner,
idx: s_ownerToCatsTokenId[catOwner].length
});
​
+ s_ownerToCatsTokenId[catOwner].push(tokenId);
emit NFTBridged(block.chainid, tokenId);
_safeMint(catOwner, 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`

Support

FAQs

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