veToken Finance contest - kirk-baird's results

Lines of code

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

Vulnerability details

Impact

It is possible to call addPool() for the same gauge multiple times. That is because there are no restrictions in Booster.addPool().

The impact is the shutting down one pool will cause all of the funds to be withdrawn from the other gauge and withdrawn to the Booster contract. Therefore not earning any rewards.

This issue is rated a medium as although there is a check in PoolManager.sol to ensure that a gauge is not already in use there is no gaurantee the poolManager is a PoolManager contract. Especially since the constructor will assign poolManager = msg.sender which cannot be a PoolManager contract.

Proof of Concept

Booster.addPool() does not have any restrictions on if _gauge is already in use.

    function addPool(
        address _lptoken,
        address _gauge,
        uint256 _stashVersion
    ) external returns (bool) {
        require(msg.sender == poolManager && !isShutdown, "!add");
        require(_gauge != address(0) && _lptoken != address(0), "!param");


        //the next pool's pid
        uint256 pid = poolInfo.length;


        //create a tokenized deposit
        address token = ITokenFactory(tokenFactory).CreateDepositToken(_lptoken);
        //create a reward contract for veAsset rewards
        address newRewardPool = IRewardFactory(rewardFactory).CreateVeAssetRewards(pid, token);


        //create a stash to handle extra incentives
        address stash = IStashFactory(stashFactory).CreateStash(
            pid,
            veAsset,
            _gauge,
            staker,
            _stashVersion
        );


        //add the new pool
        poolInfo.push(
            PoolInfo({
                lptoken: _lptoken,
                token: token,
                gauge: _gauge,
                veAssetRewards: newRewardPool,
                stash: stash,
                shutdown: false
            })
        );
        gaugeMap[_gauge] = true;


        //give stashes access to rewardfactory and voteproxy
        //   voteproxy so it can grab the incentive tokens off the contract after claiming rewards
        //   reward factory so that stashes can make new extra reward contracts if a new incentive is added to the gauge
        if (stash != address(0)) {
            poolInfo[pid].stash = stash;
            IStaker(staker).setStashAccess(stash, true);
            IRewardFactory(rewardFactory).setAccess(stash, true);
        }
        emit PoolAdded(_lptoken, _gauge, token, newRewardPool);


        return true;
    }

Booster.shutdownPool() will then withdraw all funds from the gauge even if other pools have deposited in this gauge.

    function shutdownPool(uint256 _pid) external returns (bool) {
        require(msg.sender == poolManager, "!auth");
        PoolInfo storage pool = poolInfo[_pid];

        //withdraw from gauge
        try IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) {} catch {}

        pool.shutdown = true;
        gaugeMap[pool.gauge] = false;

        emit PoolShuttedDown(_pid);
        return true;
    }

Recommended Mitigation Steps

It is recommended to add the check to the start of Booster.addPool().

require(!gaugeMap[guage], "Gauge already in use");

Consider also adding the remaining zero checks in PoolManager.addPool() to Booster.addPool().

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L102-L112 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L145-L172

Vulnerability details

Impact

The function addReward() allows the owner to add a new reward token to the list rewardTokens.

However, this is an unbounded list that when appended to cannot be shortened. The impact is it is possible to reach a state where the list is so long it cannot be iterated through due to the gas cost being larger than the block gas limit. This would cause a state where all transactions which iterate over this list will revert.

Since the modifier updateReward() iterates over this list it is possible that there will reach a state where the we are unable to call any functions with this modifier. The list includes

• stake()
• stakeAll()
• stakeFor()
• withdraw()
• withdrawAll()
• getReward()
• notifyRewardAmount()

As a result it would therefore be impossible to withdraw any rewards from this contract.

The same issue exists in VE3DLocker. Where rewards can be added by either Booster or the owner.

