Redacted Cartel contest - xiaoming90's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L330 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L422 https://github.com/gmx-io/gmx-contracts/blob/516c78624bc7f6e10096f49ce05984990ade2ab0/contracts/staking/StakedGlp.sol#L86

Vulnerability details

Proof of Concept

Users call the AutoPxGlp.depositFsGlp function of the auto-compounding vault to deposit their staked GLP (fsGLP) tokens in exchange for apxGPL tokens. Within this function:

• In Line 344 - The caller's staked GLP (fsGLP) tokens will be transferred from the caller into the vault.
• In Line 349 - The vault will call the PirexGmx(platform).depositFsGlp function to deposit the received staked GLP (fsGLP) tokens to the PirexGmx contract to mint pxGLP tokens.

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L330

File: AutoPxGlp.sol
330:     function depositFsGlp(uint256 amount, address receiver)
331:         external
332:         nonReentrant
333:         returns (uint256)
334:     {
335:         if (amount == 0) revert ZeroAmount();
336:         if (receiver == address(0)) revert ZeroAddress();
337: 
338:         if (totalAssets() != 0) beforeDeposit(address(0), 0, 0);
339: 
340:         ERC20 stakedGlp = ERC20(address(PirexGmx(platform).stakedGlp()));
341: 
342:         // Transfer fsGLP from the caller to the vault
343:         // before approving PirexGmx to proceed with the deposit
344:         stakedGlp.safeTransferFrom(msg.sender, address(this), amount);
345: 
346:         // Approve as needed here since the stakedGlp address is mutable in PirexGmx
347:         stakedGlp.safeApprove(platform, amount);
348: 
349:         (, uint256 assets, ) = PirexGmx(platform).depositFsGlp(
350:             amount,
351:             address(this)
352:         );

Within the PirexGmx.depositFsGlp function, the stakedGlp.transferFrom function will be called at Line 436 to transfer the staked GLP (fsGLP) tokens from the vault (caller) to the PirexGmx contract.

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

File: PirexGmx.sol
422:     function depositFsGlp(uint256 amount, address receiver)
423:         external
424:         whenNotPaused
425:         nonReentrant
426:         returns (
427:             uint256,
428:             uint256,
429:             uint256
430:         )
431:     {
432:         if (amount == 0) revert ZeroAmount();
433:         if (receiver == address(0)) revert ZeroAddress();
434: 
435:         // Transfer the caller's fsGLP (unstaked for the user, staked for this contract)
436:         stakedGlp.transferFrom(msg.sender, address(this), amount);

However, by inspecting the GMX's stakedGlp contract, it was observed that there is a cooldown duration for the transfer of staked GLP (fsGLP) tokens, as shown below. Currently, the glpManager.cooldownDuration() is configured to 900 seconds or 15 minutes.

https://github.com/gmx-io/gmx-contracts/blob/516c78624bc7f6e10096f49ce05984990ade2ab0/contracts/staking/StakedGlp.sol#L86

    function _transfer(address _sender, address _recipient, uint256 _amount) private {
        require(_sender != address(0), "StakedGlp: transfer from the zero address");
        require(_recipient != address(0), "StakedGlp: transfer to the zero address");

        require(
            glpManager.lastAddedAt(_sender).add(glpManager.cooldownDuration()) <= block.timestamp,
            "StakedGlp: cooldown duration not yet passed"
        );

        IRewardTracker(stakedGlpTracker).unstakeForAccount(_sender, feeGlpTracker, _amount, _sender);
        IRewardTracker(feeGlpTracker).unstakeForAccount(_sender, glp, _amount, _sender);

        IRewardTracker(feeGlpTracker).stakeForAccount(_sender, _recipient, glp, _amount);
        IRewardTracker(stakedGlpTracker).stakeForAccount(_recipient, _recipient, feeGlpTracker, _amount);
    }

This means there is a 15 min cooldown duration after minting GLP. This amount of time needs to pass before stakedGlp.transferFrom can be called for their account. Refer to https://gmxio.gitbook.io/gmx/contracts#transferring-staked-glp

Impact

A malicious user can perform a denial-of-service attack against the vault by calling AutoPxGlp.depositGlp function to deposit a dust amount of tokens to mint new GLP tokens every 15 minutes to reset the cooldown timer. As a result, the stakedGlp.transferFrom will revert every time it is called because the cooldown duration has not yet passed.

AutoPxGlp.depositFsGlp function depends on PirexGmx.depositFsGlp function. PirexGmx.depositFsGlp function depends on the stakedGlp.transferFrom function to transfer the staked GLP from the caller account to the vault account. Since stakedGlp.transferFrom function reverts, no one could call the AutoPxGlp.depositFsGlp of the vault to deposit their staked GLP (fsGLP) tokens into the vault. Thus, this feature is essentially broken.

Additionally, the gas fee is much cheaper on Arbitrum or Avalanche compared to Ethereum, thus making this attack easier to execute.

Recommended Mitigation Steps

Reduce the impact of this issue by introducing a cooldown duration to ensure that the AutoPxGlp.depositGlp function can only be called by the same user once every few minutes (e.g. 15 minutes or 60 minutes).

Alternatively, discuss with the GMX's protocol team the removal of the 15-minute cooldown duration for the Redacted Protocol before launch. There has been discussion about the removal of the 15-minute cooldown duration by GMX's protocol team in the coming weeks. However, during the C4's audit period (From November 21, 2022 to November 28, 2022), the 15-minute cooldown duration is still enforced.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L305 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L281 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L373

Vulnerability details

Background

The amount of rewards accrued by global and user states is computed by the following steps:

1. Calculate seconds elapsed since the last update (block.timestamp - lastUpdate)
2. Calculate the new rewards by multiplying seconds elapsed by the last supply ((block.timestamp - lastUpdate) * lastSupply)
3. Append the new rewards to the existing rewards (rewards = rewards + (block.timestamp - lastUpdate) * lastSupply)

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

/**
    @notice Update global accrual state
    @param  globalState    GlobalState  Global state of the producer token
    @param  producerToken  ERC20        Producer token contract
*/
function _globalAccrue(GlobalState storage globalState, ERC20 producerToken)
	internal
{
    uint256 totalSupply = producerToken.totalSupply();
    uint256 lastUpdate = globalState.lastUpdate;
    uint256 lastSupply = globalState.lastSupply;

    // Calculate rewards, the product of seconds elapsed and last supply
    // Only calculate and update states when needed
    if (block.timestamp != lastUpdate || totalSupply != lastSupply) {
        uint256 rewards = globalState.rewards +
            (block.timestamp - lastUpdate) *
            lastSupply;
            
            globalState.lastUpdate = block.timestamp.safeCastTo32();
            globalState.lastSupply = totalSupply.safeCastTo224();
            globalState.rewards = rewards;
   	..SNIP..
}

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

/**
    @notice Update user rewards accrual state
    @param  producerToken  ERC20    Rewards-producing token
    @param  user           address  User address
*/
function userAccrue(ERC20 producerToken, address user) public {
    if (address(producerToken) == address(0)) revert ZeroAddress();
    if (user == address(0)) revert ZeroAddress();

    UserState storage u = producerTokens[producerToken].userStates[user];
    uint256 balance = producerToken.balanceOf(user);

    // Calculate the amount of rewards accrued by the user up to this call
    uint256 rewards = u.rewards +
    u.lastBalance *
    (block.timestamp - u.lastUpdate);
    
    u.lastUpdate = block.timestamp.safeCastTo32();
    u.lastBalance = balance.safeCastTo224();
    u.rewards = rewards;
    ..SNIP..
}

When a user claims the rewards, the number of reward tokens the user is entitled to is equal to the rewardState scaled by the ratio of the userRewards to the globalRewards. Refer to Line 403 below.

The rewardState represents the total number of a specific ERC20 reward token (e.g. WETH or esGMX) held by a producer (e.g. pxGMX or pxGPL).

The rewardState of each reward token (e.g. WETH or esGMX) will increase whenever the rewards are harvested by the producer (e.g. PirexRewards.harvest is called). On the other hand, the rewardState will decrease if the users claim the rewards.

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

File: PirexRewards.sol
373:     function claim(ERC20 producerToken, address user) external {
..SNIP..
395:             // Transfer the proportionate reward token amounts to the recipient
396:             for (uint256 i; i < rLen; ++i) {
397:                 ERC20 rewardToken = rewardTokens[i];
398:                 address rewardRecipient = p.rewardRecipients[user][rewardToken];
399:                 address recipient = rewardRecipient != address(0)
400:                     ? rewardRecipient
401:                     : user;
402:                 uint256 rewardState = p.rewardStates[rewardToken];
403:                 uint256 amount = (rewardState * userRewards) / globalRewards;
..SNIP..
417:     }

How reward tokens are distributed

The Multiplier Point (MP) effect will be ignored for simplicity. Assume that the emission rate is constant throughout the entire period (from T80 to T84) and the emission rate is 1 esGMX per 1 GMX staked per second.

The graph below represents the amount of GMX tokens Alice and Bob staked for each second during the period.

A = Alice and B = Bob; each block represents 1 GMX token staked.

Based on the above graph:

• Alice staked 1 GMX token from T80 to T84. Alice will earn five (5) esGMX tokens at the end of T84.
• Bob staked 4 GMX tokens from T83 to T84. Bob will earn eight (8) esGMX tokens at the end of T84.
• A total of 13 esGMX will be harvested by PirexRewards contract at the end of T84

The existing reward distribution design in the PirexRewards contract will work perfectly if the emission rate is constant, similar to the example above.

In this case, the state variable will be as follows at the end of T84, assuming both the global and all user states have been updated and rewards have been harvested.

• rewardState = 13 esGMX tokens (5 + 8)
• globalRewards = 13
• Accrued userRewards of Alice = 5
• Accrued userRewards of Bob = 8

When Alice calls the PirexRewards.claim function to claim her rewards at the end of T84, she will get back five (5) esGMX tokens, which is correct.

(rewardState * userRewards) / globalRewards
(13 * 5) / 13 = 5

Proof of Concept

However, the fact is that the emission rate of reward tokens (e.g. esGMX or WETH) is not constant. Instead, the emission rate is dynamic and depends on various factors, such as the following:

• The number of rewards tokens allocated by GMX governance for each month. Refer to https://gov.gmx.io/t/esgmx-emissions/272. In some months, the number of esGMX emissions will be higher.
• The number of GMX/GLP tokens staked by the community. The more tokens being staked by the community users, the more diluted the rewards will be.

The graph below represents the amount of GMX tokens Alice and Bob staked for each second during the period.

A = Alice and B = Bob; each block represents 1 GMX token staked.

The Multiplier Point (MP) effect will be ignored for simplicity. Assume that the emission rate is as follows:

• From T80 to 82: 2 esGMX per 1 GMX staked per second (Higher emission rate)
• From T83 to 84: 1 esGMX per 1 GMX staked per second (Lower emission rate)

By manually computing the amount of esGMX reward tokens that Alice is entitled to at the end of T84:

[1 staked GMX * (T82 - T80) * 2esGMX/sec] + [1 staked GMX * (T84 - T83) * 1esGMX/sec]
[1 staked GMX * 3 secs * 2esGMX/sec] + [1 staked GMX * 2secs * 1esGMX/sec]
6 + 2 = 8

Alice will be entitled to 8 esGMX reward tokens at the end of T84.

By manually computing the amount of esGMX reward tokens that Bob is entitled to at the end of T84:

[4 staked GMX * 2secs * 1esGMX/sec] = 8

Bob will be entitled to 8 esGMX reward tokens at the end of T84.

However, the existing reward distribution design in the PirexRewards contract will cause Alice to get fewer reward tokens than she is entitled to and cause Bob to get more rewards than he is entitled to.

The state variable will be as follows at the end of T84, assuming both the global and all user states have been updated and rewards have been harvested.

• rewardState = 16 esGMX tokens (8 + 8)
• globalRewards = 13
• Accrued userRewards of Alice = 5
• Accrued userRewards of Bob = 8

When Alice calls the PirexRewards.claim function to claim her rewards at the end of T84, she will only get back six (6) esGMX tokens, which is less than eight (8) esGMX tokens she is entitled to or earned.

(rewardState * userRewards) / globalRewards
(16 * 5) / 13 = 6.15 = 6

When Bob calls the PirexRewards.claim function to claim his rewards at the end of T84, he will get back nine (9) esGMX tokens, which is more than eight (8) esGMX tokens he is entitled to or earned.

(rewardState * userRewards) / globalRewards
(16 * 8) / 13 = 9.85 = 9

Impact

As shown in the PoC, some users will lose their reward tokens due to the miscalculation within the existing reward distribution design.

Recommended Mitigation Steps

Update the existing reward distribution design to handle the dynamic emission rate. Implement the RewardPerToken for users and global, as seen in many of the well-established reward contracts below, which is not vulnerable to this issue:

• https://github.com/fei-protocol/flywheel-v2/blob/dbe3cb81a3dc2e46536bb8af9c2bdc585f63425e/src/FlywheelCore.sol#L226
• https://github.com/Synthetixio/synthetix/blob/2cb4b23fe409af526de67dfbb84aae84b2b13747/contracts/StakingRewards.sol#L61

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L156 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L199 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315

Vulnerability details

Proof of Concept

Note: This issue affects both the AutoPxGmx and AutoPxGlp vaults. Since the root cause is the same, the PoC of AutoPxGlp vault is omitted for brevity.

The PirexERC4626.convertToShares function relies on the mulDivDown function in Line 164 when calculating the number of shares needed in exchange for a certain number of assets. Note that the computation is rounded down, therefore, if the result is less than 1 (e.g. 0.9), Solidity will round them down to zero. Thus, it is possible that this function will return zero.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L156

File: PirexERC4626.sol
156:     function convertToShares(uint256 assets)
157:         public
158:         view
159:         virtual
160:         returns (uint256)
161:     {
162:         uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
163: 
164:         return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
165:     }

The AutoPxGmx.previewWithdraw function relies on the PirexERC4626.convertToShares function in Line 206. Thus, this function will also "round down".

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L199

File: AutoPxGmx.sol
199:     function previewWithdraw(uint256 assets)
200:         public
201:         view
202:         override
203:         returns (uint256)
204:     {
205:         // Calculate shares based on the specified assets' proportion of the pool
206:         uint256 shares = convertToShares(assets);
207: 
208:         // Save 1 SLOAD
209:         uint256 _totalSupply = totalSupply;
210: 
211:         // Factor in additional shares to fulfill withdrawal if user is not the last to withdraw
212:         return
213:             (_totalSupply == 0 || _totalSupply - shares == 0)
214:                 ? shares
215:                 : (shares * FEE_DENOMINATOR) /
216:                     (FEE_DENOMINATOR - withdrawalPenalty);
217:     }

The AutoPxGmx.withdraw function relies on the AutoPxGmx.previewWithdraw function. In certain conditions, the AutoPxGmx.previewWithdraw function in Line 323 will return zero if the withdrawal amount causes the division within the PirexERC4626.convertToShares function to round down to zero (usually due to a small amount of withdrawal amount).

If the AutoPxGmx.previewWithdraw function in Line 323 returns zero, no shares will be burned at Line 332. Subsequently, in Line 336, the contract will transfer the assets to the users. As a result, the users receive the assets without burning any of their shares, effectively allowing them to receive assets for free.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315

File: AutoPxGmx.sol
315:     function withdraw(
316:         uint256 assets,
317:         address receiver,
318:         address owner
319:     ) public override returns (uint256 shares) {
320:         // Compound rewards and ensure they are properly accounted for prior to withdrawal calculation
321:         compound(poolFee, 1, 0, true);
322:         
323:         shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.
324: 
325:         if (msg.sender != owner) {
326:             uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.
327: 
328:             if (allowed != type(uint256).max)
329:                 allowance[owner][msg.sender] = allowed - shares;
330:         }
331: 
332:         _burn(owner, shares);
333: 
334:         emit Withdraw(msg.sender, receiver, owner, assets, shares);
335: 
336:         asset.safeTransfer(receiver, assets);
337:     }

Assume that the vault with the following state:

• Total Asset = 1000 WETH
• Total Supply = 10 shares

Assume that Alice wants to withdraw 99 WETH from the vault. Thus, she calls the AutoPxGmx.withdraw(99 WETH) function.

The PirexERC4626.convertToShares function will compute the number of shares that Alice needs to burn in exchange for 99 WETH.

assets.mulDivDown(supply, totalAssets())
99WETH.mulDivDown(10 shares, 1000WETH)
(99 * 10) / 1000
990 / 1000 = 0.99 = 0

However, since Solidity round 0.99 down to 0, Alice does not need to burn a single share. She will receive 99 WETH for free.

Impact

Malicious users can withdraw the assets from the vault for free, effectively allowing them to drain the assets of the vault.

Recommended Mitigation Steps

Ensure that at least 1 share is burned when the users withdraw their assets.

This can be mitigated by updating the previewWithdraw function to round up instead of round down when computing the number of shares to be burned.

function previewWithdraw(uint256 assets)
	public
	view
	override
	returns (uint256)
{
	// Calculate shares based on the specified assets' proportion of the pool
-	uint256 shares = convertToShares(assets);
+	uint256 shares = supply == 0 ? assets : assets.mulDivUp(supply, totalAssets());
	
	// Save 1 SLOAD
	uint256 _totalSupply = totalSupply;

	// Factor in additional shares to fulfill withdrawal if user is not the last to withdraw
	return
		(_totalSupply == 0 || _totalSupply - shares == 0)
			? shares
			: (shares * FEE_DENOMINATOR) /
				(FEE_DENOMINATOR - withdrawalPenalty);
}

Lines of code

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

Vulnerability details

Proof of Concept

If the user deposits too little GMX compared to other users (or total supply of pxGMX), the user will not be able to receive rewards after calling the PirexRewards.claim function. Subsequently, their accrued rewards will be cleared out (set to zero), and they will lose their rewards.

The amount of reward tokens that are claimable by a user is computed in Line 403 of the PirexRewards.claim function.

If the balance of pxGMX of a user is too small compared to other users (or total supply of pxGMX), the code below will always return zero due to rounding issues within solidity.

uint256 amount = (rewardState * userRewards) / globalRewards;

Since the user's accrued rewards is cleared at Line 391 within the PirexRewards.claim function (p.userStates[user].rewards = 0;), the user's accrued rewards will be lost.

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

File: PirexRewards.sol
368:     /**
369:         @notice Claim rewards
370:         @param  producerToken  ERC20    Producer token contract
371:         @param  user           address  User
372:     */
373:     function claim(ERC20 producerToken, address user) external {
374:         if (address(producerToken) == address(0)) revert ZeroAddress();
375:         if (user == address(0)) revert ZeroAddress();
376: 
377:         harvest();
378:         userAccrue(producerToken, user);
379: 
380:         ProducerToken storage p = producerTokens[producerToken];
381:         uint256 globalRewards = p.globalState.rewards;
382:         uint256 userRewards = p.userStates[user].rewards;
383: 
384:         // Claim should be skipped and not reverted on zero global/user reward
385:         if (globalRewards != 0 && userRewards != 0) {
386:             ERC20[] memory rewardTokens = p.rewardTokens;
387:             uint256 rLen = rewardTokens.length;
388: 
389:             // Update global and user reward states to reflect the claim
390:             p.globalState.rewards = globalRewards - userRewards;
391:             p.userStates[user].rewards = 0;
392: 
393:             emit Claim(producerToken, user);
394: 
395:             // Transfer the proportionate reward token amounts to the recipient
396:             for (uint256 i; i < rLen; ++i) {
397:                 ERC20 rewardToken = rewardTokens[i];
398:                 address rewardRecipient = p.rewardRecipients[user][rewardToken];
399:                 address recipient = rewardRecipient != address(0)
400:                     ? rewardRecipient
401:                     : user;
402:                 uint256 rewardState = p.rewardStates[rewardToken];
403:                 uint256 amount = (rewardState * userRewards) / globalRewards;
404: 
405:                 if (amount != 0) {
406:                     // Update reward state (i.e. amount) to reflect reward tokens transferred out
407:                     p.rewardStates[rewardToken] = rewardState - amount;
408: 
409:                     producer.claimUserReward(
410:                         address(rewardToken),
411:                         amount,
412:                         recipient
413:                     );
414:                 }
415:             }
416:         }
417:     }

The graph below represents the amount of GMX tokens Alice and Bob staked in PirexGmx for each second during the period. Note that the graph is not drawn proportionally.

Green = Number of GMX tokens staked by Alice

Blue = Number of GMX tokens staked by Bob

Based on the above graph:

• Alice staked 1 GMX token for 4 seconds (From T80 to T85)
• Bob staked 99999 GMX tokens for 4 seconds (From T80 to T85)

Assume that the emission rate is 0.1 esGMX per 1 GMX staked per second.

In this case, the state variable will be as follows at the end of T83, assuming both the global and all user states have been updated and rewards have been harvested.

• rewardState = 60,000 esGMX tokens (600,000 * 0.1)
• globalRewards = 600,000 (100,000 * 6)
• Accrued userRewards of Alice = 6
• Accrued userRewards of Bob = 599,994 (99,999 * 6)

Following is the description of rewardState for reference:

The rewardState represents the total number of a specific ERC20 reward token (e.g. WETH or esGMX) held by a producer (e.g. pxGMX or pxGPL).

The rewardState of each reward token (e.g. WETH or esGMX) will increase whenever the rewards are harvested by the producer (e.g. PirexRewards.harvest is called). On the other hand, the rewardState will decrease if the users claim the rewards.

At the end of T85, Alice should be entitled to 1.2 esGMX tokens (0.2/sec * 6).

Following is the formula used in the PirexRewards contract to compute the number of reward tokens a user is entitled to.

amount = (rewardState * userRewards) / globalRewards;

If Alice claims the rewards at the end of T85, she will get zero esGMX tokens instead of 1.2 esGMX tokens.

amount = (rewardState * userRewards) / globalRewards;
60,000 * 6 / 600,000
360,000/600,000 = 0.6 = 0

Since Alice's accrued rewards are cleared at Line 391 within the PirexRewards.claim function (p.userStates[user].rewards = 0;), Alice's accrued rewards will be lost. Alice will have to start accruing the rewards from zero after calling the PirexRewards.claim function.

Another side effect is that since the 1.2 esGMX tokens that belong to Alice are still in the contract, they will be claimed by other users.

Impact

Users who deposit too little GMX compared to other users (or total supply of pxGMX), the user will not be able to receive rewards after calling the PirexRewards.claim function. Also, their accrued rewards will be cleared out (set to zero). Loss of reward tokens for the users.

Additionally, the PirexRewards.claim function is permissionless, and anyone can trigger the claim on behalf of any user. A malicious user could call the PirexRewards.claim function on behalf of a victim at the right time when the victim's accrued reward is small enough to cause a rounding error or precision loss, thus causing the victim accrued reward to be cleared out (set to zero).

Recommended Mitigation Steps

Following are some of the possible remediation actions:

1. Use RewardPerToken approach

Avoid calculating the rewards that the users are entitled based on the ratio of userRewards and globalRewards.

Instead, consider implementing the RewardPerToken for users and global, as seen in many of the well-established reward contracts below, which is not vulnerable to this issue:

• https://github.com/fei-protocol/flywheel-v2/blob/dbe3cb81a3dc2e46536bb8af9c2bdc585f63425e/src/FlywheelCore.sol#L226
• https://github.com/Synthetixio/synthetix/blob/2cb4b23fe409af526de67dfbb84aae84b2b13747/contracts/StakingRewards.sol#L61

2. Fallback logic ifamount == 0

If the amount is zero, revert the transaction. Alternatively, if the amount is zero, do not clear out the user's accrued reward state variable since the user did not receive anything yet.

function claim(ERC20 producerToken, address user) external {
..SNIP..
			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
				);
-			}
+			} else {
+				revert ZeroRewardTokens();
+			}
..SNIP..
}

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L60 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L178 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L156

Vulnerability details

Proof of Concept

Note: This issue affects both the AutoPxGmx and AutoPxGlp vaults. Since the root cause is the same, the PoC of AutoPxGlp vault is omitted for brevity.

A well-known attack vector for almost all shares-based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L60

File: PirexERC4626.sol
60:     function deposit(uint256 assets, address receiver)
61:         public
62:         virtual
63:         returns (uint256 shares)
64:     {
65:         if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);
66: 
67:         // Check for rounding error since we round down in previewDeposit.
68:         require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
69: 
70:         // Need to transfer before minting or ERC777s could reenter.
71:         asset.safeTransferFrom(msg.sender, address(this), assets);
72: 
73:         _mint(receiver, shares);
74: 
75:         emit Deposit(msg.sender, receiver, assets, shares);
76: 
77:         afterDeposit(receiver, assets, shares);
78:     }

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L178

