veToken Finance contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

contract Booster can create pool for different lpTokens (by a call from PoolManager) and most of the logics of Booster is based on the fact that different pools has different lpToken addresses (for every lpToken there most exists only one pool). but there is no check in addPool() code to ensure that owner (by PoolManager call) don't add multiple pools with same lpToken address (in PoolManager.addPool() there is check that gauge is not duplicate for pools but it don't check lpToken duplication). so if owner by mistake or intentionally do this action, users funds and rewards would be distributed wrongly and some users would lose their funds.

Proof of Concept

This is addPool() function code in Booster contract:

    //create a new pool
    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;
    }

As you can see there is no check that added pool has different _lptoken from already created pools. and in PoolManager.addPool() there is only check for gauge to be unique:

    //add a new veAsset pool to the system.
    //gauge must be on veAsset's gaugeProxy, thus anyone can call
    // use by pickle
    function addPool(
        address _lptoken,
        address _gauge,
        address _pools,
        uint256 _stashVersion
    ) external onlyOwner returns (bool) {
        require(_lptoken != address(0), "lptoken is 0");
        require(_gauge != address(0), "gauge is 0");
        require(_pools != address(0), "pools is 0");

        bool gaugeExists = IPools(_pools).gaugeMap(_gauge);
        require(!gaugeExists, "already registered");

        IPools(_pools).addPool(_lptoken, _gauge, _stashVersion);

        return true;
    }

If owner by mistake or intentionally add multiple pool with same _lptoken, this would cause deposited user funds or rewards to be distributed wrongly and some users lose their funds. for example this is shutdownPool() code:

    //shutdown pool
    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;
    }

As you can see it calls IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) which will withdraw all amount in lpTooken and gauge address and transfer them to Booster address. so other pool with same lpToken balances will transfer to Booster too. This is _withdraw() code in Booster:

//withdraw lp tokens
    function _withdraw(
        uint256 _pid,
        uint256 _amount,
        address _from,
        address _to
    ) internal {
        PoolInfo storage pool = poolInfo[_pid];
        address lptoken = pool.lptoken;
        address gauge = pool.gauge;

        //remove lp balance
        address token = pool.token;
        ITokenMinter(token).burn(_from, _amount);

        //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);

        emit Withdrawn(_to, _pid, _amount);
    }

As you can see it calls IStaker(staker).withdraw(lptoken, gauge, _amount) and This is withdraw() code in staker (VoterProxy):

// Withdraw partial funds
    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;
    }

So it transfer lpToken balance of VoterProxy to Booster so it can be transferred to user. but if different Booster's pools have same lpToken address then balance of VoterProxy in that token will be wrongly used between those pools withdraw and deposit.

Tools Used

VIM

Recommended Mitigation Steps

Check that for same lpToken contract don't create multiple pool too.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L38-L51 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L101-L111

Vulnerability details

Impact

Contract VeAssetDepositor locks funds in veAsset Project for maxTime. veAsset project has his own maxTime and users can lock tokens bigger than that amount if they try to that the transaction will fail. in VeAssetDepositor's constructor the deployer set maxTime to use in locking. but there is no check for maxTime value and if deployer set wrong value (bigger than veAsset Project's maxTime) contract will be in broken state. and if maxTime in veAsset Project changes this will happen again, because there.because there is no functionality to change maxTime.

Proof of Concept

This is constructor code in VeAssetDepositor which sets maxTime value and there is no check for that value to be valid one based on veAssert Project's maxTime:

    constructor(
        address _staker,
        address _minter,
        address _veAsset,
        address _escrow,
        uint256 _maxTime
    ) {
        staker = _staker;
        minter = _minter;
        veAsset = _veAsset;
        escrow = _escrow;
        feeManager = msg.sender;
        maxTime = _maxTime;
    }

And this is _lockVeAsset() code:

     //lock veAsset
    function _lockVeAsset() internal {
        uint256 veAssetBalance = IERC20(veAsset).balanceOf(address(this));
        if (veAssetBalance > 0) {
            IERC20(veAsset).safeTransfer(staker, veAssetBalance);
        }

        //increase ammount
        uint256 veAssetBalanceStaker = IERC20(veAsset).balanceOf(staker);
        if (veAssetBalanceStaker == 0) {
            return;
        }

        //increase amount
        IStaker(staker).increaseAmount(veAssetBalanceStaker);

        uint256 unlockAt = block.timestamp + maxTime;
        uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK;

        //increase time too if over 2 week buffer
        if (unlockInWeeks.sub(unlockTime) > 2) {
            IStaker(staker).increaseTime(unlockAt);
            unlockTime = unlockInWeeks;
        }
        emit LockUpdated(veAssetBalanceStaker, unlockTime);
    }