Proof of Concept

    function addReward(
        address _rewardToken,
        address _veAssetDeposits,
        address _ve3TokenRewards,
        address _ve3Token
    ) external onlyOwner {
        rewardTokenInfo[_rewardToken].veAssetDeposits = _veAssetDeposits;
        rewardTokenInfo[_rewardToken].ve3TokenRewards = _ve3TokenRewards;
        rewardTokenInfo[_rewardToken].ve3Token = _ve3Token;
        rewardTokens.add(_rewardToken);
    }

    function addReward(
        address _rewardsToken,
        address _veAssetDeposits,
        address _ve3Token,
        address _ve3TokenStaking,
        address _distributor,
        bool _isVeAsset
    ) external {
        require(_msgSender() == owner() || operators.contains(_msgSender()), "!Auth");
        require(rewardData[_rewardsToken].lastUpdateTime == 0);
        require(_rewardsToken != address(stakingToken));
        rewardTokens.push(_rewardsToken);


        rewardData[_rewardsToken].lastUpdateTime = uint40(block.timestamp);
        rewardData[_rewardsToken].periodFinish = uint40(block.timestamp);
        rewardDistributors[_rewardsToken][_distributor] = true;


        rewardData[_rewardsToken].isVeAsset = _isVeAsset;
        // if reward is veAsset
        if (_isVeAsset) {
            require(_ve3Token != address(0));
            require(_ve3TokenStaking != address(0));
            require(_veAssetDeposits != address(0));
            rewardData[_rewardsToken].ve3Token = _ve3Token;
            rewardData[_rewardsToken].ve3TokenStaking = _ve3TokenStaking;
            rewardData[_rewardsToken].veAssetDeposits = _veAssetDeposits;
        }
    }

Recommended Mitigation Steps

Consider having some method for removing old reward tokens which are no longer in use.

Alternatively set a hard limit on the number of reward tokens that can be added.

A different option is too allow rewards to be iterated and distributed on a per token bases rather than all tokens at once.

Lines of code

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

Vulnerability details

Impact

The function setFeeInfo() does not place restrictions on the values _lockFeesIncentive, _stakerLockFeesIncentive. It is therefore possible to set these values grater than 100% (i.e. >FEE_DENOMINATOR).

When this is the case it will be impossible to call earmarkFees() since _lockFeesIncentive or _stakerLockFeesIncentive will be larger than the contract balance and therefore the safe transfer of fee tokens will fail.

If the values add to less than 100% then part of the fees will remain in the contract.

Proof of Concept

setFeeInfo() does not have any restrictions on setting the value of the fees.

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


        lockFeesIncentive = _lockFeesIncentive;
        stakerLockFeesIncentive = _stakerLockFeesIncentive;


        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;
        }
    }

earmarkFees() will always revert when trying to transfer fees to the lockFees or stakerLockRewards contracts.

    function earmarkFees() external returns (bool) {
        //claim fee rewards
        IStaker(staker).claimFees(feeDistro, feeToken);
        //send fee rewards to reward contract
        uint256 _balance = IERC20(feeToken).balanceOf(address(this));


        uint256 _lockFeesIncentive = _balance.mul(lockFeesIncentive).div(FEE_DENOMINATOR);
        uint256 _stakerLockFeesIncentive = _balance.mul(stakerLockFeesIncentive).div(
            FEE_DENOMINATOR
        );
        if (_lockFeesIncentive > 0) {
            IERC20(feeToken).safeTransfer(lockFees, _lockFeesIncentive);
            IRewards(lockFees).queueNewRewards(_lockFeesIncentive);
        }
        if (_stakerLockFeesIncentive > 0) {
            IERC20(feeToken).safeTransfer(stakerLockRewards, _stakerLockFeesIncentive);
            IRewards(stakerLockRewards).queueNewRewards(feeToken, _stakerLockFeesIncentive);
        }
        return true;
    }

Recommended Mitigation Steps

There should be a limitation in setFeeInfo() that require(_lockFeesIncentive + _stakerLockFeesIncentive == FEE_DENOMINATOR);. This will ensure that all of the fees are always distributed.

Lines of code

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

Vulnerability details

Impact

The owner of VoterProxy is able to call setOperator() (if the previous operator is shutdown). This allows them to then call execute(), withdraw() or withdrawAll().

Execute makes a call to any arbitrary contract with arbitrary data. This may therefore call any ERC20 token, and gauge or the VoterEscrow account and withdraw protocol funds.

The functions withdraw() and withdrawAll() can also be abused to take all funds deposited in the gauges and transfer them to the owner's malicious address.

This poses a significant centralisation risk if the owner private key is compromised or the owner decides to rug pull.

Proof of Concept

After the owner has updated the operator via setOperator() they are able to call VoterProxy.execute() to execute any call to any smart contract.

    function execute(
        address _to,
        uint256 _value,
        bytes calldata _data
    ) external returns (bool, bytes memory) {
        require(msg.sender == operator, "!auth");


        (bool success, bytes memory result) = _to.call{value: _value}(_data);
        require(success, "!success");


        return (success, result);
    }

