Missing isAttachedNFT Check in attachManagedNFT Function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xd83ef350eea26feabc117f92110bc83d3aa3994e1c52a9bc42512f6f29c6b051 Severity: medium

Description: Description\ The attachManagedNFT function in the BaseManagedNFTStrategyUpgradeable contract currently lacks a check to verify if the NFT is already attached to another managed token. This omission can lead to potential conflicts and unintended behavior, as the same NFT might be managed by multiple strategies simultaneously.

The current implementation of the attachManagedNFT function checks if the managedTokenId is already set, verifies the validity of the NFT, and ensures that the contract owns the NFT. However, it does not check if the NFT is already attached to another managed token.

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

1. Proof of Concept (PoC) File

   function attachManagedNFT(uint256 managedTokenId_) external onlyAdmin {
   if (managedTokenId != 0) {
       revert AlreadyAttached();
   }//@audit-NO CHECK OF `isAttachedNFT`
   if (
       !IManagedNFTManager(managedNFTManager).isManagedNFT(managedTokenId_) ||
       IVotingEscrow(votingEscrow).ownerOf(managedTokenId_) != address(this)
   ) {
       revert IncorrectManagedTokenId();
   }
   
   managedTokenId = managedTokenId_;
   emit AttachedManagedNFT(managedTokenId_);
   }

2. Revised Code File (Optional)

   • To ensure that the NFT is not already attached to another managed token, we need to add a check using the isAttachedNFT function from the ManagedNFTManagerUpgradeable contract. This function returns true if the NFT is already attached to any managed token.

function attachManagedNFT(uint256 managedTokenId_) external onlyAdmin {
    if (managedTokenId != 0) {
        revert AlreadyAttached();
    }
    if (
        !IManagedNFTManager(managedNFTManager).isManagedNFT(managedTokenId_) ||
        IVotingEscrow(votingEscrow).ownerOf(managedTokenId_) != address(this) ||
        IManagedNFTManager(managedNFTManager).isAttachedNFT(managedTokenId_)
    ) {
        revert IncorrectManagedTokenId();
    }

    managedTokenId = managedTokenId_;
    emit AttachedManagedNFT(managedTokenId_);
}

[0xmahdirostami]

What is the impact here?

[BohdanHrytsak]

isAttachedNFT is not needed, since you can see that it checks whether the counterparty is the owner of this NFT, so only he can own it. The very concept of pulling a token from another strategy to attach it to another requires preparation, contract upgrades, etc. and is not provided by the existing functionality