hats-finance / Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d

0 stars 1 forks source link

function `onDettachFromManagedNFT` has introduced a Panic error #57

Open hats-bug-reporter[bot] opened 1 month ago

hats-bug-reporter[bot] commented 1 month ago

Github username: @Vancelott Twitter username: vancelotx Submission hash (on-chain): 0x641247bab6c35cec178bc216a4bf8ce5036dd9e975f502df251634194d4525f7 Severity: low

Description: Description

The function onDettachFromManagedNFT has a check that verifies one of the parameters of a token, but the check is implemented via the function assert

Attack Scenario

Whenever a user calls onDettachFromManagedNFT there are two checks that verify the token is attached to a managed NFT:

        if (!tokenInfo.isAttached) {
            revert NotAttached();
        }

        assert(tokenInfo.attachedManagedTokenId != 0);

However, if the attachedManagedTokenId has value of 0, the user's NFT will never be dettached from the managed NFT. In addition to that, the assert function should only be used for internal testing, as it would cause a Panic error, as per the Solidity docs:

Assert should only be used to test for internal errors and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input.

Recommendations

Consider implementing the following changes by using one of the existent functions from the contract:

    function onDettachFromManagedNFT(uint256 tokenId_) external onlyVoter {
        TokenInfo memory tokenInfo = tokensInfo[tokenId_];

        if (!tokenInfo.isAttached) {
            revert NotAttached();
        }

-         assert(tokenInfo.attachedManagedTokenId != 0);
+        require(isManagedNFT(tokenInfo.attachedManagedTokenId), "Invalid ManagedTokenId");

        uint256 lockedRewards = IManagedNFTStrategy(IVotingEscrow(votingEscrow).ownerOf(tokenInfo.attachedManagedTokenId)).onDettach(
            tokenId_,
            tokenInfo.amount
        );

        IVotingEscrowV1_2(votingEscrow).onDettachFromManagedNFT(
            tokenId_,
            tokenInfo.attachedManagedTokenId,
            tokenInfo.amount + lockedRewards
        );

        delete tokensInfo[tokenId_];
    }