Perpetuals Protocol

In the executeSettlement function, if the settlement.insuranceTransferAmount amount is nonzero it is added to both the insuranceFund.balances and the account.balances therefore duplicating the settlement.insuranceTransferAmount across these two accounts.

The validation on the insuranceTransferAmount indicates that the settlement.insuranceTransferAmount should be deducted from the insuranceFund.balances and added or “transferred” to the account.balances.

Modify the following lines:

insuranceFund.balances[settlement.settledAssetHash] += settlement.insuranceTransferAmount;
account.balances[settlement.settledAssetHash] += settlement.insuranceTransferAmount;

Such that the balance is transferred rather than duplicated:

insuranceFund.balances[settlement.settledAssetHash] -= settlement.insuranceTransferAmount;
account.balances[settlement.settledAssetHash] += settlement.insuranceTransferAmount;

Orderly Team: Fixed.

There is a lack of access control on the setTokenDecimal function, therefore any user can set an arbitrary decimal amount for any tokenHash on any tokenChainId. This can easily be used to inflate how many tokens a user has on-chain and drain the vault.

For example, assume WETH has 18 decimals on chain A and 18 decimals on chain B. However, Alice sets the decimals for chain A as 18 and the decimals for chain B as 19. Upon converting 1 ETH from Chain A, the amount of WETH on Chain B after conversion is tokenAmount * uint128(10 ** (dstDecimal - srcDecimal)) = 1e18 * 10**1 = 1e19.

Alice was just able to turn 1 WETH on chain A into 10 WETH on chain B, and can withdraw those extra funds from the vault. The theft can be even more drastic by increasing the decimal spread between chains.

Ensure only the owner can set the token decimals.

Orderly Team: Fixed.

There is no minimum deposit amount in the Vault contract, therefore a malicious actor may make many deposits with trivial amounts to drain the CrossChainRelay of its Ether and halt execution in the system.

Implement a minimum deposit amount in the Vault contract such that DoS attacks like this one become unfeasible.

Orderly Team: We have proposed a solution to this issue. As mentioned, we will require users to cover the cross-chain fee, this feature will be released in the next sprint.

Proof of concept: PoC

When making a withdrawal, the action goes from the ledger chain to the vault chain back to the ledger chain. When sending the message to the vault, withdraw is called which converts the fee to the proper amount of decimals on the vault chain using convertDecimal. However, truncation may occur due to the collateral on the vault chain having fewer decimals than on the ledger chain.

For example, let's assume the fee is initially 1 wei and the decimal spread between chains is 12. When executeWithdrawAction is triggered on the ledger, 1 wei of fees have been accounted for such that the VaultManager freezes 1 less wei than the withdraw.tokenAmount transmitted: vaultManager.frozenBalance(tokenHash, withdraw.chainId, withdraw.tokenAmount - withdraw.fee).

Once ILedgerCrossChainManager(crossChainManagerAddress).withdraw(withdraw) is called, decimal conversion occurs and the new fee to be sent to the vault chain is 1 / 10**12 = 0. When the message finally arrives back to the ledger to finish withdrawal vaultManager.finishFrozenBalance(withdraw.tokenHash, withdraw.chainId, withdraw.tokenAmount - withdraw.fee) is called.

Because the withdraw.fee is smaller than accounted for initially (1 wei vs 0 wei), more is unfrozen than was originally frozen upon executeWithdrawAction, which will cause an arithmetic underflow in the VaultManager and brick all withdrawals until a user deposits to cover the deficit. This can continuously be done maliciously to continuously block withdrawals from occurring.

Round beforehand such that the rounded value when finishing the withdrawal is the same as when starting the withdrawal. Alternatively, use higher precision for token amounts although this may be a considerable refactor.

Orderly Team: This is an known issue and wont fix in codes. The key point is that the decimal of Ledger should be less or equal to the decimal of vaults, thus no round occurs.

When the calAverageEntryPrice function is called during a liquidation or ADL execution, the liquidationQuoteDiff has 6 decimals of precision. Therefore the resulting quoteDiff on line 72 has 14 decimals of precision.

