In VE3DRewardPool it's not possible to remove rewardTokens and if there were one bad rewardToken (by mistake or token related contract chaning) then no one can withdraw any rewards from VE3DRewardPool

[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/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.

[solvetony]

Duplicate of #136

[GalloDaSballo]

Duplicate of #136