hats-finance / Fenix-Finance-0x83dbe5aa378f3ce160ed084daf85f621289fb92f

0 stars 0 forks source link

Inconsistent between balanceOfNFT, balanceOfNFTAt and _balanceOfNFT functions #29

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

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

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

Description: Context: VotingEscrowUpgradeable.sol#L896, VotingEscrowUpgradeable.sol#L887

Description\ The balanceOfNFT function implements a flash-loan protection that returns zero voting balance if ownership_change[_tokenId] == block.number. However, this was not consistently applied to the balanceOfNFTAt and _balanceOfNFT functions.

    function balanceOfNFT(uint _tokenId) external view returns (uint) {
        if (ownership_change[_tokenId] == block.number) return 0;
        return _balanceOfNFT(_tokenId, block.timestamp);
    }

Attack Scenario\ As a result, Velodrome or external protocols calling the balanceOfNFT and balanceOfNFTAt external functions will receive different voting balances for the same veNFT depending on which function they called.

Additionally, the internal _balanceOfNFT function, which does not have flash-loan protection, is called by the VotingEscrowUpgradeable.getVotes function to compute the voting balance of an account. The VotingEscrowUpgradeable.getVotes function appears not to be used in any in-scope contracts, however, this function might be utilized by some external protocols or off-chain components to tally the votes. If that is the case, a malicious user could flash-loan the veNFTs to inflate the voting balance of their account.

Recommendation: If the requirement is to have all newly transferred veNFTs (ownership_change[_tokenId] == block.number) have zero voting balance to prevent someone from flash-loaning veNFT to increase their voting balance, the flash-loan protection should be consistently implemented across all the related functions.

BohdanHrytsak commented 4 months ago

Thank you for the submission.

This functionality is inherited from Thena & Chronos, and we can also see that Velodrome works in the same way. OOS, Although the functions do return a different result, this is done to prevent and protect the vote count with VoterUpgradeable.