This differs from the quoteDiff decimals of 16 when uploading a trade. Therefore the averageEntryPrice and openingCost values are perturbed when the quoteDiff is used to calculate the average entry during a liquidation or ADL.

Adjust the quoteDiff during liquidation or ADL such that it has the expected 16 decimals of precision.

Orderly Team: Fixed.

LayerZero implements blocking functionality such that when a transaction on the destination chain fails, before any new transactions can be executed, the failed transaction has to be retried until success.

A malicious user can submit a deposit or withdrawal they know will fail, and will prevent all other deposits and withdrawals from occurring, leading to loss of protocol functionality and loss of funds for those who already deposited.

Some methods a malicious user can use to force such a scenario include but are not limited to:

1. Broker is allowed for Vault on srcChain but disallowed on the VaultManager on dstChain. The

configuration of these two contracts aren’t atomic. 2) depositTo for an address that is blacklisted for the collateral token. When the blacklisted address triggers a withdraw action, the Vault withdrawal will revert.

Consider utilizing a non-blocking approach as described in LayerZero documentation.

Orderly Team: We can handle blocking scenarios, and we also have a force resume function to force drop a message.

Throughout the smart contract system there is a lack of validation to prevent privileged addresses from taking malicious actions or even committing errors that have drastic consequences.

• Executing malicious withdrawals, settlements, ADLs, and liquidations.
• No validation that the liquidationFee = liquidatorFee + insuranceFee.
• No validation that the ratio of positionQtyTransfer to costPositionTransfer is accurate to the adlPrice provided.
• No validation that tokens being actively used as collateral cannot be removed from support, causing liquidations and insolvency.
• No validation that trade.notional = trade.tradeQty * trade.executedPrice in the executeProcessValidatedFutures function.
• No cap on configured values such as the maxWithdrawalFee and liquidationFeeMax.
• No cap on the liquidationFee as the liquidationFeeMax is unused at the contract level.

Consider implementing validations to prevent any potential errors privileged addresses may make. And be sure to document the risks of these privileged abilities.

Orderly Team: Acknowledged.

The documentation for setAllowedToken states that the function is supposed to “Add contract address for an allowed token given the tokenHash.”

However, setAllowedToken only adds or removes the tokenHash from the allowedTokenSet. As a result, the address for the token is never added to the allowedToken mapping.

Deposits and withdrawals will revert as the zero address does not have function safeTransferFrom, and continue to revert until changeTokenAddressAndAllow is called, which according to the documentation is an "unusual case on Mainnet".

Add a parameter for the token address and perform allowedToken[_tokenHash]=_tokenAddress.

Orderly Team: Fixed.

Proof of concept: PoC

Orderly utilizes the halfUp and halfDown methods to round the quotient up or down on the magnitude of the fractional part. However, the edge case of a 0 quotient will cause the division to return a mathematically incorrect result. For example, consider the function call halfUp16_8(10, 11)

int256 quotient = dividend / divisor = 10 / 11 = 0
int256 remainder = dividend % divisor = 10 % 11 = 10
if (10 * 2 >= 11) {
    if (quotient > 0) {
        quotient += 1;
    } else {
        quotient -= 1; <- quotient = 0 - 1 = -1
    }
}

The returned quotient is -1 although 10 / 11 = 0.909 is not supposed to be a negative number and should round to 1.

Add 1 to the quotient when quotient >= 0

Orderly Team: Fixed.

When rounding operations are performed, it is safer for the protocol to round against the users so that there is a smaller likelihood of needing to use the insurance fund or ADL.

The average entry price is calculated using position.averageEntryPrice = halfDown16_8(-openingCost, currentHolding).toUint128() which rounds up if the fractional part is greater than half, and down otherwise.

When a trader is long, it is possible for the entry price to round down which would provide the user a superior entry as they want to buy as low as possible.

When a trader is short, it is possible for the entry price to round up which would provide the user a superior entry as they want to sell as high as possible.

Round against the user depending on their trade direction.

Orderly Team: The recommendation was implemented in commit 5dc96c9b.

