veToken Finance contest - sashik_eth's results

L01 - Missing zero address checks in sensitive setters

Some functions missing zero address checks when setting admin-like role addresses, which could lead to loss of control.

Apply a zero-address check and consider implementing a two-step process like in transferOwnership pattern, where the active role address assigns an account and the designated account must call the acceptOwnership() function for full transfer of role.

    function setOwner(address _owner) external { 
        require(msg.sender == owner, "!auth");
        owner = _owner;
        emit OwnerUpdated(_owner);
    }

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

    function setPoolManager(address _poolM) external { 
        require(msg.sender == poolManager, "!auth");
        poolManager = _poolM;
        emit PoolManagerUpdated(_poolM);
    }
        ...
        function setVoteDelegate(address _voteDelegate) external {  
        require(msg.sender == voteDelegate, "!auth");
        voteDelegate = _voteDelegate;
        emit VoteDelegateUpdated(_voteDelegate);
    }

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

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

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L53-L57

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

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

L02 - Missing input validation of array lengths

Function voteGaugeWeight() fails to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior.

    function voteGaugeWeight(address[] calldata _tokenVote, uint256[] calldata _weight)
        external
        returns (bool)
    {
        require(msg.sender == operator, "!auth");

        if (escrowModle == IVoteEscrow.EscrowModle.PICKLE) {
            //vote
            IVoting(gaugeProxy).vote(_tokenVote, _weight);
        } else {
            for (uint256 i = 0; i < _tokenVote.length; i++) { 
                IVoting(gaugeProxy).vote_for_gauge_weights(_tokenVote[i], _weight[i]);
            }
        }
        return true;
    }

N01 - Typos

contracts/Booster.sol:31    // platoform fee // typo - platform
contracts/VeAssetDepositor.sol:93   //increase ammount // typo - amount

N02 - Constants should be named with all capital letters with underscores separating words

contracts/BaseRewardPool.sol:57 uint256 public constant duration = 7 days; 
contracts/BaseRewardPool.sol:73 uint256 public constant newRewardRatio = 830; 
contracts/Booster.sol:33    uint256 public constant MaxFees = 2000;
contracts/VE3DRewardPool.sol:59 uint256 public constant duration = 7 days;
contracts/VE3DRewardPool.sol:64 uint256 public constant newRewardRatio = 830;
contracts/VeTokenMinter.sol:15  uint256 public constant maxSupply = 30 * 1000000 * 1e18; //30mil

N03 - Missing checks for zero addresses in constructors

A wrong user input or wallets defaulting to the zero addresses for a missing input can lead to the contract needing to redeploy:

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L38 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L95 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L104 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L38

G01 - Variables that can be changed to immutable

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

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L55-L65

contracts/VeAssetDepositor.sol:30   uint256 private maxTime; 

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

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

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

contracts/VoterProxy.sol:30 string public name; 

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

G02 - Using != 0 instead of > 0 in require statement with uint

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

contracts/BaseRewardPool.sol:173    require(_amount > 0, "RewardPool : Cannot stake 0"); 
contracts/BaseRewardPool.sol:196    require(_amount > 0, "RewardPool : Cannot stake 0");  
contracts/BaseRewardPool.sol:215    require(amount > 0, "RewardPool : Cannot withdraw 0");
contracts/VE3DRewardPool.sol:210    require(_amount > 0, "RewardPool : Cannot stake 0"); 
contracts/VE3DRewardPool.sol:234    require(_amount > 0, "RewardPool : Cannot stake 0"); 
contracts/VE3DRewardPool.sol:253    require(_amount > 0, "RewardPool : Cannot withdraw 0");  
contracts/VeAssetDepositor.sol:132  require(_amount > 0, "!>0");

G03 - More gas efficient for loop with ++i increment and unchecked block

Loop index increments can be written as unchecked { ++i } instead of simply i++ to save gas.

