veToken Finance contest - Dravee's results

Judge has assessed an item in Issue #264 as Medium risk. The relevant finding follows:

[L-05] Unbounded loop on array can lead to DoS As these arrays can grow quite large (only push operations, no pop), the transaction's gas cost could exceed the block gas limit and make it impossible to call the impacted functions at all.

contracts/BaseRewardPool.sol: 126: extraRewards.push(_reward); 176: for (uint256 i = 0; i < extraRewards.length; i++) { 199: for (uint256 i = 0; i < extraRewards.length; i++) { 218: for (uint256 i = 0; i < extraRewards.length; i++) { 245: for (uint256 i = 0; i < extraRewards.length; i++) { 282: for (uint256 i = 0; i < extraRewards.length; i++) {

contracts/Booster.sol: 282: poolInfo.push( 329: for (uint256 i = 0; i < poolInfo.length; i++) {

contracts/RewardFactory.sol: 49: for (uint256 i = 0; i < length; i++) { 52: activeList.push(pid); 66: for (uint256 i = 0; i < length; i++) { 71: activeList.pop();

contracts/VE3DLocker.sol: 123: epochs.push(Epoch({supply: 0, date: uint32(currentEpoch)})); 156: rewardTokens.push(_rewardsToken); 207: for (uint256 i; i < rewardTokens.length; i++) { 286: for (uint256 i = 0; i < userRewards.length; i++) { 457: for (uint256 i = nextUnlockIndex; i < locks.length; i++) { 500: epochs.push(Epoch({supply: 0, date: uint32(nextEpochDate)})); 546: userLocks[_account].push( 579: userLocks[_account].push( 640: for (uint256 i = nextUnlockIndex; i < length; i++) { 720: for (uint256 i; i < rewardTokens.length; i++) { 803: for (uint256 i = 0; i < rewardTokens.length; i++) {

contracts/VE3DRewardPool.sol: 138: extraRewards.push(_reward); 148: for (uint256 i = 0; i < rewardTokens.length(); i++) { 214: for (uint256 i = 0; i < length; i++) { 238: for (uint256 i = 0; i < length; i++) { 257: for (uint256 i = 0; i < length; i++) { 281: for (uint256 i = 0; i < rewardTokens.length(); i++) { 326: for (uint256 i = 0; i < length; i++) { Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L356 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L337

Vulnerability details

As arbitrary ERC20 tokens can be passed, the amount here should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Affected code:

• File: Booster.sol

345:     function deposit(
346:         uint256 _pid,
347:         uint256 _amount,
348:         bool _stake
349:     ) public returns (bool) {
...
356:         IERC20(lptoken).safeTransferFrom(msg.sender, staker, _amount); //@audit medium: not compatible with Fee On Transfer Tokens
...
372:             ITokenMinter(token).mint(address(this), _amount);
...
374:             IERC20(token).safeApprove(rewardContract, _amount);
375:             IRewards(rewardContract).stakeFor(msg.sender, _amount);
...
378:             ITokenMinter(token).mint(msg.sender, _amount);
...
381:         emit Deposited(msg.sender, _pid, _amount);
...

• File: VE3DRewardPool.sol

336:     function donate(address _rewardToken, uint256 _amount) external {
337:         IERC20(_rewardToken).safeTransferFrom(msg.sender, address(this), _amount); //@audit medium: not compatible with Fee On Transfer Tokens
338:         rewardTokenInfo[_rewardToken].queuedRewards += _amount;
339:     }

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L387 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L315 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L360 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L406

Vulnerability details

Impact

Potential DoS in loops being inversely traversed

Proof of Concept

Let's take the example of VE3DLocker.totalSupply().

The VE3DLocker.totalSupply() function has been modified to save gas with an inversely traversed for-loop:

File: VE3DLocker.sol
386:         //traverse inversely to make more current queries more gas efficient
387:         for (uint256 i = epochindex - 1; i + 1 != 0; i--) {
388:             Epoch storage e = epochs[i];
389:             if (uint256(e.date) <= cutoffEpoch) {
390:                 break;
391:             }
392:             supply = supply.add(e.supply);
393:         }

However, the Solidity version is 0.8.7, meaning that there's a default overflow check. Therefore, as i is of type uint256, the condition i + 1 != 0 can never be false: the transaction would've already reverted if i-- was called on i == 0 instead of letting it take the max value of uint256 (i == 2**256 - 1) to verify i + 1 == 0 and therefore break the for-loop.

This condition might've worked with previous Solidity versions, but not with 0.8+.

The following lines are all affected by this logic error:

contracts/VE3DLocker.sol:
  315:         for (uint256 i = locks.length - 1; i + 1 != 0; i--) {
  360:         for (uint256 i = locks.length - 1; i + 1 != 0; i--) {
  387:         for (uint256 i = epochindex - 1; i + 1 != 0; i--) {
  406:         for (uint256 i = _epoch; i + 1 != 0; i--) {

Recommended Mitigation Steps

Here, it's necessary to add a check at the end of the for-loop to prevent a DoS:

if (i == 0) {
    break;
}

The condition i + 1 != 0 will always be true (as i won't go below 0), so this tautology isn't gas efficient (evaluated at each iteration).

To save gas as intended (the comment does say that this is the goal), you could use such a syntax:

for (uint256 i = epochindex - 1; true;) {
    ...
    if (i != 0) {
        unchecked { --i; }
    } else {
        break;
    }
}

You could also keep the original condition and enable the overflow so that the loop can reach its break condition:

for (uint256 i = epochindex - 1; i + 1 != 0;) {
    ...
    unchecked { --i; }
}

Table of Contents

• [L-01] Unsafe casting may overflow
• [L-02] Check Effects Interactions pattern not respected
• [L-03] Deprecated safeApprove() function
• [L-04] Missing address(0) checks
• [L-05] Unbounded loop on array can lead to DoS
• [L-06] Lack of event emission
• [L-07] Add a timelock to critical functions
• [N-01] Missing friendly revert strings
• [N-02] Unused named returns

[L-01] Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:

contracts/VE3DLocker.sol:
  122          uint256 currentEpoch = block.timestamp.div(rewardsDuration).mul(rewardsDuration);
  123:         epochs.push(Epoch({supply: 0, date: uint32(currentEpoch)}));

  157  
  158:         rewardData[_rewardsToken].lastUpdateTime = uint40(block.timestamp);
  159:         rewardData[_rewardsToken].periodFinish = uint40(block.timestamp);

  500:                 epochs.push(Epoch({supply: 0, date: uint32(nextEpochDate)}));

  547:                 LockedBalance({amount: lockAmount, unlockTime: uint32(unlockTime)})

  585:                 userL.unlockTime = uint32(unlockTime);

  596:         e.supply = e.supply.add(uint224(lockAmount));

[L-02] Check Effects Interactions pattern not respected

To avoid unexpected behavior in the future (be it for the solution or for a fork), it's recommended to always follow the CEI pattern.

Consider always moving the state-changes before the external calls.

Affected code:

contracts/BaseRewardPool.sol:
  294      function donate(uint256 _amount) external {
  295          IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
  296:         queuedRewards = queuedRewards.add(_amount); //@audit : CEIP not respected. Move this line before the transfer
  297          emit Donated(queuedRewards);
  298      }

contracts/VE3DRewardPool.sol:
  336      function donate(address _rewardToken, uint256 _amount) external {
  337          IERC20(_rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
  338:         rewardTokenInfo[_rewardToken].queuedRewards += _amount;  //@audit : CEIP not respected. Move this line before the transfer
  339      }

contracts/VirtualBalanceRewardPool.sol:
  161      function donate(uint256 _amount) external {
  162          IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
  163:         queuedRewards = queuedRewards.add(_amount); //@audit : CEIP not respected. Move this line before the transfer
  164      }

[L-03] Deprecated safeApprove() function

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

contracts/Booster.sol:
  374:             IERC20(token).safeApprove(rewardContract, _amount); //@audit medium-risk: should do like everywhere in the solution and approve 0 first

contracts/VE3DLocker.sol:
  211:                 IERC20(rewardData[_rewardsToken].ve3Token).safeApprove(
  215:                 IERC20(rewardData[_rewardsToken].ve3Token).safeApprove(
  221:                 IERC20(_rewardsToken).safeApprove(rewardData[_rewardsToken].veAssetDeposits, 0);
  222:                 IERC20(_rewardsToken).safeApprove(

contracts/VE3DRewardPool.sol:
  287:                 IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0);
  288:                 IERC20(_rewardToken).safeApprove(
  301:                     IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
  305:                     IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(

contracts/VeAssetDepositor.sol:
  162:             IERC20(minter).safeApprove(_stakeAddress, _amount);

contracts/VoterProxy.sol:
  101:             IERC20(_token).safeApprove(_gauge, 0);
  102:             IERC20(_token).safeApprove(_gauge, balance);
  152:         IERC20(veAsset).safeApprove(escrow, 0);
  153:         IERC20(veAsset).safeApprove(escrow, _value);
  160:         IERC20(veAsset).safeApprove(escrow, 0);
  161:         IERC20(veAsset).safeApprove(escrow, _value);

[L-04] Missing address(0) checks

Affected code (from Slither):

    - operator = _operator (token/VE3Token.sol#23)
    - operator = op_ (VirtualBalanceRewardPool.sol#97)
    - staker = _staker (VeAssetDepositor.sol#45)
    - minter = _minter (VeAssetDepositor.sol#46)
    - veAsset = _veAsset (VeAssetDepositor.sol#47)
    - escrow = _escrow (VeAssetDepositor.sol#48)
    - feeManager = _feeManager (VeAssetDepositor.sol#55)
    - rewardManager = rewardManager_ (VE3DRewardPool.sol#99)
    - staker = _staker (Booster.sol#111)
    - minter = _minter (Booster.sol#116)
    - veAsset = _veAsset (Booster.sol#117)
    - feeDistro = _feeDistro (Booster.sol#118)
    - owner = _owner (Booster.sol#125)
    - feeManager = _feeM (Booster.sol#131)
    - poolManager = _poolM (Booster.sol#137)
    - rewardFactory = _rfactory (Booster.sol#152)
    - tokenFactory = _tfactory (Booster.sol#153)
    - stashFactory = _sfactory (Booster.sol#159)
    - rewardArbitrator = _arb (Booster.sol#164)
    - voteDelegate = _voteDelegate (Booster.sol#170)
    - lockRewards = _rewards (Booster.sol#184)
    - stakerRewards = _stakerRewards (Booster.sol#185)
    - stakerLockRewards = _stakerLockRewards (Booster.sol#186)
    - treasury = _treasury (Booster.sol#245)
    - dao = _dao (helper/SmartWalletWhitelist.sol#33)
    - future_checker = _checker (helper/SmartWalletWhitelist.sol#40)
    - veAsset = _veAsset (ExtraRewardStashV3.sol#49)
    - operator = _operator (ExtraRewardStashV3.sol#50)
    - staker = _staker (ExtraRewardStashV3.sol#51)
    - gauge = _gauge (ExtraRewardStashV3.sol#52)
    - rewardFactory = _rFactory (ExtraRewardStashV3.sol#53)
    - veAsset = _veAsset (ExtraRewardStashV2.sol#49)
    - operator = _operator (ExtraRewardStashV2.sol#50)
    - staker = _staker (ExtraRewardStashV2.sol#51)
    - gauge = _gauge (ExtraRewardStashV2.sol#52)
    - rewardFactory = _rFactory (ExtraRewardStashV2.sol#53)
    - operator = _operator (token/VeToken.sol#20)
    - operator = _operator (ExtraRewardStashV1.sol#46)
    - staker = _staker (ExtraRewardStashV1.sol#47)
    - gauge = _gauge (ExtraRewardStashV1.sol#48)
    - rewardFactory = _rFactory (ExtraRewardStashV1.sol#49)
    - veAsset = _veAsset (VoterProxy.sol#50)
    - escrow = _escrow (VoterProxy.sol#51)
    - gaugeProxy = _gaugeProxy (VoterProxy.sol#52)
    - minter = _minter (VoterProxy.sol#54)
    - owner = _owner (VoterProxy.sol#64)
    - operator = _operator (VoterProxy.sol#74)
    - depositor = _depositor (VoterProxy.sol#80)
    - (success,result) = _to.call{value: _value}(_data) (VoterProxy.sol#281)
    - rewardFactory = _rewardFactory (StashFactory.sol#27)
    - operator = operator_ (BaseRewardPool.sol#105)
    - rewardManager = rewardManager_ (BaseRewardPool.sol#106)
    - operator = _operator (DepositToken.sol#23)

[L-05] Unbounded loop on array can lead to DoS

As these arrays can grow quite large (only push operations, no pop), the transaction's gas cost could exceed the block gas limit and make it impossible to call the impacted functions at all.

contracts/BaseRewardPool.sol:
  126:         extraRewards.push(_reward);
  176:         for (uint256 i = 0; i < extraRewards.length; i++) {
  199:         for (uint256 i = 0; i < extraRewards.length; i++) {
  218:         for (uint256 i = 0; i < extraRewards.length; i++) {
  245:         for (uint256 i = 0; i < extraRewards.length; i++) {
  282:             for (uint256 i = 0; i < extraRewards.length; i++) {

contracts/Booster.sol:
  282:         poolInfo.push(
  329:         for (uint256 i = 0; i < poolInfo.length; i++) {

contracts/RewardFactory.sol:
  49:         for (uint256 i = 0; i < length; i++) {
  52:         activeList.push(pid);
  66:         for (uint256 i = 0; i < length; i++) {
  71:                 activeList.pop();

contracts/VE3DLocker.sol:
  123:         epochs.push(Epoch({supply: 0, date: uint32(currentEpoch)}));
  156:         rewardTokens.push(_rewardsToken);
  207:         for (uint256 i; i < rewardTokens.length; i++) {
  286:         for (uint256 i = 0; i < userRewards.length; i++) {
  457:         for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
  500:                 epochs.push(Epoch({supply: 0, date: uint32(nextEpochDate)}));
  546:             userLocks[_account].push(
  579:                 userLocks[_account].push(
  640:             for (uint256 i = nextUnlockIndex; i < length; i++) {
  720:         for (uint256 i; i < rewardTokens.length; i++) {
  803:             for (uint256 i = 0; i < rewardTokens.length; i++) {

contracts/VE3DRewardPool.sol:
  138:         extraRewards.push(_reward);
  148:         for (uint256 i = 0; i < rewardTokens.length(); i++) {
  214:         for (uint256 i = 0; i < length; i++) {
  238:         for (uint256 i = 0; i < length; i++) {
  257:         for (uint256 i = 0; i < length; i++) {
  281:         for (uint256 i = 0; i < rewardTokens.length(); i++) {
  326:             for (uint256 i = 0; i < length; i++) {

Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

[L-06] Lack of event emission

According to Slither, these should emit events (indeed, state variables are being updated):

VirtualBalanceRewardPool.queueNewRewards(uint256) (VirtualBalanceRewardPool.sol#166-188) should emit an event for: 
 - queuedRewards = 0 (VirtualBalanceRewardPool.sol#173) 
 - queuedRewards = 0 (VirtualBalanceRewardPool.sol#184) 
 - queuedRewards = _rewards (VirtualBalanceRewardPool.sol#186) 
Booster.setFeeInfo(uint256,uint256) (Booster.sol#193-217) should emit an event for: 
 - lockFeesIncentive = _lockFeesIncentive (Booster.sol#196) 
 - stakerLockFeesIncentive = _stakerLockFeesIncentive (Booster.sol#197) 
VE3DLocker.setKickIncentive(uint256,uint256) (VE3DLocker.sol#185-190) should emit an event for: 
 - kickRewardPerEpoch = _rate (VE3DLocker.sol#188) 
BaseRewardPool.queueNewRewards(uint256) (BaseRewardPool.sol#300-325) should emit an event for: 
 - queuedRewards = 0 (BaseRewardPool.sol#307) 
 - queuedRewards = 0 (BaseRewardPool.sol#320) 
 - queuedRewards = _rewards (BaseRewardPool.sol#322) 

[L-07] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Consider adding a timelock to:

contracts/Booster.sol:
  219:     function setFees(

contracts/VeAssetDepositor.sol:
  59:     function setFees(uint256 _lockIncentive) external {

[N-01] Missing friendly revert strings

Here, a friendly message should exist for users to understand what went wrong:

contracts/VE3DLocker.sol:
  154:         require(rewardData[_rewardsToken].lastUpdateTime == 0);
  155:         require(_rewardsToken != address(stakingToken));
  165:             require(_ve3Token != address(0));
  166:             require(_ve3TokenStaking != address(0));
  167:             require(_veAssetDeposits != address(0));
  180:         require(rewardData[_rewardsToken].lastUpdateTime > 0);

[N-02] Unused named returns

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

contracts/VE3DLocker.sol:
  282:         returns (EarnedData[] memory userRewards)
  295:     function lockedBalanceOf(address _user) external view returns (uint256 amount) {
  300:     function balanceOf(address _user) external view returns (uint256 amount) {
  305:     function balanceAtEpochOf(uint256 _epoch, address _user) public view returns (uint256 amount) {
  332:     function pendingLockOf(address _user) external view returns (uint256 amount) {
  376:     function totalSupply() external view returns (uint256 supply) {
  399:     function totalSupplyAtEpoch(uint256 _epoch) external view returns (uint256 supply) {
  418:     function findEpochId(uint256 _time) public view returns (uint256 epoch) {

contracts/VoterProxy.sol:
  109:     function withdraw(IERC20 _asset) external returns (uint256 balance) {

Table of Contents:

• Cheap Contract Deployment Through Clones
• SafeMath is not needed when using Solidity version 0.8+
• Boolean comparisons
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• >= is cheaper than > (and <= cheaper than <)
• Splitting require() statements that use && saves gas
• Use shift right/left instead of division/multiplication if possible
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• Increments/decrements can be unchecked in for-loops
• abi.encode() is less efficient than abi.encodePacked()
• Public functions to external
• No need to explicitly initialize variables with default values
• Variable that should be constant
• Reduce the size of error messages (Long revert Strings)
• Use Custom Errors instead of Revert Strings to save Gas
• Functions guaranteed to revert when called by normal users can be marked payable

Cheap Contract Deployment Through Clones

See @audit tag:

RewardFactory.sol:90:        BaseRewardPool rewardPool = new BaseRewardPool(
RewardFactory.sol:106:        VirtualBalanceRewardPool rewardPool = new VirtualBalanceRewardPool(
StashFactory.sol:53:            ExtraRewardStashV3 stash = new ExtraRewardStashV3(
StashFactory.sol:64:            ExtraRewardStashV1 stash = new ExtraRewardStashV1(
StashFactory.sol:74:            ExtraRewardStashV2 stash = new ExtraRewardStashV2(
TokenFactory.sol:30:        DepositToken dtoken = new DepositToken(_msgSender(), _lptoken);

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

I suggest applying a similar pattern.

SafeMath is not needed when using Solidity version 0.8+

Solidity version 0.8+ already implements overflow and underflow checks by default. Using the SafeMath library from OpenZeppelin (which is more gas expensive than the 0.8+ overflow checks) is therefore redundant.

I suggest using the built-in checks instead of SafeMath and remove SafeMath here:

contracts/BaseRewardPool.sol:
   2: pragma solidity 0.8.7;
  42: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  52:     using SafeMath for uint256;

contracts/Booster.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  21:     using SafeMath for uint256;

contracts/DepositToken.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  13:     using SafeMath for uint256;

contracts/ExtraRewardStashV1.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  18:     using SafeMath for uint256;

contracts/ExtraRewardStashV2.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  18:     using SafeMath for uint256;

contracts/ExtraRewardStashV3.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  18:     using SafeMath for uint256;

contracts/PoolManager.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  17:     using SafeMath for uint256;

contracts/RewardFactory.sol:
  2: pragma solidity 0.8.7;
  4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";

contracts/StashFactory.sol:
  2: pragma solidity 0.8.7;
  4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";

contracts/TokenFactory.sol:
  2: pragma solidity 0.8.7;
  4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";

contracts/VE3DRewardPool.sol:
   2: pragma solidity 0.8.7;
  42: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  55:     using SafeMath for uint256;

contracts/VeAssetDepositor.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  16:     using SafeMath for uint256;

contracts/VeTokenMinter.sol:
   2: pragma solidity 0.8.7;
   8: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  12:     using SafeMath for uint256;

contracts/VirtualBalanceRewardPool.sol:
   2: pragma solidity 0.8.7;
  42: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  51:     using SafeMath for uint256;
  66:     using SafeMath for uint256;

contracts/VoterProxy.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  19:     using SafeMath for uint256;

contracts/token/VE3Token.sol:
   2: pragma solidity 0.8.7;
   4: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  13:     using SafeMath for uint256;

contracts/token/VeToken.sol:
  2: pragma solidity 0.8.7;
  5: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  8:     using SafeMath for uint256;

Boolean comparisons

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here:

Booster.sol:352:        require(pool.shutdown == false, "pool is closed");
Booster.sol:498:        require(pool.shutdown == false, "pool is closed");
RewardFactory.sol:40:        require(rewardAccess[msg.sender] == true, "!auth");
RewardFactory.sol:57:        require(rewardAccess[msg.sender] == true, "!auth");
RewardFactory.sol:103:        require(operators.contains(_msgSender()) || rewardAccess[_msgSender()] == true, "!auth");
VoterProxy.sol:70:            operator == address(0) || IDeposit(operator).isShutdown() == true,
VoterProxy.sol:93:        if (protectedTokens[_token] == false) {
VoterProxy.sol:96:        if (protectedTokens[_gauge] == false) {
VoterProxy.sol:110:        require(stashPool[msg.sender] == true, "!auth");
VoterProxy.sol:113:        if (protectedTokens[address(_asset)] == true) {

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

helper/BitMath.sol:8:        require(x > 0, "BitMath::mostSignificantBit: zero");
helper/BitMath.sol:45:        require(x > 0, "BitMath::leastSignificantBit: zero");
helper/BoringMath.sol:20:        require(b > 0, "BoringMath: division by zero");
helper/BoringMath.sol:102:        require(b > 0, "BoringMath: division by zero");
helper/BoringMath.sol:122:        require(b > 0, "BoringMath: division by zero");
helper/BoringMath.sol:142:        require(b > 0, "BoringMath: division by zero");
helper/FixedPoint.sol:105:        require(other._x > 0, "FixedPoint::divuq: division by zero");
helper/FixedPoint.sol:131:        require(denominator > 0, "FixedPoint::fraction: division by zero");
BaseRewardPool.sol:173:        require(_amount > 0, "RewardPool : Cannot stake 0");
BaseRewardPool.sol:196:        require(_amount > 0, "RewardPool : Cannot stake 0");
BaseRewardPool.sol:215:        require(amount > 0, "RewardPool : Cannot withdraw 0");
VE3DLocker.sol:180:        require(rewardData[_rewardsToken].lastUpdateTime > 0);
VE3DLocker.sol:520:        require(_amount > 0, "Cannot stake 0");
VE3DLocker.sol:670:        require(locked > 0, "no exp locks");
VE3DLocker.sol:781:        require(_reward > 0, "No reward");
VE3DRewardPool.sol:210:        require(_amount > 0, "RewardPool : Cannot stake 0");
VE3DRewardPool.sol:234:        require(_amount > 0, "RewardPool : Cannot stake 0");
VE3DRewardPool.sol:253:        require(_amount > 0, "RewardPool : Cannot withdraw 0");
VeAssetDepositor.sol:132:        require(_amount > 0, "!>0");

Also, please enable the Optimizer.

>= is cheaper than > (and <= cheaper than <)

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between <= and <.

Consider replacing strict inequalities with non-strict ones to save some gas here:

helper/Babylonian.sol:51:        return (r < r1 ? r : r1);
helper/FixedPoint.sol:58:        uint256 z = FullMath.mulDiv(self._x, uint256(y < 0 ? -y : y), Q112);
helper/FixedPoint.sol:60:        return y < 0 ? -int256(z) : int256(z);
helper/MathUtil.sol:12:        return a < b ? a : b;

Splitting require() statements that use && saves gas

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement:

Booster.sol:261:        require(msg.sender == poolManager && !isShutdown, "!add");
Booster.sol:262:        require(_gauge != address(0) && _lptoken != address(0), "!param");
StashFactory.sol:87:        require(!isV1 && !isV2 && !isV3, "stash version mismatch");

Please, note that this might not hold true at a higher number of runs for the Optimizer (10k). However, it indeed is true at 200.

Use shift right/left instead of division/multiplication if possible

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated.

• Use >> 1 instead of / 2
• Use >> 2 instead of / 4
• Use << 3 instead of * 8

Affected code:

helper/FixedPoint.sol:167:                        ((112 - safeShiftBits) / 2)
VE3DLocker.sol:428:            uint256 mid = (min + max + 1) / 2;

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration, like at VE3DRewardPool.sol:148 or VE3DRewardPool.sol:281), the amount of gas wasted can be massive.

Here, I suggest storing the array's length in a variable before the for-loop, and use this new variable instead:

BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {
Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
VE3DLocker.sol:207:        for (uint256 i; i < rewardTokens.length; i++) {
VE3DLocker.sol:286:        for (uint256 i = 0; i < userRewards.length; i++) {
VE3DLocker.sol:315:        for (uint256 i = locks.length - 1; i + 1 != 0; i--) {
VE3DLocker.sol:360:        for (uint256 i = locks.length - 1; i + 1 != 0; i--) {
VE3DLocker.sol:457:        for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
VE3DLocker.sol:720:        for (uint256 i; i < rewardTokens.length; i++) {
VE3DLocker.sol:803:            for (uint256 i = 0; i < rewardTokens.length; i++) {
VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {

++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

helper/BitMath.sol:38:        if (x >= 0x2) r += 1;
helper/BitMath.sol:83:        if (x & 0x1 > 0) r -= 1;
helper/FullMath.sol:11:        if (mm < l) h -= 1;
helper/FullMath.sol:44:        if (mm > l) h -= 1;
BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {
Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
ExtraRewardStashV2.sol:71:            for (uint256 i = 0; i < length; i++) {
ExtraRewardStashV2.sol:78:            for (uint256 i = 0; i < length; i++) {
ExtraRewardStashV2.sol:137:        for (uint256 i = 0; i < maxRewards; i++) {
ExtraRewardStashV2.sol:140:                for (uint256 x = i; x < tokenCount; x++) {
ExtraRewardStashV2.sol:181:        for (uint256 i = 0; i < tokenCount; i++) {
ExtraRewardStashV2.sol:213:        for (uint256 i = 0; i < tokenCount; i++) {
ExtraRewardStashV3.sol:84:        for (uint256 i = 0; i < maxRewards; i++) {
ExtraRewardStashV3.sol:126:        for (uint256 i = 0; i < tokenCount; i++) {
RewardFactory.sol:49:        for (uint256 i = 0; i < length; i++) {
RewardFactory.sol:66:        for (uint256 i = 0; i < length; i++) {
VE3DLocker.sol:207:        for (uint256 i; i < rewardTokens.length; i++) {
VE3DLocker.sol:286:        for (uint256 i = 0; i < userRewards.length; i++) {
VE3DLocker.sol:315:        for (uint256 i = locks.length - 1; i + 1 != 0; i--) {
VE3DLocker.sol:360:        for (uint256 i = locks.length - 1; i + 1 != 0; i--) {
VE3DLocker.sol:383:            epochindex--;
VE3DLocker.sol:387:        for (uint256 i = epochindex - 1; i + 1 != 0; i--) {
VE3DLocker.sol:406:        for (uint256 i = _epoch; i + 1 != 0; i--) {
VE3DLocker.sol:425:        for (uint256 i = 0; i < 128; i++) {
VE3DLocker.sol:457:        for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
VE3DLocker.sol:463:                idx++;
VE3DLocker.sol:555:                idx--;
VE3DLocker.sol:593:            eIndex--;
VE3DLocker.sol:640:            for (uint256 i = nextUnlockIndex; i < length; i++) {
VE3DLocker.sol:665:                nextUnlockIndex++;
VE3DLocker.sol:720:        for (uint256 i; i < rewardTokens.length; i++) {
VE3DLocker.sol:803:            for (uint256 i = 0; i < rewardTokens.length; i++) {
VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:214:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:238:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:257:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:326:            for (uint256 i = 0; i < length; i++) {
VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Affected code:

BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {
Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
ExtraRewardStashV2.sol:71:            for (uint256 i = 0; i < length; i++) {
ExtraRewardStashV2.sol:78:            for (uint256 i = 0; i < length; i++) {
ExtraRewardStashV2.sol:137:        for (uint256 i = 0; i < maxRewards; i++) {
ExtraRewardStashV2.sol:140:                for (uint256 x = i; x < tokenCount; x++) {
ExtraRewardStashV2.sol:181:        for (uint256 i = 0; i < tokenCount; i++) {
ExtraRewardStashV2.sol:213:        for (uint256 i = 0; i < tokenCount; i++) {
ExtraRewardStashV3.sol:84:        for (uint256 i = 0; i < maxRewards; i++) {
ExtraRewardStashV3.sol:126:        for (uint256 i = 0; i < tokenCount; i++) {
RewardFactory.sol:49:        for (uint256 i = 0; i < length; i++) {
RewardFactory.sol:66:        for (uint256 i = 0; i < length; i++) {
VE3DLocker.sol:207:        for (uint256 i; i < rewardTokens.length; i++) {
VE3DLocker.sol:286:        for (uint256 i = 0; i < userRewards.length; i++) {
VE3DLocker.sol:425:        for (uint256 i = 0; i < 128; i++) {
VE3DLocker.sol:457:        for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
VE3DLocker.sol:640:            for (uint256 i = nextUnlockIndex; i < length; i++) {
VE3DLocker.sol:720:        for (uint256 i; i < rewardTokens.length; i++) {
VE3DLocker.sol:803:            for (uint256 i = 0; i < rewardTokens.length; i++) {
VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:214:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:238:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:257:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:326:            for (uint256 i = 0; i < length; i++) {
VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existant for uint256 here.

abi.encode() is less efficient than abi.encodePacked()

Changing abi.encode function to abi.encodePacked can save gas since the abi.encode function pads extra null bytes at the end of the call data, which is unnecessary. Also, in general, abi.encodePacked is more gas-efficient (see Solidity-Encode-Gas-Comparison).

Consider using abi.encodePacked() here:

StashFactory.sol:92:        bytes memory data = abi.encode(rewarded_token);

Public functions to external

An external call cost is less expensive than one of a public function. The following functions could be set external to save gas and improve code quality (extracted from Slither).

	- VirtualBalanceRewardPool.withdraw(address,uint256) (VirtualBalanceRewardPool.sol#141-146)
	- VE3DRewardPool.addOperator(address) (VE3DRewardPool.sol#114-116)
	- VE3DRewardPool.removeOperator(address) (VE3DRewardPool.sol#118-120)
	- VE3DRewardPool.stakeFor(address,uint256) (VE3DRewardPool.sol#233-250)
	- RewardFactory.addOperator(address,address) (RewardFactory.sol#24-27)
	- RewardFactory.removeOperator(address) (RewardFactory.sol#29-32)
	- Booster.withdrawAll(uint256) (Booster.sol#434-439)
	- SmartWalletWhitelist.approveWallet(address) (helper/SmartWalletWhitelist.sol#48-53)
	- VeTokenMinter.addOperator(address) (VeTokenMinter.sol#32-34)
	- VeTokenMinter.removeOperator(address) (VeTokenMinter.sol#36-38)
	- VE3DLocker.decimals() (VE3DLocker.sol#126-128)
	- VE3DLocker.name() (VE3DLocker.sol#130-132)
	- VE3DLocker.symbol() (VE3DLocker.sol#134-136)
	- VE3DLocker.version() (VE3DLocker.sol#138-140)
	- VE3DLocker.addOperator(address) (VE3DLocker.sol#197-199)
	- VE3DLocker.removeOperator(address) (VE3DLocker.sol#201-203)
	- VE3DLocker.lastTimeRewardApplicable(address) (VE3DLocker.sol#266-268)
	- VoterProxy.isValidSignature(bytes32,bytes) (VoterProxy.sol#199-205)
	- StashFactory.addOperator(address) (StashFactory.sol#30-32)
	- StashFactory.removeOperator(address) (StashFactory.sol#34-36)
	- BaseRewardPool.stakeFor(address,uint256) (BaseRewardPool.sol#195-212)
	- Migrations.setCompleted(uint256) (Migrations.sol#13-15)
	- TokenFactory.addOperator(address) (TokenFactory.sol#19-21)
	- TokenFactory.removeOperator(address) (TokenFactory.sol#23-25)

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

helper/FixedPoint.sol:50:        uint256 z = 0;
BaseRewardPool.sol:66:    uint256 public periodFinish = 0;
BaseRewardPool.sol:67:    uint256 public rewardRate = 0;
BaseRewardPool.sol:70:    uint256 public queuedRewards = 0;
BaseRewardPool.sol:71:    uint256 public currentRewards = 0;
BaseRewardPool.sol:72:    uint256 public historicalRewards = 0;
BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {
Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
ExtraRewardStashV1.sol:29:    uint256 public historicalRewards = 0;
ExtraRewardStashV2.sol:71:            for (uint256 i = 0; i < length; i++) {
ExtraRewardStashV2.sol:78:            for (uint256 i = 0; i < length; i++) {
ExtraRewardStashV2.sol:137:        for (uint256 i = 0; i < maxRewards; i++) {
ExtraRewardStashV2.sol:181:        for (uint256 i = 0; i < tokenCount; i++) {
ExtraRewardStashV2.sol:213:        for (uint256 i = 0; i < tokenCount; i++) {
ExtraRewardStashV3.sol:84:        for (uint256 i = 0; i < maxRewards; i++) {
ExtraRewardStashV3.sol:126:        for (uint256 i = 0; i < tokenCount; i++) {
RewardFactory.sol:49:        for (uint256 i = 0; i < length; i++) {
RewardFactory.sol:66:        for (uint256 i = 0; i < length; i++) {
VE3DLocker.sol:106:    bool public isShutdown = false;
VE3DLocker.sol:286:        for (uint256 i = 0; i < userRewards.length; i++) {
VE3DLocker.sol:420:        uint256 min = 0;
VE3DLocker.sol:425:        for (uint256 i = 0; i < 128; i++) {
VE3DLocker.sol:613:        uint256 reward = 0;
VE3DLocker.sol:803:            for (uint256 i = 0; i < rewardTokens.length; i++) {
VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:214:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:238:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:257:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:326:            for (uint256 i = 0; i < length; i++) {
VeAssetDepositor.sol:28:    uint256 public incentiveVeAsset = 0;
VirtualBalanceRewardPool.sol:74:    uint256 public periodFinish = 0;
VirtualBalanceRewardPool.sol:75:    uint256 public rewardRate = 0;
VirtualBalanceRewardPool.sol:78:    uint256 public queuedRewards = 0;
VirtualBalanceRewardPool.sol:79:    uint256 public currentRewards = 0;
VirtualBalanceRewardPool.sol:80:    uint256 public historicalRewards = 0;
VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {
VoterProxy.sol:227:        uint256 _balance = 0;

I suggest removing explicit initializations for default values.

Variable that should be constant

This variable is never edited:

File: VirtualBalanceRewardPool.sol
81:     uint256 public newRewardRatio = 830;

Consider marking it as constant.

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

helper/BitMath.sol:8:        require(x > 0, "BitMath::mostSignificantBit: zero");
helper/BitMath.sol:45:        require(x > 0, "BitMath::leastSignificantBit: zero");
helper/FixedPoint.sol:85:        require(upper <= type(uint112).max, "FixedPoint::muluq: upper overflow");
helper/FixedPoint.sol:105:        require(other._x > 0, "FixedPoint::divuq: division by zero");
helper/FixedPoint.sol:131:        require(denominator > 0, "FixedPoint::fraction: division by zero");
helper/FixedPoint.sol:149:        require(self._x != 0, "FixedPoint::reciprocal: reciprocal of zero");
Migrations.sol:9:        require(msg.sender == owner, "This function is restricted to the contract's owner");

I suggest shortening the revert strings to fit in 32 bytes.

Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

I suggest replacing all revert strings with custom errors in the solution. Affected files:

• helper/BitMath.sol
• helper/BoringMath.sol
• helper/FixedPoint.sol
• helper/FullMath.sol
• helper/SmartWalletWhitelist.sol
• token/VE3Token.sol
• token/VeToken.sol
• BaseRewardPool.sol
• Booster.sol
• DepositToken.sol
• ExtraRewardStashV3.sol
• Migrations.sol
• PoolManager.sol
• RewardFactory.sol
• StashFactory.sol
• TokenFactory.sol
• VE3DLocker.sol
• VE3DRewardPool.sol
• VeAssetDepositor.sol
• VeTokenMinter.sol
• VirtualBalanceRewardPool.sol
• VoterProxy.sol

Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

PoolManager.sol:27:    ) external onlyOwner returns (bool) {
PoolManager.sol:40:    function shutdownPool(address _pools, uint256 _pid) external onlyOwner returns (bool) {
RewardFactory.sol:24:    function addOperator(address _newOperator, address _veAsset) public onlyOwner {
RewardFactory.sol:29:    function removeOperator(address _operator) public onlyOwner {
StashFactory.sol:30:    function addOperator(address _newOperator) public onlyOwner {
StashFactory.sol:34:    function removeOperator(address _operator) public onlyOwner {
TokenFactory.sol:19:    function addOperator(address _newOperator) public onlyOwner {
TokenFactory.sol:23:    function removeOperator(address _operator) public onlyOwner {
VE3DLocker.sol:179:    ) external onlyOwner {
VE3DLocker.sol:185:    function setKickIncentive(uint256 _rate, uint256 _delay) external onlyOwner {
VE3DLocker.sol:193:    function shutdown() external onlyOwner {
VE3DLocker.sol:197:    function addOperator(address _newOperator) public onlyOwner {
VE3DLocker.sol:201:    function removeOperator(address _operator) public onlyOwner {
VE3DLocker.sol:789:    function recoverERC20(address _tokenAddress, uint256 _tokenAmount) external onlyOwner {
VE3DRewardPool.sol:107:    ) external onlyOwner {
VE3DRewardPool.sol:114:    function addOperator(address _newOperator) public onlyOwner {
VE3DRewardPool.sol:118:    function removeOperator(address _operator) public onlyOwner {
VeTokenMinter.sol:32:    function addOperator(address _newOperator) public onlyOwner {
VeTokenMinter.sol:36:    function removeOperator(address _operator) public onlyOwner {
VeTokenMinter.sol:41:    function updateveAssetWeight(address veAssetOperator, uint256 newWeight) external onlyOwner {
VeTokenMinter.sol:77:    function withdraw(address _destination, uint256 _amount) external onlyOwner {