QA Report

[code423n4]

No two-step process for critical ownership transfer

Impact

The current admin transfer process involves the current admin calling TransparentUpgradeableProxy.changeAdmin(). This function checks the new admin address is not the zero address and proceeds to update the admin slot. If the nominated EOA account is not a valid account, it is entirely possible the admin might be accidentally transferred to an uncontrolled account, breaking all functions with the ifAdmin() modifier.

Proof-of-Concept

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

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

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

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

Recommended Mitigation Steps

Consider implementing a two step process where the admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of admin to fully succeed. This ensures the nominated EOA account is a valid and active account.

Reference

code-423n4/2021-08-notional-findings#94

SafeApprove Has Been Deprecated

Proof-of-Concept

OpenZeppelin SafeERC20 safeApprove() function was found to be used in a number of contracts. This function has been deprecated. Refer to the comments in the SafeERC20's source code.

An example of the usage of safeApprove() is shown below:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/828fe365eeff13e7aa188e449005ad81f7222189/contracts/token/ERC20/utils/SafeERC20.sol#L39-L44

/**
 * @dev Deprecated. This function has issues similar to the ones found in
 * {IERC20-approve}, and its usage is discouraged.
 *
 * Whenever possible, use {safeIncreaseAllowance} and
 * {safeDecreaseAllowance} instead.
 */
function safeApprove(
    IERC20 token,
    address spender,
    uint256 value
) internal {
    // safeApprove should only be called when setting an initial allowance,
    // or when resetting it to zero. To increase and decrease it, use
    // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
    require(
        (value == 0) || (token.allowance(address(this), spender) == 0),
        "SafeERC20: approve from non-zero to non-zero allowance"
    );
    _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}

Recommended Mitigation Steps

Use safeIncreaseAllowance() instead of safeApprove()

Reference

OpenZeppelin/openzeppelin-contracts#2219

Hardcoded BLOCKS_PER_YEAR

Proof-of-Concept

The number of block per day is hardcoded in the BaseRewardPool contract, and it is used for the calculation of APY. However, number of block per day is not fixed at the same rate every year. For instance, the block per day is around 5000 in 2016, while the block per day is around 6000 in 2022. Thus, the rate might change in the future.

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

uint256 constant BLOCKS_PER_DAY = 6450;
uint256 constant BLOCKS_PER_YEAR = BLOCKS_PER_DAY * 365;
uint256 constant EXTRA_REWARD_POOLS = 3;

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

Recommended Mitigation Steps

Consider having a setter method for BLOCKS_PER_DAY and make it non-constant.

Did not safeApprove(0) First

Impact

Some tokens (e.g. USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and followed by the actual allowance to be approved.

Proof-of-Concept

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

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

[GalloDaSballo]

No two-step process for critical ownership transfer

NC

SafeApprove Has Been Deprecated

NC

## Hardcoded BLOCKS_PER_YEAR
Valid Low, the merge will standardize to 12s block times and the change can be dramatic

Did not safeApprove(0) First

Disagree as it sets the allowance to 0

Short and sweet
1L, 2NC