Contract Updates

The isValidSignature function is intended to not revert to allow the processActions loop to complete when a _handleRevokeLinkedSigner or _handleLinkSigner action is performed.

However isValidSignature execution can be forced to revert by the sender account address if it houses bytecode and implements a isValidSignature function that returns an invalid return payload.

For example, if the isValidSignature function returns empty returndata but successfully executes, the _verifyEip1271Signature function will still attempt to decode the returndata and revert with an abi.decode error.

This will block the processActions queue from processing the batch of actions and hold up the exchange.

Decode the magicValue by casting the returnData to a bytes4 object to capture only the first 4 bytes of the returnData, and produce an empty bytes4 object if no returnData exists:

bytes4 magicValue = bytes4(returnData);

When a withdrawal remains pending for an extended period, the rescueFunds function is invoked to finalize the withdrawal and recover the funds.

However, the rescueFunds function is not payable, while the _completeWithdrawal function called during this process requires msg.value. As a result, funds cannot be rescued if withdrawn via LayerZero OFT.

Make the rescueFunds function payable.

The processActions function may invoke an untrusted external call when it attempts an ERC1271 contract signature validation for the _handleLinkSigner or _handleRevokeLinkedSigner actions.

This call is protected as much as possible, using staticcall and success handling to ensure no reverts can occur. However the amount of gas that is forwarded to the sender address is only limited by the 63/64 rule and therefore allows the untrusted sender address to expend an enormous amount of gas, leaving an insufficient amount for the completion of the processActions loop.

Furthermore, all of the untrusted sender's returnData is loaded in, even if a cap is made on the amount of gas that is forwarded to the sender, the sender address can cause an unexpected amount of gas to be used by loading in a large amount of returnData. This can additionally cause unexpectedly high gas usage which may prevent the processing of the processActions loop, even if a mere gas forwarding limit is put in place.

Put in place a limit on the amount of gas forwarded to the sender address with the invocation of the isValidSignature function. And furthermore, only load in the necessary 4 bytes of returnData from the sender with a low level call.

The finalizeWithdrawals function in the Claimer sends msg.value inside a for loop. During the first iteration, the entire msg.value is sent to the exchangeGateway, leaving no balance for subsequent iterations. As a result, only the first withdrawal succeeds, while the remaining ones fail.

Transfer only the required amount of value in each iteration.

The addition of the fundingCoverageCharge indicates that the protocol intends to support insolvent liquidations, particularly in the case where the cumulative funding across all products for the account is greater than the available account usde margin balance. However across many cases, the accounting for this breaks down.

The userEquityAfterFunding is determined as the usde token balance of the account onchain less the cumulative funding across all products. This amount is then assigned as the referenceFundingCoverageCharge which must match the provided action.fundingCoverageCharge within a precision threshold of 13 wei, although the comment purports this to be exact, not requiring this threshold.

In many cases, the user will not be able to cover their funding charges net of PnL, however this will not be captured by the userEquityAfterFunding as it does not include negative PnL from positions.

Consider the following example:

• Trader A has 100 USDe available margin balance
• Trader A has -$80 PnL
• Trader A has a pending funding of $30 to pay
• Trader A certainly qualifies for the insolvent fundingCoverageCharge case, however the computed userEquityAfterFunding is $70 which does not qualify as it is positive

Furthermore, accounts that are liquidated while they have a net profit overstate their equity after funding and deduct more than they should from the liquidator:

• Trader A has 20 USDe available margin balance
• Trader A has $30 PnL
• Trader A has a pending funding of $30 to pay
• Trader A can technically cover this funding charge if all is settled, however they still appear as if they are insolvent as measured by the userEquityAfterFunding which reports an equity of -$10

One solution to this would be to settle the account PnL right before liquidation every time, however this still comes with rounding and potential price discrepancy concerns.

The funding coverage charge logic should instead be updated to account for the account’s net balance factoring in PnL to cover true account insolvency cases when and account cannot cover funding net of their PnL.

