QA Report

[code423n4]

QA Report

Table of Contents

• Non-Critical Findings
  • [NC-01] - Lack of require messages
  • [NC-02] - Misleading storage variable name
  • [NC-03] - Function state mutability can be restricted to pure
• Low Risk
  • [L-01] - Events not emitted for important state changes
  • [L-02] - Zero-address checks are missing
  • [L-03] - Open @TODOs left in the code
  • [L-04] - Single-step process for critical ownership/governance transfer is risky

Non-Critical Findings

[NC-01] - Lack of require messages

Description

A message should be specified in the require statement so that if the condition fails, the caller can know the reason for failing.

Findings

Require messages are missing in many cases across contracts. You can find all occurrences with the following regex:

require\([^,]*\);$

Recommended mitigation steps

Add require messages

[NC-02] - Misleading storage variable name

Description

The storage variable last_gauge suggests storing the last gauge. However, it gets the address of the most recently created Bribe assigned.

Findings

factories/BribeFactory.sol#L7

address public last_gauge; // @audit-info misleading variable name - should be `last_bribe`

Recommended mitigation steps

Consider renaming the variable to something like last_bribe.

[NC-03] - Function state mutability can be restricted to pure

Description

Functions that do not read and modify any storage variables and only use the arguments provided, can be restricted to pure.

Findings

Bribe.getEpochStart

Recommended mitigation steps

Set function state mutability to pure.

Low Risk

[L-01] - Events not emitted for important state changes

Description

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

Bribe.sol

L32\ L71\ L80

Gauge.sol

L631\ L644

Minter.sol

L66\ L71\ L77

factories/PairFactory.sol

L42\ L47\ L52\ L57\ L62\ L65

Recommended mitigation steps

Emit events for state variable changes.

[L-02] - Zero-address checks are missing

Description

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters.

Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens or force redeployment of contracts.

Findings

Bribe.sol

L32: gauge = _gauge;

Gauge.sol

97: stake = _stake;\ 98: bribe = _bribe;\ 99: _ve = __ve;\ 100: voter = _voter;

Minter.sol

L43: _voter = IVoter(__voter);\ L44: _ve = IVotingEscrow(__ve);\ L45: _rewards_distributor = IRewardsDistributor(__rewards_distributor);

VeloGovernor.sol

L41: team = newTeam;

factories/GaugeFactory.sol

L14: pairFactory = _pairFactory;\ L19: team = _team;

redeem/RedemptionReceiver.sol

L25: USDC = IERC20(_usdc);\ L26: VELO = IVelo(_velo);\ L28: endpoint = _endpoint;\ L53: fantomSender = _fantomSender;

redeem/RedemptionSender.sol

L22: weve = _weve;\ L24: endpoint = _endpoint;\ L25: optimismReceiver = _optimismReceiver;

Recommended mitigation steps

Add zero-address checks, e.g.:

require(_asset != address(0), "Zero-address");

[L-03] - Open @TODOs left in the code

Description

There are several open TODOs left in the code.

Findings

VotingEscrow.sol#L314

// TODO delegates

VotingEscrow.sol#L465

// TODO add delegates

VotingEscrow.sol#L524

// TODO add delegates

Minter.sol#L11

// TODO: decide on whether to abstract from VELO or not. currently, it's only somewhat abstracted (e.g. L38) // @audit open todo @LOW

VelodromeLibrary.sol#L9

IRouter internal immutable router; // TODO make modifiable?

Recommended mitigation steps

Check, fix and remove the todos before it is deployed in production

[L-04] - Single-step process for critical ownership/governance transfer is risky

Description

The team plays a critical role in the Velodrome protocol.

Given that changing team uses a single-step process for ownership transfer, it is very risky to perform those changes in a single step because it is irrecoverable from any mistakes.

Findings

factories/GaugeFactory.sol#L17-L20

function setTeam(address _team) external {
    require(msg.sender == team);
    team = _team;
}

VeloGovernor.sol#L39-L42

function setTeam(address newTeam) external {
    require(msg.sender == team, "not team");
    team = newTeam;
}

Recommended mitigation steps

Consider implementing a two-step process where the current team nominates an account and the nominated account needs to call an acceptTeam() function (similar as it's already done in the Minter contract) for the transfer of team ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

[GalloDaSballo]

[NC-01] - Lack of require messages

NC

[NC-02] - Misleading storage variable name

NC

[NC-03] - Function state mutability can be restricted to pure

Valid NC

[L-01] - Events not emitted for important state changes

Valid NC

 [L-02] - Zero-address checks are missing

Valid Low

[L-03] - Open @TODOs left in the code

Valid NC

[L-04] - Single-step process for critical ownership/governance transfer is risky

Valid NC

Nice short and sweet report, 1L 6 NC