veToken Finance contest - Funen's results

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

Title : Value of certain range can be used instead of nothing at Booster.sol (Line.233) was used :

    //values must be within certain ranges

but at certain range was not used, so it can be deleted as it should be or you can use implementation down below :

function setFees(uint256 _lockFees, uint256 _stakerFees, uint256 _callerFees, uint256 _platform) external { require(msg.sender==feeManager, "!auth");

   uint256 total = _lockFees.add(_stakerFees).add(_callerFees).add(_platform);
   require(total <= MaxFees, ">MaxFees");

   //values must be within certain ranges     
   if(_lockFees >= 1000 && _lockFees <= 1500 
       && _stakerFees >= 300 && _stakerFees <= 600
       && _callerFees >= 10 && _callerFees <= 100
       && _platform <= 200) {
       lockIncentive = _lockFees;
       stakerIncentive = _stakerFees;
       earmarkIncentive = _callerFees;
       platformFee = _platform;
   }

so the value was used to be on certain range and not gonna do unusual behavior.

Low

1. Title : Value of certain range can be used instead of nothing

at Booster.sol (Line.233) was used :

        //values must be within certain ranges

but at certain range was not used, so it can be deleted as it should be or you can use implementation down below :

   function setFees(uint256 _lockFees, uint256 _stakerFees, uint256 _callerFees, uint256 _platform) 
   external {
       require(msg.sender==feeManager, "!auth");

       uint256 total = _lockFees.add(_stakerFees).add(_callerFees).add(_platform);
       require(total <= MaxFees, ">MaxFees");

       //values must be within certain ranges     
       if(_lockFees >= 1000 && _lockFees <= 1500 
           && _stakerFees >= 300 && _stakerFees <= 600
           && _callerFees >= 10 && _callerFees <= 100
           && _platform <= 200) {
           lockIncentive = _lockFees;
           stakerIncentive = _stakerFees;
           earmarkIncentive = _callerFees;
           platformFee = _platform;
       }

so the value was used to be on certain range and not gonna do unusual behavior.

2. Title : Comment was not the same as Actual code

        //token factory can also be immutable

on this code was using token factory can also be immutable but at the actual code can't be an immutable, so comment can be removed or it can be changed instead.

Tool Used

Manual Review

3. Title : Unnecessary Code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L282

        require(success, "!success");

this checked was unnecessary since it would pass to return, so instead for being checked and no checked was used. it can be deleted instead and pass it to return.

Non Critical

4. Title : Typo Comment

This cam be changed as it should be for better readibility

1.) File : contracts/VeAssetDepositor.sol (Line.93)

changed into amount

        //increase ammount

2.) File : contracts/Booster.sol (Line.31)

changed into platfrom

    // platoform fee

5. Title : simplify the number of maxSupply

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L15

    uint256 public constant maxSupply = 30 * 1000000 * 1e18; //30mil

changed to :

uint256 public constant maxSupply = 3e25 //30mil

6. Title : change add,mul,div into math operator

Since update into pragma ^0.8.0

Optional: If you use SafeMath or a similar library, change x.add(y) to x + y, x.mul(y) to x * y etc.

1.) File : contracts/VeAssetDepositor.sol (Line.140)

                _amount = _amount.add(incentiveVeAsset);

2.) File : contracts/VeAssetDepositor.sol (Line.147)

              uint256 callIncentive = _amount.mul(lockIncentive).div(FEE_DENOMINATOR);

3.) File : contracts/VeAssetDepositor.sol (Line.148)

              _amount = _amount.sub(callIncentive);

4.) File : contracts/VeAssetDepositor.sol (Line.151)

              incentiveVeAsset = incentiveVeAsset.add(callIncentive);

5.) File : contracts/Booster.sol (Line.228)

             uint256 total = _lockFees.add(_stakerFees).add(_callerFees).add(_platform).add(

6.) File : contracts/Booster.sol (Lines.518-523)

            uint256 _lockIncentive = veAssetBal.mul(lockIncentive).div(FEE_DENOMINATOR);
            uint256 _stakerIncentive = veAssetBal.mul(stakerIncentive).div(FEE_DENOMINATOR);
            uint256 _stakerLockIncentive = veAssetBal.mul(stakerLockIncentive).div(
                FEE_DENOMINATOR
            );
            uint256 _callIncentive = veAssetBal.mul(earmarkIncentive).div(FEE_DENOMINATOR);

