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

Incorrect Collateral delta Amount Funds retrieval from GMX

bigsam

Summary

When closing a position the Perpetual calls with a collateral delta amount this amount is sent to GMX during order creation but this is not retrieved back correctly as it will either be partially collected or not collected at all.

Vulnerability Details

When ever a user calls to withdraw his token and such withdraws can set the total order under the liquidation factor the entire order is closed but this doesn't accurately recalculate the collateral delta / reset it to the appropriate value.

/**
* @notice Handles the internal withdrawal process for a given deposit ID.
* @dev This function calculates the share of the withdrawal, handles the swap if necessary,
* and updates the position accordingly.
* @param depositId The ID of the deposit to withdraw.
*/
function _withdraw(uint256 depositId, bytes memory metadata, MarketPrices memory prices) internal {
uint256 shares = depositInfo[depositId].shares;
if (shares == 0) {
revert Error.ZeroValue();
}
if (positionIsClosed) {
_handleReturn(0, true, false);
} else if (_isLongOneLeverage(beenLong)) { // beenLong && leverage == BASIS_POINTS_DIVISOR
uint256 swapAmount = IERC20(indexToken).balanceOf(address(this)) * shares / totalShares;
nextAction.selector = NextActionSelector.SWAP_ACTION; // we no spend na note
// abi.encode(swapAmount, swapDirection): if swap direction is true, swap collateralToken to indexToken
nextAction.data = abi.encode(swapAmount, false);
} else if (curPositionKey == bytes32(0)) { // vault liquidated
_handleReturn(0, true, false);
} else {
IVaultReader.PositionData memory positionData = vaultReader.getPositionInfo(curPositionKey, prices);
uint256 collateralDeltaAmount = positionData.collateralAmount * shares / totalShares;
uint256 sizeDeltaInUsd = positionData.sizeInUsd * shares / totalShares;
// we always charge the position fee of negative price impact case.
uint256 feeAmount = vaultReader.getPositionFeeUsd(market, sizeDeltaInUsd, false) / prices.shortTokenPrice.max;
int256 pnl = vaultReader.getPnl(curPositionKey, prices, sizeDeltaInUsd);
​
if (pnl < 0) {
​
@audit>> collateralDeltaAmount = collateralDeltaAmount - feeAmount - uint256(-pnl) / prices.shortTokenPrice.max;
} else {
​
@audit>> collateralDeltaAmount = collateralDeltaAmount - feeAmount;
}
uint256 acceptablePrice = abi.decode(metadata, (uint256));
@audit>> _createDecreasePosition(collateralDeltaAmount, sizeDeltaInUsd, beenLong, acceptablePrice, prices);
}
}

But collateral delta is not over riding when size usd is

​
/**
* @notice Creates a decrease position request.
* @dev We register position information with GMX peripheral contracts to open perp positions.
* This function doesn't close the position actually; it just registers the request of creating position.
* The actual opening/closing is done by a keeper of the GMX vault.
* @param collateralDeltaAmount The token amount of collateral to withdraw.
* @param sizeDeltaInUsd The USD value of the change in position size (decimals is 30).
* @param _isLong If true, the position is long; if false, the position is short.
*/
function _createDecreasePosition(
uint256 collateralDeltaAmount,
uint256 sizeDeltaInUsd,
bool _isLong,
uint256 acceptablePrice,
MarketPrices memory prices
) internal {
address[] memory swapPath;
Order.OrderType orderType = Order.OrderType.MarketDecrease;
uint256 sizeInUsd = vaultReader.getPositionSizeInUsd(curPositionKey);
@audit>> if (
sizeDeltaInUsd == 0 ||
vaultReader.willPositionCollateralBeInsufficient(
prices,
curPositionKey,
market,
_isLong,
sizeDeltaInUsd,
collateralDeltaAmount
)
) {
@audit>> sizeDeltaInUsd = sizeInUsd; // note updated the size
}
IGmxProxy.OrderData memory orderData = IGmxProxy.OrderData({
market: market,
indexToken: indexToken,
initialCollateralToken: address(collateralToken),
swapPath: swapPath,
isLong: _isLong,
sizeDeltaUsd: sizeDeltaInUsd,
​
@audit>> initialCollateralDeltaAmount: collateralDeltaAmount, // is this not the profit if yes why do we not take all when we settle all because a position leave the place insolvent
amountIn: 0,
callbackGasLimit: callbackGasLimit,
acceptablePrice: acceptablePrice, // his acceptable price is set fro all
minOutputAmount: 0
});
_gmxLock = true;
gmxProxy.createOrder(orderType, orderData);
}
​