In the finalizeWithdrawals function, the whitelisted withdrawalClaimers or the owner process batches of withdrawal objects to send these funds to users.

The execution of finalizeWithdraw makes an untrusted external call to the account with the safeTransferUSDe function.

The safeTransferUSDe safely does not load in any returnData from the account address invocation, however it does forward the entire gas available at the time of the call execution.

As a result, if the account address is home to a contract or an EOA that has associated bytecode through EIP 7702 it can expend an unexpected amount of gas and use up the entire 63/64 remaining gas that was forwarded to it.

It is unlikely that the finalizeWithdrawals function would be able to complete execution with roughly 1/64th of the initial gas provided, especially if there are other withdrawals to process. So this results in a DoS of the finalizeWithdrawals loop for any withdrawal batches that would include malicious actions.

Create a version of the safeTransferUSDe function that only forwards a configured safe gas amount so that the account cannot gas grief the executor and potentially halt the processing of other withdrawals.

For EOA signature validation the ECDSA.recover function from the Solady library is used to verify the signer of the message, however this recover function reverts upon receiving malformed signatures.

The verifierLib isValidSignature function intends to not revert but instead return a boolean indicating successful verification to prevent stopping the processActions loop.

As a result if a malformed signature is provided to the signer update actions this can result in a block of the processActions loop.

Instead of ECDSA.recover, use the ECDSA.tryRecover function from the Solady library.

In the realizePnl function the trader’s pending unrealized PnL is moved into their account balance without having to be matched against other orders on the orderbook. This allows a trader to realize their on-paper gains on a perp position without experiencing any market impact.

In some cases with very large positions or especially thin order books near the market price, a trader may net more from the (realizePnl action - initial margin) than compared to actually exiting their position and withdrawing the resulting usde balance that remains after experiencing price impact in the market.

When realizing the PnL for a trader with the realizePnl function, be sure to only realize profit to a conservative market price, taking into account the current market price and the orderbook depth relative to the size of the position being realized.

In the FLIP case in the _settleLiquidatorPosition function the newCost is calculated based on:

newCost = eTransferredNotional - (eLiquidatorRealizedPnl * oldSideSign) - cOldCost

Which is then cast to a uint value. However in some cases, where the flipped amount is small this can lead to an underflow revert. Consider the following scenario, which requires a small lotSize.

Index token price is $5. The liquidator has a 100,000 index token long position, the cost is $500,000

Liquidated Position collateral = 2505 USDe

Liquidated Position size = 100000e9 + 1, short

Liquidated Position Notional = (100000e9 + 1) * $5 = 500000000000005

Liquidated Position cost = 500000000000005

maxLeverage = 100x

Margin factor = 0.5x

The index token price stays at $5

A Funding of $10 is charged

The position is left with:

Position Notional = (100e9 + 1) * 1,000 = 100000000001000 Pending PnL = 0 Net Margin = 2495e9 Maintenance margin is 500000000000005 * 1/200 = 2500000000000

Liquidation Occurs

rt.oldSize = 100000e9
rt.newSize = -1
rt.quantity = 100000e9 + 1

eTransferredNotional = (100000e9 + 1) * 5 + 2495e9 = 502495000000005

cOldCost = 500000e9
cCloseNotional = 100000e9 * 502495000000005 / (100000e9 + 1) = 502494999999999
oldSideSign = 1

eLiquidatorRealizedPnl = 502494999999999 - 500000e9 = 2494999999999 (with exact accuracy to the contracts reference)

newCost = 502495000000005 - (2494999999999) - 500000e9 = 6

The eLiquidatorRealizedPnl inaccuracy threshold is however (eTransferredNotional >> 52) + 13, which is trivially larger than 6, thus allowing the newCost resulting value to often be negative in this case and resulting in a casting underflow revert.

