user's reward loss because of incorrect rewardsTokens in PirexRewards contract when rewardsTokens are not configured yet or configured incorrectly

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L373-L417
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L338-L366
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L739-L816

Vulnerability details

Impact

PirexRewards contract start tracking gmxBaseReward and pxGmx as rewardsToken for pxGmx and pxGlp as producerTokens from the begining of contract deployment because this values hardcoded in claimRewards() function of PirexGmx But those primary rewardTokens didn't get inserted to PirexRewards contract state from the beginning and owner should add them later. When rewardTokens state are wrong in PirexRewards contract (gmxBaseReward and pxGmx is not in list), users calling PirexReward.claim() would lose their rewards because contract set userStates[user].rewards = 0. This condition can happen in contract early stage or when owner tries to change or update rewardTokens. Those primary rewardTokens should be hardcoded and added to rewards token list when contract get initialized because their values are hardcoded in PirexGmx and they should be hardcoded in PirexRewards too.

Proof of Concept

This is function claim() code in PirexRewards contract:

    function claim(ERC20 producerToken, address user) external {
        if (address(producerToken) == address(0)) revert ZeroAddress();
        if (user == address(0)) revert ZeroAddress();

        harvest();
        userAccrue(producerToken, user);

        ProducerToken storage p = producerTokens[producerToken];
        uint256 globalRewards = p.globalState.rewards;
        uint256 userRewards = p.userStates[user].rewards;

        // Claim should be skipped and not reverted on zero global/user reward
        if (globalRewards != 0 && userRewards != 0) {
            ERC20[] memory rewardTokens = p.rewardTokens;
            uint256 rLen = rewardTokens.length;

            // Update global and user reward states to reflect the claim
            p.globalState.rewards = globalRewards - userRewards;
            p.userStates[user].rewards = 0;

            emit Claim(producerToken, user);

            // Transfer the proportionate reward token amounts to the recipient
            for (uint256 i; i < rLen; ++i) {
                ERC20 rewardToken = rewardTokens[i];
                address rewardRecipient = p.rewardRecipients[user][rewardToken];
                address recipient = rewardRecipient != address(0)
                    ? rewardRecipient
                    : user;
                uint256 rewardState = p.rewardStates[rewardToken];
                uint256 amount = (rewardState * userRewards) / globalRewards;

                if (amount != 0) {
                    // Update reward state (i.e. amount) to reflect reward tokens transferred out
                    p.rewardStates[rewardToken] = rewardState - amount;

                    producer.claimUserReward(
                        address(rewardToken),
                        amount,
                        recipient
                    );
                }
            }
        }
    }

As you can see the code transfers all the rewards of user for reward tokens registered for this producerToken based on user share of total rewards and also it sets the value of producerTokens[producerToken].userStates[user].rewards as zero(user share of rewards of this producerToken become zero) so if in the moment of calling the function claim() the rewards token list was wrong(some tokens were missing) then user would lose his right to those rewards. Function claim() is callable by others for any user so attacker can call this function for other users and make them lose access to their rewards. the rewards calculation for producer tokens pxGmx, pxGlp with rewards tokens gmxBaseReward, pxGmx are done from the beginning of the contract but adding those reward tokens should be done by owner dynamically so in contract early stage the reward list of those producer tokens are wrong and even in other times when owner tries to update the reward list(because of any reason like GMX protocol update or by mistake or ....) the list of rewards would be wrong and user would lose funds if attacker call claim() for them. there is no pause mechanism in the PirexRewards contract to prevent calling claim() in those states when rewards lists are wrong.
This is harvest() code:

    function harvest()
        public
        returns (
            ERC20[] memory _producerTokens,
            ERC20[] memory rewardTokens,
            uint256[] memory rewardAmounts
        )
    {
        (_producerTokens, rewardTokens, rewardAmounts) = producer
            .claimRewards();
        uint256 pLen = _producerTokens.length;

        // Iterate over the producer tokens and update reward state
        for (uint256 i; i < pLen; ++i) {
            ERC20 p = _producerTokens[i];
            uint256 r = rewardAmounts[i];

            // Update global reward accrual state and associate with the update of reward state
            ProducerToken storage producerState = producerTokens[p];

            _globalAccrue(producerState.globalState, p);

            if (r != 0) {
                producerState.rewardStates[rewardTokens[i]] += r;
            }
        }

        emit Harvest(_producerTokens, rewardTokens, rewardAmounts);
    }

As you can see it update the state of producer and rewards tokens returned by PirexGmx.claimRewards(). This is some part of claimRewards() code in PirexGmx:

    function claimRewards()
        external
        onlyPirexRewards
        returns (
            ERC20[] memory producerTokens,
            ERC20[] memory rewardTokens,
            uint256[] memory rewardAmounts
        )
    {
        // Assign return values used by the PirexRewards contract
        producerTokens = new ERC20[](4);
        rewardTokens = new ERC20[](4);
        rewardAmounts = new uint256[](4);
        producerTokens[0] = pxGmx;
        producerTokens[1] = pxGlp;
        producerTokens[2] = pxGmx;
        producerTokens[3] = pxGlp;
        rewardTokens[0] = gmxBaseReward;
        rewardTokens[1] = gmxBaseReward;
        rewardTokens[2] = ERC20(pxGmx); // esGMX rewards distributed as pxGMX
        rewardTokens[3] = ERC20(pxGmx);
...
....

As you can see the function works all the time and producer tokens pxGmx, pxGlp with rewards tokens gmxBaseReward, pxGmx are hardcoded in the code so it always return them and harvest() would always calculate rewards for those tokens. so in this Scenario:

1. owner deployed contracts but rewards list are not yet updated and it is incorrect for producer tokens pxGmx, pxGlp. (or when owner tries to change or update reward tokens list and list is incorrect in the moment)
2. users deposit their tokens in PirexGmx and protocol stake them in GMX and start getting reward in GMX.
3. attacker would call PirexRewards.harvest() so PirexGmx.claimRewards() would get rewards from GMX and PirexRewards would calculate rewards for primary rewards tokens(gmxBaseReward, pxGmx)
4. attacker would call PirexRewards.claim(PxGmx/PxGLP, user) for users and contract would set users share of rewards of those producer tokens to zero but don't transfer any rewards to user because rewards list for those producer tokens are incorrect.

As you can see when contract is in some states the logics won't work correctly and users would lose their rewards. contract should protect itself from this type of bugs with on-chain mechanism.

Tools Used

VIM

Recommended Mitigation Steps

add producer tokens pxGmx, pxGlp with rewards tokens gmxBaseReward, pxGmx in PirexRewards when contract initialize or make sure claim() can't be called when reward tokens are not yet configured or in wrong states.

[c4-judge]

Picodes marked the issue as duplicate of #271

[c4-judge]

Picodes marked the issue as satisfactory