Orderbook DEX

When matching orders, taker and maker fees are not deducted from traders' balances, yet they are still added to the fee collector’s balance.

However, when the fee collector claims these fees, both the global balance and the fee collector’s personal balance are reduced. This results in insolvency, preventing depositors from withdrawing their balances.

Deduct maker and taker fees from traders' balances.

Ethereal Team: The issue was resolved in PR#94.

In the protocol, PNL is calculated based on the whole position size regardless of the actual filled amount. This means that even if there aren’t enough counterparties to fill the full position at a given price, the system still accounts for the profit as if the whole size was successfully exited, which creates a mismatch between realized profits and what is actually achievable in the market.

This creates an issue especially when the liquidity depth is not enough to close a large position. For example:

• Trader opens a 100-size long at $1000
• The price rises to $1500, but it’s a resistance level with strong sell pressure and limited buy liquidity.
• Trader closes only size of 1.
• The protocol realizes profit on the full 100-size position at $1500, even though only 1 unit was actually sold.
• Realized profit: $50,000

Normally, equilibrium would be reached as the price declines, with the trader closing the remaining 99-size position at a lower price and realizing a loss. However, knowing they cannot exit the full size at reasonable prices, the trader can withdraw these realized but unbacked profits and leave the remaining position for liquidation.

For this position above (at 10x leverage):

• Initial position value: $100,000
• Deposit: $10,000
• Realized profit: $50,000
• New balance: $60,000
• New position value: 99 * 1500 = $148,500
• New required margin: $14,850
• Trader can withdraw up to $45,150, and leave the rest of the position for liquidation.

Consider realizing PNL only when decreasing or closing positions and calculating it based on the filled amount rather than the entire position size. This ensures that realized profits accurately reflect actual market execution.

When increasing positions, calculate and adjust the average entry price instead of realizing PNL and setting the latest match price as the new entry. This ensures that unrealized gains or losses remain properly accounted for until an actual position reduction occurs.

Ethereal Team: The issue was resolved in PR#114.

Inside the the _verifyOrder function the order of operations is incorrect. It checks if the order.sender is an excluded account.

However, at this point the order.sender could be the linked signer of the account. In these situations the validation would not be sufficient and an excluded account could open a trade.

Perform the validation with account.sender instead of order.sender.

Ethereal Team: The issue was resolved in PR#90.

The liquidator account has the power to open trade positions for any account. The liquidator can increase or decrease another account's position size without their approval or signature.

Similar to how when a market is in a closed status require that _isIncreasingPosition returns false. This will limit the liquidator to only decreasing the position's size.

Ethereal Team: The issue was resolved in PR#files.

A user can temporarily cause a DoS of the processActions() function by submitting a link signer request and depositing. Doing so will prevent other transactions that are meant to be submitted from being posted on-chain.

In turn, this can delay liquidations and users from closing trades at crucial times. To carry this attack out a malicious user needs to create a link signer action. Then, deposit() can be called by the user directly. This will add them to the excludedSigners mapping.

Inside of _handleLinkSigner(), a revert will occur when it validates that the user is not in the excludedSigners mapping.

Wrap internal calls inside of processAction() in try-catch blocks to prevent a revert from causing a DoS of the entire transaction.

Ethereal Team: The issue was resolved in PR#116.

The ExchangeConfig contract allows tokens to be removed and specifically contains logic for removing the USD token used in the contract.

However, it is realistically impossible to remove the USD token as the USD token must be used as the quote token in the PerpEngine.sol contract, thus marking it as removeProtected.

// Only allow usdToken (i.e. USDe) as the perp settlement currency. if
(quoteToken.tokenAddress = accountGlobal.usdToken)
     {revert InvalidParameter("quoteTokenName", P_ERR_BAD_ADDR);} if
(quoteToken.removeProtected) {quoteToken.removeProtected = true;}

Furthermore, protected tokens in general can never be removed as they are required to be removeProtected = false, but there is not way to assign removeProtected back to false for tokens which are only used in deprecated markets.

Be aware of this behavior and if you wish to be able to remove tokens that are only used in deprecated markets then consider refactoring the removeProtected logic.

Ethereal Team: This is expected behaviour and documented in the code. removeToken is largely in-place to undo fat finger configuration when an asset is initially added and needs to be updated before enabling deposits.

