veToken Finance contest - pauliax's results

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

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;

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

• uint is always >= 0, can't be negative:

  function setFees(uint256 _lockIncentive)
    ...
    if (_lockIncentive >= 0 && _lockIncentive <= 30)

• No need for safe math where overflow/underflow is impossible, e.g.:

  if (_balance < _amount) {
  _amount = _withdrawSome(_gauge, _amount.sub(_balance));

• When you declared the named return value, then you do not need to return afterward explicitly:

  function withdraw(IERC20 _asset) external returns (uint256 balance) {
   ...
   return balance;

• No need to initialize to default values, e.g.:

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

• Repeated storage access should be cached, e.g. veAsset is accessed 3 times here:

        uint256 veAssetBalance = IERC20(veAsset).balanceOf(address(this));
        if (veAssetBalance > 0) {
            IERC20(veAsset).safeTransfer(staker, veAssetBalance);
        }
        //increase ammount
        uint256 veAssetBalanceStaker = IERC20(veAsset).balanceOf(staker);