sherlock-audit / 2024-06-velocimeter-judging

11 stars 7 forks source link

MohammedRizwan - `balanceOfNFTAt()` in `VotingEscrow.sol` does not implement flashloan protection similar to `balanceOfNFT()` function #658

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

MohammedRizwan

Medium

balanceOfNFTAt() in VotingEscrow.sol does not implement flashloan protection similar to balanceOfNFT() function

Summary

balanceOfNFTAt() in VotingEscrow.sol does not implement flashloan protection similar to balanceOfNFT() function

Vulnerability Detail

_This issue is actually referenced from spearbit's Velodrome audit which can be checked here_

The balanceOfNFT() function implements a flash-loan protection that returns zero voting balance if ownership_change[_tokenId] == block.number.

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

However, this was not consistently applied to the balanceOfNFTAt()and _balanceOfNFT() functions.

    function balanceOfNFTAt(uint _tokenId, uint _t) external view returns (uint) {
        return _balanceOfNFT(_tokenId, _t);         @audit // missing flashloan protection
    }

As a result, Velocimeter 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 and its used in following functions:

1) tokenURI() 2) getVotes() 3) getPastVotes()

The vote function getters computes the voting balance of an account and can be used to inflate the voting balance.

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.

Impact

Malicious user could flash-loan the veNFTs to inflate the voting balance of their account, since users are able to inflate their voting power, which they can use to vote for a malicious proposal

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1031-L1038

Tool used

Manual Review

Recommendation

Flashloan protection check should be implemented in balanceOfNFTAt() and _balanceOfNFT() functions and to be inconsistent with flashloan prevented balanceOfNFT() function.

Duplicate of #286