QA Report

[code423n4]

Low Risk

1. Title : Missing checks for address(0x0) when assigning values to address state variables

1.) File : contracts/contracts/Voter.sol (Line.55)

        _ve = __ve;

2.) File : contracts/contracts/Voter.sol (Line.56)

        factory = _factory;

3.) File : contracts/contracts/Voter.sol (Line.58)

        gaugefactory = _gauges;

4.) File : contracts/contracts/Voter.sol (Line.59)

        bribefactory = _bribes;

5.) File : contracts/contracts/PairFees.sol (Line.15)

        token0 = _token0;

6.) File : contracts/contracts/PairFees.sol (Line.16)

        token1 = _token1;

7.) File : contracts/contracts/Velo.sol (Lines.68-72)

check account

         function mint(address account, uint amount) external returns (bool) {
        require(msg.sender == minter);
        _mint(account, amount);
        return true;
    }

8.) File : contracts/contracts/Gauge.sol (Lines.678-683)

check spender

    function _safeApprove(address token, address spender, uint256 value) internal {
        require(token.code.length > 0);
        (bool success, bytes memory data) =
        token.call(abi.encodeWithSelector(IERC20.approve.selector, spender, value));
        require(success && (data.length == 0 || abi.decode(data, (bool))));
    }

2. Title : Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

1.) File : contract/contracts/VotingEscrow.sol (Line.60)

    event Supply(uint prevSupply, uint supply);

3. Title : require() should be used instead of assert()

1.) File : main/contracts/contracts/VotingEscrow.sol (Line.262)

        assert(_operator != msg.sender);

2.) File : main/contracts/contracts/VotingEscrow.sol (Line.272)

        assert(idToOwner[_tokenId] == _owner);

3.) File : main/contracts/contracts/VotingEscrow.sol (Line.447)

       assert(idToOwner[_tokenId] == address(0));

4.) File : main/contracts/contracts/VotingEscrow.sol (Line.464)

       assert(_to != address(0));

5.) File : main/contracts/contracts/VotingEscrow.sol (Line.508)

       assert(idToOwner[_tokenId] == _from);

6.) File : main/contracts/contracts/VotingEscrow.sol (Line.748)

       assert(IERC20(token).transferFrom(from, address(this), _value));

7.) File : main/contracts/contracts/VotingEscrow.sol (Line.815)

        assert(_isApprovedOrOwner(msg.sender, _tokenId));

8.) File : main/contracts/contracts/VotingEscrow.sol (Line.819)

        assert(_value > 0); // dev: need non-zero value

9.) File : main/contracts/contracts/VotingEscrow.sol (Line.829)

         assert(_isApprovedOrOwner(msg.sender, _tokenId));

10.) File : main/contracts/contracts/VotingEscrow.sol (Line.845)

         assert(_isApprovedOrOwner(msg.sender, _tokenId));

11.) File : main/contracts/contracts/VotingEscrow.sol (Line.861)

          assert(IERC20(token).transfer(msg.sender, value));

12.) File : main/contracts/contracts/VotingEscrow.sol (Line.937)

          assert(_block <= block.number);

13.) File : main/contracts/contracts/VotingEscrow.sol (Line.991)

          assert(_block <= block.number);

4. Title : Lack of event

Event is an inheritable member of a contract. An event is emitted, it stores the arguments passed in transaction logs

1.) File : contracts/contracts/Velo.sol : (Lines.17-18)

Recommended Mitigation

adding event Mint

5. Title : Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

1.) File : contracts/contracts/VelodromeLibrary.sol (Line.9)

     // TODO make modifiable?

2.) File : contracts/contracts/VotingEscrow.sol (Line.39)

     * What we can do is to extrapolate ***At functions */

3.) File : contracts/contracts/VotingEscrow.sol#L633 (Line.633)

       // Hopefully it won't happen that this won't get used in 5 years!

4.) File : contracts/contracts/VotingEscrow.sol#L633 (Line.689)

        // old_dslope was <something> - u_old.slope, so we cancel that