In the addToken function the tokensByAddress mapping is updated even in the case where the tokenAddress is address(0). Therefore the entry for address(0) is a valid token and is the last token that was added with the addToken function.

Only write to the tokensByAddress mapping inside of the tokenAddress = address(0) else case.

Ethereal Team: The issue was resolved in PR#105.

In both the register and updateFees functions there are no validations performed on the maker and taker fees to ensure that the maker fee is lower than the taker fee and also within a reasonable range.

Consider adding validations in both the register and updateFees functions so that the maker fee cannot be configured higher than the taker fee and that both fees are within a reasonable range.

Ethereal Team: The issue was resolved in PR#106.

There is no signature validation necessary for the UpdateFunding action from the sequencer. Therefore a compromised or errant sequencer may submit invalid funding updates.

This affords the sequencer the power to apply arbitrarily large funding amounts and subsequently liquidate users or a malicious actor who has compromised the sequencer may assign a high positive funding amount and immediately close their account to withdraw ill gotten gains.

There are already TODO comments which suggest adding caps on the magnitude of the fundingDeltaUsd and limiting how often update funding can be called, which appropriately addresses the risk. Consider implementing these TODO comments.

Additionally, consider adding a specific FundingUpdater signer which must attest to the validity of the funding updates. This way even if the sequencer were compromised they cannot submit any inaccurate funding updates.

Ethereal Team: These TODO comments will be implemented at a later time before the initial launch.

In the addSequencer, updateFeeCollector, and updateLiquidator functions there is no validation to ensure the added account is not an excludedSigners or excludedAccounts exclusion before adding it.

Consider validating that the newly assigned account is not already set as true in either the excludedSigners or excludedAccounts mappings.

For the updateFeeCollector and updateLiquidator just be sure that the account is not the same as the existing fee collector and liquidator accounts before performing this validation.

Ethereal Team: The issue was resolved in PR#files.

In the offchain Matching Engine, initialMarginRate is the reciprocal of maxLeverage. maintenanceMarginRate is half of initialMarginRate. If an account fails to satisfy the maintenance margin they will be liquidated. Therefore, if the maxLeverage for a product is decreased the maintenanceMargin will increase. Accounts with open positions may be moved under the maintenanceMargin and into liquidation zone. Consider the following example:

STATE 1 initialMaxLeverage = 10 initialMarginRate = 1/( initialMaxLeverage) = 1/10 maintenanceMarginRate = (1/2)*initialMarginRate = 1/20

STATE 2 newMaxLeverage = 5 newInitialMarginRate = 1/(newMaxLeverage) = 1/5 maintenanceMarginRate = (1/2)*newInitialMarginRate = 1/10 Let's say an account has a positionSize = $10,000 and collateral = $750

In STATE 1 minMaintenanceCollateral = (1/20)*positionSize minMaintenanceCollateral = (1/20)*$10,000 minMaintenanceCollateral = $500 $750 > $500, SAFE FROM LIQUIDATION

In STATE 2 minMaintenanceCollateral = (1/10)*positionSize minMaintenanceCollateral = (1/10)*$10,000 minMaintenanceCollateral = $1000 $750 < $1000, LIQUIDATABLE

Therefore, the collateral requirements increased after the maxLeverage was decreased.

First put the product in a closed status. Once all trades are closed, update the max leverage and then change the status of the product to open.

Ethereal Team: Acknowledged.

Even with sequencer checks if the user makes a deposit right before a batch of orders goes through the sequencer could check and see that the account is underwater.

But by the time liquidations go through the user could have deposited funds and had a healthy account, but still get liquidated since there are no on chain margin checks.

Document to users that if the account is at any point in time liquidatable they are eligible to be liquidated and pay the liquidation fee. Regardless of the accounts health when the actual liquidation occurs.

Ethereal Team: The issue was resolved in PR#110.

When a cancellation occurs it is intended to be handled off-chain. But the signatures will still be valid on chain. This means that even after a user cancels an order that signature can still be used on chain.

Even if this information is maintained off chain all it would take is the off chain data to be lost, overlooked, or changed, at which point all cancelled orders could be used despite the original sender's request to cancel the order.

