veToken Finance contest - MiloTruck's results

QA Report

Low Risk Issues

Mistake in check in setFees() of VeAssetDepositor.sol

The function setFees() checks if _lockIncentive is >= 0 before assigning it to the storage variable lockIncentive, as shown below:

contracts/VeAssetDepositor.sol:
  62:        if (_lockIncentive >= 0 && _lockIncentive <= 30) {
  63:            lockIncentive = _lockIncentive;
  64:            emit FeesUpdated(_lockIncentive);
  65:        }

Since _lockIncentive has type uint256, it will always pass this check. I suspect the check was meant to avoid writing 0 to storage variables, thus it should be changed to:

    if (_lockIncentive > 0 && _lockIncentive <= 30) {

Wrong check in _lockVeAsset() of VeAssetDepositor.sol

The comment states over 2 week buffer. However, the check compares if the difference in time is above 2, which I believe should be 2 WEEK instead:

contracts/VeAssetDepositor.sol:
 105:        //increase time too if over 2 week buffer
 106:        if (unlockInWeeks.sub(unlockTime) > 2) {   // Should be 2 WEEK
 107:            IStaker(staker).increaseTime(unlockAt);
 108:            unlockTime = unlockInWeeks;
 109:        }

This wrong check causes unnecessary calls to increaseTime() and wastes gas due to unneeded function calls.

Non-Critical Issues

Use modifiers instead of require statements for access roles

Instead of using a require statement to check that msg.sender belongs to a certain role (e.g. msg.sender is owner), consider using modifiers. This would help improve code clarity and prevent accidental mistakes in future code.

For example, to check that msg.sender is owner, a modifier can be written as such:

modifier isOwner() {
    require(msg.sender == owner, "!auth");
    _;
}

Functions can then use isOwner to validate msg.sender, for example:

// setOwner() in Booster.sol
function setOwner(address _owner) external {
    require(msg.sender == owner, "!auth");
    // ...
}

can be rewritten to:

// setOwner() in Booster.sol
function setOwner(address _owner) external isOwner {
    // ...
}

Other instances of this include:

contracts/VoterProxy.sol:
  63:        require(msg.sender == owner, "!auth");
  68:        require(msg.sender == owner, "!auth");
  78:        require(msg.sender == owner, "!auth");
  84:        require(msg.sender == operator, "!auth");
  92:        require(msg.sender == operator, "!auth");
 128:        require(msg.sender == operator, "!auth");
 139:        require(msg.sender == operator, "!auth");
 151:        require(msg.sender == depositor, "!auth");
 159:        require(msg.sender == depositor, "!auth");
 167:        require(msg.sender == depositor, "!auth");
 173:        require(msg.sender == depositor, "!auth");
 186:        require(msg.sender == operator, "!auth");
 211:        require(msg.sender == operator, "!auth");
 225:        require(msg.sender == operator, "!auth");
 257:        require(msg.sender == operator, "!auth");
 263:        require(msg.sender == operator, "!auth");
 279:        require(msg.sender == operator, "!auth");

contracts/VeAssetDepositor.sol:
  54:        require(msg.sender == feeManager, "!auth");
  60:        require(msg.sender == feeManager, "!auth");
  69:        require(msg.sender == feeManager, "!auth");

contracts/Booster.sol:
 124:        require(msg.sender == owner, "!auth");
 130:        require(msg.sender == feeManager, "!auth");
 136:        require(msg.sender == poolManager, "!auth");
 146:        require(msg.sender == owner, "!auth");
 163:        require(msg.sender == owner, "!auth");
 169:        require(msg.sender == voteDelegate, "!auth");
 179:        require(msg.sender == owner, "!auth");
 194:        require(msg.sender == feeManager, "!auth");
 226:        require(msg.sender == feeManager, "!auth");
 244:        require(msg.sender == feeManager, "!auth");
 261:        require(msg.sender == poolManager && !isShutdown, "!add");
 309:        require(msg.sender == poolManager, "!auth");
 326:        require(msg.sender == owner, "!auth");
 448:        require(msg.sender == rewardContract, "!auth");
 458:        require(msg.sender == voteDelegate, "!auth");
 468:        require(msg.sender == voteDelegate, "!auth");
 477:        require(msg.sender == stash, "!auth");
 485:        require(msg.sender == stash, "!auth");
 604:        require(msg.sender == rewardContract || msg.sender == lockRewards, "!auth");

contracts/BaseRewardPool.sol:
 122:        require(msg.sender == rewardManager, "!authorized");
 132:        require(msg.sender == rewardManager, "!authorized");
 301:        require(msg.sender == operator, "!authorized");

contracts/VE3DRewardPool.sol:
 135:        require(msg.sender == rewardManager, "!authorized");
 142:        require(msg.sender == rewardManager, "!authorized");

Use native time units rather than numbers of seconds

Use 1 weeks instead of 7 * 86400:

contracts/VeAssetDepositor.sol:
  18:        uint256 private constant WEEK = 7 * 86400;

Typos

Should be amount:

contracts/VeAssetDepositor.sol:
  93:        //increase ammount

safeApprove() is deprecated

With reference to SafeERC20.sol, safeApprove() is deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance().

Consider using these functions instead of safeApprove() in these instances:

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);

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