6. Multiple address mappings can be combined into a `single mapping of an address to a struct

1.) File : contracts/contracts/Voter.sol (Lines.30-32)

    mapping(address => address) public gauges; // pool => gauge
    mapping(address => address) public poolForGauge; // gauge => pool
    mapping(address => address) public bribes; // gauge => bribe

7. Require to check if value > 0

1.) File : contracts/contracts/Velo.sol (Lines.68-72)

         function mint(address account, uint amount) external returns (bool) {
        require(msg.sender == minter);
        _mint(account, amount);
        return true;
   }

8. Need to be checked value so no transfer amount exceeeds allowance

File : contracts/contracts/Velo.sol (Lines.60-66)

Recommended Mitigation

adding

       require(_value <= allowed_from,
            "Velo: transfer amount exceeds allowance");

NON CRITICAL

9. Title : Consider make the constracts pausable

There are many external risks so the suggestion was it should be consider making the contracts pausable, so if in the case of an unexpected event, the admin can pause transfers.

Tool Used

Manual Review

POC

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol

Recommended Mitigation Steps

Consider making contracts Pausable

10. Title : NatSpec is incomplete

1.) File : contracts/contracts/VotingEscrow.sol (Line.222-224)

Missing @return

    /// @dev Checks if `_operator` is an approved operator for `_owner`.
    /// @param _owner The address that owns the NFTs.
    /// @param _operator The address that acts on behalf of the owner.
    function isApprovedForAll(address _owner, address _operator) external view returns (bool) {
        return (ownerToOperators[_owner])[_operator];
    }

2.) File : contracts/contracts/VotingEscrow.sol (Lines.778-781)

Missing @return

    /// @notice Deposit `_value` tokens for `_to` and lock for `_lock_duration`
    /// @param _value Amount to deposit
    /// @param _lock_duration Number of seconds to lock tokens for (rounded down to nearest week)
    /// @param _to Address to deposit
    function _create_lock(uint _value, uint _lock_duration, address _to) internal returns (uint) {
        uint unlock_time = (block.timestamp + _lock_duration) / WEEK * WEEK; // Locktime is rounded down to weeks

        require(_value > 0); // dev: need non-zero value
        require(unlock_time > block.timestamp, 'Can only lock until time in the future');
        require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 4 years max');

        ++tokenId;
        uint _tokenId = tokenId;
        _mint(_to, _tokenId);

        _deposit_for(_tokenId, _value, unlock_time, locked[_tokenId], DepositType.CREATE_LOCK_TYPE);
        return _tokenId;
    }

3.) File : contracts/contracts/VotingEscrow.sol (Lines.778-781)

Missing @return

    /// @notice Deposit `_value` tokens for `msg.sender` and lock for `_lock_duration`
    /// @param _value Amount to deposit
    /// @param _lock_duration Number of seconds to lock tokens for (rounded down to nearest week)
    function create_lock(uint _value, uint _lock_duration) external nonreentrant returns (uint) {
        return _create_lock(_value, _lock_duration, msg.sender);
    }

4.) File : contracts/contracts/VotingEscrow.sol (Lines.778-781)

Missing @return

    /// @notice Deposit `_value` tokens for `_to` and lock for `_lock_duration`
    /// @param _value Amount to deposit
    /// @param _lock_duration Number of seconds to lock tokens for (rounded down to nearest week)
    /// @param _to Address to deposit
    function create_lock_for(uint _value, uint _lock_duration, address _to) external nonreentrant returns (uint) {
        return _create_lock(_value, _lock_duration, _to);
    }

11. require()/revert() statements should have descriptive reason strings

1.) File : contracts/contracts/Gauge.sol (Line.512)

        require(amount > 0);

2.) File : contracts/contracts/Gauge.sol (Line.591)

        require(token != stake);

3.) File : contracts/contracts/Gauge.sol (Line.592)

        require(amount > 0);

4.) File : contracts/contracts/Gauge.sol (Line.613)

        require(rewardRate[token] > 0);

12. Title : Consider to use // SPDX-License-Identifier: MIT for interface for removing warning

Since this was out of scope but some of contracts was gonna do warning bout // SPDX , it better to used // SPDX for interface to remove this warn for good.

Tool Used

Remix

1.) File : contracts/contracts/interfaces/IGaugeFactory.sol (1)

2.) File : contracts/contracts/interfaces/IPairCallee.sol (2)

3.) File : contracts/contracts/interfaces/IERC20.sol (3)

4.) File : contracts/contracts/interfaces/IGauge.sol (4)

5.) FIle : contracts/contracts/interfaces/IMinter.sol (5)

6.) File : contracts/contracts/interfaces/IPair.sol (6)

7.) File : contracts/contracts/interfaces/IPairFactory.sol (7)

8.) File : contracts/contracts/interfaces/IVoter.sol (8)

9.) File : contracts/contracts/interfaces/IVotingEscrow.sol (9)

10.) File : contracts/contracts/interfaces/IVelo.sol (10)

11.) File : contracts/contracts/interfaces/IRewardsDistributor.sol (11)

[GalloDaSballo]

Title : Missing checks for address(0x0) when assigning values to address state variables

Valid Low

Title : Event is missing indexed fields

Disagree in this case, why would you index the amount?

Title : require() should be used instead of assert()

Valid Low

Title : Lack of event

Valid NC

Title : Open TODOs

Valid NC

[GalloDaSballo]

Multiple address mappings can be combined into a `single mapping of an address to a struct

Valid NC, and better as gas

7

Valid NC

8

It will underflow no need for check

9

Disagree

Title : NatSpec is incomplete

Valid NC

require()/revert() statements should have descriptive reason strings

Valid NC

 Title : Consider to use // SPDX-License-Identifier: MIT for interface for removing warning

Valid NC

Overall pretty good report that would benefit from better formatting

2L, 7 NC