veToken Finance contest - sseefried's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L36-L38 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L41-L4 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L598-L614

Vulnerability details

Impact

The totalWeight state variable of the VeTokenMinter contract is used to work out the amount of veAsset earned when the Booster.rewardClaimed function is called.

However, while totalWeight is modified inside the VeTokenMinter contract when function updateveAssetWeight is called, the totalWeight is not similarly reduced when function removeOperator is called.

The impact is that remaining operators do not receive a fair share of the total rewards and a portion of the rewards are not given out at all.

Proof of Concept

• Operator 1 is added with weight 9
• Operator 2 is added with weight 1

The totalWeight is now 10.

This means that Operator 1 receives 90% of the amount while Operator 2 receives 10%.

If we then call removeOperator on Operator 1 then 90% of the reward is no longer minted and distributed. This is unfair to the remaining operators.

The can be seen on lines 607 - 608 of the Booster contract. Function rewardClaimed will never be called for (removed) Operator 1. But for Operator 2 they will still receive 10% of the rewards even though Operator 1 is no longer registered in the system.

Tools Used

Manual Inspection

Recommended Mitigation Steps

The totalWeight should be reduced so that the remaining operators receive a fair share of the total rewards.

Using just method calls from VeTokenMinter one could rectify this situation by

• adding the removed operator with addOperator
• setting the weight to 0 using updateveAssetWeight. This will have the effect of reducing the totalWeight by the right amount.
• removing the operator again using removeOperator

However, the removeOperator function should just be rewritten to be as follows:

function removeOperator(address _operator) public onlyOwner {
    totalWeight -= veAssetWeights[_operator];
    veAssetWeights[_operator] = 0;
    operators.remove(_operator);
}

You might also want to modify addOperator so that a weight can be provided as an extra argument. This saves having to call addOperator and then updateveAssetWeight which could save on gas.

QA Report

Remarks/Recommendations

• In contract VeAssetDepositor there is not a whole lot of documentation. It's unclear what the escrow address is used for.

Non-critical: Not checking return value of operators.add in VeTokenMinter.addOperator

function addOperator(address _newOperator) public onlyOwner {
     operators.add(_newOperator);
 }

The return value is not checked so you will not know if addOperator was successful or not. It is recommended that either you do this or return the value to the contract caller. e.g.

function addOperator(address _newOperator) public onlyOwner returns (bool) {
    return operators.add(_newOperator);
}

Non-critical: Rename variables in VeTokenMinter.mint to improve understanding

The variables names reduction and reductionPerCliff were very confusing. Also, they both contain the word "reduction" but mean very different things.

• reductionPerCliff would be better described as cliffWidth. It's the "width" of each cliff because it is equal to maxSupply.div(totalCliffs).
• reduction would be better described as cliffsLeft as it is a the number of cliffs until totalCliffs is reached. This is because, as cliff increases from 0 to totalCliffs, cliffsLeft decreases from totalCliffs to 0.

Non-critical: Use local variable to avoid repeated array subscript operations

You could rewrite VE3DRewardPool.update_reward as follows:

modifier updateReward(address account) {
    address _rewardToken;
    for (uint256 i = 0; i < rewardTokens.length(); i++) {
        _rewardToken = rewardTokens.at(i);
        RewardTokenInfo memory rti = rewardTokenInfo[_rewardToken];
        rti.rewardPerTokenStored = rewardPerToken(_rewardToken);
        rti.lastUpdateTime = lastTimeRewardApplicable(_rewardToken);
        if (account != address(0)) {
            rti.rewards[account] = earnedReward(_rewardToken, account);
            rti.userRewardPerTokenPaid[account] = rti.rewardPerTokenStored;
        }
    }
    _;
}

By declaring RewardToken memory rti you avoid repeated uses of rewardTokenInfo[_rewardToken]. It's easier on the eyes and saves gas.

This can be done for several other function in the VE3DRewardPool contract.

Non-critical: Change RewardPaid event to include rewardToken address

The VE3DRewardPool.getReward function emits a RewardPaid event just like the original Convex code did. However, it does this once for each reward token. Perhaps it would be a good idea to add an extra field to tell people which reward token the reward was paid for? e.g.

event RewardPaid(address indexed user, address indexed rewardToken, uint256 reward);

The same is suggested for the RewardPaid event.

Low Risk: totalSupply should be updated before safeTransfer in VeTokenMinter.mint

At the end of the mint function we have:

veToken.safeTransfer(_to, _amount);
totalSupply += _amount;

This violates the check-effects-interactions pattern which prevents possible re-entrancy attacks. Swapping these two lines will means that the "interactions" (with the veToken contract) occurs after the "effect" (totalSupply += amount)

Low Risk: Arithmetic error in VeAssetDepositor._lockVeAsset means that locking can occur more often than every two weeks

The arithmetic on line VeAssetDepositor.sol:106 is incorrect. The unlockInWeeks variable has units of seconds not weeks so the expression should actually be:

unlockInWeeks.sub(unlockTime) > 2 * WEEK

This could lead to tokens being locked more often that when the two week window has passed. A Proof of Concept has been written here.

However, discussions with the sponsor lead this warden to only assign this as low risk since there is not much downside to locking the tokens more often that every two weeks, except possibly that there is higher gas costs.