This is also not handled by the proxy as it accepts any value we pass in and also sets the receiver address to perp vault

/**
* @notice Creates an order.
* @dev This function requires the receipient to be the perpetual vault and ensures sufficient ETH balance for the execution fee.
* It handles token approvals, transfers, and constructs the order parameters before creating the order via `gExchangeRouter`.
* @param orderType The type of the order (e.g., MarketIncrease, MarketDecrease, etc.).
* @param orderData The data associated with the order.
* @return The request key of the created order.
*/
function createOrder(
Order.OrderType orderType,
IGmxProxy.OrderData memory orderData
) public returns (bytes32) {
require(msg.sender == perpVault, "invalid caller");
uint256 positionExecutionFee = getExecutionGasLimit(
orderType,
orderData.callbackGasLimit
) * tx.gasprice;
require(
address(this).balance >= positionExecutionFee,
"insufficient eth balance"
);
// check if execution feature is enabled
bytes32 executeOrderFeatureKey = keccak256(
abi.encode(
EXECUTE_ORDER_FEATURE_DISABLED,
orderHandler,
orderType
)
);
require(
dataStore.getBool(executeOrderFeatureKey) == false,
"gmx execution disabled"
);
​
gExchangeRouter.sendWnt{value: positionExecutionFee}(
orderVault,
positionExecutionFee
);
if (
orderType == Order.OrderType.MarketSwap ||
orderType == Order.OrderType.MarketIncrease
) {
IERC20(orderData.initialCollateralToken).safeApprove(
address(gmxRouter),
orderData.amountIn
);
gExchangeRouter.sendTokens(
orderData.initialCollateralToken,
orderVault,
orderData.amountIn
);
}
CreateOrderParamsAddresses memory paramsAddresses = CreateOrderParamsAddresses({
​
@audit>> receiver: perpVault,
cancellationReceiver: address(perpVault),
callbackContract: address(this),
uiFeeReceiver: address(0),
market: orderData.market,
​
@audit>> initialCollateralToken: orderData.initialCollateralToken,
swapPath: orderData.swapPath
});
​
CreateOrderParamsNumbers memory paramsNumber = CreateOrderParamsNumbers({
sizeDeltaUsd: orderData.sizeDeltaUsd,
​
@audit>> initialCollateralDeltaAmount: orderData.initialCollateralDeltaAmount,
triggerPrice: 0,
acceptablePrice: orderData.acceptablePrice,
executionFee: positionExecutionFee,
callbackGasLimit: orderData.callbackGasLimit,
​
@audit>> minOutputAmount: orderData.minOutputAmount, // this param is used when swapping. is not used in opening position even though swap involved.
validFromTime: 0
});
CreateOrderParams memory params = CreateOrderParams({
addresses: paramsAddresses,
numbers: paramsNumber,
orderType: orderType,
decreasePositionSwapType: Order
.DecreasePositionSwapType
.SwapPnlTokenToCollateralToken,
isLong: orderData.isLong,
shouldUnwrapNativeToken: false,
autoCancel: false,
referralCode: referralCode
});
bytes32 requestKey = gExchangeRouter.createOrder(params);
queue.requestKey = requestKey;
return requestKey;
}

When processing this order in the GMX contract we accept the delta amount when oder is to decrease the market

