Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #224

Open
code423n4 opened this issue May 30, 2022 · 1 comment
Open

QA Report #224

code423n4 opened this issue May 30, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

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
@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels May 30, 2022
code423n4 added a commit that referenced this issue May 30, 2022
@GalloDaSballo
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

2 participants