which uses maxTime to increase lock time in veAsset Project. so if deployer set wrong value for maxTime or the value of maxTime decreases in veAsset Project then the VeAssetDepositor would be in broken state and users funds can be lost, becasue VoterEscrow contract in VeAsset Project don't allow to create lock bigger than its maxTime so veToken protocol can't lock funds in it.

Tools Used

VIM

Recommended Mitigation Steps

check for maxTime value to be valid and add a mechanism to change maxTime value if this value changes in veAsset Project's contract.

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/VE3DRewardPool.sol#L275-L321

Vulnerability details

Impact

contract VE3DRewardPool distributes reward tokens to stakers but if owner by mistake add wrong info for one of reward tokens or some of related contract to one of reward tokens changes or became broken then no one an call getReward() and get withdraw any rewards from contract and contracts will become useless because there is no mechanism to change one reward token info or delete it and it's not possible to get one or subset of reward tokens rewards.

Proof of Concept

This is addReward() function code which add multiple related contract address for one token:

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

This is getReward() function code:

    function getReward(
        address _account,
        bool _claimExtras,
        bool _stake
    ) public updateReward(_account) {
        address _rewardToken;
        for (uint256 i = 0; i < rewardTokens.length(); i++) {
            _rewardToken = rewardTokens.at(i);

            uint256 reward = earnedReward(_rewardToken, _account);
            if (reward > 0) {
                rewardTokenInfo[_rewardToken].rewards[_account] = 0;
                IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0);
                IERC20(_rewardToken).safeApprove(
                    rewardTokenInfo[_rewardToken].veAssetDeposits,
                    reward
                );
                IVeAssetDeposit(rewardTokenInfo[_rewardToken].veAssetDeposits).deposit(
                    reward,
                    false
                );

                uint256 ve3TokenBalance = IERC20(rewardTokenInfo[_rewardToken].ve3Token).balanceOf(
                    address(this)
                );
                if (_stake) {
                    IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
                        rewardTokenInfo[_rewardToken].ve3TokenRewards,
                        0
                    );
                    IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
                        rewardTokenInfo[_rewardToken].ve3TokenRewards,
                        ve3TokenBalance
                    );
                    IRewards(rewardTokenInfo[_rewardToken].ve3TokenRewards).stakeFor(
                        _account,
                        ve3TokenBalance
                    );
                } else {
                    IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeTransfer(
                        _account,
                        ve3TokenBalance
                    );
                }
                emit RewardPaid(_account, ve3TokenBalance);
            }
        }

        //also get rewards from linked rewards
        if (_claimExtras) {
            uint256 length = extraRewards.length;
            for (uint256 i = 0; i < length; i++) {
                IRewards(extraRewards[i]).getReward(_account);
            }
        }
    }

As you can see it loops through all reward tokens and make some external contract calls for reward token related contract address. so if owner set veAssetDeposits or rewardToken or ve3Token address by mistake or their contract was broken or in shutdown state then the contract calls will fails for that reward token and whole transaction will fail and no one can withdraw rewards and rewards will be lost forever because there is no mechanism to change one reward token info or just call getReward() for subset of reward tokens.

Tools Used

VIM

Recommended Mitigation Steps

add some mechanism to change one reward token info or delete it or call getReward() for subset of reward tokens.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L327-L345 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L367-L384

Vulnerability details

Impact

Function notifyRewardAmount() calculates rewardRate for reward token/s in VE3DRewardPool and BaseRewardPool. to calculate rewardRate it divides reward amount to duration but because of the rounding error in division, some of reward amount wouldn't get distributed and stuck in contract (rewardRate * duration < reward). and contract don't redistributes them or don't have any mechanism to recover them. This bug can be more damaging if the precision of rewardToken is low or token price is high.

Proof of Concept

This is notifyRewardAmount() code in BaseRewardPool: (contract VE3DRewardPool is similar)

    function notifyRewardAmount(uint256 reward) internal updateReward(address(0)) {
        historicalRewards = historicalRewards.add(reward);
        if (block.timestamp >= periodFinish) {
            rewardRate = reward.div(duration);
        } else {
            uint256 remaining = periodFinish.sub(block.timestamp);
            uint256 leftover = remaining.mul(rewardRate);
            reward = reward.add(leftover);
            rewardRate = reward.div(duration);
        }
        currentRewards = reward;
        lastUpdateTime = block.timestamp;
        periodFinish = block.timestamp.add(duration);
        emit RewardAdded(reward);
    }