File: PirexERC4626.sol
178:     function previewDeposit(uint256 assets)
179:         public
180:         view
181:         virtual
182:         returns (uint256)
183:     {
184:         return convertToShares(assets);
185:     }

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L156

File: PirexERC4626.sol
156:     function convertToShares(uint256 assets)
157:         public
158:         view
159:         virtual
160:         returns (uint256)
161:     {
162:         uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
163: 
164:         return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
165:     }

The vault share minting formula is taken from Line 164 in the above convertToShares function:

supply == 0 ? assets : assets.mulDivDown(supply, totalAssets())

This can be simplified into the following in a layman's term:

if (supply == 0)
Then shares = assets
Else shares = (assets * supply) / totalAssets

If an attacker, who is the first depositor, deposits 1 pxGMX, he will receive 1 apxGMX share. At this point, the totalAssets is 1 and supply is 1.

The attacker obtains 9999 pxGMX can be obtained from the open market (e.g. via PirexGMX). He proceeds to do a direct transfer of 9999 pxGMX to the vault. At this point, the totalAssets is 10000 and supply is 1.

If assets >= totalAssets

The following describes a scenario in which a user's assets are lost and stolen by an attacker. Assume that Alice deposits 19999 pxGMX. Based on the formula, Alice will only receive 1 apxGLP share. She immediately loses 9999 pxGMX or half of her assets if she exits the vault or redeems the share right after the deposit.

