Summary
The KittyConnect::mintBridgedNFT() function is crucial for minting new tokens for a user, received from another chain.
This function is invoked via the KittyBridge::_ccipReceive callback.
Within this process, the mapping(address user => uint256[]) private s_ownerToCatsTokenId is utilized to manage token IDs per user.
However, a critical issue arises wherein the mintBridgedNFT() function mints tokens for the user without updating internal accounting.
Note that mintCatToNewOwner() function also mints a new token for the user and effectively updates the internal accounting by utilizing the s_ownerToCatsTokenId[catOwner].push(tokenId); line of code.
function mintBridgedNFT(bytes memory data) external onlyKittyBridge {
...
@>
emit NFTBridged(block.chainid, tokenId);
_safeMint(catOwner, tokenId);
}
Vulnerability Details
-
A test is being implemented to expose a logic flaw. This test simulates a callback from another chain, minting tokens with IDs 0 and 1
for a specific user.
-
Post-mint, we query the tokens owned by the user via the KittyConnect.getCatsTokenIdOwnedBy(newOwner) function and observe that no
tokens are recorded.
-
Subsequently, an attempt to transfer one of the minted tokens to another chain fails due to the absence of internal data required by the
mock_bridgeNftToAnotherChain function.
-
To aid testing, a mock of the bridgeNftToAnotherChain function is utilized. Two lines relevant 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_badMintBridgedNFT() public {
address newOwner = makeAddr("newOwner");
vm.startPrank(address(kittyBridge));
bytes memory dataToken0 = abi.encode(newOwner, "meowdy", "ragdoll", "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62", block.timestamp, partnerA);
bytes memory dataToken1 = abi.encode(newOwner, "pepe", "ragdoll", "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62", block.timestamp, partnerA);
kittyConnect.mintBridgedNFT(dataToken0);
kittyConnect.mintBridgedNFT(dataToken1);
vm.stopPrank();
console.log("::Tokens for new owner");
uint256[] memory newOwnerTokenIds = kittyConnect.getCatsTokenIdOwnedBy(newOwner);
for (uint256 i = 0; i < newOwnerTokenIds.length; i++) {
console.log("Token : ", newOwnerTokenIds[i]);
}
vm.startPrank(address(newOwner));
kittyConnect.mock_bridgeNftToAnotherChain(uint64(0), address(0), 0);
vm.stopPrank();
}
Output
Logs:
::Tokens for new owner
@> We can see no tokens are accounted after the minting
@> And the tracecall failing, the function cannot found the data for the token
...
[9724] KittyConnect::mock_bridgeNftToAnotherChain(0, 0x0000000000000000000000000000000000000000, 0)
│ ├─ emit Transfer(from: newOwner: [0x7240b687730BE024bcfD084621f794C2e4F8408f], to: 0x0000000000000000000000000000000000000000, tokenId: 0)
│ └─ ← panic: arithmetic underflow or overflow (0x11)
└─ ← panic: arithmetic underflow or overflow (0x11)
Impact
Transfers of tokens received from another chain lack internal accounting, leading to failures in dependent functions such as the
bridgeNftToAnotherChain function.
Tools Used
Foundry and manual review
Recommendations
Add the line to account for the token being received before minting it.
Additionally, it's advisable to refactor the shared code between mintBridgedNFT and mintCatToNewOwner functions for improved code efficiency and maintenance.
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);
}
