Velodrome Finance contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

In Bribe.notifyRewardAmount() function, everyone can call this function with trash token. And rewards list has a limit MAX_REWARD_TOKENS.

So anyone can spam to add any number of token to rewards list, making it’s full and impossible to add more bribe token.

Proof of concept

1. Attacker deployed MAX_REWARD_TOKENS trash ERC20 token
2. Attacker call notifyRewardAmount() with each deployed token.
3. If gauge call addRewardToken() or notifyRewardAmount(), it will revert because rewards list is full.

require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");

Tools Used

Manual Review

Recommended Mitigation Steps

Add check for only gauge can call notifyRewardAmount()

require(msg.sender == gauge);

1. Inconsistent between implementation and documentation.

Impact

In Minter contract, MAX_TEAM_RATE = 50 and PRECISION = 1000, so it’s 5%. But in the comment same line, it says 50bps = 0.05%.

Also in Minter.constructor(), teamRate is set to 30 and comment says 30bps = 0.03%. But actually it’s 3%

Proof of concept

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

Change the code or document to match

2. Should not be able to revive not gauge address

Impact

In Voter.reviveGauge(), any address can be passed in as _gauge param and set to isAlive[_gauge] = true even when that address is not a gauge.

Proof of concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Should add check if that address is a gauge

require(isGauge[_gauge]);

3. Rewards list can have duplicated token

Impact

There is a mapping to check if a token is existed in rewards list or not isReward[token]. But in swapOutRewardToken() function, there is no check using isReward.

An existing token can be passed in as newToken and rewards list will have a duplicated token.

Proof of concept

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

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

1. Current list rewards rewards = [USDT, DAI, UST]
2. Team mistakenly called swapOutRewardToken(2, UST, USDT). Because we do not check if USDT is already in the list. The TX will not revert. New list rewards is
   rewards = [USDT, DAI, USDT] Token USDT appears 2 times in the list.

Tools Used

Manual Review

Recommended Mitigation Steps

Check if newToken is in the list or not

require(!isReward[newToken]);