Summary

In the divideNft function, the check for whether the NFT transfer was successful is done after the transfer.

Vulnerability Details

Specifically, the line of code IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, ""); comes before the conditional check if(IERC721(nftAddress).ownerOf(tokenId) == msg.sender) { revert TokenDivider__NftTransferFailed(); This means that the check for whether the NFT transfer was successful is done after the transfer has already taken place.

Impact

To understand the impact of this vulnerability, let's break it down further. When a user initiates a transfer of their NFT to the TokenDivider contract, the safeTransferFrom function is called. This function is supposed to transfer the NFT from the user's wallet to the contract. However, if the transfer fails for any reason (e.g., insufficient gas, network issues, etc.), the contract will not be aware of it until after the transfer has already occurred.

Tools Used

Manual review

Recommendations

To mitigate this issue, the ownership validation logic should be moved before the safeTransferFrom call. The code should check that the msg.sender is the current owner of the token before attempting the transfer.

Like this:

// Validate ownership before proceeding with the transfer

if (IERC721(nftAddress).ownerOf(tokenId) != msg.sender) {
revert TokenDivider__NftTransferFailed();
}

// Proceed with the transfer

IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");