Strategies Contracts - GoldLink

The creation of decrease orders mainly involves the execution of the OrderLogic.createDecreaseOrder function (i.e.: the caller) and the DeltaConvergenceMath.getDecreaseOrderValues function (i.e.: the callee). This latter function does not set the value of the position size (result.positionSizeNextUsd), which means that its value is 0. As a result, the following consequences are triggered:

Consequence #1:

Bypass of the order size validation during the execution of the OrderLogic.createDecreaseOrder function when creating decrease orders:

if (result.positionSizeNextUsd != 0 && sizeDeltaUsd != 0) {
  // Validate the order size. Only do this if remainingPositionSize != 0, since it may be impossible to
  // reduce the position size in the event the remaining size is less than the min order size.
  OrderValidation.validateOrderSize(
    marketConfig.orderPricingParameters.minOrderSizeUsd,
    marketConfig.orderPricingParameters.maxOrderSizeUsd,
    order.numbers.sizeDeltaUsd
  );
}

Consequence #2:

Bypass of the position size validation during the execution of the OrderValidation.validatePositionSize function when creating decrease orders:

Code Location

The getDecreaseOrderValues function does not set the value of result.positionSizeNextUsd:

function getDecreaseOrderValues(
  IGmxFrfStrategyManager manager,
  uint256 sizeDeltaUsd,
  DeltaCalculationParameters memory values
) internal view returns (DecreasePositionResult memory result) {
  PositionTokenBreakdown memory breakdown = getAccountMarketDelta(
    manager,
    values.account,
    values.marketAddress,
    sizeDeltaUsd,
    false
  );

  // The total cost amount is equal to the sum of the fees associated with the decrease, in terms of the collateral token.
  // This accounts for negative funding fees, borrowing fees,
  uint256 collateralLostInDecrease = breakdown
    .positionInfo
    .fees
    .totalCostAmount;

  {
    uint256 profitInCollateralToken = SignedMath.abs(
      breakdown.positionInfo.pnlAfterPriceImpactUsd
    ) / values.longTokenPrice;

    if (breakdown.positionInfo.pnlAfterPriceImpactUsd > 0) {
      collateralLostInDecrease -= Math.min(
        collateralLostInDecrease,
        profitInCollateralToken
      ); // Offset the loss in collateral with position profits.
    } else {
      collateralLostInDecrease += profitInCollateralToken; // adding because this variable is meant to represent a net loss in collateral.
    }
  }

  uint256 sizeDeltaActual = Math.min(
    sizeDeltaUsd,
    breakdown.positionInfo.position.numbers.sizeInUsd
  );

  uint256 shortTokensAfterDecrease;

  {
    uint256 proportionalDecrease = sizeDeltaActual.fractionToPercent(
      breakdown.positionInfo.position.numbers.sizeInUsd
    );

    shortTokensAfterDecrease =
    breakdown.tokensShort -
    breakdown
      .positionInfo
      .position
      .numbers
      .sizeInTokens
      .percentToFraction(proportionalDecrease);
  }

  uint256 longTokensAfterDecrease = breakdown.tokensLong -
    collateralLostInDecrease;

  // This is the difference in long vs short tokens currently.
  uint256 imbalance = Math.max(
    shortTokensAfterDecrease,
    longTokensAfterDecrease
  ) - Math.min(shortTokensAfterDecrease, longTokensAfterDecrease);

  if (shortTokensAfterDecrease < longTokensAfterDecrease) {
    // We need to remove long tokens equivalent to the imbalance to make the position delta neutral.
    // However, it is possible that there are a significant number of long tokens in the contract that are impacting the imbalance.
    // If this is the case, then if we were to simply remove the imbalance, it can result in a position with very high leverage. Therefore, we will simply remove
    // the minimum of `collateralAmount - collateralLostInDecrease` the difference in the longCollateral and shortTokens. The rest of the delta imbalance can be left to rebalancers.
    uint256 remainingCollateral = breakdown
    .positionInfo
    .position
    .numbers
    .collateralAmount - collateralLostInDecrease;

    if (remainingCollateral > shortTokensAfterDecrease) {
      result.collateralToRemove = Math.min(
        remainingCollateral - shortTokensAfterDecrease,
        imbalance
      );
    }
  }

  if (result.collateralToRemove != 0) {
    (uint256 expectedSwapOutput, , ) = manager
      .gmxV2Reader()
      .getSwapAmountOut(
        manager.gmxV2DataStore(),
        values.market,
        _makeMarketPrices(
          values.shortTokenPrice,
          values.longTokenPrice
        ),
        values.market.longToken,
        result.collateralToRemove,
        values.uiFeeReceiver
      );

    result.estimatedOutputUsd =
    expectedSwapOutput *
    values.shortTokenPrice;
  }

  if (breakdown.positionInfo.pnlAfterPriceImpactUsd > 0) {
    result.estimatedOutputUsd += SignedMath.abs(
      breakdown.positionInfo.pnlAfterPriceImpactUsd
    );
  }

  result.executionPrice = breakdown
    .positionInfo
    .executionPriceResult
    .executionPrice;
}

