Velodrome Finance contest - xiaoming90's results

Lines of code

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

Vulnerability details

Background

Based on the code of Gauge contract, there are two types of rewards that can be claimed from the Gauge contract by its users.

1. Gauge Rewards - For users (Liquidity providers) who deposit their LP tokens (a.k.a LP Token Staker) into the liquidity gauge. New VELO tokens will be minted weekly and portion of the newly minted VELO tokens will be allocated to the gauges. Coin rates which the gauge will be getting depends on the gauge weight. Each LP Token staker receives a share of newly minted VELO proportional to the amount of LP tokens locked.
2. Bribe Rewards - For veVELO token holders who voted for a given gauge with bribe rewards attached. They will be entitled a portion of the bribe rewards.

It appears that Velodrome's Gauge contract attempted to implement a single unified reward system for both LP Token Staker and veVELO holders for their Gauge Rewards and Bribe Rewards respectively.

Vulnerability Details

It was observed that a LP Token Staker will not be able to get their gauge rewards unless they hold veVELO token and has voted in a liquidity gauge.

This is an issue because a LP Token Staker should be able to get their gauge rewards regardless of whether they have voted or not. As far as I'm aware, all the major protocols do not require their LP Token Staker to vote in order to receive their gauge rewards. A LP Token Staker is not always a Voter, and vice versa.

Proof-of-Concept

The following show the Gauge.earned function, which is used to calculated the gauge rewards earned by LP Token Staker. The if (cp.voted) { condition will cause LP Token Stakers who did not vote to be not able to receive any gauge rewards as the following piece of code will not be reached/executed:

reward += cp0.balanceOf * (_rewardPerTokenStored1 - _rewardPerTokenStored0) / PRECISION;

Thus, the reward variable will always be 0.

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

// earned is an estimation, it won't be exact till the supply > rewardPerToken calculations have run
function earned(address token, address account) public view returns (uint) {
	uint _startTimestamp = Math.max(lastEarn[token][account], rewardPerTokenCheckpoints[token][0].timestamp);
	if (numCheckpoints[account] == 0) {
		return 0;
	}

	uint _startIndex = getPriorBalanceIndex(account, _startTimestamp);
	uint _endIndex = numCheckpoints[account]-1;

	uint reward = 0;

	if (_endIndex > 0) {
		for (uint i = _startIndex; i < _endIndex; i++) {
			Checkpoint memory cp0 = checkpoints[account][i];
			Checkpoint memory cp1 = checkpoints[account][i+1];
			(uint _rewardPerTokenStored0,) = getPriorRewardPerToken(token, cp0.timestamp);
			(uint _rewardPerTokenStored1,) = getPriorRewardPerToken(token, cp1.timestamp);
			// @audit - User who did not vote will not receive any reward, including LP Token stakers.
			if (cp0.voted) {
				reward += cp0.balanceOf * (_rewardPerTokenStored1 - _rewardPerTokenStored0) / PRECISION;
			}
		}
	}

	Checkpoint memory cp = checkpoints[account][_endIndex];
	uint lastCpWeeksVoteEnd = cp.timestamp - (cp.timestamp % (7 days)) + BRIBE_LAG + DURATION;
	if (block.timestamp > lastCpWeeksVoteEnd) {
		(uint _rewardPerTokenStored,) = getPriorRewardPerToken(token, cp.timestamp);
		// @audit - User who did not vote will not receive any reward, including LP Token stakers.
		if (cp.voted) {
			reward += cp.balanceOf * (rewardPerToken(token) - Math.max(_rewardPerTokenStored, userRewardPerTokenStored[token][account])) / PRECISION;
		}
	}

	return reward;
}

Recommended Mitigation Steps

Update the reward system in Gauge so that LP Token Staker who did not vote are still able to get their gauge rewards. Instead of having a single contract that handles both gauge and bribe rewards logic, consider segregating gauge and bribe reward logic into two separate contracts, similar to Solidly design.

In Solidy, bribe rewards are added to BaseV1-bribes.sol while gauge rewards are added to BaseV1-gauges.sol. Bribers call BaseV1-bribes.getReward() to get their bribe rewards while LP Token stakers call BaseV1-gauges.getReward() to get their gauge rewards. This approach supports clear separation of duties.

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L35 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L41 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Voter.sol#L315 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L173 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L83

Vulnerability details

Vulnerability Details

Bribe rewards added to the Bribe contract in the first epoch will not be claimable by any voters, and the rewards will struck in the Bribe contract.

Proof-of-Concept

Assume that the current epoch is epoch 0, and start date of epoch 0 is Day 0.

When a briber adds a new rewards by calling Bribe.notifyRewardAmount(), the Bribe.getEpochStart() will return the start date of current epoch (epoch 0) + 1 day (Bribe Lag)

Thus, adjustedTstamp will be set to Day 1. tokenRewardsPerEpoch[token][adjustedTstamp] will evaluate to tokenRewardsPerEpoch[DAI][Day 1] and the briber's rewards will be stored in tokenRewardsPerEpoch[DAI][Day 1]

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

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/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L41

function notifyRewardAmount(address token, uint amount) external lock {
  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);
}