Because a withdrawal message can be sent to a Vault on any supported chain, a malicious user can deposit in a Vault on one chain and withdraw from a vault on another chain. This can be used to drain the Vault on a chain, forcing other users to withdraw their liquidity on other chains.

Consider restricting what Vault a user can withdraw from, such as only allowing withdrawals from the chain they performed a deposit.

Orderly Team: Will fix in the next version. Rebalance (with CCIP) is considered to solve this issue.

Proof of concept: PoC

Before creating a position, a user is required to deposit a balance sufficient for their position size. Upon the call to executeWithdrawAction, there is validation that the withdraw amount is not greater than the balance. However, there is no validation at the contract level in the executeWithdrawAction function that restricts a user from withdrawing their entire balance once they have an open position.

If a user’s position is in loss, the user could simply withdraw their entire collateral and not risk losing any of their balance during settlement. This terribly disrupts the operations of the protocol as funds won’t be available to pay profitable traders.

Before allowing a withdrawal, consider validating that the position will not be in a liquidatable state on chain. This may require passing a price with the WithdrawData.

Orderly Team: Won’t fix now. maybe in the future version.

The restrict of withdraw is controlled by engine team, so the contract will not lose balance.

In order to restrict withdrawal by contract, a lot of work need take into consideration with a big update:

1. align price oracle for liquidation, for both contract and engine
2. liquidation should be triggered before withdrawals, for prevent bad debts
3. as the suggestion, add a price with the WithdrawData

In the VaultManager contract, token balances are stored in mappings such as tokenBalanceOnchain or tokenFrozenBalanceOnchain. These mappings take a token hash and a chainID and point to a token balance.

If a chain were to experience a hard fork that chainID will change. This will result in the token balance being inaccessible as there will be a difference between what is being stored in state and the actual chainID of the chain.

Add an onlyOwner restricted function that will allow the protocol to migrate balances from the old chainID to the new chainID.

Orderly Team: Won’t fix. Will add migrate method only this really happens.

1. When a hard fork happens, the legitimate chain should remain the same ChainID, and we should

follow this chain.

1. Even the ChainID changes in HF, we can upgrade the contract to support migrate.

In the executeWithdrawAction function, there is a check to ensure that the withdraw.fee is less than the maxWithdrawFee. However the maxWithdrawFee is a fixed value that will be used for all withdrawals. While the withdraw.fee will be a percentage based on the size of the individual withdraw.

Because the two are inherently misaligned, there is a risk that large withdraws will fail as their withdraw.fee will be greater than the maxWithdrawFee. Users in this scenario will need to break up their withdrawals into multiple smaller withdrawals leading to operational inefficiency.

Make maxFee percentage based so that the fee is never more then X percentage of the amount being withdrawn. This will ensure that all valid withdrawals are still possible.

Orderly Team: Won’t fix.

1. In design, maxWithdrawFee should be a net value, not a percentage value.
2. maxWithdrawFee will be only used in emergency case.

The verify function uses ecrecover without any validation that the s value is from one half of the valid s range, therefore it is possible for signatures to be maliciously replayed with a different s. At present, the OperatorManager is the only address that can perform this signature malleability, however, the opportunity should be removed.

Use the OpenZeppelin ECDSA library, which automatically restricts the valid s range, to verify signatures.

Orderly Team: Fixed.

In the updateMarketUpload functions for both the perp prices and sumUnitaryFundings the lastMarkPriceUpdated and lastFundingUpdated are set to the block.timestamp. However, these attributes ought to be set to the perpPrice.timestamp and sumUnitaryFunding.timestamp as these are the timestamps from which the data was recorded.

Using the block.timestamp for the lastFundingUpdated and lastMarkPriceUpdated values is not in line with the logic in the Ledger.executeProcessValidatedFutures function where the lastFundingUpdated is assigned to the trade.timestamp rather than the block.timestamp.

Replace the cfg.setLastFundingUpdated(block.timestamp) lines with cfg.setLastFundingUpdated(data.timestamp).

Orderly Team: Fixed.

In the event that the accountDeposit function execution reverts, there is no recourse for the user to recover their deposit in the vault.