Similarly, for withdraw() and withdrawAll()

    function withdraw(
        address _token,
        address _gauge,
        uint256 _amount
    ) public returns (bool) {
        require(msg.sender == operator, "!auth");
        uint256 _balance = IERC20(_token).balanceOf(address(this));
        if (_balance < _amount) {
            _amount = _withdrawSome(_gauge, _amount.sub(_balance));
            _amount = _amount.add(_balance);
        }
        IERC20(_token).safeTransfer(msg.sender, _amount);
        return true;
    }


    function withdrawAll(address _token, address _gauge) external returns (bool) {
        require(msg.sender == operator, "!auth");
        uint256 amount = balanceOfPool(_gauge).add(IERC20(_token).balanceOf(address(this)));
        withdraw(_token, _gauge, amount);
        return true;
    }

Recommended Mitigation Steps

This issue may be mitigated removing the ability for the owner to change the operator in VoterProxy.

If the functionality is require ensure it is behind a time lock and multisig / dao.

QA Report

VeAssetDeposit.lockVeAsset() has the potential for reentrancy when minting incentiveVeAsset

While mint() will not give an attacker control of execution it is worth applying the check-effects-interactions pattern to avoid issues which may occur when contracts are updated.

function lockVeAsset() external {
        _lockVeAsset();

        //mint incentives
        if (incentiveVeAsset > 0) {
            ITokenMinter(minter).mint(msg.sender, incentiveVeAsset);
            incentiveVeAsset = 0;
        }
    }

Recommendation

The check-effects-interaction patter ssts all state variables before making potential external calls.

function lockVeAsset() external {
        _lockVeAsset();

        //mint incentives
        if (incentiveVeAsset > 0) {
            uint256 amount = incentiveVeAsset;
            incentiveVeAsset = 0;
            ITokenMinter(minter).mint(msg.sender, amount);
        }
    }

VoterProxy.sol and Booster.sol does not perform zero checks in the setter functions

Zero checks for addresses and other fields are important as it prevent accidently calling a function when an input field has not been completed. This is a common issue when taking user input off-chain.

Recommendation

Add input checks as seen in the Booster.setOwner() function.

    function setOwner(address _owner) external {
        require(msg.sender == owner, "!auth");
		require(_owner != address(0), "!=0"); // @audit add this check
        owner = _owner;
        emit OwnerUpdated(_owner);
    }

The following functions should have additional checks:

• Booster.sol
  • setOwner()
  • setFeeManager()
  • setFactories()
  • setArbitrator()
  • setVoteDelegate()
  • setRewardContracts()
  • setTreasury()

SafeMath is not require for solidity 0.8.7

On solidity 0.8.x overflow checks are automatically turned on. As a result there is no need to use the SafeMath library.

Recommendation

Remove SafeMath from the following contracts and use native +, -, * and / operators:

• Booster.sol
• BaseRewardPool.sol
• DepositToken.sol
• ExtraRewardStashV1.sol
• ExtraRewardStashV2.sol
• ExtraRewardStashV3.sol
• PoolManager.sol
• VE3DRewardPool.sol
• VeAssetDepositor.sol
• VeTokenMinter.sol
• VirtualBalanceRewardPool.sol
• VoterProxy.sol
• VeToken.sol
• VE3Token.sol

Interface implementations should inherit the interface

An implementation of an interface should include all the defined functions. In solidity this may be verified at compile time by inheriting the interface in the implementation.

As a result the compiler will ensure that all functions of the interface have been implemented with the correct function signatures and return values. Noting that the only difference is all interfaces will have external as the visibility whereas the implementation may have these as external or public.

The list of contracts and there interfaces may be seen below:

• VeTokenMinter is ITokenMinter
• VoterProxy is IStaker
• RewardFactory is IRewardFactory
• Booster is IPools, IDeposit()

Recommendation

For each contract which is the implementation of an interface should inherit the interface. For exmaple consider VoterProxy.sol which is IStaker implementation, the following may be added to the contract definition.

contract VoterProxy is IStaker {
...

RewardFactory.addOperator() may accidently unalign the veAssets[_newOperator] = _veAsset

If the _veAsset does not match the veAsset in the operator then CreateVeAssetRewards() function will malfunction.

Recommendation

Fetch the veAsset from the operator as seen below.

veAssets[_newOperator] = Booster(_newOperator).veAsset();

Note it would be more gas efficient to create an IBooster interface rather than importing the entire Booster contract.

PoolManager is unable to call Booster.setPoolManager()

The function setFeeManager() can only be called by the poolManager. When this is set to the contract PoolManager.sol it is unable to be changed since this contract does not have a method of transferring the role to another address.

There is limited benefit by having a PoolManager.sol contract. The functionality could be implemented in Booster.sol to simplify logic and save gas.

Recommendation

To simply remove the PoolManager.sol contract and move all functionality to Boost.sol would reduce code complexity and reduce the attack surface.