diff --git a/contracts/FuturesMarket.sol b/contracts/FuturesMarket.sol index 988cff5460..bb7eec3be2 100644 --- a/contracts/FuturesMarket.sol +++ b/contracts/FuturesMarket.sol @@ -29,14 +29,12 @@ import "./interfaces/IFuturesMarket.sol"; * As the funding rate is the same (but negated) on both sides of the market, there is an excess quantity of * funding being charged, which is collected by the debt pool, and serves to reduce the system debt. * - * To combat front-running, the system does not confirm a user's order until the next price is received from - * the oracle. Therefore opening a position is a three stage procedure: depositing margin, submitting an order, - * and waiting for that order to be confirmed. The last transaction is performed by a keeper, - * once a price update is detected. - * * The contract architecture is as follows: * * - FuturesMarket.sol: one of these exists per asset. Margin is maintained isolated per market. + * this contract is composed of several mixins: `base` contains all the core logic, + * `nextPrice` contains the next-price order flows, and `views` contains logic + * that is only used by external / manager contracts. * * - FuturesMarketManager.sol: the manager keeps track of which markets exist, and is the main window between * futures markets and the rest of the system. It accumulates the total debt @@ -47,9 +45,6 @@ import "./interfaces/IFuturesMarket.sol"; * the base asset, these settings determine the behaviour of each market. * See that contract for descriptions of the meanings of each setting. * - * Each futures market and the manager operates behind a proxy, and for efficiency they communicate with one another - * using their underlying implementations. - * * Technical note: internal functions within the FuturesMarket contract assume the following: * * - prices passed into them are valid; diff --git a/contracts/FuturesMarketBase.sol b/contracts/FuturesMarketBase.sol index 28db6ea66b..1e220662d9 100644 --- a/contracts/FuturesMarketBase.sol +++ b/contracts/FuturesMarketBase.sol @@ -223,12 +223,7 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType // marketSize is in baseAsset units so we need to convert from USD units require(price > 0, "price can't be zero"); uint skewScaleBaseAsset = _skewScaleUSD(baseAsset).divideDecimal(price); - - // parameters may not be set, don't divide by zero - if (skewScaleBaseAsset == 0) { - return 0; - } - + require(skewScaleBaseAsset != 0, "skewScale is zero"); // don't divide by zero return int(marketSkew).divideDecimal(int(skewScaleBaseAsset)); } @@ -331,7 +326,7 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType * If the result is not an error, callers of this function that use it to update a position's margin * must ensure that this is accompanied by a corresponding debt correction update, as per `_applyDebtCorrection`. */ - function _realisedMargin( + function _recomputeMarginWithDelta( Position memory position, uint price, int marginDelta @@ -403,8 +398,10 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType // Hence, expanding the definition of remainingMargin the exact price // at which a position can first be liquidated is: // margin + profitLoss + funding = liquidationMargin - // profitLoss = (price - last-price) * positionSize - // price = lastPrice + (liquidationMargin - margin) / positionSize - netFundingPerUnit + // substitute with: profitLoss = (price - last-price) * positionSize + // and also with: funding = netFundingPerUnit * positionSize + // we get: margin + (price - last-price) * positionSize + netFundingPerUnit * positionSize = liquidationMargin + // moving around: price = lastPrice + (liquidationMargin - margin) / positionSize - netFundingPerUnit int result = int(position.lastPrice).add(int(liqMargin).sub(int(position.margin)).divideDecimal(positionSize)).sub( fundingPerUnit @@ -511,8 +508,6 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType return (oldPos, 0, Status.CanLiquidate); } - int newSize = int(oldPos.size).add(params.sizeDelta); - // get the dynamic fee rate SIP-184 (uint dynamicFeeRate, bool tooVolatile) = _dynamicFeeRate(); if (tooVolatile) { @@ -524,21 +519,27 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType // Deduct the fee. // It is an error if the realised margin minus the fee is negative or subject to liquidation. - (uint newMargin, Status status) = _realisedMargin(oldPos, params.price, -int(fee)); + (uint newMargin, Status status) = _recomputeMarginWithDelta(oldPos, params.price, -int(fee)); if (_isError(status)) { return (oldPos, 0, status); } + + // construct new position Position memory newPos = Position({ id: oldPos.id, lastFundingIndex: uint64(_latestFundingIndex()), margin: uint128(newMargin), lastPrice: uint128(params.price), - size: int128(newSize) + size: int128(int(oldPos.size).add(params.sizeDelta)) }); - // Check that the user has sufficient margin given their order. - // We don't check the margin requirement if the position size is decreasing + // always allow to decrease a position, otherwise a margin of minInitialMargin cen never + // decrease a position as as the price goes against them. + // we also add the paid out fee for the minInitialMargin because otherwise minInitialMargin + // is never the actual minMargin, because the first trade will always deduct + // a fee (so the margin that otherwise would need to be transferred would have to include the future + // fee as well, making the UX and definition of min-margin confusing). bool positionDecreasing = _sameSide(oldPos.size, newPos.size) && _abs(newPos.size) < _abs(oldPos.size); if (!positionDecreasing) { // minMargin + fee <= margin is equivalent to minMargin <= margin - fee @@ -548,11 +549,21 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType } } - // Check that the maximum leverage is not exceeded (ignoring the fee). + // check that new position margin is above liquidation margin + // (above, in _recomputeMarginWithDelta() we checked the old position, here we check the new one) + // Liquidation margin is considered without a fee, because it wouldn't make sense to allow + // a trade that will make the position liquidatable. + if (newMargin <= _liquidationMargin(newPos.size, params.price)) { + return (newPos, 0, Status.CanLiquidate); + } + + // Check that the maximum leverage is not exceeded when considering new margin including the paid fee. + // The paid fee is considered for the benefit of UX of allowed max leverage, otherwise, the actual + // max leverage is always below the max leverage parameter since the fee paid for a trade reduces the margin. // We'll allow a little extra headroom for rounding errors. { // stack too deep - int leverage = newSize.multiplyDecimal(int(params.price)).divideDecimal(int(newMargin.add(fee))); + int leverage = int(newPos.size).multiplyDecimal(int(params.price)).divideDecimal(int(newMargin.add(fee))); if (_maxLeverage(baseAsset).add(uint(_UNIT) / 100) < _abs(leverage)) { return (oldPos, 0, Status.MaxLeverageExceeded); } @@ -600,9 +611,7 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType // True if and only if two positions a and b are on the same side of the market; // that is, if they have the same sign, or either of them is zero. function _sameSide(int a, int b) internal pure returns (bool) { - // Since we only care about the sign of the product, we don't care about overflow and - // aren't using SignedSafeDecimalMath - return 0 <= a * b; + return (a >= 0) == (b >= 0); } /* @@ -679,9 +688,14 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType */ function _positionDebtCorrection(Position memory position) internal view returns (int) { /** - Debt correction calculation from the SIP https://sips.synthetix.io/sips/sip-80/ + This method only returns the correction term for the debt calculation of the position, and not it's + debt. This is needed for keeping track of the _markedDebt() in an efficient manner to allow O(1) marketDebt + calculation in _marketDebt(). + + Explanation of the full market debt calculation from the SIP https://sips.synthetix.io/sips/sip-80/: + The overall market debt is the sum of the remaining margin in all positions. The intuition is that - the debt of a single positino is the value withdrawn upon closing that position. + the debt of a single position is the value withdrawn upon closing that position. single position remaining margin = initial-margin + profit-loss + accrued-funding = = initial-margin + q * (price - last-prise) + q * funding-accrued-per-unit @@ -694,7 +708,9 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType = skew (price + funding) + sum( initial-margin + q * last-price + q * initial-funding ) The last term: sum( initial-margin + q * last-price + q * initial-funding ) being the position debt correction - that is tracked with each position change. And the first term is calculated globally in _marketDebt(), + that is tracked with each position change using this method. + + The first term and the full debt calculation using current skew, price, and funding is calculated globally in _marketDebt(). */ return int(position.margin).sub( @@ -767,7 +783,7 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType ) internal { Position memory oldPosition = position; // Determine new margin, ensuring that the result is positive. - (uint margin, Status status) = _realisedMargin(oldPosition, price, marginDelta); + (uint margin, Status status) = _recomputeMarginWithDelta(oldPosition, price, marginDelta); _revertIfError(status); // Update the debt correction. @@ -787,12 +803,13 @@ contract FuturesMarketBase is MixinFuturesMarketSettings, IFuturesMarketBaseType // The user can always decrease their margin if they have no position, or as long as: // * they have sufficient margin to do so - // * the resulting margin would not be lower than the minimum margin + // * the resulting margin would not be lower than the liquidation margin or min initial margin // * the resulting leverage is lower than the maximum leverage if (marginDelta < 0) { _revertIfError( - margin < _minInitialMargin() || - _maxLeverage(baseAsset) < _abs(_currentLeverage(position, price, margin)), + (margin < _minInitialMargin()) || + (margin <= _liquidationMargin(position.size, price)) || + (_maxLeverage(baseAsset) < _abs(_currentLeverage(position, price, margin))), Status.InsufficientMargin ); } diff --git a/contracts/test-helpers/TestableFuturesMarket.sol b/contracts/test-helpers/TestableFuturesMarket.sol index 84ff484c41..97527420d1 100644 --- a/contracts/test-helpers/TestableFuturesMarket.sol +++ b/contracts/test-helpers/TestableFuturesMarket.sol @@ -46,7 +46,7 @@ contract TestableFuturesMarket is FuturesMarket { * @return lMargin liquidation margin to maintain in sUSD fixed point decimal units */ function liquidationMargin(address account) external view returns (uint lMargin) { - require(positions[account].size != 0, "0 size position"); + require(positions[account].size != 0, "0 size position"); // reverts because otherwise minKeeperFee is returned (uint price, ) = assetPrice(); return _liquidationMargin(int(positions[account].size), price); }