It is recommended to correctly set the value of the position size when creating decrease orders.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The isLiquidationFinished function in the AccountGetters library incorrectly assigns the addresses for the long and short tokens. It happens because when calling the GmxMarketGetters.getMarketTokens function, the returned values are assigned to the addresses in the opposite order, i.e.: short token has the address of the long one and viceversa.

As a consequence, the isLiquidationFinished function won't return accurate results, which means that some lenders wouldn't receive their payments even if the liquidation process had indeed finished.

Code Location

The AccountGetters.isLiquidationFinished function incorrectly assigns the addresses for the long and short tokens when calling GmxMarketGetters.getMarketTokens:

// Get all available markets to check funding fees for.
address[] memory markets = manager.getAvailableMarkets();

for (uint256 i = 0; i < markets.length; ++i) {
  (address longToken, address shortToken) = GmxMarketGetters
    .getMarketTokens(dataStore, markets[i]);

The GmxMarketGetters.getMarketTokens function returns the values of the short and long tokens respectively, which is the opposite from what is expected in the function above:

function getMarketTokens(
  IGmxV2DataStore dataStore,
  address market
) internal view returns (address shortToken, address longToken) {
  return (
    getShortToken(dataStore, market),
    getLongToken(dataStore, market)
  );
}

It is recommended to assign the addresses correctly for the long and short tokens.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

In order to prevent gaming and token rebalances by continuously sending tokens to the account, the swapRebalancePosition function in the LiquidationLogic library requires that the size of the swap rebalance must either:

1. Leave the account with zero delta.

2. Leave the account with zero tokens that can be atomically swapped.

3. Leave the account with a balance that is greater than or equal to the minimum swap rebalance size.

However, the first condition is not correctly implemented because it compares the value of breakdown.tokensLong - breakdown.tokensShort against the remaining variable, instead of comparing it against the rebalanceAmount variable. As a consequence, when executing swap rebalances, the transaction could unnecessarily revert, making the operation temporarily unavailable.

Code Location

The swapRebalancePosition function does not correctly implement the first condition in the require function:

require(
  remaining == breakdown.tokensLong - breakdown.tokensShort ||
  remaining == 0 ||
  remaining >= unwindConfig.minSwapRebalanceSize,
  GmxFrfStrategyErrors
    .LIQUIDATION_MANAGEMENT_REBALANCE_AMOUNT_LEAVE_TOO_LITTLE_REMAINING_ASSETS
);

It is recommended to correctly implement the conditions regarding the size of the swap rebalances.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The isInMulticall_ variable has a initial value of 1 when declared in the GmxStrategyStorage contract. This is equivalent to setting this value in the constructor, and as such, will not work for upgradeable contracts, which means that any upgradeable instances will not have this field set.

In this particular case, the multicall function in the GmxFrfStrategyAccount will always revert with the error message Nested multicalls are not allowed when invoking it because the isInMulticall_ variable will remain with 0 as a value, instead of 1 as expected. The described issue directly affects the logic for paying execution fee / gas.

Code Location

The isInMulticall_ variable has a initial value of 1 when declared in the GmxStrategyStorage contract:

/// @notice Should be set when a multicall is active to prevent nested multicalls.
/// The value `1` implies the contract is not current executing a multicall.
/// The value `2` implies that the contract is currently executing a multicall.
uint256 internal isInMulticall_ = 1;

It is recommended to make sure that all initial values are set in an initializer function.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The GmxFrfStrategyAccount contract is using the Initializable module from OpenZeppelin. In order to prevent leaving the contract uninitialized, OpenZeppelin's documentation recommends adding the _disableInitializers function in the constructor to automatically lock the contract when it is deployed.

Code Location

The constructor in the GmxFrfStrategyAccount contract does not include the _disableInitializers function:

// ============ Constructor ============

/**
 * @notice Constructor for upgradeable contract, distinct from initializer.
 *
 *  The constructor is used to set immutable variables, and for top-level upgradeable
 *  contracts, it is also used to disable the initializer of the logic contract.
 *
 *  Note that since this contract is used with beacon proxies, the immutable variables will
 *  be constant across all proxies pointing to the same beacon.
 */
constructor(IGmxFrfStrategyManager manager) GmxStrategyStorage(manager) {}

It is recommended to call the _disableInitializers function in the contract's constructor.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

Reading the length of the array at each iteration of the loop requires 6 gas (3 for mload and 3 to place memory_offset) onto the stack. Caching the length of the array on the stack saves about 3 gas per iteration. The affected functions are the following:

• AccountGetters.isLiquidationFinished

• AccountGetters.getAccountOrdersValueUSD

• AccountGetters.getAccountPositionsValueUSD

• AccountGetters.getSettledFundingFeesValueUSD

• AccountGetters._getAccountTokenValueUSD

• OrderValidation.validateNoPendingOrdersInMarket

• GmxFrfStrategyAccount.multicall

• GmxFrfStrategyManager.setMarket

It is recommended to consider caching the length of the arrays.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The rebalancePosition function in the LiquidationLogic library reverts if the position's delta proportion is less or equal than the threshold defined in the market's unwind configuration. However, in order to maintain the consistency along the codebase (e.g.: with swapRebalancePosition function), the function should revert only when the position's delta proportion is less than the mentioned threshold.

It is recommended to update the logic of the rebalancePosition function to revert only when the position's delta proportion is less than the configured threshold.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

Some functions in the codebase do not include a zero address check for their parameters. If one of those parameters is mistakenly set to zero, it could affect the correct operation of the protocol. The affected functions are the following:

• MarketConfigurationManager._setUiFeeReceiver

• GmxStrategyStorage.constructor

• GmxFrfStrategyAccount.initialize

• GmxFrfStrategyDeployer.constructor

• GmxFrfStrategyDeployer.deployAccount

It is recommended to add a zero address check in the functions mentioned above.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

Some functions in the GmxFrfStrategyAccount contract do not verify if the markets they are interacting with have been approved by the GmxFrfStrategyManager contract, e.g.: by using the onlyApprovedMarket modifier. The affected functions are the following:

• GmxFrfStrategyAccount.executeClaimFundingFees

• GmxFrfStrategyAccount.executeClaimCollateral

• GmxFrfStrategyAccount.executeLiquidatePosition

• GmxFrfStrategyAccount.executeReleveragePosition

• GmxFrfStrategyAccount.executeSwapRebalance

• GmxFrfStrategyAccount.executeRebalancePosition

This issue has been classified as Informational because in the mentioned functions, there cannot be position / funding fees for non-approved markets. However, it is included in the report as part of a security-in-depth approach to harden the functions if the codebase is later refactored.

It is recommended to verify in the mentioned functions if the markets are approved before further processing.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The getMinimumAcceptablePriceForDecrease function in the OrderHelpers library calculates the maximum acceptable price used as threshold when validating the prices for decrease orders. However, the name of the function suggests that the value calculated represents a minimum threshold instead of a maximum one, which could mislead users and even developers if the codebase is later refactored.

It is recommended to update the name of the mentioned function to reflect its real behavior.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

Along the codebase, there are some functions that include inaccurate comments, which could mislead users or developers (if code is later refactored) when trying to understand the expected behavior of the functions. The affected resources are the following:

DeltaConvergenceMath._getLeverage:

The comment indicates the following regarding the net collateral value:

// Net Collateral Value: Calculated using the following formula: (collateral amount tokens - funding fees tokens - borrowing fees tokens) * token price usd + pnl usd.

However the code calculates its value in a different way (using totalCostAmount):

uint256 collateralInTokens = breakdown
         .positionInfo
         .position
         .numbers
         .collateralAmount - breakdown.positionInfo.fees.totalCostAmount;

GmxFrfStrategyAccount.executeLiquidateAssets:

The comment indicates the following regarding the executeLiquidateAssets function:

* @notice Liquidates the specified `asset` in the amount of `amount` to the `receiever` address. Can only be called when the account has no active loan. The `callback` address must be

However, the function uses the hasActiveLoan modifier, which indicates the opposite behavior:

function executeLiquidateAssets(
  address asset,
  uint256 amount,
  address callback,
  address receiever
) external strategyNonReentrant whenLiquidating hasActiveLoan {

It is recommended to update the comments in the code to reflect the actual behavior of the mentioned functions.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.