contracts/BaseRewardPool.sol:176    for (uint256 i = 0; i < extraRewards.length; i++) { 
contracts/BaseRewardPool.sol:199    for (uint256 i = 0; i < extraRewards.length; i++) {
contracts/BaseRewardPool.sol:218    for (uint256 i = 0; i < extraRewards.length; i++) { 
contracts/BaseRewardPool.sol:245    for (uint256 i = 0; i < extraRewards.length; i++) { 
contracts/BaseRewardPool.sol:282    for (uint256 i = 0; i < extraRewards.length; i++) { 
contracts/Booster.sol:329   for (uint256 i = 0; i < poolInfo.length; i++) { 
contracts/VE3DRewardPool.sol:148    for (uint256 i = 0; i < rewardTokens.length(); i++) { 
contracts/VE3DRewardPool.sol:214    for (uint256 i = 0; i < length; i++) { 
contracts/VE3DRewardPool.sol:238    for (uint256 i = 0; i < length; i++) { 
contracts/VE3DRewardPool.sol:257    for (uint256 i = 0; i < length; i++) {
contracts/VE3DRewardPool.sol:281    for (uint256 i = 0; i < rewardTokens.length(); i++) { 
contracts/VE3DRewardPool.sol:326    for (uint256 i = 0; i < length; i++) { 
contracts/VoterProxy.sol:217    for (uint256 i = 0; i < _tokenVote.length; i++) { 

G04 - Cache array length in for

Caching the array length before for loop could save gas if array stores in memory:

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

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

G05 - No need to initialize variables with default values

If a variable is not set, it is assumed to have the default value (0, false etc).

    uint256 public periodFinish = 0;
    uint256 public rewardRate = 0;
    uint256 public queuedRewards = 0;
    uint256 public currentRewards = 0;
    uint256 public historicalRewards = 0;

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L66-L72

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

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

G06 - No need to compare bool variable with true/false in require or if statements

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

G07 - No need to use SafeMath in Solidity 0.8

All contracts in the scope using SafeMath library, which is not necessary

G08 - unchecked block can be used for gas efficiency of the expression that can't overflow/underflow

Some lines could be without SafeMath and included in unchecked block since they can't overflow/underflow:

contracts/BaseRewardPool.sol:330    rewardRate = reward.div(duration); // could be unchecked since duration has non-zero constant value 
contracts/BaseRewardPool.sol:335    rewardRate = reward.div(duration); // could be unchecked since duration has non-zero constant value 
contracts/BaseRewardPool.sol:339    periodFinish = block.timestamp.add(duration); // could be unchecked since duration has constant value
contracts/VE3DRewardPool.sol:373    rewardTokenInfo[_rewardToken].rewardRate = reward.div(duration); // could be unchecked since duration has non-zero constant value 
contracts/VE3DRewardPool.sol:378    rewardTokenInfo[_rewardToken].rewardRate = reward.div(duration); // could be unchecked since duration has non-zero constant value 
contracts/VE3DRewardPool.sol:382    rewardTokenInfo[_rewardToken].periodFinish = block.timestamp.add(duration); // could be unchecked since duration has constant value
contracts/VeAssetDepositor.sol:74   uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK; // could be unchecked since duration has non-zero constant value 
contracts/VeAssetDepositor.sol:103  uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK; // could be unchecked since duration has non-zero constant value 
contracts/VeTokenMinter.sol:29  reductionPerCliff = maxSupply.div(totalCliffs); // could be unchecked since totalCliffs has non-zero value, that doesn't change
contracts/VeTokenMinter.sol:57  uint256 cliff = supply.div(reductionPerCliff); // could be unchecked since totalCliffs has non-zero value, that doesn't change
contracts/VeTokenMinter.sol:61  uint256 reduction = totalCliffs.sub(cliff); // could be unchecked due to check on L59
contracts/VeTokenMinter.sol:66  uint256 amtTillMax = maxSupply.sub(supply); // could be unchecked since supply == totalSupply, and it's can't be greater than maxSupply
contracts/VeTokenMinter.sol:73  totalSupply += _amount; // could be unchecked due to function logic