Lines of code
<https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L315-L317&gt;
<https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L402-L403&gt;
<https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PxGmxReward.sol#L53-L55&gt;
<https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PxGmxReward.sol#L75-L77&gt;
<https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PxGmxReward.sol#L121-L122&gt;

Vulnerability details

Impact

The current time based px rewards calculation system is not accurate, and not fair for users. Due to GMX protocol reward rate fluctuation, px users stake and claim at different time could get less or more rewards they deserve.

Some users could abuse the rule to time the high and low GMX rewards periods, effectively steal rewards from other users. Evermore, anyone can call claim() for other users. Innocent users could lose px rewards unconsciously.

Proof of Concept

Take PirexRewards.sol as example, PxGmxReward.sol is analogous.

Users rewards and globalRewards/globalState.rewards are purely time based weights:

File: src/PirexRewards.sol
    function userAccrue(ERC20 producerToken, address user) public {
289:         uint256 rewards = u.rewards +
290:             u.lastBalance *
291:             (block.timestamp - u.lastUpdate);

    function _globalAccrue(GlobalState storage globalState, ERC20 producerToken)
315:             uint256 rewards = globalState.rewards +
316:                 (block.timestamp - lastUpdate) *
317:                 lastSupply;

File: src\vaults\PxGmxReward.sol
    function _userAccrue(address user) internal {
75:         uint256 rewards = u.rewards +
76:             u.lastBalance *
77:             (block.timestamp - u.lastUpdate);

    function _globalAccrue() internal {
53:         uint256 rewards = globalState.rewards +
54:             (block.timestamp - globalState.lastUpdate) *
55:             globalState.lastSupply;

The rewards users get is based on the above weights in the real claimed rewards from GMX.

File: src\PirexRewards.sol
    function claim(ERC20 producerToken, address user) external {
402:                 uint256 rewardState = p.rewardStates[rewardToken];
403:                 uint256 amount = (rewardState * userRewards) / globalRewards;

File: src\vaults\PxGmxReward.sol
105:     function claim(address receiver) external {
121:             uint256 _rewardState = rewardState;
122:             uint256 amount = (_rewardState * userRewards) / globalRewards;

Assuming:
Here use GLP as example.There are only 2 users Alice and Bob. Will look at day 0, day 100, and day 200, 3 time points.

case 1: only Alice

1. day 0, Alice deposits 100 GLP.
2. day 100, harvest() is called. Alice’s staked GLP has reward of 1,000 EsGmx, but Alice did not claim the rewards. If call userAccrue(), producerTokens[pxGlp].userStates[Alice] should be 100 pxGlp * 100 days = 10,000, producerTokens[pxGlp][EsGmx] should be 1,000.
3. day 200, total reward is 1,500 GsGmx. Which means the 2nd 100 days, the rewards are less, only 500 EsGmx.
4. day 200, Alice claims, and receives 1500 pxGmx as rewards. producerTokens[pxGlp].userStates[Alice] is 10,000 + 100 pxGlp * 100 days = 20,000, producerTokens[pxGlp][EsGmx] is 1,500, and are reset to 0.

case 2: Alice and Bob

1. day 0, Alice deposits 100 GLP.
2. day 100, harvest() is called. Alice’s staked GLP has reward of 1,000 EsGmx. Alice did not claim the rewards. If call userAccrue(), producerTokens[pxGlp].userStates[Alice] will be 10,000, producerTokens[pxGlp][EsGmx] 1,000.
3. day 100, Bob deposits 100 GLP.
4. day 200, total reward is 2,000 GsGmx. Which means the 2nd 100 days, the rewards are 1,000 EsGmx, the reward rate is lower, but with more stakes from Bob.
5. Bob claims.
   producerTokens[pxGlp].userStates[Alice] is 20,000, producerTokens[pxGlp].userStates[Bob] is 100 pxGlp * 100 days = 10,000.
   producerTokens[pxGlp][EsGmx] is 2,000. globalRewards is the sum of them 30,000.
   The rewards Bob will get is 2,000 * 10,000 / 30,000 = 667 pxGmx.
6. Alice claims and receives 2,000 * 20,000 / 30,000 = 1,333 pxGmx.

Compared to case 1, Alice’s rewards is 167 pxGmx less. In fact, for the 2nd 100 days, total rewards is 1,000 EsGmx, Alice and Bob each deserves 500 pxGmx.

The reason is, GMX protocol rewards are not uniform for different periods. But the current pxGmx rewards rule is time based, variation of GMX protocol rewards rate is not accounted for, which creates inaccuracy for px rewards.

Malicious users could monitor GMX protocol rewards rate of high and low periods, stake and claim accordingly, effectively dilute or steal other px users’s rewards. Even worse, claim() can be called for other users. Malicious users could abuse this to dilute other users rewards. on the other hand, innocent users could lose some portion of their rewards.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• implement a fair and accurate rewards calculation system, might need to maintain checkpoints of user balance
• add access control for claim() in PirexRewards.sol

---

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

All reactions