Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Depositor can mint shares without paying execution fee or bearing negative price impact when positionIsClosed is True

t0x1c

Summary

A vault with no open positions & no ongoing flows can be exploited by a depositor since their shares are minted immediately and are not subjected to price impact calculations or are asked to pay any execution fees. This benefits both:

  1. The first depositor of the vault.

  2. If Keeper ever decides to momentarily close all open positions due to volatile market conditions, any user who deposits at this point of time.

Note that positionIsClosed = true simply means Gamma does not currently have a open position in the target GMX market. The GMX market liquidity exists independently and hence any position opened up by Gamma, even if it is the first from the PerpetualVault's perspective is capable of impacting GMX's existing market's prices and hence is subject to price impact calculations by GMX.

Description

deposit() mints tokens to the user via _mint() whenever positionIsClosed = true:

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));
230: totalDepositAmount += amount;
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: }

Note that:

  • positionIsClosed is true for the first depositor to the vault.

  • Also, if Keeper ever decides to momentarily close all open positions due to volatile market conditions, OR there is some time gap between closing of an existing position & opening of a new one, then positionIsClosed is true for this duration, with no ongoing flows.

  • Any deposits made in such conditions can -

    • escape paying the execution fee.

    • escape being subjected to price impact calculations which happens only in the case of FLOW.DEPOSIT inside the afterOrderExecution() callback:

File: contracts/PerpetualVault.sol
​
481: if (orderResultData.orderType == Order.OrderType.MarketIncrease) {
482: curPositionKey = positionKey;
483:@---> if (flow == FLOW.DEPOSIT) {
484: uint256 amount = depositInfo[counter].amount;
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);

Inside _mint() it's never checked if execution fee has been paid or not. So let's suppose Keeper decides to create a long position in the next run, they have to fund this execution fee out of their own pockets.
As the contest page specifies for Depositors:

Must provide sufficient execution fees for operations

Impact

  1. Depositor is able to escape any price impact penalties and receives more than their rightful proportion of shares.

  2. Other depositors have to bear a higher burden.

  3. User receives shares without paying for execution fee.

  4. This could lead to a situation where the Keeper doesn't have enough gas to execute the tx and the queue remains stuck. However this impact could be ignored since the contest page states that:

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

Proof Of Concept

This PoC shows that the depositor can pay no execution fees and still get shares minted. Add this inside test/PerpetualVault.t.sol and run via forge test --mt test_Deposit_With_No_Exec_Fee -vv --via-ir --rpc-url arbitrum to see it pass:

function test_Deposit_With_No_Exec_Fee() external {
uint256 amount = 1e10;
address alice = makeAddr("alice");
​
uint256[] memory userDeposits = PerpetualVault(vault).getUserDeposits(alice);
uint256 userSharesInit;
uint256 userSharesFinal;
if(userDeposits.length > 0) {
(,userSharesInit,,,,) = PerpetualVault(vault).depositInfo(userDeposits[0]);
}
emit log_named_decimal_uint("Alice Shares Initial =", userSharesInit, 18);
// Deposit
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
vm.startPrank(alice);
deal(address(collateralToken), alice, amount);
collateralToken.approve(vault, amount);
// No execution fee provided by user
PerpetualVault(vault).deposit{value: 0}(amount);
vm.stopPrank();
​
userDeposits = PerpetualVault(vault).getUserDeposits(alice);
if(userDeposits.length > 0) {
(,userSharesFinal,,,,) = PerpetualVault(vault).depositInfo(userDeposits[0]);
}
emit log_named_decimal_uint("Alice Shares Final =", userSharesFinal, 18);
assertGt(userSharesFinal, userSharesInit);
}

Recommendation

The fix could involve multiple changes but at its core:

  • The protocol needs to do the price impact calculations even when a user deposits during positionIsClosed = true and mint shares to them only after these calculations are taken into account.

  • Charge the execution fee even when positionIsClosed = true.

Updates

Lead Judging Commences

n0kto Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

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.

Appeal created

t0x1c Submitter
3 months ago
n0kto Lead Judge
3 months ago
n0kto Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

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.

Support

FAQs

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