First Flight #12: Kitty Connect

Summary

When a user uses 'function safeTransferFrom' and a cat NFT has a newOwner, we have to check if the cat NFT has transferred to newOwner or not?

Vulnerability Details

The function does not check if the nFT has transferred or not

Impact

In the safeTransferFrom function, we're specifically checking if the currCatOwner is the current owner of the cat NFT and if newOwner is approved to receive the transfer. However, in the bridgeNftToAnotherChain function, we're assuming that the caller (msg.sender) is the current owner of the cat NFT without explicitly checking if it is the newOwner.

To maintain consistency and ensure that the ownership transfer is properly handled before bridging the NFT to another chain, we should modify the bridgeNftToAnotherChain function to check if the caller (msg.sender) is indeed the current owner (catOwner) or the approved new owner (newOwner).

PoC

Test Function:

   // Test function to demonstrate that transferred cat NFT cannot be used in bridgeNftToAnotherChain function
       function testCannotUseTransferredCatInBridgeFunction() public {
       // Mint a cat NFT and transfer it to a new owner
    uint256 tokenId = 1;
    string memory catIpfsHash = "hash123";
    string memory catName = "Fluffy";
    string memory breed = "Persian";
    uint256 dob = 20220101;
    kittyConnect.mintCatToNewOwner(
        partnerA,
        catIpfsHash,
        catName,
        breed,
        dob
    );

    // Attempt to bridge the NFT to another chain using the new owner's address
    try
        kittyConnect.bridgeNftToAnotherChain(
            1,
            address(kittyBridge),
            tokenId
        )
    {
        revert("Expected bridgeNftToAnotherChain to revert");
    } catch Error(string memory /*reason*/) {
        // Expected revert, test passes
        assert(true);
    } catch (bytes memory /*error*/) {
        assert(false);
    }
}

Tools Used

Manual analysis

Recommendations

Here's how you can modify the bridgeNftToAnotherChain function to incorporate this check:

    function bridgeNftToAnotherChain(
    uint64 destChainSelector,
     address destChainBridge,
     uint256 tokenId
      ) external {
  // Determine the current owner of the cat NFT
 address catOwner = _ownerOf(tokenId);
 address approvedOwner = getApproved(tokenId);

// Check if the caller is the current owner or the approved new owner
require(msg.sender == catOwner || msg.sender == approvedOwner, "KittyConnect__NotOwnerOrApproved");

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
);

}

With this modification, the bridgeNftToAnotherChain function checks if the caller (msg.sender) is either the current owner (catOwner) or the approved new owner (approvedOwner) before proceeding with the bridging process. This ensures that the ownership transfer has been properly handled before initiating the bridge to another chain.