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

totalShares Not Decremented If Deposit Order Was Cancelled

sakshamseth5

Summary

Under an edge case where an order is cancelled , the deposit amounts and total deposited amounts have been accounted correctly , but the totalShares has not been accounted for which would lead to total shares being over inflated.

Vulnerability Details

Consider the following ->

1.) A perp vault position is already open on GMX side.

2.) Alice comes to deposit , calls deposit() and since positionIsClosed would be false ( as there is a position open on GMX) nextAction would be assigned to INCREASE_ACTION and data to beenLong

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);
}

and the current flow would be flow = FLOW.DEPOSIT; (L226 in PerpetualVault.sol)

3.) Keeper would trigger the function runNextAction()

and execution would enter this branch if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {

and at L369 _createIncreasePosition() would be invoked.

4.) Inside _createIncreasePosition firstly since flow is DEPOSIT we would ->

if (flow == FLOW.DEPOSIT) {
amountIn = depositInfo[counter].amount;
flowData = vaultReader.getPositionSizeInTokens(curPositionKey);
}

and then create an order on GMX of type MarketIncrease (L886)

5.) Then the afterOrderExecution() would be invoked (Callback function that is called when an order on GMX is executed successfully)

and since order type was market increase ->

if (orderResultData.orderType == Order.OrderType.MarketIncrease) {
curPositionKey = positionKey;
if (flow == FLOW.DEPOSIT) {
uint256 amount = depositInfo[counter].amount;
uint256 feeAmount = vaultReader.getPositionFeeUsd(market, orderResultData.sizeDeltaUsd, false) / prices.shortTokenPrice.min;
uint256 prevSizeInTokens = flowData;
int256 priceImpact = vaultReader.getPriceImpactInCollateral(curPositionKey, orderResultData.sizeDeltaUsd, prevSizeInTokens, prices);
uint256 increased;
if (priceImpact > 0) {
increased = amount - feeAmount - uint256(priceImpact) - 1;
} else {
increased = amount - feeAmount + uint256(-priceImpact) - 1;
}
_mint(counter, increased, false, prices);
nextAction.selector = NextActionSelector.FINALIZE;
}

based on the priceImpact _mint would be called.

6.) And in _mint depending on the totalShares and totalAmount new shares would be assigned in L777-L778

depositInfo[depositId].shares = _shares;
totalShares = totalShares + _shares;

7.) And then next action would be assigned as FINALIZE on L495 ->

nextAction.selector = NextActionSelector.FINALIZE;

8.) Now for any reason keeper calls cancelFlow() (can be called since gmxLock is set to false afterOrderExecution hence is a possible action that can be taken according to the code) ->

and inside _cancelFlow() ->

function _cancelFlow() internal {
if (flow == FLOW.DEPOSIT) {
uint256 depositId = counter;
collateralToken.safeTransfer(depositInfo[depositId].owner, depositInfo[depositId].amount);
totalDepositAmount = totalDepositAmount - depositInfo[depositId].amount;
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
try IGmxProxy(gmxProxy).refundExecutionFee(
depositInfo[counter].owner,
depositInfo[counter].executionFee
) {} catch {}
delete depositInfo[depositId];
}

9.) We can see that totalDepositAmount decreases and depositInfo gets deleted (which would set user's shares and amount to 0) but totalShares have
not been updated/decremented by the user's shares.

10.) This means in this case totalShares would be overinflated and when the next user deposits he would be minted way higher shares ->

_shares = amount * totalShares / totalAmountBefore; (inside _mint)

Impact

totalShares would be higher than expected and new minted shares would be over inflated

Tools Used

Manual analysis

Recommendations

In the _cancelFlow if user had non-zero shares (in his depositInfo mapping) , then reduce that amount from the totalShares.

Updates

Lead Judging Commences

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

finding_cancelFlow_can_be_called_after_order_execution_leading_to_disturb_shares_and_refund_too_many_fees

Likelihood: None/Very Low, when the keeper call cancelFlow after an order execution Impact: High, Inflation/deflation of total shares, and too many fees refunded.

Support

FAQs

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