veToken Finance contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

The fee manager of the Booster contract can set any high fee values for both lockFeesIncentive and stakerLockFeesIncentive via the Booster.setFeeInfo function.

Both fee values should sum up to a maximum of 100% (FEE_DENOMINATOR = 10000). If set to higher values, it can DoS the Booster.earmarkFees function if there are insufficient fee tokens in the contract.

Proof of Concept

Booster.setFeeInfo(uint256 _lockFeesIncentive, uint256 _stakerLockFeesIncentive)

function setFeeInfo(uint256 _lockFeesIncentive, uint256 _stakerLockFeesIncentive) external {
    require(msg.sender == feeManager, "!auth");

    lockFeesIncentive = _lockFeesIncentive; //  @audit-info missing upper bound validation
    stakerLockFeesIncentive = _stakerLockFeesIncentive; //  @audit-info missing upper bound validation

    address _feeToken = IFeeDistro(feeDistro).token();
    if (feeToken != _feeToken) {
        //create a new reward contract for the new token
        lockFees = IRewardFactory(rewardFactory).CreateTokenRewards(_feeToken, lockRewards);

        if (_feeToken != veAsset) {
            IRewards(stakerLockRewards).addReward(
                _feeToken,
                address(0),
                address(0),
                address(0),
                address(this),
                false
            );
        }

        feeToken = _feeToken;
    }
}

Tools Used

Manual review

Recommended mitigation steps

Consider adding a reasonable upper bounds for both lockFeesIncentive and stakerLockFeesIncentive.

QA Report

Table of Contents

• Low Risk
  • [L-01] Zero-address checks are missing
  • [L-02] Single-step process for critical ownership/governance transfer is risky
  • [L-03] Contracts inheriting Ownable have renounceOwnership() functionality
  • [L-04] Events not emitted for important state changes
  • [L-05] Missing input validation on array length
  • [L-06] Event RewardContractsUpdated always emits even if reward contracts have not changed
  • [L-07] Anyone can claim (and stake) rewards on behalf of someone else

Low Risk

[L-01] Zero-address checks are missing

Description

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters.

Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens or force redeployment of contracts.

Findings

BaseRewardPool.sol

L103: stakingToken = IERC20(stakingToken_);
L104: rewardToken = IERC20(rewardToken_);
L105: operator = operator_;
L106: rewardManager = rewardManager_;

Booster.sol

L111: staker = _staker;
L116: minter = _minter;
L117: veAsset = _veAsset;
L118: feeDistro = _feeDistro;
L131: feeManager = _feeM;
L137: poolManager = _poolM;
L164: rewardArbitrator = _arb;
L170: voteDelegate = _voteDelegate;

VE3DRewardPool.sol

L97: stakingToken = IERC20(stakingToken_);
L99: rewardManager = rewardManager_;

VeAssetDepositor.sol

L45: staker = _staker;
L46: minter = _minter;
L47: veAsset = _veAsset;
L48: escrow = _escrow;
L55: feeManager = _feeManager;

VeTokenMinter.sol

L27: veToken = ERC20(veTokenAddress);

VoterProxy.sol

L50: veAsset = _veAsset;
L51: escrow = _escrow;
L52: gaugeProxy = _gaugeProxy;
L54: minter = _minter;
L64: owner = _owner;

Recommended mitigation steps

Add zero-address checks, e.g.:

require(_asset != address(0), "Zero-address");

[L-02] Single-step process for critical ownership/governance transfer is risky

Description

The owner plays a critical role in the Booster and VoterProxy contracts.

Given that changing owner uses a single-step process for ownership transfer, it is very risky to perform those changes in a single step because it is irrecoverable from any mistakes.

Findings

Booster.sol#L123-L127

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

VoterProxy.sol#L62-L65

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

Recommended mitigation steps

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

[L-03] Contracts inheriting Ownable have renounceOwnership() functionality

Renouncing ownership will leave the contract without an owner. This functionality is desirable in certain scenarios and is typically allowed by libraries such as Ownable.

Findings

Following contracts inherit from Ownable, thus having the functionality to renounce ownership:

VE3DRewardPool.sol
VeTokenMinter.sol

Recommended mitigation steps

Consider overriding renounceOwnership() function in contracts to prevent renouncing ownership accidentally.

[L-04] Events not emitted for important state changes

Description

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

VE3DRewardPool.sol

L102: function addReward(
L114: function addOperator(address _newOperator)
L118: function removeOperator(address _operator)
L134: function addExtraReward(address _reward){
L141: function clearExtraRewards()

VeTokenMinter.sol

L32: function addOperator(address _newOperator)
L36: function removeOperator(address _operator)
L41: function updateveAssetWeight(address veAssetOperator, uint256 newWeight)

VoterProxy.sol

L62: function setOwner(address _owner)
L67: function setOperator(address _operator)
L77: function setDepositor(address _depositor)
L83: function setStashAccess(address _stash, bool _status)

Recommended mitigation steps

Emit events for state variable changes.

[L-05] Missing input validation on array length

Description

The VoterProxy.voteGaugeWeight function accepts two arrays _tokenVote and _weight and fails to perform input validation to check if their lengths match. A mismatch could lead to an exception in the best case or undefined behavior in the worst case.

Findings

VoterProxy.sol#L207

Recommended mitigation steps

Add input validation to check if the length of both arrays is equal.

[L-06] Event RewardContractsUpdated always emits even if reward contracts have not changed

Description

The event RewardContractsUpdated in the Booster.setRewardContracts function always emits, even if the reward contract address have not changed.

The event should only be emitted if contract addresses have actually changed.

Findings

Booster.sol#L189

Recommended mitigation steps

Only emit the event RewardContractsUpdated if reward contracts have actually changed.

[L-07] Anyone can claim (and stake) rewards on behalf of someone else

Description

The VE3DRewardPool.getReward function allows claiming the rewards of other users.

While the tokens are sent to the correct address _account, this can lead to issues with smart contracts that might rely on claiming the rewards themselves.

As one example, suppose the _account address corresponds to a smart contract that has a function of the following form:

function claimAndDoSomething(epochs) {
  uint256 claimedAmount = token.balanceOf(address(this));

  vE3DRewardPool.getReward(address(this), true, false);

  claimedAmount = token.balanceOf(address(this)) - claimedAmount;

  token.transfer(externalWallet, claimedAmount);
}

If the contract has no other functions to transfer out funds, they may be locked forever in this contract.

Claiming can also incur a taxable event and the timing is better left to the actual owner.

Findings

VE3DRewardPool.sol#L279

Recommended mitigation steps

Do not allow users to claim on behalf of other addresses.