shares = (assets * supply) / totalAssets
shares = (19999 * 1) / 10000 = 1

If the attacker exits the vault right after Alice's deposit, the attacker will receive 14999 pxGMX. He profited 4999 pxGMX from this attack

assets = (shares * totalAssets) / supply
bptReceived = (1 * 29999) / 2 = 14999

Impact

The attacker can profit from future users' deposits, while the late users will lose part of their funds to the attacker.

Recommended Mitigation Steps

Consider requiring a minimal amount of apxGMX shares to be minted for the first minter, and send a portion of the initial mints as a reserve to the Reacted Treasury so that the pricePerShare can be more resistant to manipulation.

How Uniswap mitigates the issue?

This is a known issue that has been mitigated in popular Uniswap. When providing the initial liquidity to the contract (i.e. when totalSupply is 0), the liquidity provider must sacrifice 1000 LP tokens (by sending them to address(0)). By doing so, we can ensure the granularity of the LP tokens is always at least 1000 and the malicious actor is not the sole holder. This approach may bring an additional cost for the initial liquidity provider, but this cost is expected to be low and acceptable.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L26 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242

Vulnerability details

Proof of Concept

Uniswap v3 offers LPs three separate fee tiers per pair — 0.05%, 0.30%, and 1.00%. Based on the existing setting in the contract, the default pool tier fee being used is 0.30%.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L26