7.) File : contracts/Booster.sol (Lines.528-529)

            uint256 _platform = veAssetBal.mul(platformFee).div(FEE_DENOMINATOR);
            veAssetBal = veAssetBal.sub(_platform);

8.) File : contracts/Booster.sol (Lines.534-L538)

            veAssetBal = veAssetBal
                .sub(_lockIncentive)
                .sub(_callIncentive)
                .sub(_stakerIncentive)
                .sub(_stakerLockIncentive);

9.) File : contracts/Booster.sol (Lines.582-584)

           uint256 _lockFeesIncentive = _balance.mul(lockFeesIncentive).div(FEE_DENOMINATOR);
           uint256 _stakerLockFeesIncentive = _balance.mul(stakerLockFeesIncentive).div(
                FEE_DENOMINATOR

10.) File : contracts/BaseRewardPool.sol (Line.149)

           return Math.min(block.timestamp, periodFinish);

11.) File : contracts/BaseRewardPool.sol (Lines.157-159)

            rewardPerTokenStored.add(
                lastTimeRewardApplicable().sub(lastUpdateTime).mul(rewardRate).mul(1e18).div(
                    totalSupply()

12.) File : contracts/BaseRewardPool.sol (Lines.166-169)

            balanceOf(account)
                .mul(rewardPerToken().sub(userRewardPerTokenPaid[account]))
                .div(1e18)
                .add(rewards[account]);

13.) File : contracts/BaseRewardPool.sol (Lines.180-181)

           _totalSupply = _totalSupply.add(_amount);
           _balances[msg.sender] = _balances[msg.sender].add(_amount);

14.) File : contracts/BaseRewardPool.sol (Lines.204-205)

           _totalSupply = _totalSupply.add(_amount);
           _balances[_for] = _balances[_for].add(_amount);

15.) File : contracts/BaseRewardPool.sol (Lines.L222-223)

           _totalSupply = _totalSupply.sub(amount);
           _balances[msg.sender] = _balances[msg.sender].sub(amount);

16.) File : contracts/BaseRewardPool.sol (Lines.249-250))

            _totalSupply = _totalSupply.sub(amount);
            _balances[msg.sender] = _balances[msg.sender].sub(amount);

17.) File : contracts/BaseRewardPool.sol (Line.296))

            queuedRewards = queuedRewards.add(_amount);

18.) File : contracts/BaseRewardPool.sol (Line.303))

             _rewards = _rewards.add(queuedRewards);

19.) File : contracts/BaseRewardPool.sol (Line.312))

             uint256 elapsedTime = block.timestamp.sub(periodFinish.sub(duration));

20.) File : contracts/BaseRewardPool.sol (Line.315))

            uint256 queuedRatio = currentAtNow.mul(1000).div(_rewards);

21.) File : contracts/BaseRewardPool.sol (Line.328))

            historicalRewards = historicalRewards.add(reward);

22.) File : contracts/BaseRewardPool.sol (Line.330))

             rewardRate = reward.div(duration);

23.) File : contracts/BaseRewardPool.sol (Lines.332-335))

            uint256 remaining = periodFinish.sub(block.timestamp);
            uint256 leftover = remaining.mul(rewardRate);
            reward = reward.add(leftover);
            rewardRate = reward.div(duration);

24.) File : contracts/VE3DRewardPool.sol (Lines.176-182)

            rewardTokenInfo[_rewardToken].rewardPerTokenStored.add(
                lastTimeRewardApplicable(_rewardToken)
                    .sub(rewardTokenInfo[_rewardToken].lastUpdateTime)
                    .mul(rewardTokenInfo[_rewardToken].rewardRate)
                    .mul(1e18)
                    .div(supply)
            );

25.) File : contracts/VE3DRewardPool.sol (Lines.187-194)

               balanceOf(account)
                .mul(
                    rewardPerToken(_rewardToken).sub(
                        rewardTokenInfo[_rewardToken].userRewardPerTokenPaid[account]
                    )
                )
                .div(1e18)
                .add(rewardTokenInfo[_rewardToken].rewards[account]);

26.) File : contracts/VE3DRewardPool.sol (Line.202)

              uint256 fees = r.mul(depositFeeRate).div(FEE_DENOMINATOR);

