veToken Finance contest - ellahi's results

Low

[L-01] Missing checks for address(0) when assigning values to address state variables.

Impact

Setters of address type parameters should include a zero-address check otherwise contract functionality may become inaccessible or tokens burnt forever.

Proof of Concept

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L38-L51

constructor(
        address _staker,
        address _minter,
        address _veAsset,
        address _escrow,
        uint256 _maxTime
    ) {
        staker = _staker;
        minter = _minter;
        veAsset = _veAsset;
        escrow = _escrow;
        feeManager = msg.sender;
        maxTime = _maxTime;
    }

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L52-L57

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

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

constructor(
        address _staker,
        address _minter,
        address _veAsset,
        address _feeDistro
    ) {
        isShutdown = false;
        staker = _staker;
        owner = msg.sender;
        voteDelegate = msg.sender;
        feeManager = msg.sender;
        poolManager = msg.sender;
        minter = _minter;
        veAsset = _veAsset;
        feeDistro = _feeDistro;
    }

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

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

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

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

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

function withdrawTo(
        uint256 _pid,
        uint256 _amount,
        address _to
    ) external returns (bool) {
        address rewardContract = poolInfo[_pid].veAssetRewards;
        require(msg.sender == rewardContract, "!auth");
        // @audit require(_to != address(0));
        _withdraw(_pid, _amount, msg.sender, _to);
        return true;
    }

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L95-L107

 constructor(
        uint256 pid_,
        address stakingToken_,
        address rewardToken_,
        address operator_,
        address rewardManager_
    ) {
        pid = pid_;
        stakingToken = IERC20(stakingToken_);
        rewardToken = IERC20(rewardToken_);
        operator = operator_;
        rewardManager = rewardManager_;
    }

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

constructor(address stakingToken_, address rewardManager_) {
        stakingToken = IERC20(stakingToken_);

        rewardManager = rewardManager_;
    }

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

   function addReward(
        address _rewardToken,
        address _veAssetDeposits,
        address _ve3TokenRewards,
        address _ve3Token
    ) external onlyOwner {
        rewardTokenInfo[_rewardToken].veAssetDeposits = _veAssetDeposits;
        rewardTokenInfo[_rewardToken].ve3TokenRewards = _ve3TokenRewards;
        rewardTokenInfo[_rewardToken].ve3Token = _ve3Token;
        rewardTokens.add(_rewardToken);
    }

    function addOperator(address _newOperator) public onlyOwner {
        operators.add(_newOperator);
    }

Recommendation

Add address(0) checks.

[L-02] Use Two-Step Transfer Pattern for Access Controls

Impact

Contracts implementing access control's, e.g. owner, should consider implementing a Two-Step Transfer pattern. Otherwise it's possible that the role mistakenly transfers ownership to the wrong address, resulting in a loss of the role

Proof of Concept

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

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

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

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

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

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

Recommendation

Consider following the example below where the new role will have to accept.

Example

address owner;
address pendingOwner;

// ...

function setPendingOwner(address newPendingOwner) external {
    require(msg.sender == owner, "!owner");
    emit NewPendingOwner(newPendingOwner);
    pendingOwner = newPendingOwner;
}

function acceptOwnership() external {
    require(msg.sender == pendingOwner, "!pendingOwner");
    emit NewOwner(pendingOwner);
    owner = pendingOwner;
    pendingOwner = address(0);
}

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

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

[L-03] Require owner

Impact

Require owner to set priviledged addresses.

Proof of Concept

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

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

    function setPoolManager(address _poolM) external {
-       require(msg.sender == poolManager, "!auth");
+       require(msg.sender == owner, "!auth")
        poolManager = _poolM;
        emit PoolManagerUpdated(_poolM);
    }

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

    function setVoteDelegate(address _voteDelegate) external {
-       require(msg.sender == voteDelegate, "!auth");
+       require(msg.sender == owner, "!auth")
        voteDelegate = _voteDelegate;
        emit VoteDelegateUpdated(_voteDelegate);
    }

Recommendation

Use require (msg.sender == owner, "!auth") for all the functions that need authorization when assigning a new address to a role.

[L-04] Do not use Deprecated Library Functions

Impact

The usage of deprecated library functions should be discouraged.

Proof of Concept

  Booster.sol::374 => IERC20(token).safeApprove(rewardContract, _amount);
  VE3DRewardPool.sol::287 => IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0);
  VE3DRewardPool.sol::288 => IERC20(_rewardToken).safeApprove(
  VE3DRewardPool.sol::301 => IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
  VE3DRewardPool.sol::305 => IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
  VeAssetDepositor.sol::162 => IERC20(minter).safeApprove(_stakeAddress, _amount);
  VoterProxy.sol::101 => IERC20(_token).safeApprove(_gauge, 0);
  VoterProxy.sol::102 => IERC20(_token).safeApprove(_gauge, balance);
  VoterProxy.sol::152 => IERC20(veAsset).safeApprove(escrow, 0);
  VoterProxy.sol::153 => IERC20(veAsset).safeApprove(escrow, _value);
  VoterProxy.sol::160 => IERC20(veAsset).safeApprove(escrow, 0);
  VoterProxy.sol::161 => IERC20(veAsset).safeApprove(escrow, _value);

Recommendation

Use safeIncreaseAllowance / safeDecreaseAllowance instead of safeApprove.

Non-Critical

[N-01] Typo

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

-    //increase ammount
+    //increase amount

Tools used

manual, slither

Gas

[G-01] No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

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

    uint256 public periodFinish = 0;
    uint256 public rewardRate = 0;
    uint256 public queuedRewards = 0;
    uint256 public currentRewards = 0;
    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::110 => isShutdown = false;
  Booster.sol::329 => for (uint256 i = 0; i < poolInfo.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++) {
  VoterProxy.sol::227 => uint256 _balance = 0;

Recommendation

Remove explicit default initializations.

[G-02] Use != 0 instead of > 0 for Unsigned Integer Comparison in require statements.

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Proof of Concept

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

Recommendation

Use != 0 instead of > 0.

[G-03] Cache Array Length Outside of Loop.

Impact

Reading 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.

Proof of Concept

  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++) {
  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++) {

Recommendation

Store the array’s length in a variable before the for-loop.

[G-04] ++i costs less gas compared to i++ or i += 1

Impact

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

Proof of Concept

  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++) {
  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++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

[G-05] SafeMath is unnecessary in solidity versions >0.8.0.

Impact

Arithmetic operations revert on underflow and overflow in solidity versions >0.8.0. Using SafeMath is redundant and a waste of gas.

Proof of Concept

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

pragma solidity 0.8.7;

import "@openzeppelin/contracts/utils/math/SafeMath.sol";

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L2-L4

pragma solidity 0.8.7;

import "@openzeppelin/contracts/utils/math/SafeMath.sol";

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

import "@openzeppelin/contracts/utils/math/SafeMath.sol";

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

import "@openzeppelin/contracts/utils/math/SafeMath.sol";

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

pragma solidity 0.8.7;

import "@openzeppelin/contracts/utils/math/SafeMath.sol";

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

import "@openzeppelin/contracts/utils/math/SafeMath.sol";

Recommendation

Get rid of SafeMath.sol.

Tools used

c4udit, manual, slither