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

The function `KittyConnect::bridgeNftToAnotherChain` burns the NFT before verifying the success of the bridge on the destination chain causing potential loss of users' assets

seeu

Summary

The function KittyConnect::bridgeNftToAnotherChain is designed to facilitate the bridging of Non-Fungible Tokens (NFTs) from one blockchain to another. However, it suffers from a critical flaw where the user's NFT is burned before ensuring the successful completion of the bridging process. This creates a vulnerability where if the bridging operation fails after the user's NFT is burned, the asset is irreversibly lost.

Vulnerability Details

In the following code, you could notice the process of the function with added comments.

Code
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
);
​
// The NFT is burned in the following two lines
_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;
}
​
// The NFT is then bdriged with KittyBridge::bridgeNftWithData
// If it reverts, the user's NFT is lost as it has already been burned in the previous operations
emit NFTBridgeRequestSent(
block.chainid,
destChainSelector,
destChainBridge,
tokenId
);
​
i_kittyBridge.bridgeNftWithData(
destChainSelector,
destChainBridge,
data
);
}

You can add the following test to the KittyTest.t.sol test suite (Note: the test will fail and that's ok for the following inspections). Run it with the command forge test --mt test_bridgeNftBurnOnRevert -vvv.

Code
function test_bridgeNftBurnOnRevert() public {
address catOwner = makeAddr("catOwner");
​
string
memory catImageIpfsHash = "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62";
vm.warp(block.timestamp + 10 weeks);
​
uint256 tokenId = kittyConnect.getTokenCounter();
​
vm.expectEmit(false, false, false, true);
emit CatMinted(tokenId, catImageIpfsHash);
vm.startPrank(partnerA);
kittyConnect.mintCatToNewOwner(
catOwner,
catImageIpfsHash,
"Meowdy",
"Ragdoll",
block.timestamp - 3 weeks
);
vm.stopPrank();
​
// Bridge the NFT to another chain
vm.startPrank(catOwner);
kittyConnect.bridgeNftToAnotherChain(
123,
kittyConnect.getKittyBridge(),
tokenId
);
vm.stopPrank();
​
vm.prank(catOwner);
}

The output of the test would be like the following. Notice the emission of the event emit Transfer(from: catOwner: [0x9E472C0AD173aef746b6635407B2D22cD835e428], to: 0x0000000000000000000000000000000000000000, tokenId: 0) coming from _burn(tokenId); in the function KittyConnect::bridgeNftToAnotherChain.

Output
Ran 1 test for test/KittyTest.t.sol:KittyTest
[FAIL. Reason: KittyBridge__DestinationChainNotAllowlisted(123)] test_bridgeNftBurnOnRevert() (gas: 307754)
Traces:
[307754] KittyTest::test_bridgeNftBurnOnRevert()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← catOwner: [0x9E472C0AD173aef746b6635407B2D22cD835e428]
├─ [0] VM::label(catOwner: [0x9E472C0AD173aef746b6635407B2D22cD835e428], "catOwner")
│ └─ ← ()
├─ [0] VM::warp(6048001 [6.048e6])
│ └─ ← ()
├─ [2381] KittyConnect::getTokenCounter() [staticcall]
│ └─ ← 0
├─ [0] VM::expectEmit(false, false, false, true)
│ └─ ← ()
├─ emit CatMinted(tokenId: 0, catIpfsHash: "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62")
├─ [0] VM::startPrank(0x70997970C51812dc3A010C7d01b50e0d17dc79C8)
│ └─ ← ()
├─ [264069] KittyConnect::mintCatToNewOwner(catOwner: [0x9E472C0AD173aef746b6635407B2D22cD835e428], "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62", "Meowdy", "Ragdoll", 4233601 [4.233e6])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: catOwner: [0x9E472C0AD173aef746b6635407B2D22cD835e428], tokenId: 0)
│ ├─ emit CatMinted(tokenId: 0, catIpfsHash: "ipfs://QmbxwGgBGrNdXPm84kqYskmcMT3jrzBN8LzQjixvkz4c62")
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [0] VM::startPrank(catOwner: [0x9E472C0AD173aef746b6635407B2D22cD835e428])
│ └─ ← ()
├─ [259] KittyConnect::getKittyBridge() [staticcall]
│ └─ ← KittyBridge: [0x2b42C737b072481672Bb458260e9b59CB2268dc6]
├─ [23088] KittyConnect::bridgeNftToAnotherChain(123, KittyBridge: [0x2b42C737b072481672Bb458260e9b59CB2268dc6], 0)
│ ├─ emit Transfer(from: catOwner: [0x9E472C0AD173aef746b6635407B2D22cD835e428], to: 0x0000000000000000000000000000000000000000, tokenId: 0)
│ ├─ emit NFTBridgeRequestSent(sourceChainId: 31337 [3.133e4], destChainSelector: 123, destBridge: KittyBridge: [0x2b42C737b072481672Bb458260e9b59CB2268dc6], tokenId: 0)
│ ├─ [3160] KittyBridge::bridgeNftWithData(123, KittyBridge: [0x2b42C737b072481672Bb458260e9b59CB2268dc6], 0x0000000000000000000000009e472c0ad173aef746b6635407b2d22cd835e42800000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000140000000000000000000000000000000000000000000000000000000000040998100000000000000000000000070997970c51812dc3a010c7d01b50e0d17dc79c800000000000000000000000000000000000000000000000000000000000000064d656f77647900000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007526167646f6c6c000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000035697066733a2f2f516d62787747674247724e6458506d38346b7159736b6d634d54336a727a424e384c7a516a6978766b7a346336320000000000000000000000)
│ │ └─ ← KittyBridge__DestinationChainNotAllowlisted(123)
│ └─ ← KittyBridge__DestinationChainNotAllowlisted(123)
└─ ← KittyBridge__DestinationChainNotAllowlisted(123)

Impact

If i_kittyBridge.bridgeNftWithData reverts, the user's NFT is burned and lost without the possibility to recover it.

Tools Used

VSCodium, Foundry

Recommendations

It is adivsable to ensure i_kittyBridge.bridgeNftWithData succeed before proceeding with burning the user's NFT. The fixed function might look like this:

Code
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
);
​
+ _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;
+ }
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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