First Flight #12: Kitty Connect

Summary

Emitting the NFTBridgeRequestSent event after burning the token and cleaning up data means that observers won't have visibility into the bridging request if an error occurs during the burn or cleanup process. It's typically better to emit events first to ensure that important actions are logged, even if subsequent operations encounter issues.

Vulnerability Details

Function burns before emitting the NFTBridgeRequestSent

Impact

PoC

 // Test function to demonstrate the importance of emitting events before state changes
 function testBridgeNftToAnotherChain() public {


// Assume we have a cat NFT minted and ready for bridging
uint256 tokenId = 1;

// Call the function to bridge the NFT to another chain
// Assume the contract owner (msg.sender) initiates the bridging process
kittyConnect.bridgeNftToAnotherChain(1, address(kittyBridge), tokenId);

// Check that the event NFTBridgeRequestSent is emitted with the correct parameters
// Since the event should be emitted before state changes, the tokenId should still exist
// Thus, we can check if the tokenId is present in the contract after the function call
assertEq(true, true, "Function execution successful");

// For now, let's just check if the function was executed without reverting
// We can assume that if the function didn't revert, it means the event was emitted successfully
// But it reverts
assertEq(
    kittyConnect.getCatInfo(tokenId).catName,
    "",
    "Cat info should be empty after emitting event"
);

}

Tools Used

Manual analysis

Recommendations

It's typically better to emit events first to ensure that important actions are logged, even if subsequent operations encounter issues.

Here's how the code could be adjusted to emit the event before burning the token and cleaning up data:

    emit NFTBridgeRequestSent(
         block.chainid,
         destChainSelector,
         destChainBridge,
         tokenId
          );

          _burn(tokenId);
          delete s_catInfo[tokenId];

By making this adjustment, the event is emitted first, ensuring that it's logged even if an error occurs during the subsequent state changes.