Not only will this revert prevent the liquidation from occurring, but it will also halt the batch that is actively being processed.

Firstly, do not configure products with a lotSize less than an order of magnitude of 100 wei to be safe and consider adding validation at the contract level to enforce this configuration.

Furthermore, consider flooring the newCost to zero in the event that this case does arise to avoid harmful reverts.

The finalizeWithdrawals function in the Claimer contract advances lastWithdrawRequestId to the last ID in the batch, even if some withdrawals in the batch fail. Since the function expects IDs to be strictly increasing, the failed withdrawals cannot be retried by the claimer.

Consider tracking the IDs of failed withdrawals and allowing the claimer to retry them if necessary.

In the _completeWithdrawal function there is validation to ensure that the provided msg.value is greater than or equal to the necessary messagingFee.nativeFee fee. However there is no logic to refund the msg.sender in the event that the provided msg.value is greater than the necessary messagingFee.nativeFee.

The layerzero oft.send call does use the msg.sender as the refund recipient, however this oft.send call will not provide any refund ever since the forwarded value is simply the messagingFee.nativeFee + amountLD.

Implement refund logic at the end of the _completeWithdrawal function that refunds the excess msg.value, notice that the oft.send refund cannot be used since it requires exact payment.

In the exchange.verifyBeforeActionExpiry function, if the expiry of the action has come to pass then the operation reverts, blocking subsequent action processing.

This is in contrast to other areas where non-reverting failures are preferred to allow other actions to continue processing such as invalid signatures with the isSignatureValid function.

Be aware that it may be possible for actors to block stuff for a short period of time to delay a processActions batch until one of these expiries are breached.

Consider if actions should silently be skipped or revert the whole processActions batch if they are expired. This has implications for how an action might affect others in the batch if they are unsuccessful.

The verifyBeforeActionExpiry validation comes after the failed signature validation early return case where the pendingSignerRevoke entry is assigned to true. As a result, signatures that are made invalid by the sender after sending them, such as a gnosis safe consuming the related nonce, can be marked as pending in the pendingSignerRevoke mapping.

These actions cannot be replayed due to the nonce check in the revokeSigner function, however to avoid any unexpected behavior the expiry validation should be carried out before the invalid signature early return case.

Move the verifyBeforeActionExpiry validation to before the !isSignatureValid case handling.

In the updateFunding function there are no guardrails which prevent the sequencer from calling the function with a completely invalid fundingDelta that either awards users with a large sum of USDe, or causes them to immediately become insolvent. This may be a mechanism through which a compromised sequencer account drains the funds in the ExchangeGateway contract.

Be aware of this risk and consider adding guardrails to how much the fundingDeltaUsd can change over time.

During the finalizeWithdrawals flow, the Claimer sends msg.value to the exchangeGateway, but this value is not based on the LayerZero quote. This means the Claimer must send more than enough to the exchangeGateway. However, the exchangeGateway transfers only the exact required amount to LayerZero, and the excess is never refunded to the Claimer.

Additionally, the Claimer does not implement a receive function to accept refunds, even if a refund mechanism were introduced.

Add a receive function to the Claimer contract and refund any excess back to the Claimer.

When action.length == MATCH_ORDER_TAKER_SIGNATURE_LENGTH, the if condition below evaluates to false and the code proceeds. However, accessing action[MATCH_ORDER_TAKER_SIGNATURE_LENGTH] is out of bounds, since valid indices are 0..action.length-1. This results in a panic error instead of reverting with the intended InvalidActionEncoding custom error.

// There must be enough data to read both signature lengths
if (action.length < MATCH_ORDER_TAKER_SIGNATURE_LENGTH) {
    (ActionId actionId, ActionType actionType) = decodeActionHeader(action);
    revert InvalidActionEncoding(actionId, actionType);
}
uint16 makerSignatureLength = uint8(action[MATCH_ORDER_MAKER_SIGNATURE_LENGTH]);
uint16 takerSignatureLength = uint8(action[MATCH_ORDER_TAKER_SIGNATURE_LENGTH]);

