VE3DRewardPool and VE3DLocker adds to an unbounded array which may potentially lock all rewards in the contract

[code423n4]

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.

[jetbrain10]

will going to add a bound , same as #222, #125

[GalloDaSballo]

My guesstimate of the math is that each reward would add 100k gas to the updateReward modifier, meaning we'd need 120 reward tokens before any consideration about running out of gas would happen.

You also don't seem to be able to add a second one (provided someone has used the contract at least once after an addition)

I'll think about it but am thinking Med is stretching it

[GalloDaSballo]

Likelyhood is very low, however per the rules if enough rewards are added then claiming and withdrawing can be bricked permanently.

I'd recommend end users to ensure the unlikely number of 120 rewards is never reached.

Marking the finding as Valid and of Medium Severity