Rysk

The function GmxHedgingReactor::gmxPositionCallback, which updates the internalDelta storage variable of the reactor as part of the two step process used by GMX to execute positions, does not impose a limit on the msg.sender or the amount of times that it may be called. An attacker could exploit this to completely mess up the reactor’s internalDelta.

A requirement that the msg.sender is the gmxPositionRouter should be added, as this would ensure that the gmxPositionCallback call comes after a successful call to gmxPositionRouter::execute(Increase/Decrease)Position made by a GMX keeper.

In the GmxHedgingReactor constructor the gmxPositionRouter is approved to spend an infinite amount of _collateralAsset. It appears that this is unneeded and potentially dangerous, as the transfer of _collateralAsset is actually handled by the GMX router, which gets approved for the exact amount needed in the function _increasePosition, and not by the gmxPositionRouter.

The function GmxHedgingReactor::_changePosition is not consistent with the values it returns. Even though it should always return the resulting difference in delta exposure, it does not do so at the end of the if-branch of the if (_amount > 0) { … } statement. If the control flow reaches that point, it jumps at the end of the function leading to 0 being returned, i.e., as if there was no change in delta.

function _changePosition(int256 _amount) internal returns (int256) {
  // ..
  if (_amount > 0) {
    // ..
    // Dedaub: last statement is not a return
    increaseOrderDeltaChange[positionKey] += deltaChange;
  } else {
    // ..
    return deltaChange + closedPositionDeltaChange;
  }
  return 0;
}

We would suggest the following fixes:

function _changePosition(int256 _amount) internal returns (int256) {
  // ..
  if (_amount > 0) {
    // ..
    return deltaChange + closedPositionDeltaChange;
  } else if (_amount < 0) {
    // ..
    return deltaChange + closedPositionDeltaChange;
  }
  return 0;
}

Currently the return value of _changePosition is further returned by the function hedgeDelta and remains unused by its callers. However, this could change in future versions of the protocol leading to bugs.

GMX treats longs and shorts as completely separate positions, and charges borrowing fees on both simultaneously, thus the reactor deals with positions in such a way that ensures only a single position is open at a certain time. Nevertheless, due to the two-step process that GMX uses to create positions and the fact that the reactor does not take into account that a new position might be created while another one is waiting to be finalized, there exists a scenario in which the reactor could end up with a long and a short position at the same time. The scenario is the following:

• Initially, there are no open positions
• A long or short position is opened on GMX but is not executed immediately, i.e., GmxHedgingReactor::gmxPositionCallback is not called.

The LiquidityPool reckons that a counter position should be opened and calls GmxHedgingReactor::hedgeDelta to do so.

• When the two position orders are finally executed by GMX the reactor will have a long and a short position open simultaneously.

The above scenario might not be likely to happen as it requires the LiquidityPool to open two opposite positions in a very short period of time, i.e., before the first position order is executed by a GMX keeper or a keeper of the protocol. Nevertheless, we believe it would be better to also handle such a scenario, as it could mess up the reactor’s accounting and the fix should be relatively easy.

Whenever the hedging reactor asks for an increase of a position, _getCollateralSizeDeltaUsd() computes the extra collateral needed using collateralToTransfer (collateral needed to be added or removed from the position before its increase, to maintain the health to the desired value) and extraPositionCollateral (the extra collateral needed for the increase of the position).If isAboveMax==true and extraPositionCollateral > collateralToTransfer, then the collateral which is actually added is just totalCollateralToAdd= extraPositionCollateral - collateralToTransfer, which could be not sufficient to collateralize the increased position.

Let us try to explain this with an example. Suppose that initially there is a long position with position[0]=10_000, position[1]=5_000. Hedging reactor then asks for an increase of its position by 11_000. extraPositionCollateral will be 5_500. Suppose than in the meantime this position had substantial profits i.e. positive unrealised pnl=5_000. colateralToTransfer will be 5_000 and totalCollateralToAdd will be 5_500-5_000=500. Therefore the "leverage without pnl" of the new position will be (10_000+11_000)/(5_000+500)=21_000/5_500=3.8. If this scenario is repeated, it could lead to the liquidation of the position.

We suggest adding a check that the total size of the position does not exceed its total collateral times maxLeverage, similar to the one used in the case of decreasing a position.

The function GmxHedgingReactor::setPositionRouter sets gmxPositionRouter to the new GMX PositionRouter contract that is provided and calls approvePlugin on the GMX Router contract to approve it. It does not revoke the approval to the old PositionRouter contract, which from now on is irrelevant to the reactor, by calling the function denyPlugin of the GMX Router contract.

If a position is in loss, the formula of the health variable is the following one:

// GmxHedgingReactor.sol::_getCollateralSizeDeltaUsd():344
health=(uint256((int256(position[1])-int256(position[8])).div(int256(position[0]))) * MAX_BIPS) / 1e18;