As you can see it sets rewardRate = reward.div(duration); and this is where the rounding error happens. and even if contract distributes all in all the duration it will distribute rewardRate * duration which can be lower than reward and the extra reward amount will stuck in contract. This is queueNewRewards() code which calls notifyRewardAmount():

    function queueNewRewards(uint256 _rewards) external returns (bool) {
        require(msg.sender == operator, "!authorized");

        _rewards = _rewards.add(queuedRewards);

        if (block.timestamp >= periodFinish) {
            notifyRewardAmount(_rewards);
            queuedRewards = 0;
            return true;
        }

        //et = now - (finish-duration)
        uint256 elapsedTime = block.timestamp.sub(periodFinish.sub(duration));
        //current at now: rewardRate * elapsedTime
        uint256 currentAtNow = rewardRate * elapsedTime;
        uint256 queuedRatio = currentAtNow.mul(1000).div(_rewards);

        //uint256 queuedRatio = currentRewards.mul(1000).div(_rewards);
        if (queuedRatio < newRewardRatio) {
            notifyRewardAmount(_rewards);
            queuedRewards = 0;
        } else {
            queuedRewards = _rewards;
        }
        return true;
    }

As you can see it queues rewardToken and when the reward amount reach some point it calls notifyRewardAmount() and set queuedRewards to 0x0. notifyRewardAmount() will set rewardRate based on reward amount but because of the rounding error some of the reward token 0 =< unused < duration will stuck in contract and pool will not distribute it. if the token has low precision or has higher price then this amount value can be very big because notifyRewardAmount() can be called multiple times.

Tools Used

VIM

Recommended Mitigation Steps

add extra amount to queuedRewards so it would be distributed on next notifyRewardAmount() or add other mechanism to recover it.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L152-L162 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L170-L183

Vulnerability details

Impact

The VE3DRewardPool and BaseRewardPool contract is supposed to distribute rewards to stackers, but if in some period, totalSupply() was equal to 0, then for that time period, rewards will not added to rewardPerTokenStored and those period rewards would not distribute to any address and those rewards will stuck in contract forever.

Proof of Concept

This is notifyRewardAmount() code in BaseRewardPool contract: (VE3DRewardPool code is similar)

      function notifyRewardAmount(uint256 reward) internal updateReward(address(0)) {
        historicalRewards = historicalRewards.add(reward);
        if (block.timestamp >= periodFinish) {
            rewardRate = reward.div(duration);
        } else {
            uint256 remaining = periodFinish.sub(block.timestamp);
            uint256 leftover = remaining.mul(rewardRate);
            reward = reward.add(leftover);
            rewardRate = reward.div(duration);
        }
        currentRewards = reward;
        lastUpdateTime = block.timestamp;
        periodFinish = block.timestamp.add(duration);
        emit RewardAdded(reward);
    }

As you can see, in the line rewardRate = reward.div(duration); the value of rewardRate has been set to the division of available reward to duration. so if we distribute rewardRate amount in every second between stackers, then all rewards will be used by contract. contract uses updateReward() modifier to update rewardPerTokenStored (this variable keeps track of distributed tokens) and this modifier uses rewardPerToken() to update BaseRewardPool:

      modifier updateReward(address account) {
        rewardPerTokenStored = rewardPerToken();
        lastUpdateTime = lastTimeRewardApplicable();
        if (account != address(0)) {
            rewards[account] = earned(account);
            userRewardPerTokenPaid[account] = rewardPerTokenStored;
        }
        emit RewardUpdated(account, rewards[account], rewardPerTokenStored, lastUpdateTime);
        _;
    }

This is rewardPerToken() code in BaseRewardPool:

    function rewardPerToken() public view returns (uint256) {
        if (totalSupply() == 0) {
            return rewardPerTokenStored;
        }
        return
            rewardPerTokenStored.add(
                lastTimeRewardApplicable().sub(lastUpdateTime).mul(rewardRate).mul(1e18).div(
                    totalSupply()
                )
            );
    }

If for some period totalSupply() was 0 then contract won't increase rewardPerTokenStored and those periods reward stuck in contract forever, because there is no mechanism to calculate them and withdraw them in contract. For example if operator deploy and initialize the pool immediately before others having a chance of stacking their tokens, and use queueNewRewards() to queue the rewards then The rewards for early period of pool will be locked forever.

