veToken Finance contest - horsefacts's results

Low

Missing approval reset

Booster#deposit calls safeApprove without first ensuring that the approval balance is zero. However, safeApprove will revert if the approval amount is not zero. If the reward contract has an existing approval amount, this may cause deposits to revert.

Booster#deposit

        if (_stake) {
            //mint here and send to rewards on user behalf
            ITokenMinter(token).mint(address(this), _amount);
            address rewardContract = pool.veAssetRewards;
            IERC20(token).safeApprove(rewardContract, _amount);
            IRewards(rewardContract).stakeFor(msg.sender, _amount);
        } else {
            //add user balance directly
            ITokenMinter(token).mint(msg.sender, _amount);
        }

Recommendation: set approval to zero before setting the new amount:

        if (_stake) {
            //mint here and send to rewards on user behalf
            ITokenMinter(token).mint(address(this), _amount);
            address rewardContract = pool.veAssetRewards;
            IERC20(token).safeApprove(rewardContract, 0);
            IERC20(token).safeApprove(rewardContract, _amount);
            IRewards(rewardContract).stakeFor(msg.sender, _amount);
        } else {
            //add user balance directly
            ITokenMinter(token).mint(msg.sender, _amount);
        }

Noncritical

Prefer two step authorization changes

Several contracts set a privileged owner/operator address in a single step. If this operator address is set to zero or an incorrect value, ownership of these contracts may be permanently lost.

Suggestion: handle ownership changes with two steps and two transactions. First, allow the current owner/operator to propose a new owner address. Second, allow the proposed address (and only the proposed address) to accept ownership, and update the contract owner internally.

• VeToken#setOperator
• Ve3Token#setOperator
• VoterProxy#setOwner
• VoterProxy#setoperator
• VoterProxy#setDepositor

Emit events from permissioned functions

Consider adding events to protected functions that change contract state, especially when updating privileged user addresses. This enables you to monitor these events off chain for suspicious activity, and in the case of protocol parameter changes, allows end users to observe and trust changes made to these parameters.

• VoterProxy#setOwner
• VoterProxy#setoperator
• VoterProxy#setDepositor
• VeTokenMinter#addOperator
• VeTokenMinter#removeOperator
• VeTokenMinter#updateveAssetWeight
• VE3DRewardPool#addOperator
• VE3DRewardPool#removeOperator
• VE3DRewardPool#addOperator
• TokenFactory#addOperator
• TokenFactory#removeOperator
• `StashFactory#removeOperator
• `StashFactory#addOperator
• RewardFactory#removeActiveReward
• RewardFactory#addActiveReward
• RewardFactory#removeOperator
• RewardFactory#removeOperator
• RewardFactory#addOperator
• VeToken#setOperator
• Ve3Token#setOperator

QA

Calculate APY off chain

BaseRewardPool uses a rough estimate of BLOCKS_PER_YEAR to calculate APY. However, blocktimes are currently variable and using this fixed value may over- or under-estimate the APY. Consider calculating this value offchain using rewardRate and totalSupply rather than using a hardcoded blocks per day estimate.

BaseRewardPool#getAPY

    function getAPY() external view returns (uint256) {
        return rewardRate.mul(BLOCKS_PER_YEAR).mul(1e18).div(totalSupply());
    }
}

Omit SafeMath library

Solidity versions >= 0.8.x perform checked arithmetic by default, so the SafeMath library is unnecessary in most cases. (However, it may be convenient to include it in some cases to maintain compatibility with forked contracts, like Synthetix BaseRewardPool).

Usages of SafeMath:

contracts/VeAssetDepositor.sol
16:    using SafeMath for uint256;

contracts/DepositToken.sol
13:    using SafeMath for uint256;

contracts/BaseRewardPool.sol
52:    using SafeMath for uint256;

contracts/VirtualBalanceRewardPool.sol
51:    using SafeMath for uint256;
66:    using SafeMath for uint256;

contracts/VE3DRewardPool.sol
55:    using SafeMath for uint256;

contracts/ExtraRewardStashV2.sol
18:    using SafeMath for uint256;

contracts/ExtraRewardStashV1.sol
18:    using SafeMath for uint256;

contracts/VeTokenMinter.sol
12:    using SafeMath for uint256;

contracts/ExtraRewardStashV3.sol
18:    using SafeMath for uint256;

contracts/VoterProxy.sol
19:    using SafeMath for uint256;

contracts/PoolManager.sol
17:    using SafeMath for uint256;

contracts/Booster.sol
21:    using SafeMath for uint256;

contracts/token/VeToken.sol
8:    using SafeMath for uint256;

contracts/token/VE3Token.sol
13:    using SafeMath for uint256;

Omit unused libraries

There are several places throughout the codebase where the SafeERC20, Address, and SafeMath libraries are imported and attached, but unused.

For example, see Ve3Token.sol:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.7;

import "@openzeppelin/contracts/utils/math/SafeMath.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/utils/Address.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract VE3Token is ERC20 {
    using SafeERC20 for IERC20;
    using Address for address;
    using SafeMath for uint256;

    address public operator;

    constructor(string memory name, string memory symbol) ERC20(name, symbol) {
        operator = msg.sender;
    }

    function setOperator(address _operator) external {
        require(msg.sender == operator, "!auth");
        operator = _operator;
    }

    function mint(address _to, uint256 _amount) external {
        require(msg.sender == operator, "!authorized");

        _mint(_to, _amount);
    }

    function burn(address _from, uint256 _amount) external {
        require(msg.sender == operator, "!authorized");

        _burn(_from, _amount);
    }
}

Since none of this contract's functions make use of SafeERC20, Address, or SafeMath library functions, they may all be safely omitted.

Log previous values in events

Consider logging the previous value in events that log parameter state changes. This makes it easier to identify the impact of these changes when monitoring off-chain.

Booster#setOwner

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

Unused imports

• EnumerableSet.sol in PoolManager

Incorrect comment

A comment in Booster#setFees suggests that fee values must be limited to certain ranges, but the range validation from the upstream Convex booster contract has been removed in the veToken booster.

Typos

• platform in Booster
• Exceed in VeToken#mint

Gas

The operator state variable in DepositToken is set in the constructor and cannot change. It can be declared immutable:

DepositToken

    address public operator;

    constructor(address _operator, address _lptoken)
        ERC20(
            string(abi.encodePacked(ERC20(_lptoken).name(), " Vetoken Deposit")),
            string(abi.encodePacked("VE3", ERC20(_lptoken).symbol()))
        )
    {
        operator = _operator;
    }