function createOrder(
DataStore dataStore,
EventEmitter eventEmitter,
OrderVault orderVault,
IReferralStorage referralStorage,
address account,
IBaseOrderUtils.CreateOrderParams memory params
) external returns (bytes32) {
AccountUtils.validateAccount(account);
​
ReferralUtils.setTraderReferralCode(referralStorage, account, params.referralCode);
​
@audit>> uint256 initialCollateralDeltaAmount;
​
address wnt = TokenUtils.wnt(dataStore);
​
bool shouldRecordSeparateExecutionFeeTransfer = true;
​
@audit>> if (
params.orderType == Order.OrderType.MarketSwap ||
params.orderType == Order.OrderType.LimitSwap ||
params.orderType == Order.OrderType.MarketIncrease ||
params.orderType == Order.OrderType.LimitIncrease ||
params.orderType == Order.OrderType.StopIncrease
) {
// for swaps and increase orders, the initialCollateralDeltaAmount is set based on the amount of tokens
// transferred to the orderVault
initialCollateralDeltaAmount = orderVault.recordTransferIn(params.addresses.initialCollateralToken);
if (params.addresses.initialCollateralToken == wnt) {
if (initialCollateralDeltaAmount < params.numbers.executionFee) {
revert Errors.InsufficientWntAmountForExecutionFee(initialCollateralDeltaAmount, params.numbers.executionFee);
}
​
@audit>> initialCollateralDeltaAmount -= params.numbers.executionFee;
shouldRecordSeparateExecutionFeeTransfer = false;
}
} else if (
params.orderType == Order.OrderType.MarketDecrease ||
params.orderType == Order.OrderType.LimitDecrease ||
params.orderType == Order.OrderType.StopLossDecrease
) {
​
@audit>> // for decrease orders, the initialCollateralDeltaAmount is based on the passed in value
@audit>> initialCollateralDeltaAmount = params.numbers.initialCollateralDeltaAmount;
} else {
revert Errors.OrderTypeCannotBeCreated(uint256(params.orderType));
}
​
if (shouldRecordSeparateExecutionFeeTransfer) {
uint256 wntAmount = orderVault.recordTransferIn(wnt);
if (wntAmount < params.numbers.executionFee) {
revert Errors.InsufficientWntAmountForExecutionFee(wntAmount, params.numbers.executionFee);
}
​
params.numbers.executionFee = wntAmount;
}
​
if (BaseOrderUtils.isPositionOrder(params.orderType)) {
MarketUtils.validatePositionMarket(dataStore, params.addresses.market);
}
​
if (BaseOrderUtils.isMarketOrder(params.orderType) && params.numbers.validFromTime != 0) {
revert Errors.UnexpectedValidFromTime(uint256(params.orderType));
}
​
// validate swap path markets
MarketUtils.validateSwapPath(dataStore, params.addresses.swapPath);
​
Order.Props memory order;
​
order.setAccount(account);
​
@audit>> order.setReceiver(params.addresses.receiver);
order.setCancellationReceiver(params.addresses.cancellationReceiver);
order.setCallbackContract(params.addresses.callbackContract);
order.setMarket(params.addresses.market);
order.setInitialCollateralToken(params.addresses.initialCollateralToken);
order.setUiFeeReceiver(params.addresses.uiFeeReceiver);
order.setSwapPath(params.addresses.swapPath);
order.setOrderType(params.orderType);
order.setDecreasePositionSwapType(params.decreasePositionSwapType);
order.setSizeDeltaUsd(params.numbers.sizeDeltaUsd);
​
@audit>> order.setInitialCollateralDeltaAmount(initialCollateralDeltaAmount);

Note we are closing all the positions to allow this user to withdraw but the Collateral delta was not overriding and hence some collateral delta are not accounted for when GMX processes this, The Receiver will get the token amount they specified.

​
// @dev process a swap order
// @param params BaseOrderUtils.ExecuteOrderParams
function processOrder(BaseOrderUtils.ExecuteOrderParams memory params) external returns (EventUtils.EventLogData memory) {
if (params.order.market() != address(0)) {
revert Errors.UnexpectedMarket();
}
​
if (params.minOracleTimestamp < params.order.updatedAtTime()) {
revert Errors.OracleTimestampsAreSmallerThanRequired(
params.minOracleTimestamp,
params.order.updatedAtTime()
);
}
​
if (
!BaseOrderUtils.isMarketOrder(params.order.orderType()) &&
params.minOracleTimestamp < params.order.validFromTime()
) {
revert Errors.OracleTimestampsAreSmallerThanRequired(
params.minOracleTimestamp,
params.order.validFromTime()
);
}
​
uint256 requestExpirationTime = params.contracts.dataStore.getUint(Keys.REQUEST_EXPIRATION_TIME);
​
if (
params.order.orderType() == Order.OrderType.MarketSwap &&
params.maxOracleTimestamp > params.order.updatedAtTime() + requestExpirationTime
) {
revert Errors.OracleTimestampsAreLargerThanRequestExpirationTime(
params.maxOracleTimestamp,
params.order.updatedAtTime(),
requestExpirationTime
);
}
​
(address outputToken, uint256 outputAmount) = SwapUtils.swap(SwapUtils.SwapParams(
params.contracts.dataStore,
params.contracts.eventEmitter,
params.contracts.oracle,
params.contracts.orderVault,
params.key,
params.order.initialCollateralToken(),
​
@audit>> params.order.initialCollateralDeltaAmount(),
params.swapPathMarkets,
@audit>> params.order.minOutputAmount(),
@audit>> params.order.receiver(),
params.order.uiFeeReceiver(),
params.order.shouldUnwrapNativeToken()
));

Following through the struct

/**
* @param dataStore The contract that provides access to data stored on-chain.
* @param eventEmitter The contract that emits events.
* @param oracle The contract that provides access to price data from oracles.
* @param bank The contract providing the funds for the swap.
* @param key An identifying key for the swap.
* @param tokenIn The address of the token that is being swapped.
* @param amountIn The amount of the token that is being swapped.
* @param swapPathMarkets An array of market properties, specifying the markets in which the swap should be executed.
* @param minOutputAmount The minimum amount of tokens that should be received as part of the swap.
* @param receiver The address to which the swapped tokens should be sent.
* @param uiFeeReceiver The address of the ui fee receiver.
* @param shouldUnwrapNativeToken A boolean indicating whether the received tokens should be unwrapped from the wrapped native token (WNT) if they are wrapped.
*/
struct SwapParams {
DataStore dataStore;
EventEmitter eventEmitter;
Oracle oracle;
Bank bank;
bytes32 key;
address tokenIn;
uint256 amountIn;
Market.Props[] swapPathMarkets;
uint256 minOutputAmount;
address receiver;
address uiFeeReceiver;
bool shouldUnwrapNativeToken;
}
​

If 0 is specified nothing is done if less amount is specified the action performs the action based on the specified amount in.

/**
* @dev Swaps a given amount of a given token for another token based on a
* specified swap path.
* @param params The parameters for the swap.
* @return A tuple containing the address of the token that was received as
* part of the swap and the amount of the received token.
*/
function swap(SwapParams memory params) external returns (address, uint256) {
​
@audit>> if (params.amountIn == 0) {
@audit>> return (params.tokenIn, params.amountIn);
}
​
if (params.swapPathMarkets.length == 0) {
​
@audit>> if (params.amountIn < params.minOutputAmount) {
revert Errors.InsufficientOutputAmount(params.amountIn, params.minOutputAmount);
}
​
if (address(params.bank) != params.receiver) {
params.bank.transferOut(
params.tokenIn,
params.receiver,
params.amountIn,
params.shouldUnwrapNativeToken
);
}
​
@audit>> return (params.tokenIn, params.amountIn);
}

Also note when the keeper calls run

function run(
bool isOpen,
bool isLong,
MarketPrices memory prices,
bytes[] memory metadata
) external nonReentrant {
_noneFlow();
_onlyKeeper();
if (gmxProxy.lowerThanMinEth()) {
revert Error.LowerThanMinEth();
}
flow = FLOW.SIGNAL_CHANGE;
​
if (isOpen) {
if (positionIsClosed) {
if (_isFundIdle() == false) {
revert Error.InsufficientAmount();
}
if (_isLongOneLeverage(isLong)) {
_runSwap(metadata, true, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(isLong, acceptablePrice, prices);
}
} else {
if (beenLong == isLong) {
revert Error.NoAction();
} else {
// Close current position first and then open the requested position in the next action
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(isLong);
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, false, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
​
@audit>> _createDecreasePosition(0, 0, beenLong, acceptablePrice, prices);
}
}
}
} else {
if (positionIsClosed == false) {
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, false, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
​
@audit>> _createDecreasePosition(0, 0, beenLong, acceptablePrice, prices);
}
} else {
revert Error.NoAction();
}
}
}

Even settle handles this as it should as GMX proxy accurately set delta to 1 to process the settling action

function settle(
IGmxProxy.OrderData memory orderData
) external returns (bytes32) {
require(msg.sender == perpVault, "invalid caller");
uint256 positionExecutionFee = getExecutionGasLimit(
Order.OrderType.MarketDecrease,
orderData.callbackGasLimit
) * tx.gasprice;
require(
address(this).balance >= positionExecutionFee,
"insufficient eth balance"
);
gExchangeRouter.sendWnt{value: positionExecutionFee}(
orderVault,
positionExecutionFee
);
CreateOrderParamsAddresses memory paramsAddresses = CreateOrderParamsAddresses({
​
@audit>>> receiver: perpVault,
​
cancellationReceiver: address(perpVault),
callbackContract: address(this),
uiFeeReceiver: address(0),
market: orderData.market,
initialCollateralToken: orderData.initialCollateralToken,
swapPath: new address[](0)
});
CreateOrderParamsNumbers memory paramsNumber = CreateOrderParamsNumbers({
​
@audit>>> sizeDeltaUsd: 0,
​
@audit>>> initialCollateralDeltaAmount: 1,
triggerPrice: 0,
acceptablePrice: 0,
executionFee: positionExecutionFee,
callbackGasLimit: orderData.callbackGasLimit,
minOutputAmount: 0, // this param is used when swapping. is not used in opening position even though swap involved.
validFromTime: 0
});

Impact

Wrong collateral delta amount will impact the closing order has, a force closure of all orders lead to other users not being able to access their collateral delta amount.

Tools Used

Manual review

Recommendations

Retrieve all collateral amount and process order has though it is a closed order.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

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.

Give us feedback!