Velodrome Finance contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L42

Vulnerability details

Impact

The Bribe.notifyRewardAmount function does not have any access restriction. Anyone (an attacker) can frontrun and call this function to add arbitrary (even malicious) reward tokens up to MAX_REWARD_TOKENS = 16.

An attacker is able to frontrun and add 16 fake ERC20 token contracts. Due to the limit of MAX_REWARD_TOKENS, no more reward tokens can be added. Any attempt to add a new token will revert.

This will prevent adding the proper reward tokens due to DoS when calling the Bribe.notifyRewardAmount and results in DoS when calling the Gauge.claimFees function.

Proof of Concept

Bribe.sol#L42

function notifyRewardAmount(address token, uint amount) external lock { // @audit-info no access restriction
    require(amount > 0);
    if (!isReward[token]) {
      require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
    }
    // bribes kick in at the start of next bribe period
    uint adjustedTstamp = getEpochStart(block.timestamp);
    uint epochRewards = tokenRewardsPerEpoch[token][adjustedTstamp];

    _safeTransferFrom(token, msg.sender, address(this), amount);
    tokenRewardsPerEpoch[token][adjustedTstamp] = epochRewards + amount;

    if (!isReward[token]) {
        isReward[token] = true;
        rewards.push(token);
        IGauge(gauge).addBribeRewardToken(token);
    }

    emit NotifyReward(msg.sender, token, adjustedTstamp, amount);
}

Tools Used

Manual review

Recommended mitigation steps

Consider adding access restriction to the Bribe.notifyRewardAmount function to prevent malicious actors from calling the function or add a whitelist of possible reward tokens.

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.