The sequencer is trusted to not re-use these signatures/nonces and the off-chain component is trusted to not allow re-submissions of these signatures/nonces.

Ensure that the off-chain systems cannot allow nonces to be re-submitted when they haven’t been used on-chain. Optionally, consider adding a deadline parameter to the signature so that naturally after a set amount of time the order cannot be used.

Ethereal Team: The issue was resolved in PR#118.

Fee collectors are excluded accounts and claim fees using the claimFees function. The owner can change the fee collector address, but this change does not perform a balance check.

Setting a new fee collector without claiming the previous balance will lock the previously earned fees. This happens because the previous fee collector can no longer call the claimFees function.

Additionally, they cannot use the withdraw function due to their excluded status. The new fee collector also cannot withdraw those fees, as they remain in the previous collector’s balance.

Perform a balance check and claim earned fees before updating the fee collector address. Otherwise be aware of this behavior and ensure that fees are always claimed before changing the fee collector or that the fee collector address should be reset to the original address to claim any fees that went unclaimed.

Ethereal Team: Acknowledged.

Whenever the a trade is opened it checks to make sure the quantity product.lotSize = 0. Thus, if the owner makes updates the product.lotSize, there is a good chance that open positions will not be able to be 100% liquidate-able. For example, the original lotSize = 2.

A trade is made with quantity of 4 and passes all checks. Then the lostSize is changed to 5 and the position cannot be liquidated because 4 * 5 = 0. In reality the amount would be small, but there is a very high chance this will happen if the lotSize is ever updated.

Validate that the existing order fulfillment is not impacted by lot size changes prior to changing the lot size.

Ethereal Team: The issue was resolved in PR#104.

The LiquidateTradeOrder struct includes the TraderOrder struct, along with liquidator and liquidatorSubaccount fields. The type hash for this struct is created in the code as follows:

“LiquidateTradeOrder(address sender, bytes32 subaccount, uint128 quantity, uint128 price, uint8 side, uint8, engineType, uint32 productId, uint64 nonce,address liquidator, bytes32 liquidatorSubaccount)".

However, according to EIP-712, when encoding structs that contain nested structs, each struct should be encoded separately. For the LiquidateTradeOrder type above, the correct encoding should be:

"LiquidateTraderOrder(TradeOrder order, address liquidator, bytes32 liquidatorSubaccount)TraderOrder(address sender, bytes32 subaccount, uint128 quantity, uint128 price, uint8 side, uint8 engineType, uint32 productId, uint64 nonce)”.

Update the struct encoding according to the EIP standard.

Ethereal Team: Although this does not strictly follow the EIP standard, the current encoding still produces a unique hash for each order, just implemented differently.

Withdrawals are a two-step process in the protocol. Users initiate withdrawals and finalize them after the lockout period has passed.

The lockout period check is performed during the second step of the process in the finalizeWithdraw function:

validAfter = withdraw.initiatedAt + exchange.withdrawLockout.

However, the lockout period can be changed at any time, and this change not only affects future withdrawals but also pending withdrawals. A withdrawal request should have the validAfter time based on the lockout period at the time of the request's creation.

Determine the validAfter parameter during the first step of the withdrawal process.

Ethereal Team: Acknowledged.

The maxLeverage value is configured and updatable. However, a trader’s leverage is never validated. This allows traders to open positions that vastly exceed the leverage tolerance for a product.

Validate the trader is not using more leverage than is allowed for the product.

Ethereal Team: maxLeverage and other attributes mentioned here, such as maxOpenInterest although not validated onchain, is validated offchain. The reason we don't validate maxLeverage during trades is it can significantly lower settlement throughput and without formal coordination could lead to temporary settlement halts.

In the case where there is a withdraw fee, the fee is subtracted from the action.message.amount. It is checked that the message amount is at least as large as the withdrawfee → if (withdrawFee > action.message.amount) { revert…}. However, this presents an edge case where action.message.amount = withdrawFee. In this case, the amount written for the withdraw request will be 0.

However, some tokens revert upon transferring 0-value, causing the call to finalizeTransfer() to revert. Since a user can only have one pending withdrawal at a time, it is imperative that the user can finalize each withdrawal.