File: AutoPxGmx.sol
25:     // Uniswap pool fee
26:     uint24 public poolFee = 3000;

The AutoPxGmx.compound function is permissionless and can be called by anyone. Callers are also allowed to configure the Uniswap pool tier fee via the fee parameter.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242

File: AutoPxGmx.sol
242:     function compound(
243:         uint24 fee,
244:         uint256 amountOutMinimum,
245:         uint160 sqrtPriceLimitX96,
246:         bool optOutIncentive
247:     )

When the AutoPxGmx.compound function is called, the function will swap the received WETH rewards for GMX tokens via Uniswap. The GMX tokens will then be deposited into the PirexGMX contract to obtain more pxGMX tokens for the vault to achieve the auto-compounding effect.

Assume that Bob is a liquidity provider of the ETH / GMX 1% Uniswap Pool. He could benefit himself and other LPs within the ETH/GMX 1% pool by having all trades of the compounding be done against their ETH/GMX 1% pool and earn a 1% transaction fee from the vault, attempting to extract value from the vault.

Impact

The malicious caller will not be incentivized to find the optimal trade option. The liquidity providers of the ETH / GMX 1% Uniswap Pool will end up "fighting/front-running" to force the vault to perform the "compound" trades against their pool so as to earn its transaction fee instead of helping the vault shareholders to find the most optimal trade with least slippage to maximum the value of the assets within the vault. Loss of assets for the vault shareholders due to non-optimal trade performed by bad actors.