The accountDeposit function may revert if the AccountDeposit data carries a brokerHash or tokenHash & srcChainId that does not agree with the configuration in the vaultManager contract.

Consider implementing a method for the user to recover their funds in the event that the cross-chain deposit transaction cannot succeed even upon retry.

Orderly Team: Won’t fix, but should pay attention:

1. Ledger should update whitelist before vault update.
2. Even the situation occurs, the cc tx will be payload-store, and can be retried after updating

Ledger’s whitelist.

When liquidatable accounts cannot cover the liquidatorFee with their remaining margin, all positions in the account and the remaining margin balance are transferred to the insurance fund.

In times of volatility, several accounts may become insolvent and all have their positions transferred to the insurance account. The insurance account may then find itself to be insolvent, in which case ADL will not be sufficient to remedy the situation.

The insurance account is also intended to cover insolvent accounts where the settled PnL is more negative than the account margin. This behavior can also be a pathway for the insurance account to become insolvent, especially when combined with receiving positions from insolvent accounts.

Though this scenario may be rare, it is a distinct possibility. Have a contingency plan in the event that this scenario ever plays out, for example a deposit can automatically be made for the insurance fund when it nears insolvency.

Orderly Team: Yes this is a risk; but this risk is also present in other CEXes Reply from our product.

When the Vault is paused, functions deposit and depositTo are prevented from being processed. As a result, the protocol can reach a state where liquidations can occur while deposits are paused, preventing users from keeping their positions solvent.

Consider pausing liquidations when users are unable to increase their collateral and/or clearly document this scenario for users.

Orderly Team: Won’t fix.

1. Should only pause Vault in emergency case, that is, Vault is hacked, or under HF, or unstable state.
2. Even one chain may be paused, other vault is still working. 3. So no need to pause liquidation in

unstable situation. The engine team will take more actions.

There are several Component abstract contracts that intend to be parents of upgradeable contracts, however they lack an appropriate _gap storage variable.

For example, the LedgerComponent contract is an abstract contract that is meant to be inherited by upgradeable contracts, however there is a ledgerAddress storage variable defined in the LedgerComponent followed by no gap variable in the event that more variables would be added to the LedgerComponent contract.

Add a storage _gap variable so that storage variables may be added to the LedgerComponent contract without causing storage collisions.

For more information on the _gap variable refer to the OpenZeppelin documentation.

Orderly Team: Wont fix, but will continously pay attention to this issue.

1. LedgerComponent is an abstract contract, very simple, and will not add any new fields.
2. Contract Ledger and OperatorManager is already use a DataLayout contract with _gap
3. Namespaced Storage is better than _gap method, we are still investigating

https://blog.openzeppelin.com/introducing-openzeppelin-contracts-5.0#Namespaced

In the _futuresTradeUploadData function, a batch of trades are uploaded in a single transaction which allows a single malicious trade to DoS the entire batch if it reverts.

For example, one trade in the batch could have an invalid symbolHash, which would cause the executeProcessValidatedFutures execution to revert. Similarly, in the _eventUploadData function, a batch of events are uploaded to be processed in a single tx which allows a single invalid event to DoS the entire batch.

Be aware of this DoS risk and be sure to simulate batches to verify that they contain no invalid trades before sending a batch upload transaction. Additionally, consider implementing logic at the contract to handle invalid events or trades.

Orderly Team: Won’t fix, acknowledged. These two function are triggered by engine team, with onlyOperator modifier. In design, there should be no invalid call. We also add many checks to avoid malicious call(signature check, whitelist symbolHash check, etc).

Proof of concept: PoC

When a position is updated after a trade, the average entry price is calculated using calAverageEntryPrice.

However, if the openingCost and currentHolding are of the same sign, the calculation of position.averageEntryPrice = halfDown16_8(-openingCost, currentHolding).toUint128() will revert with a SafeCastOverflow. This is because halfDown16_8(-openingCost, currentHolding) will return a negative int which cannot be cast to a uint.