Revert if action.length <= MATCH_ORDER_TAKER_SIGNATURE_LENGTH

Throughout the Ethereal exchange casts are made without the use of SafeCastLib.

In particular the cast of the currentCost to int128 is at risk of overflow in the realizePnL function in the rare case where the currentCost exceeds the int128.max value.

int128 newCost = int128(currentCost) + costDelta;

Use SafeCast for this cast in the realizePnL function.

When action.length < 9, decodeMatchOrdersAction calls decodeActionHeader to populate InvalidActionEncoding, but decodeActionHeader reads bytes8(action[:8]) and action[8], which will revert with an out-of-bounds panic. This produces an unexpected generic revert rather than the intended InvalidActionEncoding error, complicating upstream error handling.

Before calling decodeActionHeader for error reporting, first check action.length >= ACTION_HEADER_OFFSET (9). If shorter, revert InvalidActionEncoding with zeroed id/type or a dedicated error that does not attempt to parse the header.

The removeToken function only checks Account.DataGlobal.totalBalance and totalPendingBalance, but does not verify whether any withdrawals for the token are pending. Removing a token zeroes its Token.Data (including tokenAddress and oftAddress), which can cause finalizeWithdraw to lack required metadata (e.g., ERC20 address or OFT adapter) and revert or behave incorrectly for already-initiated withdrawals.

Before allowing removal, also verify there are no pending withdrawals for the token across all accounts, perhaps by maintaining this count in storage per token.

In the _parsePythPriceFeed function there is no validation that confirms the channel that is being used for the Pyth lazer price.

The Pyth Lazer example project includes a channel validation to validate that the channel is the RealTime channel.

Consider adding an explicit validation at the contract level that the channel is as expected and matches the RealTime channel:

if (channel != PythLazerLib.Channel.RealTime) { revert("expected update from RealTime channel"); }

Throughout the PerpEngine and Liquidation contracts the verification functions which assess the tolerance difference between the provided offchain value and the value computed onchain emit an event when there is a mismatch between the offchain and onchain values that is below the tolerance threshold and thus acceptable.

However if the mismatch is 1 wei then no event is emitted, even though a mismatch was found, this is because the event emission case always uses else if (absDiff > 1).

Compare the absDiff to see if it is greater than zero: else if (absDiff > 0).

In the updateDelegateDepositMaxBatchSize function there is no event emitted to denote the update as described in the README and as is done in other configuration functions.

Consider adding an event to signal the update made in the updateDelegateDepositMaxBatchSize function.

In the liquidation flow the pythFee is forwarded to the pythLazer contract which refunds excess native that might have been sent. There is however no logic to process refunds that may have been sent back by Pyth Lazer on the liquidation action.

Consider adding a function that allows the contract owner to sweep any native assets which may have been refunded.

In the _handleRevokeLinkedSigner function the pendingSignerRevoke mapping entry for the subaccount and signer is set as true when the signature is invalid as reported by the isValidSignature function. When this case occurs, the revokeSigner function will be called in the finalizeRevokeLinkedSigner function to clear this pending withdrawal.

While the pending withdrawal is present, the signer cannot be revoked for that subaccount with any other signature.

As a result, if a message is ever submitted that uses a nonce that was already consumed for the RevokeLinkedSigner action type, then the revocation will be stuck forever, as the finalizeRevokeLinkedSigner also validates that the nonce was not previously used in the revokeSigner function.

Since the signature verification is being skipped in the finalizeRevokeLinkedSigner function, consider if the nonce verification can be skipped in this instance as well, while still marking it idempotently as used after the finalization. Otherwise ensure that this case can never arise through the sequencer.

In the _completeWithdrawal function when withdrawals are made using the native L3 chain instead of OFT withdrawals, the withdraw.destinationAddress is ignored and the withdrawal is always sent to the account.

