Redacted Cartel contest - __141345__'s results

Lines of code

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

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

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L68 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L311-L315 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L394-L398 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L142-L144 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L164-L166

Vulnerability details

Impact

Although the PirexERC4626 and AutoPxGlp contract check for 0 shares, the rounding down error can still be used to steal new user deposit. Part of the new deposit could be stolen. The attacker may monitor the pool activities to catch the steal opportunities.

Part of the deposit fund from the 2nd user will be stolen by the attacker for new vault. Potentially the attacker can intentionally monitor the pool activities to find steal conditions.

Proof of Concept

The checks for 0 share are not enough to prevent the exchange rate manipulation, just slightly better.

File: src/vaults/PirexERC4626.sol
68:         require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

File: src/vaults/AutoPxGlp.sol
311:         if (
312:             (shares = supply == 0
313:                 ? assets
314:                 : assets.mulDivDown(supply, totalAssets() - assets)) == 0
315:         ) revert ZeroShares();

File: src/vaults/AutoPxGmx.sol
    function depositGmx(uint256 amount, address receiver) {
385:         (, uint256 assets, ) = PirexGmx(platform).depositGmx(
386:             amount,
387:             address(this)
388:         );

392:         uint256 supply = totalSupply;
393: 
394:         if (
395:             (shares = supply == 0
396:                 ? assets
397:                 : assets.mulDivDown(supply, totalAssets() - assets)) == 0
398:         ) revert ZeroShares();

File: src/PirexGmx.sol
    function depositGmx(uint256 amount, address receiver) {
388:         if (amount == 0) revert ZeroAmount();
389:         if (receiver == address(0)) revert ZeroAddress();

Take depositGlp() in AutoPxGlp.sol for example, assuming no fee. Others are analogous. An attacker will do the following:

1. watch the mempool to monitor new AutoPxGlp ERC4626 vault deployment.
2. make sure to be the first to depositGlp() in the pool.
3. when a new user tries to call depositGlp(), the attacker will front run it, transfer some PxGlp into the AutoPxGlp vault, not through depositGlp() function through AutoPxGlp.sol, but direct transfer with ERC20(GLP).transfer(address(vault), amount), in this way the totalAssets / shares ratio will be inflated.
4. after the manipulation, the new user will only get 1 wei of share, but the fund corresponding to the share will be half of the deposit amount.
5. attacker call withdraw()/redeem(), steal part of the user's deposit fund.

Take the asset Glp, the numbers for each steps would be:

1. watch mempool
2. attacker being the first deposit: 99 wei Glp, 99 wei share
3. user tries to deposit 1,000 Glp (1e21), so call depositGlp(Glp, 1e21, 1, 1, user). The attacker will front run to transfer (500 * 99) PxGlp into the AutoPxGlp vault directly. Now the vault has (495e20 + 99) PxGlp as totalAssets, 99 share, 1 share inflated to (5e20 + 1) Glp.
4. assets per share will be inflated. The call to AutoPxGlp#depositGlp(Glp, 1e21, 1, 1, user) -> PirexGmx(platform).depositGlp(Glp, 1e21, 1, 1, address(this)) will return assets of 1e21. Then in line 403 call _deposit(1e21, user), assets.mulDivDown(supply, totalAssets() - assets), 1e21 * 99 / (495e20 + 99) is just less than 2, so will round to 1 wei. As a results, the new user gets 1 share, the vault has (505e20 + 99) PxGlp, almost 50,500 PxGlp.
5. attacker withdraws the 99 share for about 49,995 Glp. The user end up with around 505 Glp, almost half is lost.

If the attacker does not have 49,500 PxGlp upfront, the steal still works, just the profit a little lower.

Although the condition of empty vault for this pattern seems not easy meet, if do it on purpose, the malicious user can still abuse the system in certain circumstances, in which the vector conditions are not so hard to meet. For instance in the following ways:

• monitor the new pool events and mempool, make sure to mint the first share as soon as possible.
• target the relatively small pools, by providing large supply to lower the return. As a result, other users might lose incentives to continue stay in the pool due to the low return and gradually withdraw their shares. Over a longer period, the attacker might have opportunities when the vault is empty again.

File: src/vaults/PirexERC4626.sol
68:         require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

178:     function previewDeposit(uint256 assets) {
184:         return convertToShares(assets);
185:     }

156:     function convertToShares(uint256 assets) {
162:         uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
163: 
164:         return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
165:     }


File: src\vaults\AutoPxGlp.sol
304:     function _deposit(uint256 assets, address receiver) {
309:         uint256 supply = totalSupply;
310: 
311:         if (
312:             (shares = supply == 0
313:                 ? assets
314:                 : assets.mulDivDown(supply, totalAssets() - assets)) == 0
315:         ) revert ZeroShares();
316: 
317:         _mint(receiver, shares);

322:     }

142:     function totalAssets() public view override returns (uint256) {
143:         return asset.balanceOf(address(this));
144:     }


File: src\vaults\AutoPxGmx.sol
164:     function totalAssets() public view override returns (uint256) {
165:         return asset.balanceOf(address(this));
166:     }

Tools Used

Manual analysis.

Recommended Mitigation Steps

• send a small share to address(0) for new vault. This is the most effective way.
• consider requiring minimum share to mint.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L179-L197 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L386-L415

Vulnerability details

Impact

The admin could call removeRewardToken() to change the rewardToken[] array. If before the reward token is added back, some user claims the rewards, the user could lose the rewards for the removed token.

And claim() function could be called by anyone, some users could lose rewards due to other users' operation. Malicious users could monitor admin's call to removeRewardToken(), and call claim() for other users right after. As a result, the malicious users effectively steal portion of the removed token from other users. Which could be claimed after added back.

Proof of Concept

Consider the following:

• pxGlp rewardTokens[] has 2 token, WETH and esGMX.
• The admin calls removeRewardToken(pxGlp, 1), then rewardTokens[] only has 1 token, WETH.
• Before the reward token is added back, Alice calls claim(pxGlp, Alice). But now the rewardTokens[] only has WETH.
• Alice's userRewards will be reset to 0 afterwards. The esGMX rewards will no longer be available even added back.

In this case, no esGMX will be received although px contract will get esGMX in harvest(). Those rewards will be left for the other users.

Even worse, since anyone can call claim(), Alice could lose the rewards if someone else called claim(pxGlp, Alice) right after the admin removed the token. Malicious user could abuse this to claim for other users to make them lose rewards. The lost rewards will accumulate until the reward token is added back. Effectively the malicious user can steal other users rewards portion.

rewardTokens[] array could remove token.

File: src/PirexRewards.sol
179:     function removeRewardToken(ERC20 producerToken, uint256 removalIndex)

185:         ERC20[] storage rewardTokens = producerTokens[producerToken]
186:             .rewardTokens;
187:         uint256 lastIndex = rewardTokens.length - 1;
188: 
189:         if (removalIndex != lastIndex) {
190:             // Set the element at removalIndex to the last element
191:             rewardTokens[removalIndex] = rewardTokens[lastIndex];
192:         }
193: 
194:         rewardTokens.pop();
195: 
196:         emit RemoveRewardToken(producerToken, removalIndex);
197:     }

The for loop won't do anything to the removed token, even the px contract received the reward. And user's rewards will be cleared.

373:     function claim(ERC20 producerToken, address user) external {

387:             uint256 rLen = rewardTokens.length;

390:             p.globalState.rewards = globalRewards - userRewards;
391:             p.userStates[user].rewards = 0;

396:             for (uint256 i; i < rLen; ++i) {
397:                 ERC20 rewardToken = rewardTokens[i];

409:                     producer.claimUserReward(
410:                         address(rewardToken),
411:                         amount,
412:                         recipient
413:                     );

415:             }

Tools Used

Manual analysis.

Recommended Mitigation Steps

• add access control for claim() in PirexRewards.sol
• if need to removeRewardToken(), force to distribute the related rewards before the token is deleted
• keep some record for unclaimed rewards

Variable re-arrangement by storage packing

In AutoPxGmx.sol, uint24 public poolFee can be placed next to address public platform, as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.

// src/vaults/AutoPxGmx.sol
26    uint24 public poolFee = 3000;
31    address public platform;

Reference: Layout of State Variables in Storage.

Duplicate call of `asset.balanceOf(address(this))

In totalAssets(), asset.balanceOf(address(this)) is already called, the result of totalAssets() - assetsBeforeClaim can be saved into local memory. A function call and storage sload can be saved. opcode sload which costs 100 gas, and function call has some extra overhead.

File: src/vaults/AutoPxGmx.sol
288:         if ((totalAssets() - assetsBeforeClaim) != 0) {
289:             totalFee =
290:                 ((asset.balanceOf(address(this)) - assetsBeforeClaim) *
291:                     platformFee) /
292:                 FEE_DENOMINATOR;

164:     function totalAssets() public view override returns (uint256) {
165:         return asset.balanceOf(address(this));
166:     }

Unnecessary assign of producerTokens[] and rewardTokens[]

pxGmx and pxGlp are all immutable, hence in claimRewards(), the returned producerTokens[] and rewardTokens[] will always be the same.

File: src/PirexGmx.sol
50:     ERC20 public immutable gmxBaseReward; // e.g. WETH (Ethereum)

55:     PxERC20 public immutable pxGmx;
56:     PxERC20 public immutable pxGlp;

739:     function claimRewards() {

749:         producerTokens = new ERC20[](4);
750:         rewardTokens = new ERC20[](4);

752:         producerTokens[0] = pxGmx;
753:         producerTokens[1] = pxGlp;
754:         producerTokens[2] = pxGmx;
755:         producerTokens[3] = pxGlp;
756:         rewardTokens[0] = gmxBaseReward;
757:         rewardTokens[1] = gmxBaseReward;
758:         rewardTokens[2] = ERC20(pxGmx); // esGMX rewards distributed as pxGMX
759:         rewardTokens[3] = ERC20(pxGmx);

Suggestion: Save producerTokens[] and rewardTokens[] to constants or immutable, and avoid re-assign value every time claimRewards() is called.

rewardTokens[] can be combined in harvest()

_producerTokens[] has 4 elements, but only 2 tokens, pxGmx and pxGlp, there are duplicates. rewardTokens[] is the same, has 4 elements, but only 2 tokens, gmxBaseReward and pxGmx. The same for rewardAmounts[]. The same token reward amounts can be combined. As a result, 2 storage sload for storage producerState can be saved, each for 100 gas. And in extra 2 calls for _globalAccrue(producerState.globalState, p), there are several storage reads and writes, each for 100 gas.

File: src/PirexRewards.sol
338:     function harvest() {
346:         (_producerTokens, rewardTokens, rewardAmounts) = producer
347:             .claimRewards();
348:         uint256 pLen = _producerTokens.length;
349: 
350:         // Iterate over the producer tokens and update reward state
351:         for (uint256 i; i < pLen; ++i) {
352:             ERC20 p = _producerTokens[i];
353:             uint256 r = rewardAmounts[i];
355:             // Update global reward accrual state and associate with the update of reward state
356:             ProducerToken storage producerState = producerTokens[p];
357: 
358:             _globalAccrue(producerState.globalState, p);
359: 
360:             if (r != 0) {
361:                 producerState.rewardStates[rewardTokens[i]] += r;
362:             }
363:         }