The recommended mitigation is two-fold. Firstly, ensure that the withdrawal amount is strictly greater than the withdraw fee action.message.amount > withdrawFee. Second, add some logic to mitigate general ERC20 transfer failures.

While these tokens will be on their own L3, there may be tokens with transfer restrictions like USDC’s blacklist. You can potentially check if the transfer fails and add back the token balance to the user and delete the pending withdrawal so that the user can queue another token to be withdrawn.

Ethereal Team: The issue was resolved in PR#100.

The maxOpenInterest is not verified when opening a new position. Unless the trade is verified by the Sequencer the maxOpenInterest can easily be surpassed since there are no checks inside the contract.

Since many orders will occur in a single batch it may be possible that open interest will exceed the max if a single batch is large enough.

Inside _verifyOrder verify the maxOpenInterest has not been reached similar to how they are checking the quantity, lotSize etc.

Ethereal Team: Similar to other configuration variables, this is verified offchain and largely in place to provide a central point of exchange configuration. We’ve provided more docs around config management in the README.

Off-chain market orders will be signed with a price = 0 and will offer no on-chain price protection for a user. The fulfilled price could be far from what they originally expected.

Consider implementing either on chain or off chain slippage where the user can ensure they are being matched with what they anticipate the market price to be, and if it is not, don't match the order.

Ethereal Team: The matching engine has price slippage protection for market orders such that if the slippage exceeds 5% then the order will not be filled.

In the _verifyProductSizes function the maxQuantity lotSize = 0 validation provides the P_ERR_AMT_GT_MAX error. However this error code is not the most accurate for this validation since it is not comparing against a max, but instead a modulus.

Consider using the P_ERR_BAD_VAL error code or introducing a new error code for the maxQuantity lotSize = 0 validation.

Ethereal Team: The issue was resolved in PR#108.

The code mistakenly uses the UnauthorizedAccount error instead of ExcludedAccount error when checking if the sender is an excluded account.

(accountGlobal.excludedAccounts[msg.sender]) {revert
UnauthorizedAccount(msg.sender);}

Compare this to a similar check elsewhere in the code:

(accountGlobal.excludedAccounts[action.message.account]) {revert
ExcludedAccount(idx, action.message.account);}

Use the ExcludedAccount error for both checks.

Ethereal Team: The issue was resolved in PR#107.

The function _getTradeOrderDigest() orders the parameters for the struct digest slightly differently than the TradeOrder struct.