This can occur if the currentHolding exceeds the openingCost for a short position, because halfDown16_8 will round the quotient to -1 on else { quotient -= 1; }; Note that if halfDown16_8 is modified to round up on a 0 quotient, the issue can still occur if currentHolding exceeds the openingCost for a long position, because halfDown16_8 will round the quotient to 1 and the currentHolding is also positive.

Carefully consider which markets are supported, as markets with small prices are more susceptible to this issue. Furthermore, consider implementing a minimum trade size since the attack is more susceptible to smaller quantities causing the openingCost to round down to -1.

Orderly Team: Acknowledged. In design, this will not happen. halfDown16_8 is only a stateless function, and in this situation, the return value should never be negative.

Throughout the codebase the block.timestamp is often emitted with events, however this is unnecessary as the timestamp of the event can be retrieved from the block in which it was emitted. Therefore gas does not need to be expended to emit the block.timestamp as a part of event data.

Remove the block.timestamp from each event and retrieve the timestamp from the block in which the event was emitted.

Orderly Team: The recommendation was implemented in commit 31d480d2.

The side boolean on the FuturesTradeUpload struct is unused in the contracts.

Implement a use case for the side attribute or consider removing it.

Orderly Team: Won’t fix now, the usage of this is for signature verification.

The liquidationTransferId is never accessed from the liquidationTransfer object.

Implement a use-case for the liquidationTransferId or remove it from the LiquidationTransfer struct.

Orderly Team: Won’t fix now, the usage of this is for signature verification.

In the toUint128 function if the provided int128 value is negative, the function reverts with a SafeCastOverflow error. However the function should revert with a SafeCastUnderflow error as the attempted casting would underflow rather than overflow.

Create a SafeCastUnderflow error and use this error to revert the toUint128 function in the event that a negative number is provided.

Orderly Team: Fixed.

The Vault contract and the VaultManager contract both track which tokens are valid for the vault, therefore there are two sources of truth for which tokens are supported by any given vault.

However there is no guarantee that the allowedTokenSet in the Vault contract and the allowedChainToken mapping in the VaultManager contract are in sync.

If the allowedTokenSet and allowedChainToken mapping are ever in disagreement the system accounting is perturbed and the system is may become insolvent.

Be aware of this design flaw and risk when updating configuration on a vault chain or the ledger chain.

Orderly Team: Acknowledged

1. Team should keep consistent of vualt and ledger chain’s allowedList.
2. And Ledger should update whitelist before vault update as this issue commented:

04971a6bd0f4ba0f08&p=50b425edf5af4caba8ffce2e0771c6ca&pm=s

The unused maintenanceMargin function in the AccountTypePositionHelper calculates the positionQty * markPrice * Base MMR.

MMR i = Max(Base MMR i, Base MMR i / Base IMR i * IMR Factor i * Abs(Position Notional i)^(4/5))

Either update the documentation or update the implementation of the maintenanceMargin function.

Orderly Team: Acknowledged.

The quotient returned in halfUp16_8_i256 is cast to an int128 from an int256. This casting operation is unsafe as the result can silently overflow.

For example, when the quotient = type(int256).max, casting to int128 will not revert and return -1 which is an unexpected result.

Use the OpenZeppelin SafeCast library or implement your own checks to validate the range of a type is not exceeded prior to casting.

Orderly Team: Fixed.

In the withdraw function the token amount is transferred to the user before calling withdraw on the crossChainManagerAddress.

This could result in reentrancy opportunities, currently there are no immediate risks with the withdraw function. However, best practice is to follow the Check-Effects-Interactions pattern when transferring out tokens to protect against reentrancy attacks.

Use the Check-Effects-Interactions pattern by transferring tokens out of the vault after state changes occur.

Orderly Team: Fixed.

In the executeSettlement function, the first for-loop is redundant as the totalSettleAmount can be computed inside the second for-loop and validated at the end.

Combine the validation logic into a single for-loop in the executeSettlement function.

Orderly Team: The recommendation was implemented.

The accruedFeeUncoverted variable misspells unconverted as uncoverted.

Replace accruedFeeUncoverted with accruedFeeUnconverted.

Orderly Team: The recommendation was implemented.

The balances adjustment in the executeSettlement function can use subBalance and the addBalance helper functions rather than adjusting the balances mapping directly.