27.) File : contracts/VE3DRewardPool.sol (Lines.219-221)

         _totalSupply = _totalSupply.add(_amount);
          //add to sender balance sheet
        _balances[msg.sender] = _balances[msg.sender].add(_amount);

28.) File : contracts/VE3DRewardPool.sol (Lines.243-245)

         _totalSupply = _totalSupply.add(_amount);
          //add to _for's balance sheet
        _balances[msg.sender] = _balances[msg.sender].add(_amount);

29.) File : contracts/VE3DRewardPool.sol (Lines.353-354)

        uint256 elapsedTime = block.timestamp.sub(
        rewardTokenInfo[_rewardToken].periodFinish.sub(duration)

30.) File : contracts/VE3DRewardPool.sol (Lines.373-382)

GAS OPT

1. Title : Using ++i than i++ for saving more gas

Using i++ instead ++i for all the loops, the variable i is incremented using i++. It is known that implementation by using ++i costs less gas per iteration than i++.

Tools Used

Manual Review

Occurances

main/contracts/Booster.sol#L329                for (uint256 i = 0; i < poolInfo.length; i++) {
main/contracts/BaseRewardPool.sol#L282         for (uint256 i = 0; i < poolInfo.length; i++) { 
main/contracts/BaseRewardPool.sol#L176         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L199         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L218         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L244         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L282         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/VE3DRewardPool.sol#L148         for (uint256 i = 0; i < rewardTokens.length(); i++) {
main/contracts/VE3DRewardPool.sol#L214         for (uint256 i = 0; i < length; i++) {
main/contracts/VE3DRewardPool.sol#L238         for (uint256 i = 0; i < length; i++) {
main/contracts/VE3DRewardPool.sol#L257         for (uint256 i = 0; i < length; i++) {
main/contracts/VE3DRewardPool.sol#L281         for (uint256 i = 0; i < rewardTokens.length(); i++) {
main/contracts/VE3DRewardPool.sol#L326         for (uint256 i = 0; i < length; i++) {
main/contracts/VoterProxy.sol#L217             for (uint256 i = 0; i < _tokenVote.length; i++) {

2. Title : change uint256 i = 0 into uint256 i for saving more gas

using this implementation can saving more gas for each loops.

Tool Used

Manual Review

Recommended Mitigation

Change it

Occurances

main/contracts/Booster.sol#L329                for (uint256 i = 0; i < poolInfo.length; i++) {
main/contracts/BaseRewardPool.sol#L282         for (uint256 i = 0; i < poolInfo.length; i++) { 
main/contracts/BaseRewardPool.sol#L176         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L199         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L218         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L244         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L282         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/VE3DRewardPool.sol#L148         for (uint256 i = 0; i < rewardTokens.length(); i++) {
main/contracts/VE3DRewardPool.sol#L214         for (uint256 i = 0; i < length; i++) {
main/contracts/VE3DRewardPool.sol#L238         for (uint256 i = 0; i < length; i++) {
main/contracts/VE3DRewardPool.sol#L257         for (uint256 i = 0; i < length; i++) {
main/contracts/VE3DRewardPool.sol#L281         for (uint256 i = 0; i < rewardTokens.length(); i++) {
main/contracts/VE3DRewardPool.sol#L326         for (uint256 i = 0; i < length; i++) {
main/contracts/VoterProxy.sol#L217             for (uint256 i = 0; i < _tokenVote.length; i++) {

3. Title : Caching array length can saving more gas

This implementation can be saving more gas, since if caching the array length is more gas efficient. just because access to a local variable in solidity is more efficient.

Tool Used

Manual Review

Occurances

main/contracts/Booster.sol#L329                for (uint256 i = 0; i < poolInfo.length; i++) {
main/contracts/BaseRewardPool.sol#L282         for (uint256 i = 0; i < poolInfo.length; i++) { 
main/contracts/BaseRewardPool.sol#L176         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L199         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L218         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L244         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/BaseRewardPool.sol#L282         for (uint256 i = 0; i < extraRewards.length; i++) {
main/contracts/VE3DRewardPool.sol#L148         for (uint256 i = 0; i < rewardTokens.length(); i++) {
main/contracts/VE3DRewardPool.sol#L281         for (uint256 i = 0; i < rewardTokens.length(); i++) {
main/contracts/VoterProxy.sol#L217             for (uint256 i = 0; i < _tokenVote.length; i++) {

