hats-finance / Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d

0 stars 1 forks source link

Potential Overwrite of Balances in `onAttach` Function #32

Open hats-bug-reporter[bot] opened 3 months ago

hats-bug-reporter[bot] commented 3 months ago

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

Description: Description\ The onAttach function in the CompoundVeFNXManagedNFTStrategyUpgradeable contract does not check whether the tokenId_ is already attached before calling the deposit function of the ISingelTokenVirtualRewarder contract. This can lead to overwriting balances, which may cause inconsistencies and potential loss of data.

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

  1. Proof of Concept (PoC) File

    The onAttach function in CompoundVeFNXManagedNFTStrategyUpgradeable.sol:

    function onAttach(uint256 tokenId_, uint256 userBalance_) external override onlyManagedNFTManager {
    ISingelTokenVirtualRewarder(virtualRewarder).deposit(tokenId_, userBalance_); //@audit-can only be called by strategy
    emit OnAttach(tokenId_, userBalance_);
    }

    The deposit function in SingelTokenVirtualRewarderUpgradeable.sol:

    function deposit(uint256 tokenId_, uint256 amount_) external onlyStrategy {
    if (amount_ == 0) {
        revert ZeroAmount();
    }
    
    TokenInfo storage info = tokensInfo[tokenId_];
    info.balance += amount_;
    totalSupply += amount_;
    
    uint256 currentEpoch = _currentEpoch();
    _writeCheckpoints(info, currentEpoch);
    
    emit Deposit(tokenId_, amount_, currentEpoch);
    }
    • Lack of Check for Existing Attachment: The onAttach function does not verify if the tokenId is already attached. This can lead to multiple attachments of the same tokenId, causing the balance to be overwritten or incorrectly updated.
  2. Revised Code File (Optional)

0xmahdirostami commented 3 months ago

check is done in onAttachToManagedNFT