From 5998909203272a23737de5bbafd05537959d081f Mon Sep 17 00:00:00 2001 From: Ed Noepel Date: Mon, 16 Dec 2024 19:18:56 -0500 Subject: [PATCH 1/2] prevent close order from executing if account has no position --- .../perennial-order/contracts/Manager.sol | 2 +- .../contracts/test/TriggerOrderTester.sol | 4 +- .../contracts/types/TriggerOrder.sol | 21 +++-- .../test/integration/Manager_Arbitrum.test.ts | 7 ++ .../test/unit/TriggerOrder.test.ts | 83 ++++++++++++++----- 5 files changed, 90 insertions(+), 27 deletions(-) diff --git a/packages/perennial-order/contracts/Manager.sol b/packages/perennial-order/contracts/Manager.sol index 845b7a1e1..b6ae46d85 100644 --- a/packages/perennial-order/contracts/Manager.sol +++ b/packages/perennial-order/contracts/Manager.sol @@ -131,7 +131,7 @@ abstract contract Manager is IManager, Kept { order = _orders[market][account][orderId].read(); // prevent calling canExecute on a spent or empty order if (order.isSpent || order.isEmpty()) revert ManagerInvalidOrderNonceError(); - canExecute = order.canExecute(market.oracle().latest()); + canExecute = order.canExecute(market, account); } /// @inheritdoc IManager diff --git a/packages/perennial-order/contracts/test/TriggerOrderTester.sol b/packages/perennial-order/contracts/test/TriggerOrderTester.sol index 8f3d27cf0..c656407b5 100644 --- a/packages/perennial-order/contracts/test/TriggerOrderTester.sol +++ b/packages/perennial-order/contracts/test/TriggerOrderTester.sol @@ -21,8 +21,8 @@ contract TriggerOrderTester { order.store(newOrder); } - function canExecute(TriggerOrder calldata order_, OracleVersion calldata version) external pure returns (bool) { - return order_.canExecute(version); + function canExecute(TriggerOrder calldata order_, IMarket market, address user) external view returns (bool) { + return order_.canExecute(market, user); } function notionalValue(TriggerOrder calldata order_, IMarket market, address user) external view returns (UFixed6) { diff --git a/packages/perennial-order/contracts/types/TriggerOrder.sol b/packages/perennial-order/contracts/types/TriggerOrder.sol index 6f800d335..459908394 100644 --- a/packages/perennial-order/contracts/types/TriggerOrder.sol +++ b/packages/perennial-order/contracts/types/TriggerOrder.sol @@ -38,13 +38,24 @@ library TriggerOrderLib { /// @notice Determines whether the trigger order is fillable at the latest price /// @param self Trigger order - /// @param latestVersion Latest oracle version + /// @param market Market for which the order is intended to be executed + /// @param account Market participant /// @return Whether the trigger order is fillable - function canExecute(TriggerOrder memory self, OracleVersion memory latestVersion) internal pure returns (bool) { + function canExecute(TriggerOrder memory self, IMarket market, address account) internal view returns (bool) { + OracleVersion memory latestVersion = market.oracle().latest(); + if (!latestVersion.valid) return false; - if (self.comparison == 1) return latestVersion.price.gte(self.price); - if (self.comparison == -1) return latestVersion.price.lte(self.price); - return false; + if (self.comparison == 1 && latestVersion.price.lt(self.price)) return false; + if (self.comparison == -1 && latestVersion.price.gt(self.price)) return false; + if (self.comparison != 1 && self.comparison != -1) revert TriggerOrderInvalidError(); + + if (self.delta.eq(MAGIC_VALUE_CLOSE_POSITION)) { + Position memory position = market.positions(account); + // prevent execution if the position was not opened or is already closed + if (position.empty()) return false; + } + + return true; } /// @notice Applies the order to the user's position and updates the market diff --git a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts index d9565e3c2..a4db2d2cf 100644 --- a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts +++ b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts @@ -712,6 +712,13 @@ describe('Manager_Arbitrum', () => { await ensureNoPosition(userA) await market.settle(userB.address, TX_OVERRIDES) await ensureNoPosition(userB) + + // cannot close with no position + orderId = await placeOrder(userB, Side.LONG, Compare.LTE, parse6decimal('4001'), MAGIC_VALUE_CLOSE_POSITION) + expect(orderId).to.equal(BigNumber.from(505)) + await expect( + manager.connect(keeper).executeOrder(market.address, userB.address, orderId, TX_OVERRIDES), + ).to.be.revertedWithCustomError(manager, 'ManagerCannotExecuteError') }) }) diff --git a/packages/perennial-order/test/unit/TriggerOrder.test.ts b/packages/perennial-order/test/unit/TriggerOrder.test.ts index 02c97600d..e96cd79b1 100644 --- a/packages/perennial-order/test/unit/TriggerOrder.test.ts +++ b/packages/perennial-order/test/unit/TriggerOrder.test.ts @@ -5,15 +5,11 @@ import { parse6decimal } from '../../../common/testutil/types' import { expect } from 'chai' import { FakeContract, smock } from '@defi-wonderland/smock' -import { - IMarket, - IOracleProvider, - TriggerOrderTester, - TriggerOrderTester__factory, - TriggerOrderStruct, -} from '../../types/generated' +import { IMarket, IOracleProvider, TriggerOrderTester, TriggerOrderTester__factory } from '../../types/generated' import { Compare, compareOrders, DEFAULT_TRIGGER_ORDER, MAGIC_VALUE_CLOSE_POSITION, Side } from '../helpers/order' +import { TriggerOrderStruct } from '../../types/generated/contracts/Manager' import { OracleVersionStruct } from '../../types/generated/contracts/test/TriggerOrderTester' +import { PositionStruct } from '../../types/generated/@perennial/core/contracts/interfaces/IMarket' const { ethers } = HRE @@ -48,44 +44,74 @@ function now(): BigNumber { describe('TriggerOrder', () => { let owner: SignerWithAddress + let user: SignerWithAddress let orderTester: TriggerOrderTester + let market: FakeContract + let oracle: FakeContract before(async () => { - ;[owner] = await ethers.getSigners() + ;[owner, user] = await ethers.getSigners() orderTester = await new TriggerOrderTester__factory(owner).deploy() + market = await smock.fake('IMarket') + oracle = await smock.fake('IOracleProvider') + market.oracle.returns(oracle.address) + fakeMakerPosition(constants.Zero) }) - function createOracleVersion(price: BigNumber, valid = true): OracleVersionStruct { - return { + function fakeOracleVersion(price: BigNumber, valid = true) { + const version: OracleVersionStruct = { timestamp: now(), price: price, valid: valid, } + oracle.latest.returns(version) + } + + function fakeMakerPosition(size: BigNumber) { + const position: PositionStruct = { + timestamp: now(), + maker: size, + long: constants.Zero, + short: constants.Zero, + } + market.positions.whenCalledWith(user.address).returns(position) } describe('#logic', () => { it('handles invalid oracle version', async () => { - const invalidVersion = createOracleVersion(parse6decimal('2444.66'), false) - expect(await orderTester.canExecute(ORDER_SHORT, invalidVersion)).to.be.false + fakeOracleVersion(parse6decimal('2444.66'), false) + expect(await orderTester.canExecute(ORDER_SHORT, market.address, user.address)).to.be.false }) it('compares greater than', async () => { // ORDER_SHORT price is 2444.55 - expect(await orderTester.canExecute(ORDER_SHORT, createOracleVersion(parse6decimal('3000')))).to.be.true - expect(await orderTester.canExecute(ORDER_SHORT, createOracleVersion(parse6decimal('2000')))).to.be.false + fakeOracleVersion(parse6decimal('3000')) + expect(await orderTester.canExecute(ORDER_SHORT, market.address, user.address)).to.be.true + fakeOracleVersion(parse6decimal('2000')) + expect(await orderTester.canExecute(ORDER_SHORT, market.address, user.address)).to.be.false }) it('compares less than', async () => { // ORDER_LONG price is 1999.88 - expect(await orderTester.canExecute(ORDER_LONG, createOracleVersion(parse6decimal('1800')))).to.be.true - expect(await orderTester.canExecute(ORDER_LONG, createOracleVersion(parse6decimal('2000')))).to.be.false + fakeOracleVersion(parse6decimal('1800')) + expect(await orderTester.canExecute(ORDER_LONG, market.address, user.address)).to.be.true + fakeOracleVersion(parse6decimal('2000')) + expect(await orderTester.canExecute(ORDER_LONG, market.address, user.address)).to.be.false }) it('handles invalid comparison', async () => { const badOrder = { ...ORDER_SHORT } badOrder.comparison = 0 - expect(await orderTester.canExecute(badOrder, createOracleVersion(parse6decimal('1800')))).to.be.false - expect(await orderTester.canExecute(badOrder, createOracleVersion(parse6decimal('2000')))).to.be.false + fakeOracleVersion(parse6decimal('1800')) + await expect(orderTester.canExecute(badOrder, market.address, user.address)).to.be.revertedWithCustomError( + orderTester, + 'TriggerOrderInvalidError', + ) + fakeOracleVersion(parse6decimal('2000')) + await expect(orderTester.canExecute(badOrder, market.address, user.address)).to.be.revertedWithCustomError( + orderTester, + 'TriggerOrderInvalidError', + ) }) it('allows execution greater than 0 trigger price', async () => { @@ -97,7 +123,26 @@ describe('TriggerOrder', () => { delta: parse6decimal('200'), maxFee: parse6decimal('0.55'), } - expect(await orderTester.canExecute(zeroPriceOrder, createOracleVersion(parse6decimal('1')))).to.be.true + fakeOracleVersion(parse6decimal('1')) + expect(await orderTester.canExecute(zeroPriceOrder, market.address, user.address)).to.be.true + }) + + it('can execute close position order', async () => { + const position = parse6decimal('12.2') + const closeOrder = { + ...DEFAULT_TRIGGER_ORDER, + comparison: Compare.GTE, + price: 0, + delta: MAGIC_VALUE_CLOSE_POSITION, + maxFee: parse6decimal('0.56'), + } + fakeOracleVersion(parse6decimal('123')) + // can execute with position + fakeMakerPosition(position) + expect(await orderTester.canExecute(closeOrder, market.address, user.address)).to.be.true + // cannot execute without position + fakeMakerPosition(constants.Zero) + expect(await orderTester.canExecute(closeOrder, market.address, user.address)).to.be.false }) }) From b50253ba7b8745f208da71f99233bc69fef29456 Mon Sep 17 00:00:00 2001 From: Ed Noepel Date: Tue, 17 Dec 2024 08:19:59 -0500 Subject: [PATCH 2/2] move logic out of type, handle pending orders --- .../perennial-order/contracts/Manager.sol | 16 ++++- .../contracts/test/TriggerOrderTester.sol | 6 +- .../contracts/types/TriggerOrder.sol | 28 +++----- .../test/integration/Manager_Arbitrum.test.ts | 11 ++- .../test/unit/TriggerOrder.test.ts | 71 +++++++++---------- 5 files changed, 68 insertions(+), 64 deletions(-) diff --git a/packages/perennial-order/contracts/Manager.sol b/packages/perennial-order/contracts/Manager.sol index b6ae46d85..1517d6eab 100644 --- a/packages/perennial-order/contracts/Manager.sol +++ b/packages/perennial-order/contracts/Manager.sol @@ -7,14 +7,15 @@ import { Fixed6, Fixed6Lib } from "@equilibria/root/number/types/Fixed6.sol"; import { UFixed6, UFixed6Lib } from "@equilibria/root/number/types/UFixed6.sol"; import { UFixed18, UFixed18Lib } from "@equilibria/root/number/types/UFixed18.sol"; import { Token6 } from "@equilibria/root/token/types/Token6.sol"; -import { IMarket, IMarketFactory } from "@perennial/core/contracts/interfaces/IMarketFactory.sol"; +import { IMarket, Order, Position } from "@perennial/core/contracts/interfaces/IMarket.sol"; +import { IMarketFactory } from "@perennial/core/contracts/interfaces/IMarketFactory.sol"; import { IManager } from "./interfaces/IManager.sol"; import { IOrderVerifier } from "./interfaces/IOrderVerifier.sol"; import { Action } from "./types/Action.sol"; import { CancelOrderAction } from "./types/CancelOrderAction.sol"; import { InterfaceFee } from "./types/InterfaceFee.sol"; -import { TriggerOrder, TriggerOrderStorage } from "./types/TriggerOrder.sol"; +import { TriggerOrder, TriggerOrderLib, TriggerOrderStorage } from "./types/TriggerOrder.sol"; import { PlaceOrderAction } from "./types/PlaceOrderAction.sol"; /// @notice Base class with business logic to store and execute trigger orders. @@ -131,7 +132,16 @@ abstract contract Manager is IManager, Kept { order = _orders[market][account][orderId].read(); // prevent calling canExecute on a spent or empty order if (order.isSpent || order.isEmpty()) revert ManagerInvalidOrderNonceError(); - canExecute = order.canExecute(market, account); + Position memory position; + if (order.delta.eq(TriggerOrderLib.MAGIC_VALUE_CLOSE_POSITION)) { + position = market.positions(account); + Order memory pending = market.pendings(account); + position.update(pending); + } else { + position = Position(0, UFixed6Lib.ZERO, UFixed6Lib.ZERO, UFixed6Lib.ZERO); + } + + canExecute = order.canExecute(market.oracle().latest(), position); } /// @inheritdoc IManager diff --git a/packages/perennial-order/contracts/test/TriggerOrderTester.sol b/packages/perennial-order/contracts/test/TriggerOrderTester.sol index c656407b5..a676dc4f3 100644 --- a/packages/perennial-order/contracts/test/TriggerOrderTester.sol +++ b/packages/perennial-order/contracts/test/TriggerOrderTester.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.13; import { UFixed6, UFixed6Lib } from "@equilibria/root/number/types/UFixed6.sol"; -import { IMarket, OracleVersion } from "@perennial/core/contracts/interfaces/IMarket.sol"; +import { IMarket, OracleVersion, Position } from "@perennial/core/contracts/interfaces/IMarket.sol"; import { TriggerOrder, TriggerOrderLib, @@ -21,8 +21,8 @@ contract TriggerOrderTester { order.store(newOrder); } - function canExecute(TriggerOrder calldata order_, IMarket market, address user) external view returns (bool) { - return order_.canExecute(market, user); + function canExecute(TriggerOrder calldata order_, OracleVersion calldata version, Position calldata position) external pure returns (bool) { + return order_.canExecute(version, position); } function notionalValue(TriggerOrder calldata order_, IMarket market, address user) external view returns (UFixed6) { diff --git a/packages/perennial-order/contracts/types/TriggerOrder.sol b/packages/perennial-order/contracts/types/TriggerOrder.sol index 459908394..f36f6afea 100644 --- a/packages/perennial-order/contracts/types/TriggerOrder.sol +++ b/packages/perennial-order/contracts/types/TriggerOrder.sol @@ -30,7 +30,7 @@ using TriggerOrderLib for TriggerOrder global; /// @notice Logic for interacting with trigger orders /// @dev (external-unsafe): this library must be used internally only library TriggerOrderLib { - Fixed6 private constant MAGIC_VALUE_CLOSE_POSITION = Fixed6.wrap(type(int64).min); + Fixed6 public constant MAGIC_VALUE_CLOSE_POSITION = Fixed6.wrap(type(int64).min); // sig: 0x5b8c7e99 /// @custom:error side or comparison is not supported @@ -38,24 +38,18 @@ library TriggerOrderLib { /// @notice Determines whether the trigger order is fillable at the latest price /// @param self Trigger order - /// @param market Market for which the order is intended to be executed - /// @param account Market participant + /// @param latestVersion Latest oracle version /// @return Whether the trigger order is fillable - function canExecute(TriggerOrder memory self, IMarket market, address account) internal view returns (bool) { - OracleVersion memory latestVersion = market.oracle().latest(); - + function canExecute( + TriggerOrder memory self, + OracleVersion memory latestVersion, + Position memory position + ) internal pure returns (bool) { if (!latestVersion.valid) return false; - if (self.comparison == 1 && latestVersion.price.lt(self.price)) return false; - if (self.comparison == -1 && latestVersion.price.gt(self.price)) return false; - if (self.comparison != 1 && self.comparison != -1) revert TriggerOrderInvalidError(); - - if (self.delta.eq(MAGIC_VALUE_CLOSE_POSITION)) { - Position memory position = market.positions(account); - // prevent execution if the position was not opened or is already closed - if (position.empty()) return false; - } - - return true; + if (self.delta.eq(MAGIC_VALUE_CLOSE_POSITION) && position.empty()) return false; + if (self.comparison == 1) return latestVersion.price.gte(self.price); + if (self.comparison == -1) return latestVersion.price.lte(self.price); + return false; } /// @notice Applies the order to the user's position and updates the market diff --git a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts index a4db2d2cf..f82bfe9ea 100644 --- a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts +++ b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts @@ -707,6 +707,13 @@ describe('Manager_Arbitrum', () => { await executeOrder(userA, 509) await commitPrice() + // cannot close with pending closed position + orderId = await placeOrder(userB, Side.LONG, Compare.LTE, parse6decimal('4001'), MAGIC_VALUE_CLOSE_POSITION) + expect(orderId).to.equal(BigNumber.from(505)) + await expect( + manager.connect(keeper).executeOrder(market.address, userB.address, orderId, TX_OVERRIDES), + ).to.be.revertedWithCustomError(manager, 'ManagerCannotExecuteError') + // settle and confirm positions are closed await market.settle(userA.address, TX_OVERRIDES) await ensureNoPosition(userA) @@ -714,8 +721,8 @@ describe('Manager_Arbitrum', () => { await ensureNoPosition(userB) // cannot close with no position - orderId = await placeOrder(userB, Side.LONG, Compare.LTE, parse6decimal('4001'), MAGIC_VALUE_CLOSE_POSITION) - expect(orderId).to.equal(BigNumber.from(505)) + orderId = await placeOrder(userB, Side.LONG, Compare.LTE, parse6decimal('4002'), MAGIC_VALUE_CLOSE_POSITION) + expect(orderId).to.equal(BigNumber.from(506)) await expect( manager.connect(keeper).executeOrder(market.address, userB.address, orderId, TX_OVERRIDES), ).to.be.revertedWithCustomError(manager, 'ManagerCannotExecuteError') diff --git a/packages/perennial-order/test/unit/TriggerOrder.test.ts b/packages/perennial-order/test/unit/TriggerOrder.test.ts index e96cd79b1..5cfcafbc0 100644 --- a/packages/perennial-order/test/unit/TriggerOrder.test.ts +++ b/packages/perennial-order/test/unit/TriggerOrder.test.ts @@ -38,6 +38,13 @@ const ORDER_SHORT: TriggerOrderStruct = { maxFee: parse6decimal('0.66'), } +const EMPTY_POSITION: PositionStruct = { + timestamp: BigNumber.from(0), + maker: BigNumber.from(0), + long: BigNumber.from(0), + short: BigNumber.from(0), +} + function now(): BigNumber { return BigNumber.from(Math.floor(Date.now() / 1000)) } @@ -46,72 +53,59 @@ describe('TriggerOrder', () => { let owner: SignerWithAddress let user: SignerWithAddress let orderTester: TriggerOrderTester - let market: FakeContract - let oracle: FakeContract before(async () => { ;[owner, user] = await ethers.getSigners() orderTester = await new TriggerOrderTester__factory(owner).deploy() - market = await smock.fake('IMarket') - oracle = await smock.fake('IOracleProvider') - market.oracle.returns(oracle.address) - fakeMakerPosition(constants.Zero) }) - function fakeOracleVersion(price: BigNumber, valid = true) { - const version: OracleVersionStruct = { + function createOracleVersion(price: BigNumber, valid = true): OracleVersionStruct { + return { timestamp: now(), price: price, valid: valid, } - oracle.latest.returns(version) } - function fakeMakerPosition(size: BigNumber) { - const position: PositionStruct = { + function createMakerPosition(size: BigNumber): PositionStruct { + return { timestamp: now(), maker: size, long: constants.Zero, short: constants.Zero, } - market.positions.whenCalledWith(user.address).returns(position) } describe('#logic', () => { it('handles invalid oracle version', async () => { - fakeOracleVersion(parse6decimal('2444.66'), false) - expect(await orderTester.canExecute(ORDER_SHORT, market.address, user.address)).to.be.false + expect( + await orderTester.canExecute(ORDER_SHORT, createOracleVersion(parse6decimal('2444.66'), false), EMPTY_POSITION), + ).to.be.false }) it('compares greater than', async () => { // ORDER_SHORT price is 2444.55 - fakeOracleVersion(parse6decimal('3000')) - expect(await orderTester.canExecute(ORDER_SHORT, market.address, user.address)).to.be.true - fakeOracleVersion(parse6decimal('2000')) - expect(await orderTester.canExecute(ORDER_SHORT, market.address, user.address)).to.be.false + expect(await orderTester.canExecute(ORDER_SHORT, createOracleVersion(parse6decimal('3000')), EMPTY_POSITION)).to + .be.true + expect(await orderTester.canExecute(ORDER_SHORT, createOracleVersion(parse6decimal('2000')), EMPTY_POSITION)).to + .be.false }) it('compares less than', async () => { // ORDER_LONG price is 1999.88 - fakeOracleVersion(parse6decimal('1800')) - expect(await orderTester.canExecute(ORDER_LONG, market.address, user.address)).to.be.true - fakeOracleVersion(parse6decimal('2000')) - expect(await orderTester.canExecute(ORDER_LONG, market.address, user.address)).to.be.false + expect(await orderTester.canExecute(ORDER_LONG, createOracleVersion(parse6decimal('1800')), EMPTY_POSITION)).to.be + .true + expect(await orderTester.canExecute(ORDER_LONG, createOracleVersion(parse6decimal('2000')), EMPTY_POSITION)).to.be + .false }) it('handles invalid comparison', async () => { const badOrder = { ...ORDER_SHORT } badOrder.comparison = 0 - fakeOracleVersion(parse6decimal('1800')) - await expect(orderTester.canExecute(badOrder, market.address, user.address)).to.be.revertedWithCustomError( - orderTester, - 'TriggerOrderInvalidError', - ) - fakeOracleVersion(parse6decimal('2000')) - await expect(orderTester.canExecute(badOrder, market.address, user.address)).to.be.revertedWithCustomError( - orderTester, - 'TriggerOrderInvalidError', - ) + expect(await orderTester.canExecute(badOrder, createOracleVersion(parse6decimal('1800')), EMPTY_POSITION)).to.be + .false + expect(await orderTester.canExecute(badOrder, createOracleVersion(parse6decimal('2000')), EMPTY_POSITION)).to.be + .false }) it('allows execution greater than 0 trigger price', async () => { @@ -123,8 +117,9 @@ describe('TriggerOrder', () => { delta: parse6decimal('200'), maxFee: parse6decimal('0.55'), } - fakeOracleVersion(parse6decimal('1')) - expect(await orderTester.canExecute(zeroPriceOrder, market.address, user.address)).to.be.true + + expect(await orderTester.canExecute(zeroPriceOrder, createOracleVersion(parse6decimal('1')), EMPTY_POSITION)).to + .be.true }) it('can execute close position order', async () => { @@ -136,13 +131,11 @@ describe('TriggerOrder', () => { delta: MAGIC_VALUE_CLOSE_POSITION, maxFee: parse6decimal('0.56'), } - fakeOracleVersion(parse6decimal('123')) + const oracleVersion = createOracleVersion(parse6decimal('123')) // can execute with position - fakeMakerPosition(position) - expect(await orderTester.canExecute(closeOrder, market.address, user.address)).to.be.true + expect(await orderTester.canExecute(closeOrder, oracleVersion, createMakerPosition(position))).to.be.true // cannot execute without position - fakeMakerPosition(constants.Zero) - expect(await orderTester.canExecute(closeOrder, market.address, user.address)).to.be.false + expect(await orderTester.canExecute(closeOrder, oracleVersion, createMakerPosition(constants.Zero))).to.be.false }) })