However for some accounts, especially those that use the EIP 1271 integration, they may not expect to receive the USDe funds and are expecting them to go to the destinationAddress they specified.

In some cases this could end up trapping USDe for account addresses that cannot handle it.

Either honor the withdrawal.destinationAddress during the _completeWithdrawal flow for native chain withdrawals, or explicitly validate that the destinationAddress is not configured for withdrawals that will use a destinationEndpointId of 0 or the exchange lzEndpointId.

Throughout the Exchange contracts, in liquidation and order matching, the expected pnl of the action is returned from the settlement action. The expected pnl of the action can however have a discrepancy of a handful of wei, this can cause drift from the totalBalance recorded, breaking the invariant that the sum of user balances is the same as the totalBalance.

This drift from the totalBalance can cause issues when removing a token or when the final depositor for a token is attempting to withdraw.

Firstly, it may not be possible for the totalBalance to reach zero due to the drift between the individual balances entries. Notice that the removeToken function validates that the totalBalances mapping is exactly 0 for the token before carrying out the removal.

Secondly, a final depositor may encounter a revert when attempting to withdraw all of their balance due to an underflow when deducting the withdrawal amount from the totalBalance. Or the fee claimer may experience a revert when attempting to claim their final fee amount.

Consider accounting for these last depositor/shutdown cases by maintaining track of the number of accounts with deposits for a token. If that amount of accounts would become zero, then reset the totalBalances mapping entry for the token to zero.

The _verifyFillNotional function performs precision validation on the fill notional amount with a tolerance of (internalValue >> 52) + 1.

However elsewhere in the liquidation transferred notional validation, the imprecision validation is performed with (eTransferredNotional >> 52) + 13.

This yields inconsistent precision thresholds in these cases and across the codebase which may result in unexpected tolerance invalidations if the offchain system is meeting the lower precision threshold across the board.

Consider standardizing on a precision constant, or document why these cases deviate from each other.

In the liquidateSubaccount function, the calculation of the maintenance margin divides the MAINTENANCE_MARGIN_RATIO by the product.maxLeverage before ultimately performing the multiplication with markNotionalX18.

This does not favor the highest precision possible and may contribute to some precision inaccuracies and even reverts outside of the prevision tolerance threshold.

Consider refactoring the maintenanceMargin computation such that it maintains as much precision as it possible can.

Because the Claimer contract does not revert but continues in the loop for any failed withdrawal finalizations, the ETH that was intended to be sent to these finalizations may become trapped in the Claimer contract if not carefully refunded.

Along with the fix to H-04, consider adding a refund mechanism to refund the caller for any ETH that went unused due to a failed withdrawal finalization.

In the comment for the removeProtected variable there is a typo which confuses the purpose of removeProtected: /// If the token is used by a product (as either a quote or base asset), it cannot never be removed.

When in fact the product cannot ever be removed.

Correct the comment to /// If the token is used by a product (as either a quote or base asset), it cannot ever be removed.

In the Claimer finalizeWithdrawals function finalize is misspelled as finialize.

Correct it to finalize.

The project includes two proxy contracts, EtherealProxy.sol and EtherealClaimerProxy.sol. Both contracts are identical implementations of ERC1967Proxy. Maintaining duplicate proxy contracts adds unnecessary complexity to the codebase.

Use a single proxy contract to avoid duplication. If two proxies are needed, clearly document their different roles.

In the ActionHandler contract the _delegateCallVerifier is unused as the verifier has been turned into a library.

Remove the _delegateCallVerifier function.

The _hashTypedData function in the Solady EIP712 library only re-computes the domain separator if the _domainNameAndVersionMayChange function returns true.

However the Ethereal Exchange does not override and return true for the _domainNameAndVersionMayChange function, therefore when the version of the exchange is bumped, or if the name of the exchange for the domain separator is changed then the corresponding signing domain will not be updated. This is actually the correct behavior, as an updated signing domain would change the resulting withdrawDigest that keys pending withdrawals and cause them to be zero’d out.

