DeFiFoundry
50,000 USDC
View results
Submission Details
Severity: low
Invalid

Gamma supports USDC but do not account for FoT during transfers

Summary

USDC is a well known token that has a fee switch that could be activated at any time.
If this happens, any PerpetualVault using it will be broken on deposits, as the amount of token used to mint shares do not account for fees.

Vulnerability details

The issue will cause total deposited amount and users deposit amounts to be wrongly accounted:

  1. Every time deposit is called the totalDepositAmount will be wrong

  2. For users deposit, on first deposit, this one is the less inconvenient as its very unlikely to happen

  3. on any deposits made in a vault with an open position, so on most of the deposits

I've listed some situations where this will cause wrong accounting, but there might be others.

Case 1: first deposit

During first deposit, we see L230 that the totalDepositedAmount is incremented by amount, and depositInfo[counter] initialized with amount too, while when FoT are in play, the real value received by the vault will be in reality amount - someFee.

We also see L235 that the deposit function will directly call the _mint function (as positionIsClosed is set to true on initialization), which will then mint to the user an amount of shares equal to depositInfo[depositId].amount * 1e8 where depositInfo[depositId].amount == amount.

File: contracts/PerpetualVault.sol
215: function deposit(uint256 amount) external nonReentrant payable {
216: _noneFlow();
217: if (depositPaused == true) {
218: revert Error.Paused();
219: }
220: if (amount < minDepositAmount) {
221: revert Error.InsufficientAmount();
222: }
223: if (totalDepositAmount + amount > maxDepositAmount) {
224: revert Error.ExceedMaxDepositCap();
225: }
226: flow = FLOW.DEPOSIT;
227: collateralToken.safeTransferFrom(msg.sender, address(this), amount);
228: counter++;
229: depositInfo[counter] = DepositInfo(amount, 0, msg.sender, 0, block.timestamp, address(0)); <@ wrong when FoT activated
230: totalDepositAmount += amount; <@ wrong when FoT activated
231: EnumerableSet.add(userDeposits[msg.sender], counter);
232:
233: if (positionIsClosed) {
234: MarketPrices memory prices;
235: _mint(counter, amount, false, prices);
236: _finalize(hex'');
237: } else {
238: _payExecutionFee(counter, true);
239: // mint share token in the NextAction to involve off-chain price data and improve security
240: nextAction.selector = NextActionSelector.INCREASE_ACTION;
241: nextAction.data = abi.encode(beenLong);
242: }
243: }

Case 2: subsequent deposits

Once the vault has received deposits and opened a position, the deposit function will not directly mint shares to the users, but rather set the nextAction.selector to INCREASE_ACTION.

This will trigger keepers into calling the runNextAction(), which will then call _createIncreasePosition in case of a vault with leverage >1, creating a GMX order.

Once the GMX order has been successfully created, the afterOrderExecution will be called, and the FLOW.DEPOSIT case will be executed.
We then see, L484 that the amount that will be used to mint shares to the users is read again from depositInfo[counter].amount, which as we shown before that, do not account for FoT.

Finally, that amount is used to compute increased at L490 (where feeAmount are GMX fees), and fed to the _mint function L494.

File: contracts/PerpetualVault.sol
463: function afterOrderExecution(
464: bytes32 requestKey,
465: bytes32 positionKey,
466: IGmxProxy.OrderResultData memory orderResultData,
467: MarketPrices memory prices
468: ) external nonReentrant {
469: if (msg.sender != address(gmxProxy)) {
470: revert Error.InvalidCall();
471: }
472: // MarketPrices memory marketPrices = gmxProxy.getMarketPrices(market);
473:
474: _gmxLock = false;
475: // If the current action is `settle`
476: if (orderResultData.isSettle) {
477: nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
478: emit GmxPositionCallbackCalled(requestKey, true);
479: return;
480: }
481: if (orderResultData.orderType == Order.OrderType.MarketIncrease) { //* long/
482: curPositionKey = positionKey;
483: if (flow == FLOW.DEPOSIT){
484: uint256 amount = depositInfo[counter].amount; <@ do not account for FoT
485: uint256 feeAmount = vaultReader.getPositionFeeUsd(market, orderResultData.sizeDeltaUsd, false) / prices.shortTokenPrice.min;
486: uint256 prevSizeInTokens = flowData;
487: int256 priceImpact = vaultReader.getPriceImpactInCollateral(curPositionKey, orderResultData.sizeDeltaUsd, prevSizeInTokens, prices);
488: uint256 increased;
489: if (priceImpact > 0) {
490: increased = amount - feeAmount - uint256(priceImpact) - 1;
491: } else {
492: increased = amount - feeAmount + uint256(-priceImpact) - 1;
493: }
494: _mint(counter, increased, false, prices); <@ do not account for FoT
495: nextAction.selector = NextActionSelector.FINALIZE;
496: } else {
...:
...: //* ---------- some code ----------- *//
...:

Impact

Wrong accounting for USDC if fees are activated.

  • Likelihood: low, as this require USDC to activate fees and we don't know when this will happens

  • Impact: high, as this would break accounting for the vault

Recommended Mitigation Steps

Rather than relying of the amount input in deposit(), measure the actual received amount by comparing balanceBefore and balanceAfter.

Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.