Recommended Mitigation Steps

Do not allow the caller of AutoPxGmx.compound to configure the pool tier fee. The pool tier fee to be used during swaps should only be configured by the DAO/admin.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L222 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L339 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242

Vulnerability details

Proof of Concept

The compound function is triggered whenever someone deposit, withdraw, or redeem from the vault.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L222

File: AutoPxGmx.sol
219:     /**
220:         @notice Compound pxGMX rewards before depositing
221:      */
222:     function beforeDeposit(
223:         address,
224:         uint256,
225:         uint256
226:     ) internal override {
227:         compound(poolFee, 1, 0, true);
228:     }

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315

File: AutoPxGmx.sol
315:     function withdraw(
316:         uint256 assets,
317:         address receiver,
318:         address owner
319:     ) public override returns (uint256 shares) {
320:         // Compound rewards and ensure they are properly accounted for prior to withdrawal calculation
321:         compound(poolFee, 1, 0, true);

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L339

File: AutoPxGmx.sol
339:     function redeem(
340:         uint256 shares,
341:         address receiver,
342:         address owner
343:     ) public override returns (uint256 assets) {
344:         // Compound rewards and ensure they are properly accounted for prior to redemption calculation
345:         compound(poolFee, 1, 0, true);

It was observed that the compound function is called within the deposit, withdraw, or redeem functions with the amountOutMinimum parameter set to 1. Setting it to 1 effectively means that slippage control is almost non-existent.

Within the compound function, the function will attempt to sell the newly claimed reward tokens (e.g. pxGMX) for GMX tokens via a Uniswap liquidity pool seeded by the protocol team before depositing them back to the PirexGmx contract. Refer to lines 267-284 below.

Since slippage control is almost non-existent, the trade will happen even if the trade suffers extremely bad slippage. As a result, this causes the transaction to be vulnerable to a classic sandwich attack.

Assume that a compound transaction is in the mempool. An attacker can front-run the compound transaction and place an order to perform a trade on the liquidity pool, buying up GMX tokens knowing that the GMX's price will increase later. Next, the victim (vault) will purchase GMX tokens at a higher value, suffering significant slippage. The attacker will then back-run and sell GMX tokens at a higher price afterward, profiting from the difference.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242

File: AutoPxGmx.sol
230:     /**
231:         @notice Compound pxGMX rewards
232:         @param  fee                    uint24   Uniswap pool tier fee
233:         @param  amountOutMinimum       uint256  Outbound token swap amount
234:         @param  sqrtPriceLimitX96      uint160  Swap price impact limit (optional)
235:         @param  optOutIncentive        bool     Whether to opt out of the incentive
236:         @return gmxBaseRewardAmountIn  uint256  GMX base reward inbound swap amount
237:         @return gmxAmountOut           uint256  GMX outbound swap amount
238:         @return pxGmxMintAmount        uint256  pxGMX minted when depositing GMX
239:         @return totalFee               uint256  Total platform fee
240:         @return incentive              uint256  Compound incentive
241:      */
242:     function compound(
243:         uint24 fee,
244:         uint256 amountOutMinimum,
245:         uint160 sqrtPriceLimitX96,
246:         bool optOutIncentive
247:     )
248:         public
249:         returns (
250:             uint256 gmxBaseRewardAmountIn,
251:             uint256 gmxAmountOut,
252:             uint256 pxGmxMintAmount,
253:             uint256 totalFee,
254:             uint256 incentive
255:         )
256:     {
257:         if (fee == 0) revert InvalidParam();
258:         if (amountOutMinimum == 0) revert InvalidParam();
259: 
260:         uint256 assetsBeforeClaim = asset.balanceOf(address(this));
261: 
262:         PirexRewards(rewardsModule).claim(asset, address(this));
263: 
264:         // Swap entire reward balance for GMX
265:         gmxBaseRewardAmountIn = gmxBaseReward.balanceOf(address(this));
266: 
267:         if (gmxBaseRewardAmountIn != 0) {
268:             gmxAmountOut = SWAP_ROUTER.exactInputSingle(
269:                 IV3SwapRouter.ExactInputSingleParams({
270:                     tokenIn: address(gmxBaseReward),
271:                     tokenOut: address(gmx),
272:                     fee: fee,
273:                     recipient: address(this),
274:                     amountIn: gmxBaseRewardAmountIn,
275:                     amountOutMinimum: amountOutMinimum,
276:                     sqrtPriceLimitX96: sqrtPriceLimitX96
277:                 })
278:             );
279: 
280:             // Deposit entire GMX balance for pxGMX, increasing the asset/share amount
281:             (, pxGmxMintAmount, ) = PirexGmx(platform).depositGmx(
282:                 gmx.balanceOf(address(this)),
283:                 address(this)
284:             );
285:         }

Impact

The vault frequently interacts with the liquidity pool during withdrawal, redemption, and deposit. However, the compound transaction is configured with a non-existent slippage rate (e.g. 1).

Given the fact that there are a lot of MEV searchers, performing swaps with a non-existent slippage rate puts user funds in danger. The value of the rewards, which belong to the users, will be extracted by MEV searchers or malicious users.

Recommended Mitigation Steps

The slippage rate should not be configurable by anyone. Configure the swap with a reasonable slippage rate (e.g. 3% or 5%) defined by the admin/DAO, so the transaction will revert if the trade suffers significant slippage.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L73 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L152

Vulnerability details

Proof of Concept

During initialization, the AutoPxGMX vault will grant max allowance to the platform (PirexGMX) to spend its GMX tokens in Line 97 of the constructor method below. This is required because the vault needs to deposit GMX tokens to the platform (PirexGMX) contract. During deposit, the platform (PirexGMX) contract will pull the GMX tokens within the vault and send them to GMX protocol for staking. Otherwise, the deposit feature within the vault will not work.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L73

File: AutoPxGmx.sol
73:     constructor(
74:         address _gmxBaseReward,
75:         address _gmx,
76:         address _asset,
77:         string memory _name,
78:         string memory _symbol,
79:         address _platform,
80:         address _rewardsModule
81:     ) Owned(msg.sender) PirexERC4626(ERC20(_asset), _name, _symbol) {
82:         if (_gmxBaseReward == address(0)) revert ZeroAddress();
83:         if (_gmx == address(0)) revert ZeroAddress();
84:         if (_asset == address(0)) revert ZeroAddress();
85:         if (bytes(_name).length == 0) revert InvalidAssetParam();
86:         if (bytes(_symbol).length == 0) revert InvalidAssetParam();
87:         if (_platform == address(0)) revert ZeroAddress();
88:         if (_rewardsModule == address(0)) revert ZeroAddress();
89: 
90:         gmxBaseReward = ERC20(_gmxBaseReward);
91:         gmx = ERC20(_gmx);
92:         platform = _platform;
93:         rewardsModule = _rewardsModule;
94: 
95:         // Approve the Uniswap V3 router to manage our base reward (inbound swap token)
96:         gmxBaseReward.safeApprove(address(SWAP_ROUTER), type(uint256).max);
97:         gmx.safeApprove(_platform, type(uint256).max);
98:     }

However, when the owner calls the AutoPxGmx.setPlatform function to update the platform to a new address, it does not grant any allowance to the new platform address. As a result, the new platform (PirexGMX) will not be able to pull the GMX tokens from the vault. Thus, the deposit feature of the vault will break, and no one will be able to deposit.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L152

File: AutoPxGmx.sol
152:     function setPlatform(address _platform) external onlyOwner {
153:         if (_platform == address(0)) revert ZeroAddress();
154: 
155:         platform = _platform;
156: 
157:         emit PlatformUpdated(_platform);
158:     }

Impact

The deposit feature of the vault will break, and no one will be able to deposit.

Recommended Mitigation Steps

Ensure that allowance is given to the new platform address so that it can pull the GMX tokens from the vault.

function setPlatform(address _platform) external onlyOwner {
    if (_platform == address(0)) revert ZeroAddress();
+   if (_platform == platform) revert SamePlatformAddress();
    
+   gmx.safeApprove(platform, 0); // set the old platform approval amount to zero
+   gmx.safeApprove(_platform, type(uint256).max); // approve the new platform contract address allowance to the max

    platform = _platform;

    emit PlatformUpdated(_platform);
}

Owner Can Become Owner Of The Implementation Contract

Proof of Concept

If the implementation contract is not explicitly initialized during deployment, anyone can call the initialize() function of the PirexRewards implementation contract and become the owner of the implementation contract.

File: PirexRewards.sol
85:     function initialize() public initializer {
86:         __Ownable_init();
87:     }

Recommended Mitigation Steps

1. Consider using the constructor to initialize non-proxied contracts or run initialization function atomically along with contract construction each time.
2. Add access controls so only the authorized user is able to initialize it.
3. Make sure to call it immediately after deployment and verify the transaction succeeded.