Tools Used

VIM

Recommended Mitigation Steps

Add some mechanism to recalculate rewardRate or calculated undistributed rewards(calculated undistributed reward based on rewardRate and when totalSupply() is 0).

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/VE3DRewardPool.sol#L297-L318

Vulnerability details

Impact

if owner by mistake or bad intention add a reward token that ve3Token of new reward token was equal to old reward token address then old reward token balance of VE3DRewardPool can be lost by calling getReward(account, _claimExtras=False,_stake=False) because of the line 314 which is IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeTransfer(_account,ve3TokenBalance);

Proof of Concept

This is addReward() code in VE3DRewardPool:

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

As you can see there is no check for values and it sets new rewardToken parameters. This is getReward() code:

    function getReward(
        address _account,
        bool _claimExtras,
        bool _stake
    ) public updateReward(_account) {
        address _rewardToken;
        for (uint256 i = 0; i < rewardTokens.length(); i++) {
            _rewardToken = rewardTokens.at(i);

            uint256 reward = earnedReward(_rewardToken, _account);
            if (reward > 0) {
                rewardTokenInfo[_rewardToken].rewards[_account] = 0;
                IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0);
                IERC20(_rewardToken).safeApprove(
                    rewardTokenInfo[_rewardToken].veAssetDeposits,
                    reward
                );
                IVeAssetDeposit(rewardTokenInfo[_rewardToken].veAssetDeposits).deposit(
                    reward,
                    false
                );

                uint256 ve3TokenBalance = IERC20(rewardTokenInfo[_rewardToken].ve3Token).balanceOf(
                    address(this)
                );
                if (_stake) {
                    IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
                        rewardTokenInfo[_rewardToken].ve3TokenRewards,
                        0
                    );
                    IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
                        rewardTokenInfo[_rewardToken].ve3TokenRewards,
                        ve3TokenBalance
                    );
                    IRewards(rewardTokenInfo[_rewardToken].ve3TokenRewards).stakeFor(
                        _account,
                        ve3TokenBalance
                    );
                } else {
                    IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeTransfer(
                        _account,
                        ve3TokenBalance
                    );
                }
                emit RewardPaid(_account, ve3TokenBalance);
            }
        }

        //also get rewards from linked rewards
        if (_claimExtras) {
            uint256 length = extraRewards.length;
            for (uint256 i = 0; i < length; i++) {
                IRewards(extraRewards[i]).getReward(_account);
            }
        }
    }

As you can see it loops through all rewardTokens and in line 314 it transfers all balance of rewardTokenInfo[_rewardToken].ve3Token of contract to account address. So if owner by mistake or bad intention add a new rewardToken that rewardTokenInfo[newRewardToken].ve3Token == oldRewardToken then by calling getReward(account, False, False) contract will loop through all reward tokens and when it reaches the newRewardToken it will transfer all the balance of contract in rewardTokenInfo[newRewardToken].ve3Token address to account address and rewardTokenInfo[newRewardToken].ve3Token is oldRewardToken, so it will transfer all the oldRewardToken balance of contract(which was supposed to be distributed among all stackers) to account address. This is like a backdoor that gives owner the ability to withdraw rewards tokens and if owner makes a simple mistake then one can easily withdraw that contract balance of reward token.

Tools Used

VIM

Recommended Mitigation Steps

in addReward() check that there is no mistake or any malicious values added to rewards.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L113-L121

Vulnerability details

Impact

Function lockVeAsset() code don't follow check-effect-interact pattern and calls a external contract and it's vulnerable to reentrancy attack and attacker can mint unlimited tokens for himself.

Proof of Concept

This is lockVeAsset() code in VeAssetDepositor contract:

    function lockVeAsset() external {
        _lockVeAsset();

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

As you can see there is no reentrancy protection and contract calls ITokenMinter(minter).mint(msg.sender, incentiveVeAsset); to mint for user incentiveVeAsset tokens then set the value of incentiveVeAsset to 0. so it's vulnerable to reentrancy attack and if minter make some external calls too in mint() function to attacker controlled addresses (msg.sender is sended to mint()) then attacker can call lockVeAsset() again and again and mint unlimited tokens for himself.

Tools Used

VIM

Recommended Mitigation Steps

follow heck-effect-interact pattern pattern or add lock mechanism.