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

User Will Lose Execution Fee

0xVI

Summary

In deposit(), if positionIsClosed is true (i.e., there is no active position), the user should not have to pay the execution fee. However, if there is an active position in GMX, then the user must pay the execution fee.

Suppose there is an active position, and the user calls deposit(). Since there is already an active position, the user needs to pay the execution fee. However, before their transaction is executed, if the position is closed, the user will lose their execution fee because there is no active position at the time of deposit.

Vulnerability Details

function deposit(uint256 amount) external nonReentrant payable {
....
if (positionIsClosed) {
MarketPrices memory prices;
_mint(counter, amount, false, prices);
_finalize(hex'');
} else {
_payExecutionFee(counter, true);
// Mint share token in the NextAction to involve off-chain price data and improve security
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
}

Attack Path

Assume there is already an active GMX position in the protocol:

  1. Alice calls deposit(), paying the execution fee. Her transaction is now in the mempool.

  2. Before Alice’s deposit transaction is executed, the position is closed in GMX.

  3. Alice's order gets executed, and she loses her execution fee, even though she should not have.

PoC

function testUserLoseExecutionFee() external {
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
address bob = makeAddr("bob");
uint256 amount = 1e10;
vm.startPrank(bob);
deal(address(collateralToken), bob, amount);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
collateralToken.approve(vault, amount);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(amount);
vm.stopPrank();
​
MarketPrices memory prices = mockData.getMarketPrices();
bytes;
data[0] = abi.encode(3380000000000000);
address keeper = PerpetualVault(vault).keeper();
vm.prank(keeper);
PerpetualVault(vault).run(true, false, prices, data);
GmxOrderExecuted(true);
​
(PerpetualVault.NextActionSelector selector, bytes memory data1) = PerpetualVault(vault).nextAction();
PerpetualVault.Action memory action = PerpetualVault.Action(
selector,
data1
);
​
// Owner cancels the order
vm.startPrank(PerpetualVault(vault).owner());
PerpetualVault(vault).setVaultState(
PerpetualVault.FLOW.NONE,
0,
false,
bytes32(0),
true,
false,
action
);
vm.stopPrank();
​
address alice = makeAddr("alice");
vm.deal(alice, 10e18);
vm.startPrank(alice);
deal(address(collateralToken), alice, amount);
collateralToken.approve(vault, amount);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(amount);
vm.stopPrank();
uint256 aliceETHBalance = address(alice).balance;
assert(aliceETHBalance == aliceETHBalance - (executionFee * tx.gasprice));
}

To run the test in PerpetualVault.t.sol, use:

forge test --mt testUserLoseExecutionFee --fork-url <ARBITRUM_RPC_URL> -vvvv

Output

Ran 1 test for test/PerpetualVault.t.sol:PerpetualVaultTest
[PASS] testUserLoseExecutionFee() (gas: 4551527)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 57.51ms (43.47ms CPU time)
​
Ran 1 test suite in 1.51s (57.51ms CPU time): 1 test passed, 0 failed, 0 skipped (1 total test)

Impact

Genuine users will lose their execution fee, and funds will get stuck in the PerpetualVault.

Tools Used

Manual Review, Foundry

Recommendations

Implement a mechanism to refund the execution fee if the user sends it when there is no active position.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_no_refund_when_deposit_just_after_a_keeper_close_position

Likelihood: Low, send a deposit with execution fees but a “run” keeper is made just before to close the position. Impact: Low/Medium, no refund of the execution fee, althought they were no need for them.

Appeal created

p4y4bl3 Submitter
9 months ago
n0kto Lead Judge
8 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_no_refund_when_deposit_just_after_a_keeper_close_position

Likelihood: Low, send a deposit with execution fees but a “run” keeper is made just before to close the position. Impact: Low/Medium, no refund of the execution fee, althought they were no need for them.

Support

FAQs

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

Give us feedback!