QA Report

[code423n4]

QA (LOW & NON-CRITICAL)

• The whole project have different solidity compiler ranges (0.8.7, >=0.5.0 , >=0.4.0) referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.

• safeTransfer , safeTransferFrom methods are used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable, it may likely to get broken when calling a contract's fallback function.
  Reference Link -1, Reference Link -2

• block.timestamp is used on many places at the scoped contracts. However, this can be manipulated by malicious miners like the options can be shown as expired before the end of the auction.
  Reference

• Some require statements in VE3DLocker.sol don't throw error. In case of any error pops up, it will not be possible to know the error source.
  https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L165-L167

• The project uses deprecated safeApprove in ChainlinkOracleClient.sol. Link

• Critical address changes are not done in two steps for the followings:
  In VeAssetDepositor.sol for setFeeManager(),
  In Booster.sol for setOwner(), setFeeManager(), setPoolManager(), setArbitrator() , setVoteDelegate()
  Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible. Reference, Reference

• Typo at Vetoken.sol#L25, require statement's error message (require(totalSupply().add(_amount) < maxSupply, "Exceeed max supply!");)

• The scoped contracts are having lack of NatSpec comments such as @notice @dev @param on many places.It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI) Reference

• There is no address(0) check at VeAssetDepositor.sol's constructor(). In case these adresses are missed at the deployer scripts, it will likely to cost extra gas expenses to re-deploy for the veToken Team.

• In VeAssetDepositor.sol's setFees() function, the logic is missed-out resulting in a tatutology/contradiction.

function setFees(uint256 _lockIncentive) external {
        require(msg.sender == feeManager, "!auth");

        if (_lockIncentive >= 0 && _lockIncentive <= 30) { <---- @audit-info
            lockIncentive = _lockIncentive;
            emit FeesUpdated(_lockIncentive);
        }
    }

Since the param uint256 _lockIncentive is always >= 0 as cached to uint256, the team can consider to remove the _lockIncentive >= 0 part of the code.

• In VeAssetDepositor.sol's deposit() function, IERC20(veAsset).approve is not called before calling IERC20(veAsset).safeTransferFrom(msg.sender, staker, _amount); where the transfer will fail. Reference

• VeAssetDepositor.sol's lockVeAsset() function is for incentivizing actors to lock the veAssets to the staker. However if the if the incentiveVeAsset = 0 condition is present, the call will not serves as it's designed to be. Hencei there should be a require statement and the code can be amended as below;

    function lockVeAsset() external {
        require(incentiveVeAsset > 0, "err");
        _lockVeAsset(); 

        //mint incentives
        ITokenMinter(minter).mint(msg.sender, incentiveVeAsset);
        incentiveVeAsset = 0;
        }
    }

• There is no address(0) check at BaseRewardPool.sol's constructor(). In case these adresses are missed at the deployer scripts, it will likely to cost extra gas expenses to re-deploy for the veToken Team.

• In BaseRewardPool.sol's updateReward() modifier, the logic is wrong. If any address(0) passed in the modifier,a wrong event will be emitted which will submit wrong information on chain and likely to break the flow of the off-chain watchers.

• In BaseRewardPool.sol's stake() function, the balance of the user is not checked against to the amount of the token intended to be staked, a require statement needed for checks-effects-interactions as below;

function stake(uint256 _amount) public updateReward(msg.sender) returns (bool) {
        require(_amount > 0, "RewardPool : Cannot stake 0");
        require(IERC20(stakingToken).balanceOf(msg.sender) >= _amount, "err"); // <------ @audit-info Recommended require statement
        //also stake to linked rewards
        for (uint256 i = 0; i < extraRewards.length; i++) {
            IRewards(extraRewards[i]).stake(msg.sender, _amount);
        }

Same applies with VE3DRewardPool.sol's stake() function.

• In BaseRewardPool.sol's stake() function, the IERC20(stakingToken).approve is not called before calling stakingToken.safeTransferFrom(msg.sender, address(this), _amount); where the transfer will fail. Reference
  Same applies with VE3DRewardPool.sol's stake() function.

• In BaseRewardPool.sol's stakeFor() function there is no address(0) check for the address _for param. Any actor can send 1 wei for address(0) and burn the extraRewards.
  Same applies with VE3DRewardPool.sol's stakeFor() function.

• In BaseRewardPool.sol's withdraw() function, the balance of the user is not checked against to the amount of the token intended to be withdrawn, a require statement needed for checks-effects-interactions as below;

function withdraw(uint256 amount, bool claim) public updateReward(msg.sender) returns (bool) {
        require(amount > 0, "RewardPool : Cannot withdraw 0");
        require(IERC20(stakedToken).balanceOf(msg.sender) >= _amount, "err");// <------ @audit-info Recommended require statement
        //also withdraw from linked rewards
        for (uint256 i = 0; i < extraRewards.length; i++) {
            IRewards(extraRewards[i]).withdraw(msg.sender, amount);
        }

Same applies with VE3DRewardPool.sol's withdraw() function.

• The codebase uses SafeMath library which is generally not needed starting with Solidity 0.8, since the compiler now has built in overflow checking.

• There is no address(0) check at Booster.sol's constructor(). In case these adresses are missed at the deployer scripts, it will likely to cost extra gas expenses to re-deploy for the veToken Team.

• In Booster.sol's setTreasury() function, there should be and address(0) check for not to accidentally burn the funds send to the treasury.

• In Booster.sol's deposit() function, the balance of the user is not checked against to the amount of the token intended to be deposited, a require statement needed for checks-effects-interactions as require(IERC20(depositingToken).balanceOf(msg.sender) >= _amount, "err")

• In Booster.sol's deposit() function, IERC20(lptoken).approve is not called before calling IERC20(lptoken).safeTransferFrom(msg.sender, staker, _amount); where the transfer will fail. Reference

• In Booster.sol's _withdraw() function, the balance of the user is not checked against to the amount of the token intended to be withdrawn, a require statement needed for checks-effects-interactions as require(IERC20(token).balanceOf(msg.sender) <= _amount, "err")

• There is no address(0) check at VE3DRewardPool.sol's constructor(). In case these adresses are missed at the deployer scripts, it will likely to cost extra gas expenses to re-deploy for the veToken Team.

[GalloDaSballo]

floating pragma

Not valid as the hardhat config resolves to a specific solidity version

## safeTransfer , safeTransferFrom methods are used inside the codebase
This is plain false, check your sources

block.timestamp is used on many places at the scoped contracts. However, this can be manipulated by malicious miners like the options can be shown as expired before the end of the auction.

False blanket statement, miners can delay or make a block happen faster, however each block_time(x) < block_time(x+1)

Some require statements in VE3DLocker.sol don't throw error. In case of any error pops up, it will not be possible to know the error source.

NC

The project uses deprecated safeApprove in ChainlinkOracleClient.sol. Link

Code is not from contest

Critical address changes are not done in two steps for the followings:

NC

4 findings at this point are invalid, there's more invalid as well.

Because I think this submission is of low quality, and simply doesn't stand in comparison to other well thought out submissions, I'm marking it invalid.

Please:

• Check your findings
• Add explanations about them