4. Title : Using > 0 costs more gas than != 0 when used on uints in a require() statement

1.) File : contracts/VeAssetDepositor.sol (Line.132)

        require(_amount > 0, "!>0");

2.) FIle : contracts/BaseRewardPool.sol (Line.215)

        require(amount > 0, "RewardPool : Cannot withdraw 0");

3.) File : contracts/BaseRewardPool.sol (Line.173)

        require(_amount > 0, "RewardPool : Cannot stake 0");

4.) File : contracts/BaseRewardPool.sol (Line.196)

        require(_amount > 0, "RewardPool : Cannot stake 0");

5.) contracts/BaseRewardPool.sol (Line.215)

        require(amount > 0, "RewardPool : Cannot withdraw 0");

6.) File : contracts/VE3DRewardPool.sol (Line.210)

        require(_amount > 0, "RewardPool : Cannot stake 0");

7.) File : contracts/VE3DRewardPool.sol (Line.234)

        require(_amount > 0, "RewardPool : Cannot stake 0");

8.) File : contracts/VE3DRewardPool.sol (Line.254)

        require(_amount > 0, "RewardPool : Cannot stake 0");

5. Title : Saving gas by removing = 0

This implementation code can be saving more gas by removing = 0, it because If a variable was not set/initialized, it is assumed to have default value to 0

Tool Used

Manual Review

Mitigation Step

Remove = 0

Occurances

main/contracts/BaseRewardPool.sol#L274             rewards[_account] = 0;
main/contracts/VeAssetDepositor.sol#L28             uint256 public incentiveVeAsset = 0;
main/contracts/VeAssetDepositor.sol#L119            incentiveVeAsset = 0;
main/contracts/VeAssetDepositor.sol#L141            incentiveVeAsset = 0;
main/contracts/BaseRewardPool.sol#L66               uint256 public periodFinish = 0;
main/contracts/BaseRewardPool.sol#L67               uint256 public rewardRate = 0;
main/contracts/BaseRewardPool.sol#L70                uint256 public queuedRewards = 0;
main/contracts/BaseRewardPool.sol#L71                uint256 public currentRewards = 0;
main/contracts/BaseRewardPool.sol#L72                uint256 public historicalRewards = 0;
main/contracts/VE3DRewardPool.sol#L361             rewardTokenInfo[_rewardToken].queuedRewards = 0;

6. Title : Value can be set as immutable

This can be set as immutable for saving more gas

Tool Used

Remix

Recommended Mitigation

add immutable

Occurances

1.) File : contracts/VeTokenMinter.sol (Line.16)

    ERC20 public veToken;

2.) File : contracts/VoterProxy.sol (Line.31)

    IVoteEscrow.EscrowModle public escrowModle;

3.) File : contracts/BaseRewardPool.sol (Lines.55-56)

    IERC20 public rewardToken;
    IERC20 public stakingToken;

4.) File : contracts/BaseRewardPool.sol (Lines.62-63)

    address public operator;
    address public rewardManager;

7. reorder function addExtraReward for saving more gas

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

this implementation below can be saving +- 200 gas

Tool Used

Manual Review, Remix

Recommended Mitigation

    function addExtraReward(address _reward) external returns (bool) {
        require(_reward != address(0), "!reward setting"); // => this can be excecuted first
        require(msg.sender == rewardManager, "!authorized");
        require(extraRewards.length < EXTRA_REWARD_POOLS, "!extra reward pools exceed");


        extraRewards.push(_reward);
        emit ExtraRewardAdded(_reward);
        return true; 
    }
// 3676423 before changed 
// 3676216 after changed (saving +- 200 gas)

8. Title : public functions should be declared external for saving more gas

1.) File : contracts/BaseRewardPool.sol (Line.196)

    function stakeFor(address _for, uint256 _amount) public updateReward(_for) returns (bool) {

2.) File : contracts/Booster.sol (Line.434)

    function withdrawAll(uint256 _pid) public returns (bool) {

3.) File : contracts/VeTokenMinter.sol (Line.32)

    function addOperator(address _newOperator) public onlyOwner {

4.) File : contracts/VeTokenMinter.sol (Line.36)

        function removeOperator(address _operator) public onlyOwner {

9. Title Caching in calldata instead of memory

1.) File : contracts/VoterProxy.sol (Line.199)

    function isValidSignature(bytes32 _hash, bytes memory) public view returns (bytes4) {