Prevent Collateral updates with pending orders #1711

leomassazza · 2023-07-07T19:10:10Z

Check if there's a pending order for the account for updateCollateral and commitOrder.
After some discussions the definition is that only a single pending order can exists per account, hence this PR also moves the asyncOrders mapping on PerpsMarket to a loadable AsyncOrder instance mostly used in PerpsAccount.

codecov · 2023-07-07T19:16:59Z

Codecov Report

Merging #1711 (a685a4f) into main (7e7143f) will not change coverage.
The diff coverage is n/a.

❗ Current head a685a4f differs from pull request most recent head f8449f1. Consider uploading reports for the commit f8449f1 to get more accurate results

@@           Coverage Diff           @@
##             main    #1711   +/-   ##
=======================================
  Coverage   72.77%   72.77%           
=======================================
  Files          57       57           
  Lines         720      720           
  Branches      236      236           
=======================================
  Hits          524      524           
  Misses        167      167           
  Partials       29       29

Flag	Coverage Δ
core-contracts	`93.26% <ø> (ø)`
core-modules	`90.47% <ø> (ø)`
core-utils	`68.57% <ø> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Tburm · 2023-07-11T15:08:15Z

markets/perps-market/contracts/modules/PerpsAccountModule.sol

@@ -54,6 +54,11 @@ contract PerpsAccountModule is IAccountModule {
            account.id = accountId;
        }

+        // Check if there are pending orders
+        if (account.hasPendingOrders) {


Should we add a check here that amountDelta < 0? I don't think a user should ever be prevented from depositing collateral, but it makes sense to prevent withdrawals.

we need to check both sides in order to prevent some side effects we noticed on perpsv2 (i.e. ability to manipulate appropriate time to execute an order)

How could a person manipulate the timing by adding more collateral? I see how they could withdraw to cause the execution to revert, then deposit again when they want to execute.

Yea troy's point does make sense to me

How could a person manipulate the timing by adding more collateral? I see how they could withdraw to cause the execution to revert, then deposit again when they want to execute.

@Tburm it just gives the user more tools to play with with bad intents, like imagine with me during crashing market prices, the user submits intention at the brink of what is possible with his margin, that by the time the order is elegible for execution, he doesn't have enough margin to get executed, he gets a free option to add collateral to be allowed to execute the pending order during that execution time window

It is a safer design therefore, to just allow modification of collateral if no pending order is there

so, basically (we had this discussion on perpsV2, and there are some complex vectors or side effects) we want to prevent any change of collateral while orders are pending.

@kaleb-keny That makes sense, thanks for the detail. I assumed they could somehow guarantee better execution, but in your example it would still require some favorable price action.

@Tburm yeah the attacker would need to check for a favorable with respect to the more lively prices offchain (which are expected to impact his position post-execution)

Makes sense, thanks for the details. We should just make sure frontends are well aware of this, since users should know if/when they are prevented from depositing to fix underwater positions.

markets/perps-market/test/integration/Orders/OffchainAsyncOrder.pending.test.ts

0xjocke · 2023-07-11T23:58:26Z

What's the difference between this and checking if the account have an order by PerpsMarket.loadValid(marketId).asyncOrders[accountId];?

leomassazza · 2023-07-12T00:01:17Z

What's the difference between this and checking if the account have an order by PerpsMarket.loadValid(marketId).asyncOrders[accountId];?

it needs to be done in a loop for each marketId

0xjocke · 2023-07-12T00:05:41Z

markets/perps-market/test/integration/Orders/OffchainAsyncOrder.pending.test.ts

+          systems().PerpsMarket
+        );
+      });
+    });


No sure how valuable, but we could add an assertion for an order on the same market too?

in fact that was already tested here: https://github.com/Synthetixio/synthetix-v3/blob/main/markets/perps-market/test/integration/Orders/OffchainAsyncOrder.commit.test.ts#L209

davidvuong · 2023-07-12T06:36:04Z

You can move orders from market to an account level and track a mapping of accountId -> struct { order, marketId }[] then you can avoid the need for an additional variable you have to remember to turn on/off. Would require shuffling a few things around but it's one less variable that have to be kept tracked and stored in-sync.

If you track marketId in the Order struct then it can even just be mapping(uint128 -> Order[]), without needing a wrapper struct.

Orders aren't tightly coupled to markets and they don't have an impact until settlement occurs. For settlement, as long as the keeper can find the associated marketId this order belongs to then it will operate just fine too.

* Use load instead of mapping for AsyncOrder * small fix * cleanup

sunnyvempati

minor changes; logic is clear and looks good 👍🏽

sunnyvempati · 2023-07-17T10:57:19Z

markets/perps-market/contracts/interfaces/IAccountModule.sol

+    /**
+     * @notice Gets thrown when pending orders exist and attempts to modify collateral.
+     */
+    error PendingOrdersExist();


maybe rename to PendingOrderExists since we can't have multiple orders anyway

sunnyvempati · 2023-07-17T10:58:48Z

markets/perps-market/contracts/modules/AsyncOrderModule.sol

+        AsyncOrder.Data storage order = AsyncOrder.load(commitment.accountId);

-        if (order.sizeDelta != 0) {
+        if (order.sizeDelta != 0 && order.marketId == commitment.marketId) {
            revert OrderAlreadyCommitted(commitment.marketId, commitment.accountId);
        }

+        if (order.sizeDelta != 0) {
+            revert IAccountModule.PendingOrdersExist();
+        }


maybe do AsyncOrder.createValid(..) and move all these checks into that function. would clean up this method quite a bit.

sunnyvempati · 2023-07-17T11:03:30Z

markets/perps-market/contracts/modules/PerpsAccountModule.sol

+        // Check if there are pending orders
+        if (AsyncOrder.load(accountId).sizeDelta != 0) {
+            revert PendingOrdersExist();
+        }


could move this into the account storage and do account.checkPendingOrders() or smth. as much as possible trying to move validations into storage libraries. non blocking; only if you think its cleaner

sunnyvempati · 2023-07-17T11:05:25Z

markets/perps-market/storage.dump.sol

@@ -547,6 +547,7 @@ library PerpsAccount {
    struct Data {
        mapping(uint128 => uint256) collateralAmounts;
        uint128 id;
+        bool hasPendingOrders;


dont think this is used; re-run yarn build 👍🏽

sunnyvempati

🚢

wip

bb1b78f

leomassazza self-assigned this Jul 7, 2023

leomassazza added 9 commits July 8, 2023 00:32

yarn dedupe

2019a27

upgrade missing deps

f9ebabc

Prevent also new orders with pending one

e7e1729

wip

38fc03f

checkpoint

2ed8c05

revert change

3663cf0

Merge branch 'main' into prevent-collateral-update-with-pending-orders

90f485e

add tests

333f4a5

lint

87f03d8

leomassazza marked this pull request as ready for review July 11, 2023 13:51

leomassazza changed the title ~~Prevent modifyCollateral with pending orders~~ Prevent Collateral updates with pending orders Jul 11, 2023

Tburm reviewed Jul 11, 2023

View reviewed changes

markets/perps-market/test/integration/Orders/OffchainAsyncOrder.pending.test.ts Outdated Show resolved Hide resolved

Merge branch 'main' into prevent-collateral-update-with-pending-orders

a17ffea

0xjocke reviewed Jul 12, 2023

View reviewed changes

leomassazza marked this pull request as draft July 13, 2023 13:58

leomassazza added 3 commits July 13, 2023 14:58

Merge branch 'main' into prevent-collateral-update-with-pending-orders

6a9ca8d

change location of async order (#1720)

79bc150

remove hasPendingOrder flag

255c058

leomassazza requested review from 0xjocke and Tburm July 13, 2023 16:59

leomassazza marked this pull request as ready for review July 13, 2023 17:00

leomassazza added 2 commits July 14, 2023 04:49

Merge branch 'main' into prevent-collateral-update-with-pending-orders

1b00805

Use load instead of mapping for AsyncOrder (#1721)

a77d12d

* Use load instead of mapping for AsyncOrder * small fix * cleanup

sunnyvempati suggested changes Jul 17, 2023

View reviewed changes

leomassazza added 2 commits July 17, 2023 11:42

PR Fixes

d5a4e48

Merge branch 'main' into prevent-collateral-update-with-pending-orders

a685a4f

leomassazza requested a review from sunnyvempati July 17, 2023 15:01

Merge branch 'main' into prevent-collateral-update-with-pending-orders

f8449f1

sunnyvempati approved these changes Jul 18, 2023

View reviewed changes

sunnyvempati merged commit bd7b5ce into main Jul 18, 2023
17 checks passed

sunnyvempati deleted the prevent-collateral-update-with-pending-orders branch July 18, 2023 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent Collateral updates with pending orders #1711

Prevent Collateral updates with pending orders #1711

leomassazza commented Jul 7, 2023 •

edited

Loading

codecov bot commented Jul 7, 2023 •

edited

Loading

Tburm Jul 11, 2023

leomassazza Jul 11, 2023

Tburm Jul 11, 2023

0xjocke Jul 12, 2023

kaleb-keny Jul 12, 2023 •

edited

Loading

kaleb-keny Jul 12, 2023

leomassazza Jul 12, 2023

Tburm Jul 12, 2023

kaleb-keny Jul 12, 2023

Tburm Jul 12, 2023

0xjocke commented Jul 11, 2023

leomassazza commented Jul 12, 2023

0xjocke Jul 12, 2023

leomassazza Jul 12, 2023

davidvuong commented Jul 12, 2023 •

edited

Loading

sunnyvempati left a comment

sunnyvempati Jul 17, 2023

sunnyvempati Jul 17, 2023

sunnyvempati Jul 17, 2023

sunnyvempati Jul 17, 2023

sunnyvempati left a comment

Prevent Collateral updates with pending orders #1711

Prevent Collateral updates with pending orders #1711

Conversation

leomassazza commented Jul 7, 2023 • edited Loading

codecov bot commented Jul 7, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaleb-keny Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xjocke commented Jul 11, 2023

leomassazza commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidvuong commented Jul 12, 2023 • edited Loading

sunnyvempati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnyvempati left a comment

Choose a reason for hiding this comment

leomassazza commented Jul 7, 2023 •

edited

Loading

codecov bot commented Jul 7, 2023 •

edited

Loading

kaleb-keny Jul 12, 2023 •

edited

Loading

davidvuong commented Jul 12, 2023 •

edited

Loading