Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Fix] Add maximum for intent price deviation #468

Merged
merged 6 commits into from
Oct 11, 2024
Merged

[Fix] Add maximum for intent price deviation #468

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f3e2781
add param
kbrizzle Oct 10, 2024
be1306c
update marketparameter in tests
kbrizzle Oct 10, 2024
bdab7a3
add price deviation
kbrizzle Oct 10, 2024
cda5b3e
add invariant
kbrizzle Oct 10, 2024
fb382da
fix test
kbrizzle Oct 11, 2024
d43845f
address comments
kbrizzle Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/common/testutil/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ export interface MarketParameter {
riskFee: BigNumberish
maxPendingGlobal: number
maxPendingLocal: number
maxPriceDeviation: BigNumberish
closed: boolean
settle: boolean
}
Expand Down Expand Up @@ -356,6 +357,7 @@ export const DEFAULT_MARKET_PARAMETER: MarketParameter = {
riskFee: 0,
maxPendingGlobal: 0,
maxPendingLocal: 0,
maxPriceDeviation: 0,
closed: false,
settle: false,
}
Expand Down
1 change: 1 addition & 0 deletions packages/perennial-account/test/helpers/setupHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export async function createMarket(
takerFee: 0,
maxPendingGlobal: 8,
maxPendingLocal: 8,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
...marketParamOverrides,
Expand Down
1 change: 1 addition & 0 deletions packages/perennial-extensions/test/integration/helpers/setupHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ export async function createMarket(
takerFee: 0,
maxPendingGlobal: 8,
maxPendingLocal: 8,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
...marketParamOverrides,
Expand Down
2 changes: 1 addition & 1 deletion packages/perennial-oracle/test/integration/metaquants/MetaQuantsOracleFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ testOracles.forEach(testOracle => {
takerFee: 0,
maxPendingGlobal: 8,
maxPendingLocal: 8,
settlementFee: 0,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down
2 changes: 1 addition & 1 deletion packages/perennial-oracle/test/integration/pyth/PythOracleFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ testOracles.forEach(testOracle => {
takerFee: 0,
maxPendingGlobal: 8,
maxPendingLocal: 8,
settlementFee: 0,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down
2 changes: 1 addition & 1 deletion packages/perennial-oracle/test/integrationSepolia/chainlink/ChainlinkOracleFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ testOracles.forEach(testOracle => {
takerFee: 0,
maxPendingGlobal: 8,
maxPendingLocal: 8,
settlementFee: 0,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down
1 change: 1 addition & 0 deletions packages/perennial-order/test/helpers/marketHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export async function createMarket(
maxPendingGlobal: 8,
maxPendingLocal: 8,
settlementFee: 0,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
...marketParamOverrides,
Expand Down
1 change: 1 addition & 0 deletions packages/perennial-vault/test/integration/helpers/setupHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export async function deployProductOnMainnetFork({
settlementFee: 0,
maxPendingGlobal: 8,
maxPendingLocal: 8,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down
2 changes: 2 additions & 0 deletions packages/perennial/contracts/interfaces/IMarket.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ interface IMarket is IInstance {
error MarketSettleOnlyError();
// sig: 0x1e9d2296
error MarketInvalidIntentFeeError();
// sig: 0xaf5dfc8f
error MarketIntentPriceDeviationError();

// sig: 0x2142bc27
error GlobalStorageInvalidError();
Expand Down
3 changes: 3 additions & 0 deletions packages/perennial/contracts/libs/InvariantLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ library InvariantLib {
)
) revert IMarket.MarketNotSingleSidedError();

if (newGuarantee.priceDeviation(context.latestOracleVersion.price).gt(context.marketParameter.maxPriceDeviation))
revert IMarket.MarketIntentPriceDeviationError();

if (newOrder.protected()) return; // The following invariants do not apply to protected position updates (liquidations)

if (
Expand Down
4 changes: 4 additions & 0 deletions packages/perennial/contracts/test/GuaranteeTester.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ abstract contract GuaranteeTester {
function priceAdjustment(Guarantee memory guarantee, Fixed6 price) public pure returns (Fixed6) {
return GuaranteeLib.priceAdjustment(guarantee, price);
}

function priceDeviation(Guarantee memory guarantee, Fixed6 price) public pure returns (UFixed6) {
return GuaranteeLib.priceDeviation(guarantee, price);
}
}

contract GuaranteeGlobalTester is GuaranteeTester {
Expand Down
13 changes: 13 additions & 0 deletions packages/perennial/contracts/types/Guarantee.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,19 @@ library GuaranteeLib {
return self.taker().mul(price).sub(self.notional);
}

/// @notice Returns the price deviation of the guarantee from the oracle price
/// @dev The price deviation is (intent price - oracle price) / min(intent price, oracle price)
/// Only supports new guarantees for updates, does not work for aggregated guarantees (local / global)
/// @param self The guarantee object to check
/// @param price The oracle price to compare
/// @return The price deviation of the guarantee from the oracle price
function priceDeviation(Guarantee memory self, Fixed6 price) internal pure returns (UFixed6) {
if (takerTotal(self).isZero()) return UFixed6Lib.ZERO;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use taker(self) here to correspond with the below div?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

takerTotal is more correct for measuring whether the guarantee is empty, since you can have an aggregated guarantee net to zero, but we need to use taker in the math below for the sign.


Fixed6 guarenteePrice = self.notional.div(taker(self));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spelling guaranteePrice

return guarenteePrice.sub(price).div(guarenteePrice.min(price)).abs();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to assume that price and guaranteePrice will not be 0 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be possible for price to be zero due to the latest oracle override.

The guaranteePrice could be zero, but this would correctly revert due to being infinitely out of range.

Any reverts here would only block a fresh update, it wouldn't brick the settlement flywheel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a comment stating that, to avoid audit reports on the same.

}

/// @notice Updates the current global guarantee with a new local guarantee
/// @param self The guarantee object to update
/// @param guarantee The new guarantee
Expand Down
31 changes: 19 additions & 12 deletions packages/perennial/contracts/types/MarketParameter.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ struct MarketParameter {
/// @dev The maximum amount of orders that can be pending at one time per account
uint256 maxPendingLocal;

/// @dev The maximum deviation percentage from the oracle price that is allowed for an intent price override
UFixed6 maxPriceDeviation;

/// @dev Whether the market is in close-only mode
bool closed;

Expand All @@ -49,7 +52,8 @@ using MarketParameterStorageLib for MarketParameterStorage global;
/// uint24 riskFee; // <= 1677%
/// uint16 maxPendingGlobal; // <= 65k
/// uint16 maxPendingLocal; // <= 65k
/// uint48 __unallocated__; // <= 281m
/// uint24 maxPriceDeviation; // <= 16.77x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: // <= 1677% seems more consistent and appropriate here

/// uint24 __unallocated__;
/// uint8 flags;
/// }
///
Expand All @@ -60,7 +64,7 @@ library MarketParameterStorageLib {
function read(MarketParameterStorage storage self) internal view returns (MarketParameter memory) {
uint256 slot0 = self.slot0;

uint256 flags = uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 48 - 8)) >> (256 - 8);
uint256 flags = uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 24 - 24 - 8)) >> (256 - 8);
(bool closed, bool settle) =
(flags & 0x04 == 0x04, flags & 0x08 == 0x08);

Expand All @@ -70,8 +74,9 @@ library MarketParameterStorageLib {
UFixed6.wrap(uint256(slot0 << (256 - 24 - 24 - 24)) >> (256 - 24)),
UFixed6.wrap(uint256(slot0 << (256 - 24 - 24 - 24 - 24)) >> (256 - 24)),
UFixed6.wrap(uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24)) >> (256 - 24)),
uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16)) >> (256 - 16),
uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16)) >> (256 - 16),
uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16)) >> (256 - 16),
uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16)) >> (256 - 16),
UFixed6.wrap(uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 24)) >> (256 - 24)),
closed,
settle
);
Expand All @@ -94,6 +99,7 @@ library MarketParameterStorageLib {

if (newValue.maxPendingGlobal > uint256(type(uint16).max)) revert MarketParameterStorageInvalidError();
if (newValue.maxPendingLocal > uint256(type(uint16).max)) revert MarketParameterStorageInvalidError();
if (newValue.maxPriceDeviation.gt(UFixed6.wrap(type(uint24).max))) revert MarketParameterStorageInvalidError();

_store(self, newValue);
}
Expand All @@ -103,14 +109,15 @@ library MarketParameterStorageLib {
(newValue.settle ? 0x08 : 0x00);

uint256 encoded0 =
uint256(UFixed6.unwrap(newValue.fundingFee) << (256 - 24)) >> (256 - 24) |
uint256(UFixed6.unwrap(newValue.interestFee) << (256 - 24)) >> (256 - 24 - 24) |
uint256(UFixed6.unwrap(newValue.makerFee) << (256 - 24)) >> (256 - 24 - 24 - 24) |
uint256(UFixed6.unwrap(newValue.takerFee) << (256 - 24)) >> (256 - 24 - 24 - 24 - 24) |
uint256(UFixed6.unwrap(newValue.riskFee) << (256 - 24)) >> (256 - 24 - 24 - 24 - 24 - 24) |
uint256(newValue.maxPendingGlobal << (256 - 16)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16) |
uint256(newValue.maxPendingLocal << (256 - 16)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16) |
uint256(flags << (256 - 8)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 48 - 8);
uint256(UFixed6.unwrap(newValue.fundingFee) << (256 - 24)) >> (256 - 24) |
uint256(UFixed6.unwrap(newValue.interestFee) << (256 - 24)) >> (256 - 24 - 24) |
uint256(UFixed6.unwrap(newValue.makerFee) << (256 - 24)) >> (256 - 24 - 24 - 24) |
uint256(UFixed6.unwrap(newValue.takerFee) << (256 - 24)) >> (256 - 24 - 24 - 24 - 24) |
uint256(UFixed6.unwrap(newValue.riskFee) << (256 - 24)) >> (256 - 24 - 24 - 24 - 24 - 24) |
uint256(newValue.maxPendingGlobal << (256 - 16)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16) |
uint256(newValue.maxPendingLocal << (256 - 16)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16) |
uint256(UFixed6.unwrap(newValue.maxPriceDeviation) << (256 - 24)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 24) |
uint256(flags << (256 - 8)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 24 - 24 - 8);

assembly {
sstore(self.slot, encoded0)
Expand Down
4 changes: 4 additions & 0 deletions packages/perennial/test/integration/core/happyPath.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ describe('Happy Path', () => {
takerFee: 0,
maxPendingGlobal: 8,
maxPendingLocal: 8,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down Expand Up @@ -1290,6 +1291,7 @@ describe('Happy Path', () => {
maxPendingLocal: 8,
makerFee: positionFeesOn ? parse6decimal('0.2') : 0,
takerFee: positionFeesOn ? parse6decimal('0.1') : 0,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down Expand Up @@ -1453,6 +1455,7 @@ describe('Happy Path', () => {
maxPendingLocal: 8,
makerFee: parse6decimal('0.2'),
takerFee: parse6decimal('0.1'),
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down Expand Up @@ -2529,6 +2532,7 @@ describe('Happy Path', () => {
maxPendingLocal: 8,
makerFee: positionFeesOn ? parse6decimal('0.2') : 0,
takerFee: positionFeesOn ? parse6decimal('0.1') : 0,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down
1 change: 1 addition & 0 deletions packages/perennial/test/integration/helpers/setupHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ export async function createMarket(
takerFee: 0,
maxPendingGlobal: 8,
maxPendingLocal: 8,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
...marketParamOverrides,
Expand Down
109 changes: 109 additions & 0 deletions packages/perennial/test/unit/market/Market.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,7 @@ describe('Market', () => {
takerFee: 0,
maxPendingGlobal: 5,
maxPendingLocal: 3,
maxPriceDeviation: parse6decimal('0.1'),
closed: false,
settle: false,
}
Expand Down Expand Up @@ -690,6 +691,7 @@ describe('Market', () => {
riskFee: parse6decimal('0.05'),
maxPendingGlobal: 5,
maxPendingLocal: 3,
maxPriceDeviation: parse6decimal('0.1'),
closed: true,
settle: true,
}
Expand All @@ -706,6 +708,7 @@ describe('Market', () => {
expect(marketParameter.takerFee).to.equal(defaultMarketParameter.takerFee)
expect(marketParameter.maxPendingGlobal).to.equal(defaultMarketParameter.maxPendingGlobal)
expect(marketParameter.maxPendingLocal).to.equal(defaultMarketParameter.maxPendingLocal)
expect(marketParameter.maxPriceDeviation).to.equal(defaultMarketParameter.maxPriceDeviation)
expect(marketParameter.closed).to.equal(defaultMarketParameter.closed)
expect(marketParameter.settle).to.equal(defaultMarketParameter.settle)
})
Expand Down Expand Up @@ -14713,6 +14716,10 @@ describe('Market', () => {
})

it('reverts if under margin (intent maker)', async () => {
const marketParameter = { ...(await market.parameter()) }
marketParameter.maxPriceDeviation = parse6decimal('10.00')
await market.updateParameter(marketParameter)

const intent = {
amount: POSITION.div(2),
price: parse6decimal('1250'),
Expand Down Expand Up @@ -14771,6 +14778,10 @@ describe('Market', () => {
})

it('reverts if under margin (intent taker)', async () => {
const marketParameter = { ...(await market.parameter()) }
marketParameter.maxPriceDeviation = parse6decimal('10.00')
await market.updateParameter(marketParameter)

const intent = {
amount: POSITION.div(2),
price: parse6decimal('25'),
Expand Down Expand Up @@ -14828,6 +14839,104 @@ describe('Market', () => {
).to.be.revertedWithCustomError(market, 'MarketInsufficientMarginError')
})

it('reverts if above price deviation (higher)', async () => {
const intent = {
amount: POSITION.div(2),
price: parse6decimal('136'),
fee: parse6decimal('0.5'),
originator: liquidator.address,
solver: owner.address,
collateralization: parse6decimal('0.01'),
common: {
account: user.address,
signer: user.address,
domain: market.address,
nonce: 0,
group: 0,
expiry: 0,
},
}

dsu.transferFrom.whenCalledWith(user.address, market.address, COLLATERAL.mul(1e12)).returns(true)
dsu.transferFrom.whenCalledWith(userB.address, market.address, COLLATERAL.mul(1e12)).returns(true)
dsu.transferFrom.whenCalledWith(userC.address, market.address, COLLATERAL.mul(1e12)).returns(true)

await market
.connect(userB)
['update(address,uint256,uint256,uint256,int256,bool)'](userB.address, POSITION, 0, 0, COLLATERAL, false)

await market
.connect(user)
['update(address,uint256,uint256,uint256,int256,bool)'](user.address, 0, 0, 0, COLLATERAL, false)
await market
.connect(userC)
['update(address,uint256,uint256,uint256,int256,bool)'](userC.address, 0, 0, 0, COLLATERAL, false)

verifier.verifyIntent.returns()

// taker
factory.authorization
.whenCalledWith(user.address, userC.address, user.address, liquidator.address)
.returns([false, true, parse6decimal('0.20')])

await expect(
market
.connect(userC)
[
'update(address,(int256,int256,uint256,address,address,uint256,(address,address,address,uint256,uint256,uint256)),bytes)'
](userC.address, intent, DEFAULT_SIGNATURE),
).to.be.revertedWithCustomError(market, 'MarketIntentPriceDeviationError')
})

it('reverts if above price deviation (lower)', async () => {
const intent = {
amount: POSITION.div(2),
price: parse6decimal('110'),
fee: parse6decimal('0.5'),
originator: liquidator.address,
solver: owner.address,
collateralization: parse6decimal('0.01'),
common: {
account: user.address,
signer: user.address,
domain: market.address,
nonce: 0,
group: 0,
expiry: 0,
},
}

dsu.transferFrom.whenCalledWith(user.address, market.address, COLLATERAL.mul(1e12)).returns(true)
dsu.transferFrom.whenCalledWith(userB.address, market.address, COLLATERAL.mul(1e12)).returns(true)
dsu.transferFrom.whenCalledWith(userC.address, market.address, COLLATERAL.mul(1e12)).returns(true)

await market
.connect(userB)
['update(address,uint256,uint256,uint256,int256,bool)'](userB.address, POSITION, 0, 0, COLLATERAL, false)

await market
.connect(user)
['update(address,uint256,uint256,uint256,int256,bool)'](user.address, 0, 0, 0, COLLATERAL, false)
await market
.connect(userC)
['update(address,uint256,uint256,uint256,int256,bool)'](userC.address, 0, 0, 0, COLLATERAL, false)

verifier.verifyIntent.returns()

// taker
factory.authorization
.whenCalledWith(user.address, userC.address, user.address, liquidator.address)
.returns([false, true, parse6decimal('0.20')])

await expect(
market
.connect(userC)
[
'update(address,(int256,int256,uint256,address,address,uint256,(address,address,address,uint256,uint256,uint256)),bytes)'
](userC.address, intent, DEFAULT_SIGNATURE),
).to.be.revertedWithCustomError(market, 'MarketIntentPriceDeviationError')
})

it('reverts if paused (market)', async () => {
factory.paused.returns(true)
await expect(
Expand Down
Loading
Loading