As a result, the initial value that is provided as the VERSION constant must always remain as the version once in production.

Be aware of this behavior and do not plan on updating the VERSION constant as 1) it will not update the signing domain due to a lacking override of _domainNameAndVersionMayChange and 2) even if this override is made, it would be catastrophic to update the VERSION as it would re-key pending actions.

The finalizeRevokeLinkedSigner function has no delay applied to the validation performed in the function.

As a result a signer could be immediately revoked by the owner and sequencer without providing a valid signature from the user.

Consider if this should be allowed on the exchange without a delay, and if not, consider adding delay validation similar to the rescue funds function.

In the Errors library there exist the WithdrawFound error is unused.

Consider removing the WithdrawFound error or moving it into the DeprecatedErrors library as needed.

In the realizeFunding function the provided list of productIds is iterated over and PerpProduct.load(productIds[i]) is used to load in the relevant product storage pointer.

On the following line the productId is assigned to the product.id from the storage pointer, however there is no validation performed to ensure as a sanity check that the resulting productId is the same as the productIds entry of the current index.

Consider adding a validation to ensure that the declared productId == productIds[i].

In the PerpEngine contract the _verifyOrder function validates that the order.quantity is not above the product’s specified max quantity. However the max quantity can be surpassed by an order which assigns a quantity of 0 and is treated as a full position close.

In the case where the position size exceeds the product’s specified maxQuantity, this order can be larger than expected. Especially if matched with another 0 quantity order for a position of the opposite side, this can allow for unexpectedly large matches to occur.

Furthermore, during liquidation there is no validation of the sizeDelta against the maxQuantity. Though this is expected, this finding notes that the maxQuantity does not apply to liquidations.

Be aware that the maxQuantity is not respected for orders with quantity zero that close the entire existing position, or liquidations.

In the Liquidation flow there is now validation against the status of the product being liquidated, as a result liquidations could technically be carried out for products that are not currently live.

Be aware of this possibility and consider explicitly validating that a product is in the ACTIVE status before carrying out the liquidation.

In the _verifyProductSizes function there is no validation that explicitly prevents the lotSize from being 0.

This will result in unexpected and unclear panic division by 0 reverts when validating modulo lotSize.

Consider adding an explicit lotSize nonzero validation before the modulo validation.

In the liquidateSubaccount function it may be beneficial to add a liquidator configuration check that ensures that the liquidator account and subaccount are not zero values.

Consider adding a liquidator account and subaccount address to confirm the liquidator is properly configured as a sanity check.

The finalizeRevokeLinkedSigner function does not validate that the action expiry has not passed at the time of finalization, this may be unexpected for a user if one of their pending revoke actions is executed by the owner long after their action has technically expired.

This behavior may be acceptable, however be aware of this nuance and consider documenting it for users.

In the local network withdrawal flow there is no usage of msg.value although it is used within a payable function flow.

To avoid any chance of potentially trapping Ether value by calling the complete withdrawal flow with ether value for native chain withdrawals, consider reverting if the msg.value is nonzero in the native withdrawal case in _completeWithdrawal.

In the _completeWithdrawal function when the withdrawToken does not match the usdToken and the destination chain is different from the native network, the function reverts with the InvalidDepositToken error.

This pending withdrawal cannot be cleared by either finalizeWithdrawal or rescueFunds, and therefore holds the account’s pending withdrawal balance hostage.

For now, only the usdToken in the balance mapping is supported, so this may be acceptable currently. However in the future consider validating that the pending withdrawal will not fail this validation upon creation to avoid trapping the withdrawal for users.

In the withdrawal flow it’s possible for a user to specify a withdrawal amount that is larger than the required withdrawal fee, but rounds to a cross-chain transfer amount of zero due to the decimalConversionRate flooring.

