veToken Finance contest - hyh's results

Lines of code

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

Vulnerability details

shutdownPool() marks shutdown successful even if it's not (i.e. when withdrawAll() call wasn't successful). As withdrawing logic expect that the pool in shutdown has already provided the funds, and makes no additional attempts to retrieve them, user funds will be frozen permanently as there are no mechanics in place to turn shutdown off for a pool.

Setting severity to medium as that's user principal funds freeze scenario conditional on any issues in withdrawAll().

Proof of Concept

In the case of unsuccessful withdrawAll() call the pool nevertheless will be marked as shutdown:

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

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

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

        emit PoolShuttedDown(_pid);
        return true;
    }

It will block the withdrawals as there will be not enough funds to fulfil the claim:

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

        //pull from gauge if not shutdown
        // if shutdown tokens will be in this contract
        if (!pool.shutdown) {
            IStaker(staker).withdraw(lptoken, gauge, _amount);
        }

        //some gauges claim rewards when withdrawing, stash them in a seperate contract until next claim
        //do not call if shutdown since stashes wont have access
        address stash = pool.stash;
        if (stash != address(0) && !isShutdown && !pool.shutdown) {
            IStash(stash).stashRewards();
        }

        //return lp tokens
        IERC20(lptoken).safeTransfer(_to, _amount);

This way user funds will be frozen as the system will not attempt to withdraw from the pool, while there will be no funds to transfer to the user and _withdraw() will be reverting on L422 safeTransfer call.

Recommended Mitigation Steps

The shutdownPool logic can to be updated:

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

Consider unifying the logic with shutdownSystem() and marking the pool shutdown only if withdraw was successful, for example:

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

    bool withdrawSuccess = false;

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

    if (withdrawSuccess || forced) {
	    pool.shutdown = true;
	    gaugeMap[pool.gauge] = false;
        emit PoolShuttedDown(_pid);
    	return true;
    }

    return false;
}

Lines of code

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

Vulnerability details

There is no check for the reward token amount to be transferred out in stashRewards(). As reward token list is external (controlled with IGauge(gauge).reward_tokens), and an arbitrary token can end up there, in the case when such token doesn't allow for zero amount transfers, the stashRewards() managed extra rewards retrieval can become unavailable.

I.e. stashRewards() can be blocked for even an extended period of time, so all other extra rewards gathering will not be possible. This cannot be controlled by the system as pool reward token list is external.

Setting the severity to medium as reward gathering is a base functionality of the system and its availability is affected.

Proof of Concept

stashRewards() attempts to send the amount to rewardArbitrator() without checking:

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

    if (activeCount > 1) {
        //take difference of before/after(only send new tokens)
        uint256 amount = IERC20(token).balanceOf(address(this));
        amount = amount.sub(before);

        //send to arbitrator
        address arb = IDeposit(operator).rewardArbitrator();
        if (arb != address(0)) {
            IERC20(token).safeTransfer(arb, amount);
        }
    }

If IStaker(staker).withdraw() produced no new tokens for any reason, the amount = amount.sub(before) above can be zero:

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/ExtraRewardStashV2.sol#L188-L189

    uint256 before = IERC20(token).balanceOf(address(this));
    IStaker(staker).withdraw(token);

As reward token can be arbitrary, it can also be reverting on an attempt to transfer zero amounts:

https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

If this be the case then the whole stashRewards() call will be failing until IStaker(staker).withdraw() manage to withdraw some tokens or such token be removed from gauge's reward token list. Both events aren’t directly controllable by the system.

Recommended Mitigation Steps

Consider running the transfer only when amount is positive:

-   if (activeCount > 1) {
+   if (amount > 0 && activeCount > 1) {
        //take difference of before/after(only send new tokens)
        uint256 amount = IERC20(token).balanceOf(address(this));
        amount = amount.sub(before);

        //send to arbitrator
        address arb = IDeposit(operator).rewardArbitrator();
        if (arb != address(0)) {
            IERC20(token).safeTransfer(arb, amount);
        }
    }

1. Can create a stash with an empty gauge (low)

CreateStash can create a misconfigured stashes that have zero gauge as a result of operational mistake. This is possible as calls with zero gauge will pass, all isV*() calls will be returning true, i.e. it's possible to create a stash of any type with zero gauge.

Proof of Concept

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/StashFactoryV2.sol#L53-L55

    //Create a stash contract for the given gauge.
    //function calls are different depending on the version of curve gauges so determine which stash type is needed
    function CreateStash(uint256 _pid, address _gauge, address _staker, uint256 _stashVersion) external returns(address){

x0.call will return true:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/StashFactoryV2.sol#L87-L103

    function IsV1(address _gauge) private returns(bool){
        bytes memory data = abi.encode(rewarded_token);
        (bool success,) = _gauge.call(data);
        return success;
    }

    function IsV2(address _gauge) private returns(bool){
        bytes memory data = abi.encodeWithSelector(reward_tokens,uint256(0));
        (bool success,) = _gauge.call(data);
        return success;
    }

    function IsV3(address _gauge) private returns(bool){
        bytes memory data = abi.encodeWithSelector(rewards_receiver,address(0));
        (bool success,) = _gauge.call(data);
        return success;
    }

Recommended Mitigation Steps

Consider checking _gauge and _staker addresses to be non-zero.

2. Comments misspelling (non-critical)

separate in both cases:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L411

//some gauges claim rewards when depositing, stash them in a seperate contract until next claim

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/BaseRewardPool.sol#L57

 *          distribute a child reward token (i.e. a secondary one from Curve, or a seperate one).

3. Ownership is transferred with one step procedure in Booster and VoterProxy (non-critical)

One step process offers no protection for the cases when ownership transfer is performed mistakenly or with any malicious intent.

Adding a modest complexity of an additional step and a delay is a low price to pay for having time to evaluate the ownership change.

Proof of Concept

Booster.setOwner:

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

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

VoterProxy.setOwner:

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

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

Recommended Mitigation Steps

Consider utilizing two-step ownership transferring process (proposition and acceptance in the separate actions) with a noticeable delay between the steps to enforce the transparency and stability of the system.

4. Configuration addresses are not checked (non-critical)

Similar situation in the most contracts, for instance:

BaseRewardPool:

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

    constructor(
        uint256 pid_,
        address stakingToken_,
        address rewardToken_,
        address operator_,
        address rewardManager_
    ) {
        pid = pid_;
        stakingToken = IERC20(stakingToken_);
        rewardToken = IERC20(rewardToken_);
        operator = operator_;
        rewardManager = rewardManager_;
    }

Booster:

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

    constructor(
        address _staker,
        address _minter,
        address _veAsset,
        address _feeDistro
    ) {
        isShutdown = false;
        staker = _staker;
        owner = msg.sender;
        voteDelegate = msg.sender;
        feeManager = msg.sender;
        poolManager = msg.sender;
        minter = _minter;
        veAsset = _veAsset;
        feeDistro = _feeDistro;
    }

ExtraRewardStashV1:

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/ExtraRewardStashV1.sol#L38-L50

    constructor(
        uint256 _pid,
        address _operator,
        address _staker,
        address _gauge,
        address _rFactory
    ) {
        pid = _pid;
        operator = _operator;
        staker = _staker;
        gauge = _gauge;
        rewardFactory = _rFactory;
    }

Recommended Mitigation Steps

Consider using zero checks for key addresses to protect against operational mistakes