Division by zero could cause DOS in function `harvest()` and `claim()` in PirexRewards contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L802-L804

Vulnerability details

Impact

When functions harvest() or claim() of PirexRewards are called, they will claim rewards by calling PirexGmx.claimRewards() function. If there is any case that esGmx reward is existed but not base rewards or vice versa, the value returned from _calculateRewards() is 0 and it leads to division by zero and revert.

For example,

if (baseRewards != 0) {
    // This may not be necessary and is more of a hedge against a discrepancy between
    // the actual rewards and the calculated amounts. Needs further consideration
    rewardAmounts[0] =
        (gmxBaseRewards * baseRewards) /
        (gmxBaseRewards + glpBaseRewards); // @audit division by zero
    rewardAmounts[1] = baseRewards - rewardAmounts[0];
}

Since baseRewards is calculated by using pre and post balance, attacker can simply trigger it by transferring gmxBaseReward to the contract, passing the check if (baseRewards != 0).

Proof of Concept

Function harvest() calls claimRewards(). https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L338-L348

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

In function claimRewards(), rewards amount is calculated by substracting post balance with pre balance https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L785-L790

uint256 baseRewards = gmxBaseReward.balanceOf(address(this)) -
    baseRewardBeforeClaim;
uint256 esGmxRewards = stakedGmx.depositBalances(
    address(this),
    address(esGmx)
) - esGmxBeforeClaim;

Function _calculateRewards() can return zero when there is lacking rewards in distributor. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L250-L254

uint256 distributorBalance = (isBaseReward ? gmxBaseReward : esGmx)
    .balanceOf(distributor);
uint256 blockReward = pendingRewards > distributorBalance
    ? distributorBalance
    : pendingRewards;

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding zero check before doing the division.

[Picodes]

Since baseRewards is calculated by using pre and post balance, attacker can simply trigger it by transferring gmxBaseReward to the contract, passing the check if (baseRewards != 0).

How would you this considering the fact that the attacker is not suppose to be able to execute code between the pre and post balance check ?

[Picodes]

The scenario for how to get to a point where gmxBaseRewards == 0, glpBaseRewards == 0 but baseRewards > 0 is not convincing.

[Picodes]

The scenario for how to get to a point where gmxBaseRewards == 0, glpBaseRewards == 0 but baseRewards > 0 is not convincing.

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof