QA Report

[code423n4]

L01 - Lack of event emitting after sensitive actions

Contracts do not emit relevant events after setting sensitive variables. Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity in following functions:

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Minter.sol#L64-L78

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L318-L321

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Velo.sol#L26-L34

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VeloGovernor.sol#L39-L48

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L82-90

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L1059-L1062

L02 - Missing zero address checks in sensitive setters

Some functions missing zero address checks when setting admin-like role addresses, which could lead to loss of control.

Apply a zero-address check and consider implementing a two-step process like in transferOwnership pattern, where the active role address assigns an account and the designated account must call the acceptOwnership() function for full transfer of role.

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/RewardsDistributor.sol#L318-L321

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Velo.sol#L26-L29

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VeloGovernor.sol#L39-L42

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Voter.sol#L82-90

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L1059-L1062

L03 - Missing input validation of array lengths

Function initialize() fails to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior.

    function initialize( 
        address[] memory claimants,
        uint[] memory amounts,
        uint max // sum amounts / max = % ownership of top protocols, so if initial 20m is distributed, and target is 25% protocol ownership, then max - 4 x 20m = 80m
    ) external {
        require(initializer == msg.sender);
        _velo.mint(address(this), max);
        _velo.approve(address(_ve), type(uint).max);
        for (uint i = 0; i < claimants.length; i++) { 
            _ve.create_lock_for(amounts[i], LOCK, claimants[i]);
        }
        initializer = address(0);
        active_period = ((block.timestamp + WEEK) / WEEK) * WEEK; 
    }

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Minter.sol#L49-L62

N01 - Function state mutability can be restricted to pure

Function doesn't change and can be declared as pure:

  function getEpochStart(uint timestamp) public view returns (uint) {
    uint bribeStart = timestamp - (timestamp % (7 days)) + BRIBE_LAG;
    uint bribeEnd = bribeStart + DURATION - COOLDOWN;
    return timestamp < bribeEnd ? bribeStart : bribeStart + 7 days;
  }

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Bribe.sol#L35-L39

N02 - Missing revert strings in require statements

Multiple times missing revert string in require statements, which could lead to inability to determine why transactions have failed, for example:

contracts/Gauge.sol:348    require(msg.sender == account || msg.sender == voter); 
contracts/Router.sol:164    require(amountADesired >= amountAMin); 
contracts/Voter.sol:232   require(isGauge[msg.sender]); 

N03 - Open TODO

contracts/VelodromeLibrary.sol:9    IRouter internal immutable router; // TODO make modifiable?

N04 - Typos

contracts/Voter.sol:103 for (uint i = 0; i < _poolVoteCnt; i ++) {  // extra space in i++
contracts/Voter.sol:128 for (uint i = 0; i < _poolCnt; i ++) {  // extra space in i++
contracts/VotingEscrow.sol:111  modifier nonreentrant() { // should be capital R
contracts/VotingEscrow.sol:295  /// @dev Exeute transfer of a NFT. // Execute
contracts/VotingEscrow.sol:575  /// @param old_locked Pevious locked amount / end lock time for the user // Previous

[GalloDaSballo]

Findings are valid, report is well formatted and (almost) provides links to all findings

Believe this to be Non-Critical -> L01 - Lack of event emitting after sensitive actions

2 L, 5 NC