QA Report

[code423n4]

Loss Of Precision

This issue is about arithmetic computation that could have been done more percise.
The following are places in the codebase in which you multiplied after the divisions.
Doing the multiplications at start lead to more accurate calculations.
This is a list of places in the code that this appears (Solidity file, line number, actual line):

Code instances:

    VE3DLocker.sol, 376, uint256 currentEpoch = block.timestamp.div(rewardsDuration).mul(rewardsDuration);
    VE3DLocker.sol, 121, uint256 currentEpoch = block.timestamp.div(rewardsDuration).mul(rewardsDuration); 
    VE3DLocker.sol, 485, uint256 nextEpoch = block.timestamp.div(rewardsDuration).mul(rewardsDuration).add( rewardsDuration );
    VE3DLocker.sol, 535, uint256 lockEpoch = block.timestamp.div(rewardsDuration).mul(rewardsDuration); 
    VE3DLocker.sol, 422, _time = _time.div(rewardsDuration).mul(rewardsDuration); 

Does not validate the input fee parameter

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

Code instances:

    Booster.setFees (_stakerLockIncentiveFee)
    Booster.setFees (_lockFees)
    Booster.setFeeManager (_feeM)
    Booster.setFees (_callerFees)
    VeAssetDepositor.setFeeManager (_feeManager)

safeApprove of openZeppelin is deprecated

You use safeApprove of openZeppelin although it's deprecated.
(see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
You should change it to increase/decrease Allowance as OpenZeppilin says.

Code instances:

    Deprecated safeApprove in VE3DLocker.sol line 214: IERC20(rewardData[_rewardsToken].ve3Token).safeApprove(
    Deprecated safeApprove in VoterProxy.sol line 160: IERC20(veAsset).safeApprove(escrow, _value);
    Deprecated safeApprove in VE3DLocker.sol line 220: IERC20(_rewardsToken).safeApprove(rewardData[_rewardsToken].veAssetDeposits, 0);
    Deprecated safeApprove in VE3DRewardPool.sol line 286: IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0);
    Deprecated safeApprove in VeAssetDepositor.sol line 161: IERC20(minter).safeApprove(_stakeAddress, _amount);

Require with empty message

The following requires are with empty messages.
This is very important to add a message for any require. So the user has enough information to know the reason of failure.

Code instances:

    Solidity file: VE3DLocker.sol, In line 154 with Empty Require message.
    Solidity file: VE3DLocker.sol, In line 180 with Empty Require message.
    Solidity file: VE3DLocker.sol, In line 155 with Empty Require message.

Require with not comprehensive message

The following requires has a non comprehensive messages.
This is very important to add a comprehensive message for any require. Such that the user has enough
information to know the reason of failure:

Code instances:

    Solidity file: RewardFactory.sol, In line 80 with Require message: !auth
    Solidity file: Booster.sol, In line 124 with Require message: !auth
    Solidity file: VoterProxy.sol, In line 167 with Require message: !auth
    Solidity file: Booster.sol, In line 468 with Require message: !auth
    Solidity file: Booster.sol, In line 261 with Require message: !add

Not verified input

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    VE3DLocker.sol.approveRewardDistributor _rewardsToken
    BaseRewardPool.sol.stakeFor _for
    PoolManager.sol.addPool _lptoken
    VeTokenMinter.sol.withdraw _destination
    SmartWalletWhitelist.sol.approveWallet _wallet

Treasury may be address(0)

Make sure the treasury is not address(0).

Code instance:

    Booster.sol.setTreasury _treasury

Solidity compiler versions mismatch

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Code instance:

Use safe math for solidity version <8

You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity.

Code instances:

    The contract Babylonian.sol doesn't use safe math and is of solidity version < 8
    The contract FullMath.sol doesn't use safe math and is of solidity version < 8
    The contract BitMath.sol doesn't use safe math and is of solidity version < 8
    The contract FixedPoint.sol doesn't use safe math and is of solidity version < 8
    The contract Migrations.sol doesn't use safe math and is of solidity version < 8

Not verified owner

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.

Code instances:

    Booster.sol.setOwner _owner
    VoterProxy.sol.setOwner _owner

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing.
Furthermore, removing either the actual return or the named return will save gas.

Code instances:

    VE3DLocker.sol, balanceAtEpochOf
    VE3DLocker.sol, findEpochId
    VE3DLocker.sol, totalSupplyAtEpoch
    VE3DLocker.sol, lockedBalances
    VE3DLocker.sol, totalSupply

Two Steps Verification before Transferring Ownership

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked.
It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership.
A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

Code instances:

    VoterProxy.sol
    VeAssetDepositor.sol
    IPools.sol
    Booster.sol

Missing non reentrancy modifier

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer.
Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

Code instances:

    VE3DLocker.sol, recoverERC20 is missing a reentrancy modifier
    VE3DLocker.sol, setApprovals is missing a reentrancy modifier

In the following public update functions no value is returned

In the following functions no value is returned, due to which by default value of return will be 0.
We assumed that after the update you return the latest new value.
(similar issue here: code-423n4/2021-10-badgerdao-findings#85).

Code instance:

    VeTokenMinter.sol, updateveAssetWeight

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    VoterProxy.sol: function execute parameter _data isn't used. (execute is external)
    VoterProxy.sol: function execute parameter _value isn't used. (execute is external)
    VoterProxy.sol: function execute parameter _to isn't used. (execute is external)

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/ExtraRewardStashV2.sol#L96
    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/VE3DLocker.sol#L683
    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/VeTokenMinter.sol#L78
    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/VeTokenMinter.sol#L72
    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/BaseRewardPool.sol#L208

In the following public update functions no value is returned

In the following functions no value is returned, due to which by default value of return will be 0.
We assumed that after the update you return the latest new value.
(similar issue here: code-423n4/2021-10-badgerdao-findings#85).

Code instance:

    VeTokenMinter.sol, updateveAssetWeight

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    VoterProxy.sol: function execute parameter _data isn't used. (execute is external)
    VoterProxy.sol: function execute parameter _value isn't used. (execute is external)
    VoterProxy.sol: function execute parameter _to isn't used. (execute is external)

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/Booster.sol#L243
    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/RewardFactory.sol#L79
    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/token/VE3Token.sol#L21
    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/Booster.sol#L141
    https://github.com/code-423n4/2022-05-vetoken/tree/main/contracts/VE3DLocker.sol#L206

[GalloDaSballo]

Loss Of Precision

Invalid as the code is meant to calculate epochs

## Does not validate the input fee parameter
Valid Low

safeApprove of openZeppelin is deprecated

Valid NC

Require with empty message

Valid NC

Require with not comprehensive message

Disagree as they are permissioned functions

Rest looks invalid

1L, 2 NC