code-423n4 / 2022-05-velodrome-findings

0 stars 0 forks source link

QA Report #224

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Mixed used of uint and uint256

Contract VeloGovernor.sol uses uint256 while the other contracts use uint

NATSPEC INCOMPLETE

File: VotingEscrow.sol 815

missing @param tokenid

    /// @notice Deposit `_value` additional tokens for `_tokenId` without modifying the unlock time
    /// @param _value Amount of tokens to deposit and add to the lock
    function increase_amount(uint _tokenId, uint _value) external nonreentrant {
        assert(_isApprovedOrOwner(msg.sender, _tokenId));

Other Instances: File: VotingEscrow.sol 826

File: VotingEscrow.sol line 162 Missing @ return

File: VotingEscrow.sol 216-218 Missing @ return

File: VotingEscrow.sol 190-193 Missing @ return

File: VotingEscrow.sol 186 Missing @ return

File: VotingEscrow.sol line 414 Missing @ return

TYPO

File: VotingEscrow.sol line 295 /// @dev Exeute transfer of a NFT. Exeute Instead of Execute

Overflow is desired

File: Pair.sol line 232

// subtraction overflow is desired
            uint timeElapsed = blockTimestamp - _blockTimestampLast;

The comment in above says that overflow is desired (if that's the desired output then we should use unchecked to ensure we get the desired results)

// subtraction overflow is desired
unchecked{
       uint timeElapsed = blockTimestamp - _blockTimestampLast;
}

File: Pair.sol line 206

        uint timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired

Note the comment says overflow is desired. If this is the case then we would still not achieve the desired output as solidity 0.8+ has built in checks for ensuring that our arithmetic operations do not overflow.

Use of both named returns and return statements

When we have a named return we should not use return statements for clarity

File: Pair.sol 253line 253

    // as per `current`, however allows user configured granularity, up to the full window size
    function quote(address tokenIn, uint amountIn, uint granularity) external view returns (uint amountOut) {
        uint [] memory _prices = sample(tokenIn, amountIn, granularity, 1);
        uint priceAverageCumulative;
        for (uint i = 0; i < _prices.length; i++) {
            priceAverageCumulative += _prices[i];
        }
        return priceAverageCumulative / granularity;
    }

TODO left in the code

File: line 524

File minter.sol line 11

File: VotingEscrow.sol line 314

         _removeTokenFrom(_from, _tokenId);
        // TODO delegates
        // auto re-delegate
GalloDaSballo commented 2 years ago

Mixed used of uint and uint256

Valid NC

NATSPEC INCOMPLETE

Disagree that natspec has to be on every param

TYPO

Valid NC

Overflow is desired

Valid Low

TODO left in the code

Valid NC

Short and sweet, 1L 3 NC