For example:

• The withdrawal fee is 1e9
• User A initiates a withdrawal USDe of 1e9 + 1 in normalized amounts
• This passes the withdrawal fee validation, the amountMinusFee is 1 wei in normalized form
• On withdraw finalization, the denormalized amount is 1e9
• The amountLd value rounds to zero due to the decimalConversionRate being 1e12
• The amount sent is ultimately 0 value and this results in a revert

Such pending withdrawals cannot be processed and will always remain stuck.

Be aware of this and consider validating that the remaining amount after fees is greater than or equal to the decimalConversionRate after being denormalized when a cross-chain transfer is to be executed.

In the realizePnl function there is no validation that ensures that the costDelta being realized as supposed profits to the account’s liquid margin balance in the accountData.balance[subaccount][usdToken] entry are actually from position profits.

The costDelta could be putting the position into extreme losses and allowing the balance mapping to be inflated with profits that the account has not actually received.

It is up to the sequencer to carefully invoke the realizePnl function with a cost that represents the actual current market price and the position’s actual equity value.

Be aware of this lack of validation at the Smart Contract level and ensure that the sequencer currently and in future iterations considers the current profit of positions carefully when assigning the cost for the profit realization.

When total balances are non-zero, removeToken reverts with P_ERR_AMT_ZERO, which semantically indicates an 'amount zero' error. However the issue is that the token balance is non-zero rather than zero.

Consider creating a P_ERR_AMT_NON_ZERO error or using the P_ERR_BAD_VAL error here.

The emergencyUnpause function reverts with the ExchangePaused error in the case that the owner calls the function when the exchange is not paused.

This error may be misleading to the caller in the event that it is surfaced, since it claims that the failure is due to the fact that the exchange is paused when in fact it is due to the fact that the exchange is not paused.

Consider introducing an ExchangeUnpaused error for this case.

In the partial reduction case of the _settlePosition function, the cFillData.quantity and cOldCost are multiplied together using uint128 operations.

In some rare edge case scenarios this could be at risk of overflowing the maximum uint128 value, however this is unlikely.

Out of an abundance of caution, consider using a Math library mulDiv function that handles intermediate overflow for the calculation of the cReduceCost value.

In the partial reduction case of the _settleLiquidatorPosition function the rt.quantity is casted to a uint128 to deduct from the product.openInterest. However the rt.quantity is already a uint128 and therefore does not need to be casted as such.

Remove the unnecessary uint128 cast.

The _revert function specifies a string memory return value, yet never returns any data and instead always reverts.

Remove the string memory return value from the _revert function.

The documentation for the _delegateCallPerpEngine function states that it Performs a delegate call to the ExchangeConfig with the data argument.

However, it instead makes a delegate call to the PerpEngine contract.

Correct the documentation for the _delegateCallPerpEngine function.

In the realizeFunding function when an entry in the productIds array does not map to an existing configured product, the function reverts with the ProductNotFound(id, productId, EngineType.PERP) error.

However the productId here is assigned as the resulting memory struct's product.id. That corresponds to the productIds entry. This product id will always be zero and therefore does not allow the consumer of the ProductNotFound error to debug which productIds entry failed the validation.

Correct the error to revert with ProductNotFound(id, productIds[i], EngineType.PERP).

In the decodeMatchOrdersAction function, the decoding of makerSignature currently uses the boundary expression:

makerSignature: bytes(action[MATCH_ORDER_MAKER_SIGNATURE:MATCH_ORDER_MAKER_SIGNATURE + makerSignatureLength]),

However, the value of MATCH_ORDER_MAKER_SIGNATURE + makerSignatureLength is already computed and stored in the variable takerSignatureIndex. Recomputing the addition introduces unnecessary repetition and slightly reduces code clarity.

Use the precomputed variable takerSignatureIndex when decoding makerSignature to improve readability and consistency:

