RewardDistributor's addFee can be run while ve is zero

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L98-L114

Vulnerability details

addFee() will issue no rewards to the stakers for epochs it is run while ve address wasn't set. If ve address cannot be set (as it is now, described elsewhere) or isn't set as an operational mistake, trades will go through, but no rewards to be issued for the stakers, even if there is a substantial share of them.

Notice that addFee() advances the epoch counter, so such erroneous minting cannot be corrected. It will happen automatically on every trade, i.e. isn't controlled by the protocol.

Setting the severity to be high as it is value loss for the stakers and a deviation from the expected system logic.

Proof of Concept

addFee() uses ve without checking that it's non-zero:

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L98-L114

    function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
        //console.log(block.timestamp,epoch,fee);
        if (rewardToken.totalSupply() > 1000000000 * 10**18) {
            // if supply is greater then a billion dont mint anything, dont add trades
            return;
        }

        // if 24 hours have passed since last epoch change
        if (block.timestamp > startTime + (epoch) * secsInDay) {
            // this assumes atleast 1 trade is done daily??????
            // logic to decide how much token to emit
            // emission = daily * (1 - (balance of locker/ total supply))  full if 0 locked and 0 if all locked
            // uint256 tokenToEmit = dailyEmission * rewardToken.balanceOf()/
            // emissions is decided by epoch begiining locked/circulating , and amount each nft gets also decided at epoch begining
            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
                rewardToken.totalSupply();
            uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

ve isn't initialized on construction:

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L55

    VE public ve;

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L74-L85

    constructor(
        address _weth,
        address _trader,
        address _token,
        address _governance
    ) {
        weth = ERC20(_weth);
        trader = _trader;
        rewardToken = ERC20(_token);
        _transferOwnership(_governance); // set the new owner
        startTime = 1659211200;
    }

On the usage with zero ve addFee() will act as if there is no stakers at all, inflating the reward token more than it's designed:

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L112-L114

            uint256 tokenToEmit = (dailyEmission * (rewardToken.totalSupply() - rewardToken.balanceOf(address(ve)))) /
                rewardToken.totalSupply();
            uint256 stakerReward = (tokenToEmit * rewardToken.balanceOf(address(ve))) / rewardToken.totalSupply();

And leaving no rewards to the stakers:

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L119-L121

            rewardStaker[epoch] = stakerReward;
            rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
            rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;

Recommended Mitigation Steps

Consider requiring ve to be set, i.e. allowing no trades to happen before RewardDistributor is fully configured, for example:

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L98-L103

    function addFee(address[2] memory addr, uint256 fee) public onlyTrader {
+       require(address(ve) != address(0), ' VE not added yet');
        //console.log(block.timestamp,epoch,fee);
        if (rewardToken.totalSupply() > 1000000000 * 10**18) {
            // if supply is greater then a billion dont mint anything, dont add trades
            return;
        }

[0xsaruman]

ve will be set, there is no point of add fee without VE being set.

[dmitriia]

Fees are added automatically on trades:

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L269

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L402

It's not controlled there that ve was set in distributor.

I do agree that what described here is a corner case, but its probability isn't zero. Similar issues were exploited on-chain enough times already.

[dmvt]

As with so many other reported issues with the fees system, the sponsor does need to configure things correctly for fees to be issued according to the documentation. Fees not being issued and funds being lost are two different things. No funds are stolen or lost if this part of the system is misconfigured. In the worst case, the sponsor could simply recalculate the fees off-chain and airdrop them to the participants who did not receive them. Downgrading to QA because this is a state handling and documentation issue, not a compromise or lost funds issue.

[dmitriia]

0. How many others report what issues aren't relevant at all and shouldn't be even mentioned in severity discussions.

1. In this case token emission is inflated, i.e. the supply is diluted, which is material for all reward token holders and is not fixable by airdropping as this will happen many times over (on every trade) and the complexity of such manual fix will scale up pretty fast, making it unachievable.

2. Per rules lost funds isn't the prerequisite for Medium severity, system functionality unavailability is. Here both the funds are lost via erroneous token supply inflation and system fee distribution functionality is unavailable. This way the impact itself is High. There is a prerequisite, however, an operational mistake of not setting ve address is required. What is the probability of that is debatable, but as this is performed with stand-alone operation, and such types of errors did happened in practice many times already, it can be said that such probability is material. The bottom line is the issue here is a straightforward technical oversight with at least Medium severity.