On Day 6, the voting phase has ended and the state is currently in the reward phase. Alice decided to call the Voter.distribute to trigger the distribution of bribe rewards.

However, the main issue is that calling the Voter.distribute function on Epoch 0's Day 6 (Reward Phase) will not executed theGauge.deliverBribes() because claimable[_gauge] or _claimable is currently 0.

Gauge.deliverBribes() is the main function responsible for distributing bribe rewards. Since Gauge.deliverBribes() cannot be triggered, the bribe rewards are forever struck in the Bribe Contract.

claimable[_gauge] will always be zero on the first epoch because the gauge rewards will only come in the later epoch. The value ofclaimable[_gauge] will only increase when the Minter.update_period() function starts minting VELO and distribute them to the gauges. Per the source code of Minter contract, the VELO emission will only start from third epoch onwards (active_period = ((block.timestamp + (2 * WEEK)) / WEEK) * WEEK;). Thus, before the VELO emission, claimable[_gauge]will always remain at 0.

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

function distribute(address _gauge) public lock {
	require(isAlive[_gauge]); // killed gauges cannot distribute
	uint dayCalc = block.timestamp % (7 days);
	require((dayCalc < BRIBE_LAG) || (dayCalc > (DURATION + BRIBE_LAG)), "cannot claim during votes period");
	IMinter(minter).update_period();
	_updateFor(_gauge);
	uint _claimable = claimable[_gauge];
	if (_claimable > IGauge(_gauge).left(base) && _claimable / DURATION > 0) {
		claimable[_gauge] = 0;
		IGauge(_gauge).notifyRewardAmount(base, _claimable);
		emit DistributeReward(msg.sender, _gauge, _claimable);
		// distribute bribes & fees too
		IGauge(_gauge).deliverBribes();
	}
}

If someone attempt to call Voter.distribute() on epoch 1 or subsequent epoch, it will fetch the bribe rewards in their respective epoch.

In the Gauge.deliverBribes function, the code uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG; will calculate the start date of current epoch + BRIBE_LAG (1 day). So, if someone call Gauge.deliverBribes in epoch 1, the bribeStart variable will be set to the Epoch 1 + 1 day, which is equivalent to Day 9. There is no way to fetch the bribe rewards struck in epoch 0.

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

function deliverBribes() external lock {
	require(msg.sender == voter);
	IBribe sb = IBribe(bribe);
	uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG;
	uint numRewards = sb.rewardsListLength();

	for (uint i = 0; i < numRewards; i++) {
		address token = sb.rewards(i);
		uint epochRewards = sb.deliverReward(token, bribeStart);
		if (epochRewards > 0) {
			_notifyBribeAmount(token, epochRewards, bribeStart);
		}
	}
}

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

function deliverReward(address token, uint epochStart) external lock returns (uint) {
	require(msg.sender == gauge);
	uint rewardPerEpoch = tokenRewardsPerEpoch[token][epochStart];
	if (rewardPerEpoch > 0) {
	  _safeTransfer(token, address(gauge), rewardPerEpoch);
	}
	return rewardPerEpoch;
}

Recommended Mitigation Steps

Implement logic to handle the edge case where bribe rewards are added during first epoch. Consider aligning the start of bribe period with VELO emission period.

Lines of code

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

Vulnerability details

Proof-Of-Concept

The Gauge.notifyRewardAmount notifies the contract of a newly received rewards. This updates the local accounting and streams the reward over a preset period (Five days).

It was observed that this function is callable by anyone regardless of whether the previous reward period has already expired or not. Thus, it would be possible to exploit the system by repeatedly calling it with dust reward amount to extend an active reward period, and thus dragging out the duration over which the rewards are released.

Since Velodrome is to be deployed on Layer 2 blockchain where the gas fee tends to be cheap, the effort required to carry out this attack would be minimal.

This issue was also highlighted in Curve documentation under the ChildChainStreamer.notify_reward_amount(token: address): section.

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

