veToken Finance contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L134

Vulnerability details

Impact

It was observed that addExtraReward does not check for duplicate values which can lead to duplicate reward tokens being added to extraRewards. This can lead to higher reward payment than expected. Need to be fixed for BaseRewardPool.sol as well

Proof of Concept

1. rewardManager calls the addExtraReward function and provides address A as reward contract

2. rewardManager calls the addExtraReward function and provides address B as reward contract

3. rewardManager calls the addExtraReward function and by mistake again provide address A as reward contract

4. Now extraRewards has 3 entries A,B,A

5. User X calls stake method for amount 1000

6. This executes below code

for(uint i=0; i < extraRewards.length; i++){
          IRewards(extraRewards[i]).stake(msg.sender, _amount);
      }

7. Since extraRewards has 3 entries A,B,A so the loop runs twice for reward contract A
8. This means instead of amount 1000, the amount staked in reward contract A will be 2*1000=2000 which means User X will receive double extra rewards from contract A which is incorrect

Recommended Mitigation Steps

Do not allow duplicate values in addExtraReward. For this Set can be used

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L141

Vulnerability details

Impact

rewardManager can at anytime delete the extra Rewards. This impacts the extra rewards earned by existing staker. The existing staker will have no way to claim these extra rewards. Since these extra rewards have been staked for all users making a deposit BaseRewardPool.sol#L215 so basically the extra reward gets locked with no one having access to this fund. Need to be fixed for BaseRewardPool.sol as well

Proof of Concept

1. rewardManager adds 2 extra reward token A,B using addExtraReward
2. User X makes a deposit of amount 1000 using stake function
3. This stakes extra rewards A,B for this user
4. rewardManager now calls clearExtraRewards which removes extraRewards object
5. User X has now no way to retrieve the staked extra rewards A,B of amount 1000

Recommended Mitigation Steps

If rewardManager wants to clear extra rewards then all existing stakes on extra rewards must be withdrawn and claimed so that user extra rewards are not lost

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L256

Vulnerability details

Impact

It was observed that addPool function is not checking for duplicate lpToken which allows 2 or more pools to have exact same lpToken. This can cause issue with deposits.

In case of duplicate lpToken, the first pool calling depositAll will take away all lpToken and deposit them under there own pid. This leaves no balance for 2nd pool

Proof of Concept

1. PoolManager call addPool function and uses lpToken as A
2. PoolManager again call addPool function and mistakenly provides lpToken as A
3. Now 2 pools will be created with lpToken as A
4. depositAll function is called passing first pool.
5. This takes all balance of lpToken A and depsoit it under first pool pid
6. This mean no balance is left for second pool now

Recommended Mitigation Steps

Add a global variable keeping track of all lpToken added for pool. In case of duplicate lpToken addPool function should fail.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L102 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L145

Vulnerability details

Impact

The current implementation of addReward seems to allow any number of reward token to be added. There should be a limit on the number of rewards allowed

Proof of Concept

1. Operator calls addReward function with new _rewardsToken on VE3DLocker.sol#L145
2. Operator perform Step 1 , 100 times
3. 101 new Reward tokens are added to system showing no max limit

Recommended Mitigation Steps

Add below check in addReward function:

require(rewardTokens.length<MAX_ALLOWED, "Max reward crossed")

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol

Vulnerability details

Impact

Sweep function is missing which could lead to reward token being stuck in contract. Need to be fixed for BaseRewardPool.sol as well

Proof of Concept

1. Assume new rewards were added to the contract and first user staked after 3 days.

2. This means the reward for the first 3 days will remain in contract and will not be given to anyone.

3. Currently Admin has no way to claim this 3 days rewards (will not even come within queued rewards). Ideally Admin must have a way to clean out this pending reward once duration is over

Recommended Mitigation Steps

Kindly consider adding below:

function sweepAmount(int amount) external returns (bool) {
if(block.timestamp<periodFinish){
return;
}
rewardToken.safeTransfer(adminAddress, amount);
return true;

}

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L129

Vulnerability details

Impact

Once Fee Manager has been set initially by owner, then owner has no power to change it. Owner should be allowed to change fees manager in case if he feels current fee manager is behaving maliciously

Proof of Concept

1. Observe the setFeeManager function and see that only feeManager is allowed to change it once set initially

function setFeeManager(address _feeM) external {
        require(msg.sender == feeManager, "!auth");
        feeManager = _feeM;
        emit FeeManagerUpdated(_feeM);
    }

Recommended Mitigation Steps

Change the setFeeManager function like below. Same can be done with other important functionality involving setArbitrator and setVoteDelegate

require(msg.sender == owner, "!auth");

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L102

Vulnerability details

Impact

If _rewardToken is set as _stakingToken by mistake then user funds would get lost (staking token will get sent as reward token). Need to be fixed for BaseRewardPool.sol as well

Proof of Concept

1. User A and B makes deposit of amount 100 each
2. Owner calls addReward and queueNewRewards to add 1 reward amount with _rewardToken as stakingToken (by mistake)
3. After some time reward is calculated as 5 for User A (total reward amount is same as staking amount which is 100+100+1).
4. User A makes the withdraw and obtains 105 amount and now User B is stuck since contract does not have enough funds

Recommended Mitigation Steps

Add below check in constructor

require(_stakingToken!=_rewardToken, "Incorrect reward token");

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L62

Function setOwner: The function does not perform zero address check which means owner can be updated incorrectly.

Also it is recommended to change owner in 2 step process which will ensure that new owner is indeed correct

require(_owner!=address(0), "Incorrect address");

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L207

Function voteGaugeWeight: Add below check to prevent incorrect weight updation

require(_tokenVote.length==_weight.length,"Incorrect length");

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeTokenMinter.sol#L41

Function updateveAssetWeight: newWeight cannot be zero. If need to be 0 then better to remove operator

require(newWeight!=0, "Incorrect weight");