Lines of code

Vulnerability details

Impact

Function claim() in PxGmxReward contract is used for claiming available pxGMX rewards of a user. but this function calls IAutoPxGlp(address(this)).compound(1, 1, true); to harvest new rewards and stake them to compound rewards. but this call is external call and the parameter optOutIncentive set as true so the compound() would try to transfer the incentive to msg.sender which is AutoPxGmx itself. the incentives (as pxGlp and pxGmx tokens) transferred to contract won’t be considered as current or future reward or fee or anything and it would be lost forever (only compound() function handles the rewards and fees and it only handles the received rewards during the call to PirexRewards.claim() function and incentive transfer happens later in the compound() function). so This bug would cause some funds of contract(incentive) to be locked forever. those incentive should be transferred to a user who called the contract.

Proof of Concept

This is claim() code in PxGmxReward:

    function claim(address receiver) external {
        if (receiver == address(0)) revert ZeroAddress();

        IAutoPxGlp(address(this)).compound(1, 1, true);
        _userAccrue(msg.sender);

        uint256 globalRewards = globalState.rewards;
        uint256 userRewards = userRewardStates[msg.sender].rewards;

        // Claim should be skipped and not reverted on zero global/user reward
        if (globalRewards != 0 && userRewards != 0) {
            // Update global and user reward states to reflect the claim
            globalState.rewards = globalRewards - userRewards;
            userRewardStates[msg.sender].rewards = 0;

            // Transfer the proportionate reward token amounts to the recipient
            uint256 _rewardState = rewardState;
            uint256 amount = (_rewardState * userRewards) / globalRewards;

            if (amount != 0) {
                // Update reward state (i.e. amount) to reflect reward tokens transferred out
                rewardState = _rewardState - amount;

                pxGmx.safeTransfer(receiver, amount);

                emit PxGmxClaimed(msg.sender, receiver, amount);
            }
        }
    }

As you can see it calls compound() function like this IAutoPxGlp(address(this)).compound(1, 1, true); to compound pxGLP (and additionally pxGMX) rewards. this call is external call which would change the msg.sender as AutoPxGlp itself and it sets the value of optOutIncentive as true which would make contract to transfer incentive to msg.sender which is AutoPxGlp itself. This is compound() code in AutoPxGlp contract:

    function compound(
        uint256 minUsdg,
        uint256 minGlp,
        bool optOutIncentive
    )
        public
        returns (
            uint256 gmxBaseRewardAmountIn,
            uint256 pxGmxAmountOut,
            uint256 pxGlpAmountOut,
            uint256 totalPxGlpFee,
            uint256 totalPxGmxFee,
            uint256 pxGlpIncentive,
            uint256 pxGmxIncentive
        )
    {
        if (minUsdg == 0) revert InvalidParam();
        if (minGlp == 0) revert InvalidParam();

        uint256 preClaimTotalAssets = asset.balanceOf(address(this));
        uint256 preClaimPxGmxAmount = pxGmx.balanceOf(address(this));

        PirexRewards(rewardsModule).claim(asset, address(this));
        PirexRewards(rewardsModule).claim(pxGmx, address(this));

        // Track the amount of rewards received
        gmxBaseRewardAmountIn = gmxBaseReward.balanceOf(address(this));

        if (gmxBaseRewardAmountIn != 0) {
            // Deposit received rewards for pxGLP
            (, pxGlpAmountOut, ) = PirexGmx(platform).depositGlp(
                address(gmxBaseReward),
                gmxBaseRewardAmountIn,
                minUsdg,
                minGlp,
                address(this)
            );
        }

        // Distribute fees if the amount of vault assets increased
        uint256 newAssets = totalAssets() - preClaimTotalAssets;
        if (newAssets != 0) {
            totalPxGlpFee = (newAssets * platformFee) / FEE_DENOMINATOR;
            pxGlpIncentive = optOutIncentive
                ? 0
                : (totalPxGlpFee * compoundIncentive) / FEE_DENOMINATOR;

            if (pxGlpIncentive != 0)
                asset.safeTransfer(msg.sender, pxGlpIncentive);

            asset.safeTransfer(owner, totalPxGlpFee - pxGlpIncentive);
        }

        // Track the amount of pxGMX received
        pxGmxAmountOut = pxGmx.balanceOf(address(this)) - preClaimPxGmxAmount;

        if (pxGmxAmountOut != 0) {
            // Calculate and distribute pxGMX fees if the amount of pxGMX increased
            totalPxGmxFee = (pxGmxAmountOut * platformFee) / FEE_DENOMINATOR;
            pxGmxIncentive = optOutIncentive
                ? 0
                : (totalPxGmxFee * compoundIncentive) / FEE_DENOMINATOR;

            if (pxGmxIncentive != 0)
                pxGmx.safeTransfer(msg.sender, pxGmxIncentive);

            pxGmx.safeTransfer(owner, totalPxGmxFee - pxGmxIncentive);

            // Update the pxGmx reward accrual
            _harvest(pxGmxAmountOut - totalPxGmxFee);
        } else {
            // Required to keep the globalState up-to-date
            _globalAccrue();
        }

        emit Compounded(
            msg.sender,
            minGlp,
            gmxBaseRewardAmountIn,
            pxGmxAmountOut,
            pxGlpAmountOut,
            totalPxGlpFee,
            totalPxGmxFee,
            pxGlpIncentive,
            pxGmxIncentive
        );
    }

as you can see the incentive for baseAsset and pxGmx rewards get transferred in the lines:

            if (pxGlpIncentive != 0)
                asset.safeTransfer(msg.sender, pxGlpIncentive);

            if (pxGmxIncentive != 0)
                pxGmx.safeTransfer(msg.sender, pxGmxIncentive);

and because msg.sender is AutoPxGlp itself so those amounts would get transferred to contract itself and the reward calculations in the compound() is like this:

       // Distribute fees if the amount of vault assets increased
        uint256 newAssets = totalAssets() - preClaimTotalAssets;

        // Track the amount of pxGMX received
        pxGmxAmountOut = pxGmx.balanceOf(address(this)) - preClaimPxGmxAmount;

so the incentive amount that transferred to AutoPxGlp wouldn’t be considered as rewards because the rewards calculated based on the received amounts.
and the pxGmx reward accrual is calculated by _harvest(pxGmxAmountOut - totalPxGmxFee); which doesn’t include the incentive contract received.
so those incentives won’t be calculated in the future rewards or fees and they would stuck in contract address forever and won’t be accessible.

Tools Used

VIM

Recommended Mitigation Steps

function claim() in PxGmxReward should call compound() without making it as external call or should set optOutIncentive as false.

---

The text was updated successfully, but these errors were encountered:

All reactions