function notifyRewardAmount(address token, uint amount) external lock {
    require(token != stake);
    require(amount > 0);
    if (!isReward[token]) {
        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
    }
    // rewards accrue only during the bribe period
    uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG;
    uint adjustedTstamp = block.timestamp < bribeStart ? bribeStart : bribeStart + 7 days;
    if (rewardRate[token] == 0) _writeRewardPerTokenCheckpoint(token, 0, adjustedTstamp);
    (rewardPerTokenStored[token], lastUpdateTime[token]) = _updateRewardPerToken(token);
    _claimFees();

    if (block.timestamp >= periodFinish[token]) {
        _safeTransferFrom(token, msg.sender, address(this), amount);
        rewardRate[token] = amount / DURATION;
    } else {
        uint _remaining = periodFinish[token] - block.timestamp;
        uint _left = _remaining * rewardRate[token];
        require(amount > _left);
        _safeTransferFrom(token, msg.sender, address(this), amount);
        rewardRate[token] = (amount + _left) / DURATION;
    }
    require(rewardRate[token] > 0);
    uint balance = IERC20(token).balanceOf(address(this));
    require(rewardRate[token] <= balance / DURATION, "Provided reward too high");
    periodFinish[token] = adjustedTstamp + DURATION;
    if (!isReward[token]) {
        isReward[token] = true;
        rewards.push(token);
        IBribe(bribe).addRewardToken(token);
    }

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

Recommended Mitigation Steps

Consider implementing validation to ensure that this function is callable by anyone only if the previous reward period has already expired. Otherwise, when there is an active reward period, it may only be called by the designated reward distributor account.

For sample implementation, see https://github.com/curvefi/curve-dao-contracts/blob/3bee979b7b6293c9e7654ee7dfbf5cc9ff40ca58/contracts/streamers/ChildChainStreamer.vy#L166

Lines of code

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

Vulnerability details

[High] Bribe Rewards Not Collected In Current Period Will Be Lost Forever

Vulnerability Details

It was observed that if the bribe rewards are not collected in the current period, they will not be accrued to future epoch, and they will be lost forever.

Proof-of-Concept

When a briber adds in a new bribe reward, the reward information are stored in the Bribe.tokenRewardsPerEpoch mapping. This mapping stored the number of rewards tokens per epoch.

Assume that a briber adds 100 DAI token as bribe reward in epoch 0 (current epoch), they will be stored in:

tokenRewardsPerEpoch[DAI][Epoch0+1(Bribe Lag)] = 100 DAI tokens

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

function notifyRewardAmount(address token, uint amount) external lock {
    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);
}

The Voter.distribute()function only allows users to distribute the bribe rewards in the current epoch. Calling theVoter.distribute function will in turn trigger the Gauge.deliverBribes function.

In the Gauge.deliverBribes function, the code uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG; will always return the start date of current epoch + BRIBE_LAG (1 day). So, if someone call Gauge.deliverBribes in epoch 1, the bribeStart variable will be set to the Epoch 1 + 1 day. Due to the above method of fetching bribe rewards and the use of per epoch's reward mapping, there is no way to fetch the bribe rewards in the previous epoch(s).

Assume that no one triggered the Voter.distribute() in epoch 0, the 100 DAI tokens bribe rewards will be lost forever.

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

uint internal constant BRIBE_LAG = 1 days;

function deliverBribes() external lock {
    require(msg.sender == voter);
    IBribe sb = IBribe(bribe);
    uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG;
    uint numRewards = sb.rewardsListLength();

    for (uint i = 0; i < numRewards; i++) {
        address token = sb.rewards(i);
        // @audit - This will transfer the bribe reward token from the Bribe contract to Gauge contract
        uint epochRewards = sb.deliverReward(token, bribeStart);
        if (epochRewards > 0) {
        	// @audit - Update the reward rate accordingly
            _notifyBribeAmount(token, epochRewards, bribeStart);
        }
    }
}

Recommended Mitigation Steps

Devise a new implementation to allow bribe rewards not collected to be accured to future epochs so that the bribe rewards will not be lost. Consider referencing the Solidy's Bribe contract, which support these requirements.

Lines of code

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

Vulnerability details

Proof-of-Concept

Per https://docs.velodrome.finance/tokenomics#bribes, it was understood that the phases are as follows

Below is an example of bribes, voting, and rewards claim timeline:

• Bribes are deposited from Saturday — Thursday
• Votes are collected on Friday
• A snapshot is taken at the end of Friday (11:59 UTC)
• Rewards claim is available 24–48 hours after the snapshot

This differs from the actual implementation in the contract. For instance, the voting phase is five days in the contract, while it is only one day in the documentation.

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

uint internal constant DURATION = 5 days;
uint internal constant BRIBE_LAG = 1 days;

function getVotingStage(uint timestamp) public pure returns (VotingStage) {
    uint modTime = timestamp % (7 days);
    if (modTime < BRIBE_LAG) {
        return VotingStage.BribesPhase;
    } else if (modTime > (BRIBE_LAG + DURATION)) {
        return VotingStage.RewardsPhase;
    }
    return VotingStage.VotesPhase;
}

The accuracy of Voting, Bribing, and Voting Phases play a critical role in the protocol as many key functionalities revolve around on this information (e.g. period for accepting bribes, releasing of rewards)

Additionally, discrepencies between specification and actual implementation might causes unnecessary confusion on how the protocol operates.

Recommended Mitigation Steps

Update either the contracts or the specification to ensure that they are aligned.