contracts/Booster.sol:
 374:        IERC20(token).safeApprove(rewardContract, _amount);

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

Critical changes should use two-step procedure

The lack of a two-step procedure for critical operations leaves them error-prone. This is especially important for functions that transfer roles to other addresses, such as the contract owner, as mistakenly transferring critical roles to wrong addresses could be irreversible.

Consider adding a two-step procedure for these critical functions:

contracts/VeAssetDepositor.sol:
  53:        function setFeeManager(address _feeManager) external {
      
contracts/Booster.sol:
 123:        function setOwner(address _owner) external {
 129:        function setFeeManager(address _feeM) external {
 135:        function setPoolManager(address _poolM) external {
 141:        function setFactories(
 162:        function setArbitrator(address _arb) external {
 168:        function setVoteDelegate(address _voteDelegate) external {
 174:        function setRewardContracts(
 243:        function setTreasury(address _treasury) external {

contracts/VoterProxy.sol:
  62:        function setOwner(address _owner) external {
  67:        function setOperator(address _operator) external {
  77:        function setDepositor(address _depositor) external { 

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps
Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

contracts/VeAssetDepositor.sol:
  73:        uint256 unlockAt = block.timestamp + maxTime;
 102:        uint256 unlockAt = block.timestamp + maxTime;

contracts/BaseRewardPool.sol:
 149:        return Math.min(block.timestamp, periodFinish);
 305:        if (block.timestamp >= periodFinish) {
 312:        uint256 elapsedTime = block.timestamp.sub(periodFinish.sub(duration));
 329:        if (block.timestamp >= periodFinish) {
 332:        uint256 remaining = periodFinish.sub(block.timestamp);
 338:        lastUpdateTime = block.timestamp;
 339:        periodFinish = block.timestamp.add(duration);

contracts/VE3DRewardPool.sol:
 167:        return Math.min(block.timestamp, rewardTokenInfo[_rewardToken].periodFinish);
 346:        if (block.timestamp >= rewardTokenInfo[_rewardToken].periodFinish) {
 353:        uint256 elapsedTime = block.timestamp.sub(
 372:        if (block.timestamp >= rewardTokenInfo[_rewardToken].periodFinish) {
 375:        uint256 remaining = rewardTokenInfo[_rewardToken].periodFinish.sub(block.timestamp);
 381:        rewardTokenInfo[_rewardToken].lastUpdateTime = block.timestamp;
 382:        rewardTokenInfo[_rewardToken].periodFinish = block.timestamp.add(duration);

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

contracts/VeAssetDepositor.sol:
  36:        event Deposited(address indexed user, uint256 amount, bool lock);

contracts/Booster.sol:
  73:        event Deposited(address indexed user, uint256 indexed poolid, uint256 amount);
  74:        event Withdrawn(address indexed user, uint256 indexed poolid, uint256 amount);

  86:        event FeesUpdated(
  87:                uint256 lockFees,
  88:                uint256 stakerFees,
  89:                uint256 stakerLockFee,
  90:                uint256 callerFees,
  91:                uint256 platform
  92:            );

 102:        event Voted(uint256 indexed voteId, address indexed votingAddress, bool support);

contracts/BaseRewardPool.sol:
  87:        event RewardUpdated(
  88:                address indexed user,
  89:                uint256 reward,
  90:                uint256 rewardPerTokenStored,
  91:                uint256 lastUpdateTime
  92:            );

Gas Optimizations Report

For-Loops: Cache array length outside of loops

Reading an array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

Consider making the following change to these lines:

contracts/VoterProxy.sol:
 217:        for (uint256 i = 0; i < _tokenVote.length; i++) {

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

contracts/BaseRewardPool.sol:
 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++) {

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

For example, the code below:

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

can be changed to:

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

Consider making the following change to these lines:

contracts/VoterProxy.sol:
 217:        for (uint256 i = 0; i < _tokenVote.length; i++) {

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

contracts/BaseRewardPool.sol:
 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/VE3DRewardPool.sol:
 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++) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

contracts/VoterProxy.sol:
 217:        for (uint256 i = 0; i < _tokenVote.length; i++) {

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

contracts/BaseRewardPool.sol:
 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/VE3DRewardPool.sol:
 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++) {

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

contracts/VoterProxy.sol:
 100:        if (balance > 0) {

contracts/VeAssetDepositor.sol:
  89:        if (veAssetBalance > 0) {
 117:        if (incentiveVeAsset > 0) {
 132:        require(_amount > 0, "!>0");
 138:        if (incentiveVeAsset > 0) {

contracts/Booster.sol:
 517:        if (veAssetBal > 0) {
 526:        if (treasury != address(0) && treasury != address(this) && platformFee > 0) {
 541:        if (_callIncentive > 0) {
 551:        if (_lockIncentive > 0) {
 556:        if (_stakerIncentive > 0) {
 562:        if (_stakerLockIncentive > 0) {
 586:        if (_lockFeesIncentive > 0) {
 590:        if (_stakerLockFeesIncentive > 0) {

contracts/BaseRewardPool.sol:
 173:        require(_amount > 0, "RewardPool : Cannot stake 0");
 196:        require(_amount > 0, "RewardPool : Cannot stake 0");
 215:        require(amount > 0, "RewardPool : Cannot withdraw 0");
 273:        if (reward > 0) {

contracts/VE3DRewardPool.sol:
 210:        require(_amount > 0, "RewardPool : Cannot stake 0");
 234:        require(_amount > 0, "RewardPool : Cannot stake 0");
 253:        require(_amount > 0, "RewardPool : Cannot withdraw 0");
 285:        if (reward > 0) {

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

Consider changing the visibility of the following from public to internal or private:

contracts/Booster.sol:
  33:        uint256 public constant MaxFees = 2000;

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

contracts/VeTokenMinter.sol:
  32:        function addOperator(address _newOperator) public onlyOwner {
  36:        function removeOperator(address _operator) public onlyOwner {

contracts/VoterProxy.sol:
 199:        function isValidSignature(bytes32 _hash, bytes memory) public view returns (bytes4) {

contracts/Booster.sol:
 434:        function withdrawAll(uint256 _pid) public returns (bool) {

contracts/BaseRewardPool.sol:
 195:        function stakeFor(address _for, uint256 _amount) public updateReward(_for) returns (bool) {

contracts/VE3DRewardPool.sol:
 114:        function addOperator(address _newOperator) public onlyOwner {
 118:        function removeOperator(address _operator) public onlyOwner {
 233:        function stakeFor(address _for, uint256 _amount) public updateReward(_for) {

Errors: Split multiple require statements rather than using &&

With reference to this issue, using separate require statements instead of && will have a cheaper runtime gas cost. However, note that the contract will cost more gas to deploy, which is an acceptable trade-off.

Consider splitting the following into multiple require statements:

contracts/Booster.sol:
 261:        require(msg.sender == poolManager && !isShutdown, "!add");
 262:        require(_gauge != address(0) && _lptoken != address(0), "!param");

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

• Cheaper deployment cost
• Lower runtime cost upon revert

Taken from Custom Errors in Solidity:

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 can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

contracts/VeTokenMinter.sol:
  42:        require(operators.contains(veAssetOperator), "not an veAsset operator");
  49:        require(operators.contains(_msgSender()), "not an operator");

contracts/VoterProxy.sol:
  63:        require(msg.sender == owner, "!auth");
  68:        require(msg.sender == owner, "!auth");
  78:        require(msg.sender == owner, "!auth");
  84:        require(msg.sender == operator, "!auth");
  92:        require(msg.sender == operator, "!auth");
 110:        require(stashPool[msg.sender] == true, "!auth");
 128:        require(msg.sender == operator, "!auth");
 139:        require(msg.sender == operator, "!auth");
 151:        require(msg.sender == depositor, "!auth");
 159:        require(msg.sender == depositor, "!auth");
 167:        require(msg.sender == depositor, "!auth");
 173:        require(msg.sender == depositor, "!auth");
 186:        require(msg.sender == operator, "!auth");
 211:        require(msg.sender == operator, "!auth");
 225:        require(msg.sender == operator, "!auth");
 257:        require(msg.sender == operator, "!auth");
 263:        require(msg.sender == operator, "!auth");
 279:        require(msg.sender == operator, "!auth");
 282:        require(success, "!success");

contracts/VeAssetDepositor.sol:
  54:        require(msg.sender == feeManager, "!auth");
  60:        require(msg.sender == feeManager, "!auth");
  69:        require(msg.sender == feeManager, "!auth");
 132:        require(_amount > 0, "!>0");

contracts/Booster.sol:
 124:        require(msg.sender == owner, "!auth");
 130:        require(msg.sender == feeManager, "!auth");
 136:        require(msg.sender == poolManager, "!auth");
 146:        require(msg.sender == owner, "!auth");
 163:        require(msg.sender == owner, "!auth");
 169:        require(msg.sender == voteDelegate, "!auth");
 179:        require(msg.sender == owner, "!auth");
 194:        require(msg.sender == feeManager, "!auth");
 226:        require(msg.sender == feeManager, "!auth");
 231:        require(total <= MaxFees, ">MaxFees");
 244:        require(msg.sender == feeManager, "!auth");
 261:        require(msg.sender == poolManager && !isShutdown, "!add");
 262:        require(_gauge != address(0) && _lptoken != address(0), "!param");
 309:        require(msg.sender == poolManager, "!auth");
 326:        require(msg.sender == owner, "!auth");
 350:        require(!isShutdown, "shutdown");
 352:        require(pool.shutdown == false, "pool is closed");
 360:        require(gauge != address(0), "!gauge setting");
 448:        require(msg.sender == rewardContract, "!auth");
 458:        require(msg.sender == voteDelegate, "!auth");
 468:        require(msg.sender == voteDelegate, "!auth");
 477:        require(msg.sender == stash, "!auth");
 485:        require(msg.sender == stash, "!auth");
 498:        require(pool.shutdown == false, "pool is closed");
 570:        require(!isShutdown, "shutdown");
 604:        require(msg.sender == rewardContract || msg.sender == lockRewards, "!auth");

contracts/BaseRewardPool.sol:
 122:        require(msg.sender == rewardManager, "!authorized");
 123:        require(_reward != address(0), "!reward setting");
 124:        require(extraRewards.length < EXTRA_REWARD_POOLS, "!extra reward pools exceed");
 132:        require(msg.sender == rewardManager, "!authorized");
 173:        require(_amount > 0, "RewardPool : Cannot stake 0");
 196:        require(_amount > 0, "RewardPool : Cannot stake 0");
 215:        require(amount > 0, "RewardPool : Cannot withdraw 0");
 301:        require(msg.sender == operator, "!authorized");

contracts/VE3DRewardPool.sol:
 135:        require(msg.sender == rewardManager, "!authorized");
 136:        require(_reward != address(0), "!reward setting");
 142:        require(msg.sender == rewardManager, "!authorized");
 210:        require(_amount > 0, "RewardPool : Cannot stake 0");
 234:        require(_amount > 0, "RewardPool : Cannot stake 0");
 253:        require(_amount > 0, "RewardPool : Cannot withdraw 0");
 342:        require(operators.contains(_msgSender()), "!authorized");

No need to use SafeMath

As stated here, the SafeMath library is not needed starting from Solidity v0.8 onwards, since all arithmetic operations have built-in overflow/underflow checking.

Since all the contracts in scope use Solidity v0.8.7, SafeMath is redundant and incurs excessive overhead gas costs. Consider doing the following:

• Removing SafeMath and using the built-in arithmetic checks
• Wrapping arithemtics that cannot underflow/overflow in an unchecked block to remove the built-in checks:

    unchecked { a += b; }

Using bool for storage incurs overhead

Declaring storage variables as bool is more expensive compared to uint256, as explained here:

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

Consider re-defining isShutdown as type uint256, and setting it to 0/1 instead of false/true:

contracts/Booster.sol:
  58:        bool public isShutdown;

Unnecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

Consider declaring the following lines without explicitly setting a value:

contracts/VoterProxy.sol:
 227:        uint256 _balance = 0;

contracts/VeAssetDepositor.sol:
  28:        uint256 public incentiveVeAsset = 0;

contracts/BaseRewardPool.sol:
  66:        uint256 public periodFinish = 0;
  67:        uint256 public rewardRate = 0;
  70:        uint256 public queuedRewards = 0;
  71:        uint256 public currentRewards = 0;
  72:        uint256 public historicalRewards = 0;

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. Not defining these variables can help to reduce gas cost and contract size.

Instances include:

contracts/VeAssetDepositor.sol:
 154:        bool depositOnly = _stakeAddress == address(0);

contracts/Booster.sol:
 333:        address token = pool.lptoken;
 334:        address gauge = pool.gauge;
 387:        address lptoken = poolInfo[_pid].lptoken;
 402:        address gauge = pool.gauge;
 405:        address token = pool.token;
 435:        address token = poolInfo[_pid].token;
 447:        address rewardContract = poolInfo[_pid].veAssetRewards;
 476:        address stash = poolInfo[_pid].stash;
 486:        address gauge = poolInfo[_pid].gauge;
 500:        address gauge = pool.gauge;
 603:        address rewardContract = poolInfo[_pid].veAssetRewards;

Storage variables should be declared immutable when possible

If a storage variable is assigned only in the constructor, it should be declared as immutable. This would help to reduce gas costs as calls to immutable variables are much cheaper than regular state variables, as seen from the Solidity Docs:

Compared to regular state variables, the gas costs of constant and immutable variables are much lower. Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed.

Consider declaring these variables as immutable:

contracts/VeTokenMinter.sol:
  16:        ERC20 public veToken;
  18:        uint256 public totalCliffs;
  19:        uint256 public reductionPerCliff;

contracts/VoterProxy.sol:
  30:        string public name;
  31:        IVoteEscrow.EscrowModle public escrowModle;

contracts/VeAssetDepositor.sol:
  30:        uint256 private maxTime;

contracts/BaseRewardPool.sol:
  55:        IERC20 public rewardToken;
  56:        IERC20 public stakingToken;
  62:        address public operator;
  63:        address public rewardManager;
  65:        uint256 public pid;

Boolean comparisons

Comparing to a constant (true or false) is a bit more expensive than directly checking the boolean value.

Considering changing the following lines from var == true to var, or var == false to !var:

contracts/VoterProxy.sol:
  70:        operator == address(0) || IDeposit(operator).isShutdown() == true,
  93:        if (protectedTokens[_token] == false) {
  96:        if (protectedTokens[_gauge] == false) {
 110:        require(stashPool[msg.sender] == true, "!auth");
 113:        if (protectedTokens[address(_asset)] == true) {

contracts/Booster.sol:
 352:        require(pool.shutdown == false, "pool is closed");
 498:        require(pool.shutdown == false, "pool is closed");;

Variables declared as constant are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

contracts/VeTokenMinter.sol:
  15:        uint256 public constant maxSupply = 30 * 1000000 * 1e18;

contracts/VeAssetDepositor.sol:
  18:        uint256 private constant WEEK = 7 * 86400;

contracts/BaseRewardPool.sol:
  59:        uint256 constant BLOCKS_PER_YEAR = BLOCKS_PER_DAY * 365;

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.