QA Report

[code423n4]

• No need to use SafeMath with Solidity version >0.8:

  pragma solidity 0.8.7;
  using SafeMath for uint256;

  Now it basically performs safe math operations twice, thus wasting gas.

• contract VoterProxy does not explicitly implement IStaker interface. It causes some confusion, e.g. the declaration of interface and implementation contracts differ:

  function createLock(uint256, uint256) external;
  function createLock(uint256 _value, uint256 _unlockTime) external returns (bool)

  The interface does not declare the return value, while the implementation returns a boolean indicating success. Better always explicitly implement an interface to force compile-time enforcements.

• setOwner could be a 2-step (propose-accept) process to avoid accidental errors.

• In VoterProxy when an operator deposits a new token, it is added to the list of protectedTokens. This token is never set back to false again. Consider setting protectedTokens to false in operator withdrawal functions (withdraw, withdrawAll) when the balance after becomes 0. Then tokens might be reclaimed if the operator has withdrawn all of them, and later someone accidentally sends them directly to the contract or something like that.

• You can make event parameters 'indexed' to allow for filtering, e.g. hash in VoteSet:

  event VoteSet(bytes32 hash, bool valid);

• function voteGaugeWeight could validate that the lengths of _tokenVote and _weight are equal.

• You can use built-in time keywords, e.g. here:

  uint256 private constant WEEK = 7 * 86400;
  uint256 private constant WEEK = 7 weeks;

• safeApprove is deprecated, see: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L39-L44

• The protocol does not support fee on transfer and other weird tokens, e.g.:

  IERC20(_rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
  rewardTokenInfo[_rewardToken].queuedRewards += _amount;

  or

  //add supply
  _totalSupply = _totalSupply.add(_amount);
  //add to _for's balance sheet
  _balances[_for] = _balances[_for].add(_amount);
  //take tokens from sender
  stakingToken.safeTransferFrom(msg.sender, address(this), _amount);

  Make sure that your users are informed about that, or consider validating the balance before/after to know the real amount transferred.

• totalWeight might be 0, because updateveAssetWeight does not enforce minimum total weight, thus it is possible that rewardClaimed function will revert in runtime here making the users not being able to get their rewards:

  //calc the amount of veAssetEarned
  uint256 _veAssetEarned = _amount.mul(veTokenMinter.veAssetWeights(address(this))).div(
      veTokenMinter.totalWeight()
  );

• Functions are not protected from re-entrancy. Some of them do not follow the Check-Effects-Interactions pattern, thus can be exploited if the call target contains the transfer hook. For example, here it first transfers the token and only then updates the supply:

  veToken.safeTransfer(_to, _amount);
  totalSupply += _amount;

  There are many more functions that do the external interactions and are not protected. Consider adding ReentrancyGuard to all the main user interacting functions.

[GalloDaSballo]

No need to use SafeMath with Solidity version >0.8:

Valid Refactor

contract VoterProxy does not explicitly implement IStaker interface. It causes some confusion, e.g. the declaration of interface and implementation contracts differ:

Valid Refactor

 setOwner could be a 2-step (propose-accept) process to avoid accidental errors.

NC

In VoterProxy when an operator deposits a new token, it is added to the list of protectedTokens.

I must disagree in lack of further details over this refactoring, removing protectedTokens is a rug vector

You can make event parameters 'indexed' to allow for filtering, e.g. hash in VoteSet:

For this event, valid NC

function voteGaugeWeight could validate that the lengths of _tokenVote and _weight are equal.

Valid R (reverts on failure)

You can use built-in time keywords, e.g. here:

Valid R

safeApprove is deprecated, see: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L39-L44

Valid NC for this codebase

The protocol does not support fee on transfer and other weird tokens, e.g.:

Valid Low (may bump based on other findings)

Functions are not protected from re-entrancy. Some of them do not follow the Check-Effects-Interactions pattern, thus can be exploited if the call target contains the transfer hook. For example, here it first transfers the token and only then updates the supply:

Valid Low

totalWeight might be 0, because updateveAssetWeight does not enforce minimum total weight, thus it is possible that rewardClaimed function will revert in runtime here making the users not being able to get their rewards:

Valid Low (may bump based on other findings)

Good report, short and sweet, would benefit by having better formatted headlines (the dotted list looks odd as you can see above)

[GalloDaSballo]

3L 4R 3NC

[GalloDaSballo]

TODO - Raise:

The protocol does not support fee on transfer and other weird tokens, e.g.: - > M-25

New Score: 2L 4R 3NC