Skip to content

Commit

Permalink
fix: gmx v2 leverage trading audit fixes (#1274)
Browse files Browse the repository at this point in the history 
* fix: gmx v2 leverage trading audit fixes

* fix: track only market to callback set

* fix: allow to increase exection fee for update orders

* chore: rename crea withdraw order func

* Update contracts/release/extensions/external-position-manager/external-positions/gmx-v2-leverage-trading/GMXV2LeverageTradingPositionLib.sol

Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>

* fix: claimable collateral overflow

* fix: improve comments

* fix: account for execution fee

* Update contracts/release/extensions/external-position-manager/external-positions/gmx-v2-leverage-trading/GMXV2LeverageTradingPositionLib.sol

* fix: cr fixes

---------

Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>
  • Loading branch information
@KedziaPawel @gabrocheleau
KedziaPawel and gabrocheleau authored Aug 20, 2024
1 parent 207a460 commit 0f13246
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 25 deletions.
53 changes: 42 additions & 11 deletions ...on-manager/external-positions/gmx-v2-leverage-trading/GMXV2LeverageTradingPositionLib.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,13 @@ contract GMXV2LeverageTradingPositionLib is
IGMXV2LeverageTradingPosition.UpdateOrderActionArgs memory updateOrderArgs =
abi.decode(_actionArgs, (IGMXV2LeverageTradingPosition.UpdateOrderActionArgs));

if (updateOrderArgs.executionFeeIncrease != 0) {
IERC20(address(WRAPPED_NATIVE_TOKEN)).safeTransfer({
_to: __getOrderVaultAddress(updateOrderArgs.exchangeRouter),
_value: updateOrderArgs.executionFeeIncrease
});
}

IGMXV2ExchangeRouter(updateOrderArgs.exchangeRouter).updateOrder({
_key: updateOrderArgs.key,
_sizeDeltaUsd: updateOrderArgs.sizeDeltaUsd,
Expand Down Expand Up @@ -365,8 +372,10 @@ contract GMXV2LeverageTradingPositionLib is
uint256 claimableCollateral = DATA_STORE.getUint(claimableCollateralAmountKey);
uint256 claimedCollateral = DATA_STORE.getUint(claimedCollateralAmountKey);

// check if all the collateral was claimed
if (claimableCollateral - claimedCollateral == 0) {
// Check if all the collateral was claimed.
// In GMX, MarketUtils.claimCollateral() computes the amount of collateral that can be claimed by an account for a market. As the claimable collateral can be released over time, a claimableFactor exists to determine the amount of collateral that can be claimed at a given time.
// An additional check is then performed that adjustedClaimableAmount > claimedAmount. Therefore, GMX remains conservative by handling the unlikely case where the claimed collateral amount exceeds the claimable collateral amount, and we do the same here.
if (claimableCollateral <= claimedCollateral) {
// even if the key won't be removed, because callback that adds collateral key wasn't called (due to for example out of gas error), this code won't revert
delete claimableCollateralKeyToClaimableCollateralInfo[claimableCollateralAmountKey];
bool removed = claimableCollateralKeys.removeStorageItem(claimableCollateralAmountKey);
Expand Down Expand Up @@ -468,15 +477,15 @@ contract GMXV2LeverageTradingPositionLib is

/// @dev Helper to set the saved callback contract on the GMX ExchangeRouter, it will be called on liquidations and auto deleveraging
function __setSavedCallbackContract(address _exchangeRouter, address _market) private {
if (!exchangeRouterToMarketToIsCallbackContractSet[_exchangeRouter][_market]) {
if (!marketToIsCallbackContractSet[_market]) {
IGMXV2ExchangeRouter(_exchangeRouter).setSavedCallbackContract({
_market: _market,
_callbackContract: address(this)
});

exchangeRouterToMarketToIsCallbackContractSet[_exchangeRouter][_market] = true;
marketToIsCallbackContractSet[_market] = true;

emit CallbackContractSet({exchangeRouter: _exchangeRouter, market: _market});
emit CallbackContractSet(_market);
}
}

Expand All @@ -495,6 +504,11 @@ contract GMXV2LeverageTradingPositionLib is
function __addClaimableCollateral(address _market, address _token, uint256 _timeKey) private {
bytes32 key = __claimableCollateralAmountKey({_market: _market, _token: _token, _timeKey: _timeKey});

// skip if the claimable collateral key already exists
if (claimableCollateralKeyToClaimableCollateralInfo[key].market != address(0)) {
return;
}

claimableCollateralKeys.push(key);
claimableCollateralKeyToClaimableCollateralInfo[key] =
ClaimableCollateralInfo({token: _token, market: _market, timeKey: _timeKey});
Expand Down Expand Up @@ -643,13 +657,13 @@ contract GMXV2LeverageTradingPositionLib is
/// @return assets_ Managed assets
/// @return amounts_ Managed asset amounts
/// @dev There are 5 ways that positive value can be contributed to this position
/// 1. Collateral in the GMX positions
/// 2. Pending market increase orders
/// 1. Collateral in the GMX positions, taking into account price impact and profit/loss
/// 2. Pending orders: deposited collateral in market increase orders, plus execution fees of all orders
/// 3. Assets held by the External Position. Those assets are monitored via the trackedAssets variable
/// 4. Collateral that can be claimed from GMX positions where collateral was freed up from decreased positions.
/// 4. Collateral that eventually can be claimed from GMX positions where collateral was freed up from decreased positions.
/// 5. Funding fees that can be claimed from the GMX protocol
function getManagedAssets() external view override returns (address[] memory assets_, uint256[] memory amounts_) {
// 1. Get the value of the collateral in active GMX positions
// 1. Get the value of the collateral in active GMX positions, taking into account price impact and profit/loss

bytes32[] memory positionsKeys = DATA_STORE.getBytes32ValuesAt({
_setKey: keccak256(abi.encode(ACCOUNT_POSITION_LIST_DATA_STORE_KEY, address(this))),
Expand Down Expand Up @@ -721,15 +735,25 @@ contract GMXV2LeverageTradingPositionLib is
}
}

// 2. Get pending market increase orders value
// 2. Get pending orders: deposited collateral in market increase orders, plus execution fees of all orders
IGMXV2Order.Props[] memory orders = __getAccountOrders();

uint256 totalExecutionFee;

for (uint256 i; i < orders.length; i++) {
IGMXV2Order.Props memory order = orders[i];

if (order.numbers.orderType == IGMXV2Order.OrderType.MarketIncrease) {
assets_ = assets_.addItem(order.addresses.initialCollateralToken);
amounts_ = amounts_.addItem(order.numbers.initialCollateralDeltaAmount);
}

totalExecutionFee += order.numbers.executionFee;
}

if (totalExecutionFee != 0) {
assets_ = assets_.addItem(address(WRAPPED_NATIVE_TOKEN));
amounts_ = amounts_.addItem(totalExecutionFee);
}

// 3. Get the value of the assets held by the External Position. Those assets could be here because of the: liquidations, order cancellation, or refund of the execution fee
Expand All @@ -749,7 +773,7 @@ contract GMXV2LeverageTradingPositionLib is
assets_ = assets_.addItem(address(WRAPPED_NATIVE_TOKEN));
}

// 4. Get the value of the collateral that can be claimed from GMX positions where collateral was freed up from decreased positions.
// 4. Get the value of the collateral that eventually can be claimed from GMX positions where collateral was freed up from decreased positions.
for (uint256 i; i < claimableCollateralKeys.length; i++) {
bytes32 key = claimableCollateralKeys[i];

Expand Down Expand Up @@ -816,6 +840,13 @@ contract GMXV2LeverageTradingPositionLib is
return claimableCollateralKeyToClaimableCollateralInfo[_key];
}

/// @notice Retrieves the status of whether the callback contract is set for a given market.
/// @param _market The address of the market to check.
/// @return isCallbackContractSet_ A boolean value indicating if the callback contract is set for the specified market.
function getMarketToIsCallbackContractSet(address _market) public view returns (bool isCallbackContractSet_) {
return marketToIsCallbackContractSet[_market];
}

/// @notice Retrieves the tracked assets
/// @return trackedAssets_ The trackedAssets
function getTrackedAssets() public view returns (address[] memory trackedAssets_) {
Expand Down
11 changes: 11 additions & 0 deletions ...manager/external-positions/gmx-v2-leverage-trading/GMXV2LeverageTradingPositionParser.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,17 @@ contract GMXV2LeverageTradingPositionParser is IExternalPositionParser {
assetsToTransfer_[0] = WRAPPED_NATIVE_TOKEN_ADDRESS;
amountsToTransfer_[0] = createOrderArgs.numbers.executionFee;
}
} else if (_actionId == uint256(IGMXV2LeverageTradingPosition.Actions.UpdateOrder)) {
IGMXV2LeverageTradingPosition.UpdateOrderActionArgs memory updateOrderArgs =
abi.decode(_encodedActionArgs, (IGMXV2LeverageTradingPosition.UpdateOrderActionArgs));

if (updateOrderArgs.executionFeeIncrease != 0) {
assetsToTransfer_ = new address[](1);
amountsToTransfer_ = new uint256[](1);

assetsToTransfer_[0] = WRAPPED_NATIVE_TOKEN_ADDRESS;
amountsToTransfer_[0] = updateOrderArgs.executionFeeIncrease;
}
} else if (_actionId == uint256(IGMXV2LeverageTradingPosition.Actions.CancelOrder)) {
IGMXV2LeverageTradingPosition.CancelOrderActionArgs memory cancelOrderArgs =
abi.decode(_encodedActionArgs, (IGMXV2LeverageTradingPosition.CancelOrderActionArgs));
Expand Down
3 changes: 3 additions & 0 deletions ...tion-manager/external-positions/gmx-v2-leverage-trading/IGMXV2LeverageTradingPosition.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ interface IGMXV2LeverageTradingPosition is IExternalPosition {
uint256 triggerPrice;
uint256 minOutputAmount;
bool autoCancel;
uint256 executionFeeIncrease;
address exchangeRouter;
}

Expand Down Expand Up @@ -85,6 +86,8 @@ interface IGMXV2LeverageTradingPosition is IExternalPosition {
view
returns (GMXV2LeverageTradingPositionLibBase1.ClaimableCollateralInfo memory info_);

function getMarketToIsCallbackContractSet(address _market) external view returns (bool isCallbackContractSet_);

function getTrackedAssets() external view returns (address[] memory trackedAssets_);

function getTrackedMarkets() external view returns (address[] memory trackedMarkets_);
Expand Down
4 changes: 2 additions & 2 deletions ...external-positions/gmx-v2-leverage-trading/bases/GMXV2LeverageTradingPositionLibBase1.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pragma solidity 0.8.19;
/// a numbered GMXV2LeverageTradingPositionLibBaseXXX that inherits the previous base.
/// e.g., `GMXV2LeverageTradingPositionLibBase2 is GMXV2LeverageTradingPositionLibBase1`
abstract contract GMXV2LeverageTradingPositionLibBase1 {
event CallbackContractSet(address exchangeRouter, address market);
event CallbackContractSet(address market);

event ClaimableCollateralAdded(bytes32 claimableCollateralKey, address token, address market, uint256 timeKey);

Expand All @@ -34,7 +34,7 @@ abstract contract GMXV2LeverageTradingPositionLibBase1 {
event TrackedMarketRemoved(address market);

/// @dev Keeps track of whether a particular market has been assigned the callback contract (i.e. the EP)
mapping(address => mapping(address => bool)) internal exchangeRouterToMarketToIsCallbackContractSet;
mapping(address => bool) internal marketToIsCallbackContractSet;

/// @dev Tracked assets that are receivable by the EP through actions that are triggered externally (e.g. order cancellations/liquidations)
address[] internal trackedAssets;
Expand Down
41 changes: 29 additions & 12 deletions tests/tests/protocols/gmx-v2/GMXV2LeverageTradingPosition.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ uint256 constant GMX_ONE_UNIT = 10 ** 30;
abstract contract TestBase is IntegrationTest {
using AddressArrayLib for address[];

event CallbackContractSet(address exchangeRouter, address market);
event CallbackContractSet(address market);

event ClaimableCollateralAdded(bytes32 claimableCollateralKey, address token, address market, uint256 timeKey);

Expand Down Expand Up @@ -629,7 +629,7 @@ abstract contract TestBase is IntegrationTest {
if (isWrappedNativeToken) {
assertEq(
postExecuteOrderManagedAssetAmounts[i],
postCreateOrderManagedAssetAmounts[i] + executionFee,
postCreateOrderManagedAssetAmounts[i],
"Incorrect managedAssetAmount wrapped native post execute order"
);
if (isCollateralToken) {
Expand Down Expand Up @@ -694,7 +694,7 @@ abstract contract TestBase is IntegrationTest {
IGMXV2Market.Props memory marketInfo = __getMarketInfo(_market);

expectEmit(address(externalPosition));
emit CallbackContractSet({exchangeRouter: address(exchangeRouter), market: _market});
emit CallbackContractSet(_market);

address[] memory trackedAssetsToAdd;
trackedAssetsToAdd = trackedAssetsToAdd.addItem(marketInfo.longToken);
Expand Down Expand Up @@ -738,6 +738,8 @@ abstract contract TestBase is IntegrationTest {
_assets: new address[](0)
});

assertEq(externalPosition.getMarketToIsCallbackContractSet(_market), true, "Incorrect market callback contract");

bool isCollateralWrappedNativeToken = _initialCollateralToken == address(wrappedNativeToken);

// assert that position value takes into account pending market increase order
Expand All @@ -750,15 +752,17 @@ abstract contract TestBase is IntegrationTest {
assertEq(externalPosition.getTrackedMarkets(), toArray(_market), "Incorrect tracked markets");

assertEq(
postCreateOrderManagedAssets, toArray(_initialCollateralToken), "Incorrect managedAssets post order create"
postCreateOrderManagedAssets,
isCollateralWrappedNativeToken
? toArray(_initialCollateralToken)
: toArray(_initialCollateralToken, address(wrappedNativeToken)),
"Incorrect managedAssets post order create"
);
assertEq(
postCreateOrderManagedAssetAmounts,
toArray(
isCollateralWrappedNativeToken
? _initialCollateralDeltaAmount - executionFee
: _initialCollateralDeltaAmount
),
isCollateralWrappedNativeToken
? toArray(_initialCollateralDeltaAmount)
: toArray(_initialCollateralDeltaAmount, executionFee),
"Incorrect managedAssetAmounts post order create"
);

Expand Down Expand Up @@ -874,6 +878,10 @@ abstract contract TestBase is IntegrationTest {
_orderType: IGMXV2OrderProd.OrderType.LimitDecrease
});

increaseTokenBalance({_token: wrappedNativeToken, _to: vaultProxyAddress, _amount: executionFee});

uint256 vaultWrappedNativeTokenBalancePreUpdateOrder = IERC20(wrappedNativeToken).balanceOf(vaultProxyAddress);

bytes32 orderKey = __getLastOrderKey();

IGMXV2LeverageTradingPositionProd.UpdateOrderActionArgs memory updateParams = IGMXV2LeverageTradingPositionProd
Expand All @@ -884,7 +892,8 @@ abstract contract TestBase is IntegrationTest {
triggerPrice: oldTriggerPrice - 1,
minOutputAmount: oldMinOutputAmount + 1,
exchangeRouter: address(exchangeRouter),
autoCancel: !oldAutoCancel
autoCancel: !oldAutoCancel,
executionFeeIncrease: executionFee
});

vm.recordLogs();
Expand All @@ -910,6 +919,14 @@ abstract contract TestBase is IntegrationTest {
order.numbers.minOutputAmount, updateParams.minOutputAmount, "Incorrect minOutputAmount post update order"
);
assertEq(order.flags.autoCancel, updateParams.autoCancel, "Incorrect autoCancel post update order");

// assert that execution fee was transferred, and increased
assertEq(
IERC20(wrappedNativeToken).balanceOf(vaultProxyAddress),
vaultWrappedNativeTokenBalancePreUpdateOrder - executionFee,
"Incorrect wrappedNativeToken balance post update order"
);
assertEq(order.numbers.executionFee, executionFee + executionFee, "Incorrect executionFee post update order");
}

function __test_cancelDecreaseOrder_success(
Expand Down Expand Up @@ -967,7 +984,7 @@ abstract contract TestBase is IntegrationTest {
if (postCancelOrderManagedAssets[i] == address(wrappedNativeToken)) {
assertEq(
postCancelOrderManagedAssetAmounts[i],
preCancelOrderWrappedNativeTokenBalance - externalPositionNativeBalance,
preCancelOrderWrappedNativeTokenBalance - externalPositionNativeBalance - executionFee,
"Incorrect wrapped native managedAssetAmount post cancel order"
);
}
Expand Down Expand Up @@ -2106,7 +2123,7 @@ contract GMXV2LeverageTradingPositionArbitrumTest is GMXV2LeverageTradingPositio
if (managedAssets[i] == ARBITRUM_WETH) {
assertEq(
managedAssetAmounts[i],
pendingIncreaseOrderInitialCollateralDeltaAmount - executionFee,
pendingIncreaseOrderInitialCollateralDeltaAmount,
"Incorrect WETH token EP balance post sweep"
);
}
Expand Down

0 comments on commit 0f13246

Please sign in to comment.