Use the subBalance and addBalance functions to adjust the balances mapping in the executeSettlement function.

Orderly Team: The recommendation was implemented.

In the sendMessage function _lzSend is called. One of the parameters in the _lzSend function is _zroPaymentAddress.

According to Layer Zero integration recommendations the _zroPaymentAddress should not be hardcoded. Instead it should be passed as a parameter instead.

Pass the _zroPaymentAddress as a parameter instead of hardcoding it.

Orderly Team: Acknowledged.

The maintenanceMargin function performs a multiplication on the result of a division, leading to precision loss in the final maintenance margin requirements.

The position.positionQty.abs().toInt128() * markPrice calculation will return a 16 decimal result because both quantity and price are 8 decimal precision values.

Further multiplying by the baseMaintenanceMargin in the numerator has virtually no overflow risk as the max value of the baseMaintenanceMargin is 10,000.

Perform the multiplication before the division:

position.positionQty.abs().toInt128() * markPrice * baseMaintenanceMargin / (int128(MARGIN_100PERCENT) * PRICE_QTY_MOVE_RIGHT_PRECISIONS)

Orderly Team: The recommendation was implemented.

In the depositWithFee function a deposit can be made with a fee attached. The fee is determined by the amount parameter, and depositWithFee checks that the msg.value is greater than or equal to the amount being passed in.

When the fee is sent in sendMessageWithFee the value is the original amount that was passed into the function. The issue is that if msg.value is greater then amount the excess msg.value will be left in the contract with no way of retrieving it.

Currently the function is not accessible so this possesses no immediate risk. But the issue should be fixed if there are intentions of using this function.

Either send the excess msg.value back to msg.sender or to an address that can handle the funds so that they are not stuck.

Orderly Team: Fixed, amount is removed and msg.value is taken as a fee.

The following validation ensures that any transferred insurance amount is sufficient to cover any negative collateral for an account:

if (
    balance.toInt128() + settlement.insuranceTransferAmount.toInt128() + settlement.settledAmount < 0
        || settlement.insuranceTransferAmount > settlement.settledAmount.abs()
) {
    revert InsuranceTransferAmountInvalid(
        balance, settlement.insuranceTransferAmount, settlement.settledAmount
    );
}

However the validation allows for a potentially significant amount of extra funds from the insurance account to be transferred to the user’s account.

Consider altering the validation such that if an insuranceTransferAmount is specified, it must be exactly the amount necessary to make the account solvent, or within a smaller range of an amount that would make the account solvent.

Orderly Team: Acknowledged.

Upon liquidating a position, liquidatedPosition.isFullSettled() is called to check whether the cost and quantity of the position are both 0, and clears the position if so.

However, isFullSettled is never checked upon executeSettlement nor executeAdl.

When positions are cleared in the executeSettlement or executeAdl functions, clear them when isFullSettled() is true.

Orderly Team: The recommendation was implemented for liquidations, however ADL should rarely need to clear positions.

In the CrossChainRelayUpgradeable contract, the initialize function accepts an _endpoint address, yet fails to validate that it is not address(0).

Validate that the _endpoint address is not address(0) in the initialize function to avoid improper deployments.

Orderly Team: Endpoint can be updated later using function updateEndpoint(address _endpoint) external onlyOwner. Even if endpoint address is not address(0) , it could be another wrong address. Both situations are handled by later calling updateEndpoint.

The Layer Zero documentation recommends that projects use the latest version of the solidity-examples package, rather than directly copying example contracts.

The solidity-examples package is not used in the evm-cross-chain repository, and therefore if a patch is ever issued it would not be present in evm-cross-chain.

Use the solidity-examples package as suggested in the LayerZero docs.

Orderly Team: In the future we may have custom requirements, so we will choose to maintain this code ourselves.

There is no need to have both the getBrokerHash and getTokenHash functions since their logic is exactly the same, only the naming of the parameters differ.

The getBrokerHash and getTokenHash functions simply return the result of calculateStringHash.

Use the calculateStringHash function directly.

Orderly Team: The recommendation was implemented.