There is no check if the difference (int256(position[1])-int256(position[8])) in the above formula is positive or not. It is possible, under specific economic conditions (and if the GMX Liquidators are not fast enough), that the result of this difference is negative. In such a case, the resulting value will be erroneous because of an underflow error.

Even if this scenario is not expected to happen on a regular basis, we suggest adding a check that this difference is indeed positive and if it is not extra measures should be taken to avoid liquidations.

Note that the same issue appears in getPoolDenominatedValue, leading to the execution reverting if an underflow occurs.

The function GmxHedgingReactor::getPoolDenominatedValue wastes gas by calling the function checkVaultHealth to retrieve just the currently open GMX position instead of directly calling the _getPosition function.

The function GmxHedgingReactor::gmxPositionCallback is responsible for updating the internalDelta of the reactor with the values that are stored in the mappings increaseOrderDeltaChange and decreaseOrderDeltaChange. These mappings are essentially used as temporary storage before the change in delta is applied to the internalDelta storage variable. Thus, after a successful update the associated mapping element should be deleted to receive a gas refund for freeing up space on the blockchain.

The function GmxHedgingReactor::sync is implemented to consider the scenario where a long and a short position are open on GMX at the same time.

function sync() external returns (int256) {
  _isKeeper();
  uint256[] memory longPosition = _getPosition(true);
  uint256[] memory shortPosition = _getPosition(false);
  uint256 longDelta = longPosition[0] > 0 ?
    (longPosition[0]).div(longPosition[2]) : 0;
  uint256 shortDelta = shortPosition[0] > 0 ?
    (shortPosition[0]).div(shortPosition[2]) : 0;
  internalDelta = int256(longDelta) - int256(shortDelta);
  return internalDelta;
}

However, the reactor in whole is implemented in a way that ensures that a long and a short position cannot co-exist. Thus, the sync function can be implemented to take into account only the current open position, making it more efficient in terms of gas usage.

In GmxHedgingReactor::_getCollateralSizeDeltaUsd there is the following code:

// GmxHedgingReactor.sol::_getCollateralSizeDeltaUsd():670
if (
  int256(position[1] / 1e12) - int256(adjustedCollateralToRemove) <
  int256(((position[0] - _getPositionSizeDeltaUsd(_amount, position[0])) / 1e12) / (vault.maxLeverage() / 11000))
) {
  adjustedCollateralToRemove =
    position[1] / 1e12 -
    ((position[0]-_getPositionSizeDeltaUsd(_amount,position[0])) / 1e12) /
    (vault.maxLeverage() / 11000);
  if (adjustedCollateralToRemove == 0) {
    return 0;
  }
}

Observe that the quantity (position[0]-_getPositionSizeDeltaUsd(_amount, position[0])) / 1e12) / (vault.maxLeverage() / 11000) is computed twice which can be avoided by computing it once and storing its value to a local variable. The same is true for the quantity _amount.mul(position[2] / 1e12).div(position[0] / 1e12) that appears twice in the following computation:

// GmxHedgingReactor.sol::_getCollateralSizeDeltaUsd():651
collateralToRemove =
  (1e18 -
    (
      (int256(position[0]/1e12)+int256((leverageFactor.mul(position[8]))/1e12))
        .mul(1e18-int256(_amount.mul(position[2]/1e12).div(position[0]/1e12)))
        .div(int256(leverageFactor.mul(position[1])/1e12))
    )).mul(int256(position[1]/1e12)) -
  int256(_amount.mul(position[2]/1e12).div(position[0]/1e12)
    .mul(position[8]/1e12));

The above computation can be simplified even further by applying specific mathematical properties:

uint256 d = _amount.mul(position[2]).div(position[0]);
collateralToRemove =
  (int256(position[1] / 1e12) - (
    ((int256(position[0]) + int256(leverageFactor.mul(position[8]))) / 1e12)
      .mul(1e18 - int256(d)).div(int256(leverageFactor))
  )) - int256(d.mul(position[8] / 1e12));

The functions _increasePosition and _decreasePosition of the reactor unnecessarily call gmxPositionRouter’s minExecutionFee function twice each instead of caching the returned value in a local variable after the first call.

The comment describing the parameter _collateralSize of the function _increasePosition should read “amount of collateral to add” instead of "amount of collateral to remove”.

The following errors are defined but not used:

// GmxHedgingReactor.sol::_getCollateralSizeDeltaUsd():88
error ValueFailure();
error IncorrectCollateral();
error IncorrectDeltaChange();
error InvalidTransactionNotEnoughMargin(int256 accountMarketValue, int256 totalRequiredMargin);

The code is compiled with Solidity 0.8.9, which, at the time of writing, has some known bugs, which we do not believe to affect the correctness of the contracts.