hats-finance / Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d

0 stars 1 forks source link

Panic Error is not handled properly #42

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

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

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

Description: Description

SingelTokenVirtualRewarderUpgradeable::deposit and withdraw can give panic error.

From the solidity docs

Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic

"Panic" refers to a condition indicating that the EVM has encountered an issue it cannot handle

Proof of Concept (PoC) File

    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);
    }
    function withdraw(uint256 tokenId_, uint256 amount_) external onlyStrategy {
        TokenInfo storage info = tokensInfo[tokenId_];
        if (info.balance == 0 || amount_ == 0) {
            revert ZeroAmount();
        }

        info.balance -= amount_;
        totalSupply -= amount_;

        uint256 currentEpoch = _currentEpoch();
        _writeCheckpoints(info, currentEpoch);

        emit Withdraw(tokenId_, amount_, currentEpoch);
    }

deposit will give panic error due to overflow and withdraw due to underflow.

Revised Code File (Optional)

Handle such panic errors and revert with Error

"Error" uses the "REVERT" opcode to halt execution and revert any changes made to the contract state. This allows the contract to gracefully handle unexpected conditions and return to a known, stable state

eg

    error InvalidAmount();

    function withdraw(uint256 tokenId_, uint256 amount_) external onlyStrategy {
        TokenInfo storage info = tokensInfo[tokenId_];
        if (info.balance == 0 || amount_ == 0) {
            revert ZeroAmount();
        }
       if(info.balance<amount || totalSupply < amount){
           revert InvalidAmount();
         }
        info.balance -= amount_;
        totalSupply -= amount_;

        uint256 currentEpoch = _currentEpoch();
        _writeCheckpoints(info, currentEpoch);

        emit Withdraw(tokenId_, amount_, currentEpoch);
    }
0xmahdirostami commented 1 month ago

Best practice

Tri-pathi commented 1 month ago

@0xmahdirostami

Low Severity: These are typically minor issues that pose a limited impact on the system. They might include: Inefficiencies in gas usage. Minor deviations from best practices that don't lead to security risks. Small bugs that do not affect the protocol's functionality or security.

I think it's more than best practices. A audited code should never have panic error. This effects EVM in the backend. Please revaluate according to Hats rule.