function _getTradeOrderDigest(TradeOrder memory order) private view returns
(bytes32) {bytes32 structDigest =
keccak256(abi.encode(TRADE_ORDER_ABI_PARAM_SIG, order.sender, order.quantity,
order.price, order.side, order.productId, order.engineType, order.subaccount,
order.nonce));

vs:

struct TradeOrder {address sender; bytes32 subaccount; uint128 quantity;
uint128 price; OrderSide side; EngineType engineType; uint32 productId; uint64
nonce;}

Consider ordering the digest entries the same as the TradeOrder struct entries.

Ethereal Team: Acknowledged.

The ExchangeGateway contract holds several unused errors:

• InvalidParameter
• P_ERR_BAD_ADDR
• SignerNotFound

Consider implementing the use of these errors or removing them.

Ethereal Team: The issue was resolved in PR#94.

Throughout the contract there are several typos:

• ExchangeGateway.sol:292 Accured → Accrued
• PerpEngine.sol:349 Accured → Accrued
• Architecture Diagram: Sotre -> Store
• ExchangeGateway.sol:244: withou -> without
• ProcessActions.InitiateWithdraw.t.sol:37: Inititate -> Initiate
• ProcessActions.LinkSigner.t.sol:91-93 Lined -> Linked

Consider correcting these typos.

Ethereal Team: The issue was resolved in PR#94.

The depositCap is strictly checked against the global tokenBalance. However, the FeeCollector’s unclaimed fees are contained in the global balance until they claim them.

This is somewhat misleading as the FeeCollector’s fees are already earmarked for them and are not truly a deposit.

When checking the global balance against the deposit cap, deduct the FeeCollector’s balance.

Ethereal Team: Expected behaviour. The team will ensure deposit capacity is sufficiently large enough to account for unclaimed fees. Additionally, we will have processes to periodically claim fees on a regular basis.

The comment about the OI calculation states: Total product OI in native units (e.g. 5 short, 5 long = 10 OI). But the implementation only counts long open interest.

Correct the comment to correctly reflect the OI calculation.

Ethereal Team: The issue was resolved in PR#94.

The _verifyOrder function includes a boolean parameter isTaker which indicates whether it is the maker or taker order which is being verified.

However the errors raised in the _verifyOrder function do not specify the isTaker value and therefore are ambiguous as to which order the revert occurred with.

Consider including the isTaker information in the reverts in the _verifyOrder function to provide more data about the error.

Ethereal Team: Acknowledged.

There can be gas savings made by changing libraries to read from constant storage slots.

Consider implementing constant storage slots in the libraries used.

Ethereal Team: Acknowledged.

ERC20Helpers.isERC20() validates if a token can be added by checking if it is an ERC-20 token. One of the checks looks for a return value from the decimals() function. Since this function is not required to be implemented, some ERC-20 tokens may be excluded from the use in the protocol.

If you wish to include ERC-20 tokens that do not implement decimals(), other protocols default the decimal value to 18 in a try/catch block.

Ethereal Team: We will only support ERC20 tokens with a decimals() function and assuming 18 decimals can lead to downstream problems if the assumption doesn’t hold true.

The Deposit event does not include details on how much fees were charged during the deposit, nor what the original total deposit amount was before fees.

This information may be useful to off-chain systems which are reading for Deposit events and is in contrast to the WithdrawFinalized event which includes the fee.

Consider emitting the fee amount along with the Deposit function.

Ethereal Team: The issue was resolved in PR#110.

The comment in the _handleLinkSigner function on line 403 mentions that subaccounts are an excludedSigners address. However it is normal accounts which are excluded, not sub accounts.

Correct the comment to indicate that it is accounts which can be in the excludedSigners mapping and not subaccounts.

Ethereal Team: Resolved.

The _settlePosition function is invoked for both sides of the trade when matching orders. The function calculates the PnL, determines the new USD balance, and updates positions.

The usdTokenBalance + pricePnl - fundingPnl is converted to uint256 after it is calculated. However, the toUint256 function reverts when the provided value is negative. As a result, settlements will fail when a trader loses their entire balance.

Since the liquidation process also follows the same execution flow, liquidations will also fail in that case. Even if a partial liquidation is attempted, it would also fail because the PnL is calculated based on the entire position size.

Consider setting the trader's balance to 0 when it falls below zero due to negative PnL. However, this would also require covering the negative amount, either from protocol-owned liquidity (e.g., fees) or from other traders.

Alternatively, set high liquidation thresholds and large margin requirements to ensure this situation never occurs, even during sudden and large price movements.

Ethereal Team: There will be a large enough maintenance margin buffer where this will never happen.

Furthermore, If a loss was incurred due to a e.g. MarkChange, the offchain matching engine would cap the loss to prevent an insolvency (i.e. bankruptcy price) where the balance would never go below 0. In short, the sequencer would never trigger a trade where the account would be negative. If it did, it would be a bug and this should revert and halt, which it currently does.

The protocol never charges users for gas, and all gas required to settle on-chain actions must be paid by the sequencer. Even if this is intentional, users can cause the sequencer to consume more gas by frequently linking and revoking signers.

Be aware of this situation. Either charge users gas for on-chain actions or restrict certain actions (especially those that don’t require any fees) from being called repeatedly.

Ethereal Team: This is resolved by deposit/withdrawal fees, increasing minOrderQty, API rate limits, and rate limits on maximum linked signers per day/week.

.

In the _handleInitiateWithdraw function there is no validation to ensure that the remaining deposit is larger than the configured minDeposit for the deposit token.

As a result, users may deposit and then withdraw funds in order to leave a dust amount of collateral in their account. This may cause unexpected issues in the off-chain system if a malicious actor were to create many accounts with a small amount of collateral deposited in each.

This would require the sequencer to hold and validate state for a potentially large number of accounts. In the worst case this could cause the sequencer to OOM at a certain scale, though this case is exceptionally unlikely.

Consider validating that the minDeposit is still met, optionally with a buffer to account for price changes of open positions, in the _handleInitiateWithdraw function.

Ethereal Team: We have deposit and withdrawal fees to disincentivize this behavior.