Redacted Cartel contest - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

pxGMX and pxGLP tokens can be stolen from depositors in AutoPxGmx and AutoPxGlp vaults by manipulating the price of a share.

Proof of Concept

ERC4626 vaults are subject to a share price manipulation attack that allows an attacker to steal underlying tokens from other depositors (this is a known issue of Solmate's ERC4626 implementation). Consider this scenario (this is applicable to AutoPxGmx and AutoPxGlp vaults):

1. Alice is the first depositor of the AutoPxGmx vault;
2. Alice deposits 1 wei of pxGMX tokens;
3. in the deposit function (PirexERC4626.sol#L60), the amount of shares is calculated using the previewDeposit function:

   function previewDeposit(uint256 assets)
       public
       view
       virtual
       returns (uint256)
   {
       return convertToShares(assets);
   }
   
   function convertToShares(uint256 assets)
       public
       view
       virtual
       returns (uint256)
   {
       uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
   
       return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
   }

4. since Alice is the first depositor (totalSupply is 0), she gets 1 share (1 wei);
5. Alice then sends 9999999999999999999 pxGMX tokens (10e18 - 1) to the vault;
6. the price of 1 share is 10 pxGMX tokens now: Alice is the only depositor in the vault, she's holding 1 wei of shares, and the balance of the pool is 10 pxGMX tokens;
7. Bob deposits 19 pxGMX tokens and gets only 1 share due to the rounding in the convertToShares function: 19e18 * 1 / 10e18 == 1;
8. Alice redeems her share and gets a half of the deposited assets, 14.5 pxGMX tokens (less the withdrawal fee);
9. Bob redeems his share and gets only 14.5 pxGMX (less the withdrawal fee), instead of the 19 pxGMX he deposited.

// test/AutoPxGmx.t.sol
function testSharePriceManipulation_AUDIT() external {
    address alice = address(0x31337);
    address bob = address(0x12345);
    vm.label(alice, "Alice");
    vm.label(bob, "Bob");

    // Resetting the withdrawal fee for cleaner amounts.
    autoPxGmx.setWithdrawalPenalty(0);

    vm.startPrank(address(pirexGmx));        
    pxGmx.mint(alice, 10e18);
    pxGmx.mint(bob, 19e18);
    vm.stopPrank();

    vm.startPrank(alice);
    pxGmx.approve(address(autoPxGmx), 1);
    // Alice deposits 1 wei of pxGMX and gets 1 wei of shares.
    autoPxGmx.deposit(1, alice);
    // Alice sends 10e18-1 of pxGMX and sets the price of 1 wei of shares to 10e18 pxGMX.
    pxGmx.transfer(address(autoPxGmx), 10e18-1);
    vm.stopPrank();

    vm.startPrank(bob);
    pxGmx.approve(address(autoPxGmx), 19e18);
    // Bob deposits 19e18 of pxGMX and gets 1 wei of shares due to rounding and the price manipulation.
    autoPxGmx.deposit(19e18, bob);
    vm.stopPrank();

    // Alice and Bob redeem their shares.           
    vm.prank(alice);
    autoPxGmx.redeem(1, alice, alice);
    vm.prank(bob);
    autoPxGmx.redeem(1, bob, bob);

    // Alice and Bob both got 14.5 pxGMX.
    // But Alice deposited 10 pxGMX and Bob deposited 19 pxGMX – thus, Alice stole pxGMX tokens from Bob.
    // With withdrawal fees enabled, Alice would've been penalized more than Bob
    // (14.065 pxGMX vs 14.935 pxGMX tokens withdrawn, respectively),
    // but Alice would've still gotten more pxGMX that she deposited.
    assertEq(pxGmx.balanceOf(alice), 14.5e18);
    assertEq(pxGmx.balanceOf(bob), 14.5e18);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider either of these options:

1. In the deposit function of PirexERC4626, consider requiring a reasonably high minimal amount of assets during first deposit. The amount needs to be high enough to mint many shares to reduce the rounding error and low enough to be affordable to users.
2. On the first deposit, consider minting a fixed and high amount of shares, irrespective of the deposited amount.
3. Consider seeding the pools during deployment. This needs to be done in the deployment transactions to avoiding front-running attacks. The amount needs to be high enough to reduce the rounding error.
4. Consider sending first 1000 wei of shares to the zero address. This will significantly increase the cost of the attack by forcing an attacker to pay 1000 times of the share price they want to set. For a well-intended user, 1000 wei of shares is a negligible amount that won't diminish their share significantly.

Lines of code

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

Vulnerability details

Impact

Swapping WETH for GMX during compounding in the AutoPxGmx pool can be manipulated: an attacker can use a pool with a manipulated WETH/GMX price and/or lower TVL to buy WETH from AutoPxGmx at a cheaper price, basically stealing a part of rewards from the vault.

Proof of Concept

AutoPxGmx implements the compound function to claim rewards (WETH and pxGMX) from the underlying pxGMX pool, swap WETH for GMX tokens on Uniswap, and stake pxGMX and GMX tokens (AutoPxGmx.sol#L242). Every Uniswap V3 pool is identified by three parameters: first token's address, second token's address, and the swap fee amount (see the salt here: UniswapV3PoolDeployer.sol#L35). The compound function, while hard coding token addresses (AutoPxGmx.sol#L270-L271), allows the caller to specify a swap fee (AutoPxGmx.sol#L243), allowing a caller to choose any of the deployed WETH/GMX pools. At the moment of the audit, there are three WETH/GMX pools on Arbitrum:

1. with 1% swap fees, the official one, has the highest TVL of $17.03m;
2. with 0.3% swap fees, TVL $4.06m;
3. with 0.05% swap fees, TVL $603.49.

(More pools can be deployed if new fee tiers are enabled in the Uniswap V3 Factory contract).

An attacker can use the smallest of the pools by TVL to perform the swap during compounding, which allows atomic sandwich attacks:

1. an attacker buys GMX from the 0.05% pool to increase its price in terms of WETH;
2. the attacker calls the compound function and sets: fee to 500 to use the 0.05% pool; amountOutMinimum to 1 to bypass the non-zero slippage tolerance check and still have ~100% slippage; sqrtPriceLimitX96 to 0 to swap the entire amount;
3. in the compound call, AutoPxGmx buys GMX and sells WETH at an increased price, getting less GMX and selling more WETH than expected; this pushes the price of GMX a little higher;
4. the attacker sells their GMX at the higher price, getting more WETH as a profit.

However, since compounding is done before depositing (AutoPxGmx.sol#L227), withdrawing (AutoPxGmx.sol#L321), and redeeming (AutoPxGmx.sol#L345), the underlying pxGMX pool won't likely to accumulate big rewards.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using the poolFee state variable instead of taking fee as a parameter in the compound function. However, this won't protect from sandwich attacks since the contract cannot get a WETH/GMX price from a Uniswap pool in a manipulation resilient way, thus it cannot calculate the amountOutMinimum variable to set a tight slippage tolerance. To mitigate the harm of sandwich attacks, ensure compounding is done as often as possible. Probably, consider running a keeper bot that triggers compounding when when a reward is high enough.

Lines of code

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

Vulnerability details

Impact

An old stakedGmxTracker contract can withdraw all GMX tokens held by PirexGmx.

Proof of Concept

The configureGmxState function allows to update addresses of all GMX contracts in one call (PirexGmx.sol#L272):

function configureGmxState() external onlyOwner whenPaused {
    // Variables which can be assigned by reading previously-set GMX contracts
    rewardTrackerGmx = RewardTracker(gmxRewardRouterV2.feeGmxTracker());
    rewardTrackerGlp = RewardTracker(gmxRewardRouterV2.feeGlpTracker());
    feeStakedGlp = RewardTracker(gmxRewardRouterV2.stakedGlpTracker());
    stakedGmx = RewardTracker(gmxRewardRouterV2.stakedGmxTracker());
    glpManager = gmxRewardRouterV2.glpManager();
    gmxVault = IVault(IGlpManager(glpManager).vault());

    emit ConfigureGmxState(
        msg.sender,
        rewardTrackerGmx,
        rewardTrackerGlp,
        feeStakedGlp,
        stakedGmx,
        glpManager,
        gmxVault
    );

    // Approve GMX to enable staking
    gmx.safeApprove(address(stakedGmx), type(uint256).max);
}

The function also approves spending of the unlimited amount of GMX tokens to the stakedGmx address, which is the RewardTracker contract for staked GMX. However, the previously given approval is not removed before the stakedGmx address is updated, which allows the old stakedGmxTracker contract to withdraw GMX tokens of PirexGmx.

The RewardTracker contract allows anyone to stake GMX tokens from an arbitrary address that has previously approved the contract:

• RewardTracker.sol#L105-L108:

  function stakeForAccount(address _fundingAccount, address _account, address _depositToken, uint256 _amount) external override nonReentrant {
      _validateHandler();
      _stake(_fundingAccount, _account, _depositToken, _amount);
  }

• RewardTracker.sol#L237-L250:

  function _stake(address _fundingAccount, address _account, address _depositToken, uint256 _amount) private {
      require(_amount > 0, "RewardTracker: invalid _amount");
      require(isDepositToken[_depositToken], "RewardTracker: invalid _depositToken");
  
      IERC20(_depositToken).safeTransferFrom(_fundingAccount, address(this), _amount);
  
      _updateRewards(_account);
  
      stakedAmounts[_account] = stakedAmounts[_account].add(_amount);
      depositBalances[_account][_depositToken] = depositBalances[_account][_depositToken].add(_amount);
      totalDepositSupply[_depositToken] = totalDepositSupply[_depositToken].add(_amount);
  
      _mint(_account, _amount);
  }

Thus, anyone will be able to call the stakeForAccount function in an old RewardTracker contract and pass PirexGmx's address to steal GMX tokens from the contract. The severity of the issue is reduced due to the fact PirexGmx is not designed to hold GMX tokens. But it still can hold mistakenly sent GMX tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

--- a/src/PirexGmx.sol
+++ b/src/PirexGmx.sol
@@ -270,6 +270,8 @@ contract PirexGmx is ReentrancyGuard, Owned, Pausable {
         @notice Configure GMX contract state
      */
     function configureGmxState() external onlyOwner whenPaused {
+        // Remove approval from the old `stakedGmx` contract
+        gmx.safeApprove(address(stakedGmx), 0);
         // Variables which can be assigned by reading previously-set GMX contracts
         rewardTrackerGmx = RewardTracker(gmxRewardRouterV2.feeGmxTracker());
         rewardTrackerGlp = RewardTracker(gmxRewardRouterV2.feeGlpTracker());

Lines of code

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

Vulnerability details

Impact

A user (which can also be one of the autocompounding contracts, AutoPxGlp or AutoPxGmx) can loss a reward as a result of reward tokens mismanagement by the owner.

Proof of Concept

The protocol defines a short list of reward tokens that are hard coded in the claimRewards function of the PirexGmx contract (PirexGmx.sol#L756-L759):

rewardTokens[0] = gmxBaseReward;
rewardTokens[1] = gmxBaseReward;
rewardTokens[2] = ERC20(pxGmx); // esGMX rewards distributed as pxGMX
rewardTokens[3] = ERC20(pxGmx);

The fact that these addresses are hard coded means that no other reward tokens will be supported by the protocol. However, the PirexRewards contract maintains a different list of reward tokens, one per producer token (PirexRewards.sol#L19-L31):

struct ProducerToken {
    ERC20[] rewardTokens;
    GlobalState globalState;
    mapping(address => UserState) userStates;
    mapping(ERC20 => uint256) rewardStates;
    mapping(address => mapping(ERC20 => address)) rewardRecipients;
}

// Producer tokens mapped to their data
mapping(ERC20 => ProducerToken) public producerTokens;

These reward tokens can be added (PirexRewards.sol#L151) or removed (PirexRewards.sol#L179) by the owner, which creates the possibility of a mismanagement:

1. the owner can mistakenly remove one of the reward tokens hard coded in the PirexGmx contract;
2. the owner can add reward tokens that are not supported by the PirexGmx contract.

Such mismanagement can cause users to lose rewards for two reasons:

1. reward state of a user is updated before their rewards are claimed;
2. it's the reward token addresses set by the owner of the PirexRewards contract that are used to transfer rewards.

In the claim function:

1. harvest is called to pull rewards from GMX (PirexRewards.sol#L377):

   harvest();

2. claimReward is called on PirexGmx to pull rewards from GMX and get the hard coded lists of producer tokens, reward tokens, and amounts (PirexRewards.sol#L346-L347):

   (_producerTokens, rewardTokens, rewardAmounts) = producer
   .claimRewards();

3. rewards are recorded for each of the hard coded reward token (PirexRewards.sol#L361):

   if (r != 0) {
       producerState.rewardStates[rewardTokens[i]] += r;
   }

4. later in the claim function, owner-set reward tokens are read (PirexRewards.sol#L386-L387):

   ERC20[] memory rewardTokens = p.rewardTokens;
   uint256 rLen = rewardTokens.length;

5. user reward state is set to 0, which means they've claimed their entire share of rewards (PirexRewards.sol#L391), however this is done before a reward is actually claimed:

   p.userStates[user].rewards = 0;

6. the owner-set reward tokens are iterated and the previously recorded rewards are distributed (PirexRewards.sol#L396-L415):

   for (uint256 i; i < rLen; ++i) {
       ERC20 rewardToken = rewardTokens[i];
       address rewardRecipient = p.rewardRecipients[user][rewardToken];
       address recipient = rewardRecipient != address(0)
           ? rewardRecipient
           : user;
       uint256 rewardState = p.rewardStates[rewardToken];
       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
           );
       }
   }

In the above loop, there can be multiple reasons for rewards to not be sent:

1. one of the hard coded reward tokens is missing in the owner-set reward tokens list;
2. the owner-set reward token list contains a token that's not supported by PirexGmx (i.e. it's not in the hard coded reward tokens list);
3. the rewardTokens array of a producer token turns out to be empty due to a mismanagement by the owner.

In all of the above situations rewards won't be sent, however user's reward state will still be set to 0.

Also, notice that calling claim won't revert if reward tokens are misconfigured, and the Claim event will be emitted successfully, which makes reward tokens mismanagement hard to detect.

The amount of lost rewards can be different depending on how much GMX a user has staked and how often they claim rewards. Of course, if a mistake isn't detected quickly, multiple users can suffer from this issue. The autocompounding contracts (AutoPxGlp and AutoPxGmx) are also users of the protocol, and since they're intended to hold big amounts of real users' deposits (they'll probably be the biggest stakers), lost rewards can be big.

Tools Used

Manual review

Recommended Mitigation Steps

Consider having one source of reward tokens. Since they're already hard coded in the PirexGmx contract, consider exposing them so that PirexRewards could read them in the claim function. This change will also mean that the addRewardToken and removeRewardToken functions won't be needed, which makes contract management simpler. Also, in the claim function, consider updating global and user reward states only after ensuring that at least one reward token was distributed.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L184-L190 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L162-L168

Vulnerability details

Impact

Stakers in AutoPxGmx and AutoPxGlp can pay a smaller withdrawal fee than expected by breaking down redeeming into smaller steps. The issue mostly benefits the biggest stakers in the vaults since the reduction in withdrawal fees depends on the share of the withdrawn amount in the vault.

Proof of Concept

When withdrawing from AutoPxGmx and AutoPxGlp vaults, a withdrawal fee is subtracted from the amounts being withdrawn (AutoPxGmx.sol#L184-L190, AutoPxGlp.sol#L162-L168). The mechanism of fee reduction ensures that a user gets a smaller amount of the underlying tokens for their shares. However, since the entire amount of shares is burnt, the remaining underlying assets get distributed among all stakers in the vaults, including the user who just redeemed their tokens (if they still have shares after the redeeming). Thus, by breaking redeeming into smaller steps, users can reduce the total withdrawal fee by a small percentage, which heavily depends on the size of the total shares being redeemed.

The below coded PoC demonstrated a scenario when a whale redeems 10% of shares by making 100 redeem calls and eventually reducing total withdrawal fee by ~4.5%:

// test/AutoPxGmx.t.sol
function testWithdrawalFeeOptimization_AUDIT() external {
    address alice = address(0x31337);
    address bob = address(0x12345);
    vm.label(alice, "Alice");
    vm.label(bob, "Bob");

    // Ensure the default fee is set.
    autoPxGmx.setWithdrawalPenalty(300);

    uint256 million = 22_222.2222e18; // ~$1 mil @ $45 per 1 GMX
    uint256 aliceBalance = million;
    uint256 bobBalance = 10 * million;

    vm.startPrank(address(pirexGmx));        
    pxGmx.mint(alice, aliceBalance);
    pxGmx.mint(bob, bobBalance);
    vm.stopPrank();

    vm.startPrank(alice);
    pxGmx.approve(address(autoPxGmx), type(uint256).max);
    autoPxGmx.deposit(aliceBalance, alice);
    vm.stopPrank();

    vm.startPrank(bob);
    pxGmx.approve(address(autoPxGmx), type(uint256).max);
    autoPxGmx.deposit(bobBalance, bob);
    vm.stopPrank();

    vm.startPrank(alice);

    uint256 redeemedAtOnce = autoPxGmx.previewRedeem(autoPxGmx.balanceOf(alice));
    assertEq(redeemedAtOnce, 21555.555534e18);
    assertEq(aliceBalance - redeemedAtOnce, 666.666666e18); // 666.666 * $45 = ~$30,000

    uint256 balance;
    uint256 part = aliceBalance / 100;
    uint256 redeemCalledTimes;
    for (;;) {
        balance = autoPxGmx.balanceOf(alice);
        redeemCalledTimes++;
        if (balance > part) {
            autoPxGmx.redeem(part, alice, alice);
        } else {
            autoPxGmx.redeem(balance, alice, alice);
            break;
        }
    }

    assertEq(redeemCalledTimes, 100);
    // The cost of 100 calls to `redeem` =
    // 100 * gas usage * gas price * ETH price =
    // 100 * 143677 * 0.0000000001 (0.1 Gwei) * 1200 = $1.7241

    uint256 redeemedInASeries = pxGmx.balanceOf(alice);
    assertEq(redeemedInASeries, 21585.616863067223296826e18);
    assertEq(autoPxGmx.balanceOf(alice), 0);

    uint256 feeCompensation = redeemedInASeries - redeemedAtOnce;
    assertEq(feeCompensation, 30.061329067223296826e18); // 30.0613 * $45 = $1352.7585

    // Total profit of breaking redeeming into smaller steps:
    // $1352.7585 - $1.7241 = $1351,0344
    // Which is ~4.5% of the fee taken when redeeming all shares at once.
}

In case a whale withdraws only 1% of total supply, the profit is 3.1802 pxGMX tokens, or ~0.48%.

Tools Used

Manual review

Recommended Mitigation Steps

Consider sending collected withdrawal fees to the owner of the contract (as it's done with the platform fees: AutoPxGmx.sol#L299).