makerSignature: bytes(action[MATCH_ORDER_MAKER_SIGNATURE:takerSignatureIndex]),

In the _settlePosition function in the order match flow as well as the realizeFunding in the funding realization flow, the funding for the account is computed and settled into the account balance mapping. The balance mapping stores values as a uint, meaning that an account’s liquid margin balance cannot go below zero.

However there can arise situations where the funding amount exceeds the liquid margin of an account, for example consider the following scenario:

• Trader A holds a 20 ETH long with an entry price of $5,000 and cost basis of $100,000
• Trader A has 1,000 USDe of liquid margin, opening their position at an initial leverage of 100x
• The price of ETH goes to $6,000, the profit of Trader A’s position is now $20,000 making their total margin value $21,000
• Trader A incurs a funding charge of $1,250, however the realizeFunding and _settlePosition functions will attempt to deduct this $1,250 from the Trader’s liquid margin balance of just 1,000 USDe, which results in an underflow panic revert

As a result, these actions cannot take place for traders in such a scenario, and result in a DoS of the processActions loop.

Similarly, the settling of PnL can also produce an underflow revert when attempting to settle it into the balances mapping if the PnL from other products and funding from other products are not settled beforehand.

Ensure that realizePnl and realizeFunding are invoked as needed in the correct order to allow an account’s pending PnL and pending funding amounts to fit into the account balance mapping in a positive integer and allow order matches to go through.

In the ExchangeGateway contract actions use the _delegatecall internal function to delegatecall the relevant AddressRegistry record address.

The _delegatecall function checks the returned success boolean and reverts if a call was unsuccessful.

However the target.delegatecall(data) operation will return true always for addresses which are EOAs and do not implement any code at their address.

Therefore, in the event of a misconfiguration in the AddressRegistry, if a record is assigned to an incorrect address that happens to be an EOA, the _delegatecall function will not revert.

Many actions in the ExchangeGateway contract that delegatecall to a record are payable and pass along ether to the target address. Therefore in the event of a misconfiguration this Ether could be accidentally trapped.

Consider adding a code length validation to ensure that the target address to in the _delegatecall function has bytecode.

In the CollateralManager contract _completeWithdrawal function there are several ways that excess native msg.value is refunded to the caller.

If the withdrawal is a native chain withdrawal and does not require native funds to be sent then the entire msg.value is refunded. Furthermore, if an excess of msg.value is sent for the cross-chain fee, then the excess is refunded.

Both of these refund cases are not accounted for in the Claimer contract and therefore will become trapped, because the Claimer can't even sweep it's own balance due to the msg.value < totalOftFees check.

Consider adding a rescueNative function callable by the owner to the Claimer contract. Or consider refunding the entire Ether balance of the Claimer contract at the end of the finalizeWithdrawals execution, instead of trying to keep track of the refunds.

The refund logic in _completeWithdrawal is intended to return any excess native tokens sent along with the transaction. These tokens cover LayerZero’s OFT messaging fees during cross-chain withdrawals. When no OFT transaction occurs, the full msg.value is correctly refunded to the caller. However, when an OFT transaction is executed, the refund amount is calculated as msg.value - (messagingFee.nativeFee + amountLD). The issue arises because amountLD is not part of msg.value, it is provided by the collateral manager by unwrapping WUSDe to USDe. Consequently, any excess native tokens sent to finalizeWithdraw or rescueFunds are not refunded, since the calculation incorrectly subtracts amountLD from msg.value. This results in excess native tokens getting stuck in CollateralManager.

Adjust the refund calculation to subtract only the messagingFee.nativeFee from the msg.value.

In the _verifyEip1271Signature function the mstore operation after the external call was determined successful sets the length of the returnData to the bytesToCopy. However this length has already been set in the declaration of the returnData bytes so this operation is redundant and has no effect.

Consider removing the mstore(returnData, bytesToCopy) inside the success block as it is a wholly unnecessary gas expenditure.