Omnichain Ledger

Proof of concept: PoC

Vesting._cancelVestingRequest() and Vesting._claimVestingRequest try to remove the current request by: 1. Overriding it in the storage with the last request in the array. 2. Deleting the last request.

It fails to execute the first step because it only updates the local variable to point to the last request, but no real storage update is done.

As result, the removed request is always the last one. A malicious user can use that to create a cancel request with large value followed by many 1-wei cancel requests. They can then cancel their large request many times because on each cancel, one of the 1-wei requests will be removed instead of the real one. This will cause the user's staked balance to grow indefinitely and result in stolen funds.

Do not assign values from storage to storage just before deleting them. Cache the value to move to memory, assign it to its new index, and then pop the last element from the storage.

Orderly Team: The issue was resolved in commit 366de22.

Proof of concept: PoC

When claiming vested ORDER, the OmnichainLedgerV1 sends PayloadType.ClaimVestingRequestBackward message to the ProxyLedger. However, the payload validation in ProxyLedger does not implement the ClaimVestingRequestBackward payload and instead reverts.

This will result in tokens being stuck forever in the ProxyLedger because they will be minted to the ProxyLedger when the endpoint's lzReceive gets executed and a compose message will be stored that will call the ProxyLedger. The said compose message will always revert and the user will lose their tokens.

Change the ProxyLedger to support the ClaimVestingRequestBackward payload.

Orderly Team: The issue was resolved in commit c643f97.

In order to claim USDC revenue from ledger, the owner should mark the batch as claimed using batchPreparedToClaim. This will also reduce the totalValorAmount as much as the redeemed amount in the batch.

The issue is that totalValorAmount is always zero, causing fixedValorToUsdcRateScaled to be zero as well. Consequently, attempts to call batchPreparedToClaim fail with the BatchValorToUsdcRateIsNotFixed error, making it impossible to mark the batch as claimed and preventing users from executing USDC claims.

Increase totalValorAmount by pendingValor when _updateValorVarsAndCollectUserValor is called.

Orderly Team: The issue was resolved in commit 17e40bf.

Proof of concept: PoC

LedgerOCCManager relays message requests from the OmnichainLedgerV1 to the vault chain using ledgerSendToVault. This function will call the orderTokenOft contract with the native fee and set LedgerOCCManager as the refund recipient.

These relay requests will fail due to the fact that: 1. The ProxyLedger does not specify any msg.value sent to the lzCompose function during execution. bytes memory options = OptionsBuilder.newOptions.addExecutorLzReceiveOption(_oftGas, 0).addExecutorLzComposeOption(0, _dstGas, 0);

1. Owner can't fund LedgerOCCManager with native currency, as there is no receive function.
2. If there are any excess fees, the refund transaction will revert.

Therefore, LedgerOCCManager has no native currency to pay the message fees, preventing users to execute any function on ledger chain with a backward message.

If the message fee is meant to be paid by Orderly, add a receive function to LedgerOCCManager contract to allow deposits and refunds to be processed and a privileged withdraw function. Otherwise, consider specifying the addExecutorLzComposeOption with the msg.value that needs to be sent by the executor in order to pay for the backward message.

Orderly Team: The issue was resolved in commit a2f7330.

Guardian Team: payloadType2BackwardFee[message.payloadType] was added but can lead to DoS when non-zero. Change the messaging fee to MessagingFee memory msgFee = MessagingFee(lzFee, 0);

User's pending valor is calculated based on the current accrued valor share, user's staked balance and claimed valor. The accrued valor share uses valorPerSecond and elapsed time to calculate these shares.

The issue relies on the admin being able to update this valorPerSecond state variable, using the permissioned setValorPerSecond, without realizing the accumulated valor with the old rate. Any rate hike or decrease will directly affect user's claimable valor (positively or negatively).

Consider moving the setValorPerSecond function to the Staking.sol contract and call updateValorVars before the rate change.

Orderly Team: The issue was resolved in commit 2d44e9e.

The system uses two variables to calculate the valor emission, totalValorEmitted and maximumValorEmission, and performs a check to validate if the new emission and the total emitted will not surpass the max value.

The issue is that totalValorEmitted state variable is never updated, so it's value is always 0. Therefore, the valor cap validations are non existent, and the system can emit valor indefinitely.

Update the totalValorEmitted state variable, by adding the valorEmission in _getCurrentAccValorPreShareScaled: totalValorEmitted += valorEmission

Orderly Team: The issue was resolved in commit 17e40bf.

Guardian Team: _doValorEmission is a public function, and calling it directly without calling updateValorVars will result in broken internal accounting and inaccurate valor-to-share ratios since it will emit valor without updating accValorPerShareScaled. Change the visibility of the _doValorEmission function to internal.

Furthermore, with the introduced changes valor will be emitted when paused or when there are no stakers.

Proof of concept: PoC

Users can claim ORDER and esORDER rewards based on the merkle distributions. The issue arises when claiming esORDER, as the amount claimed will be staked, but will also try to send a ClaimRewardBackward payload to the Vault chain.

The internal vaultRecvFromLedger in ProxyLedger contract will fail as this callback performs the following check: require(message.token == LedgerToken.ORDER && message.tokenAmount > 0, "InvalidClaimRewardBackward");

Therefore, every esORDER claim request will block the LayerZero pathway for the destination chain, as no new messages can be executed before clearing the failed one.

Avoid sending the message back to Vault Chain when claiming esORDER rewards, by early returning after _stake or creating an else statement.

Orderly Team: The issue was resolved in commit ec1350c.

Partial vesting claim requests occur when users claim before the vesting linear periods. When these claims are executed ORDER tokens are sent to the user in vault chain, but unclaimed amounts are transferred directly to the orderCollector in the ledger chain.

The issue is that OmnichainLedgerV1 will never have ORDER token balance, so the safeTransfer will always fail as long as there is unclaimed amount. Therefore, users will need to wait for the full 90 days vesting period to be over as no partial claims are possible.

Move the safeTransfer call to the LedgerOCCManager and create a function in OmnichainLedgerV1 so that unclaimed amounts can be transferred to the collector.

Orderly Team: The issue was resolved in commit c643f97.

Proof of concept: PoC

When the OFT token receives a request to send amountLD of tokens, it will deduct some dust amount from that. After that a slippage check is performed and if amountLD - dustAmount is less than a given minAmountLD, the transaction will revert.

The problem found in OCCManager and LedgerOCCManager is that both amountLD and dustAmount are set to the same value. This will result in inability to transfer tokens when there is a dust amount present, meaning no staking, withdrawing, claiming can be performed with these amounts.

Ignoring the UX issues, this can be quite problematic for unstaking and vesting esOrder. Imagine that a user has some esAmount that they unstake and vest. After the vesting period ends, the user is not able to claim their order tokens because esAmount has dust amount to be removed.

A possible solution may be to pass the cleared amountLD as minAmountLD. However, this can lead to some minor token losses when sending a message from the Ledger side to the Vault side. If you want to mitigate these, you can add an additional state variable that tracks dust amounts and adds them to the respective users' balances.

Orderly Team: The issue was resolved in commit bdc99c5.

Proof of concept: PoC

Several contracts in the repository inherit Pausable from OpenZeppelin and correctly implement the pause and unpause functions. However, the modifier whenNotPaused is not implemented in core contracts such as OrderOFT, OrderAdapter and ProxyLedger.

The modifier should be added to user-facing functions to allow the owner to pause operations in the event of an emergency for example. A more critical issue also exists because OmnichainLedgerV1 correctly implements the whenNotPaused modifier while ProxyLedger does not.

If the owner were to pause both contracts, users would still be able to call functions like stake on the ProxyLedger which would execute successfully but revert on destination chain. User's tokens would lose funds as their tokens would be burned on source chain, minted on destination chain but never staked on the user's behalf.

Add modifier whenNotPaused to core functions such as send, stake, and sendUserRequest.

Orderly Team: The issue was resolved in commit bb802a9.

The system uses upgradable contracts. The following parent contracts don’t use storage gaps, which will result in a corrupted storage if a variable is added/removed:

Omnichain:

• VaultOCCManager
• LedgerAccessControl
• OCCAdapterDataLayout and LzTestData
• ChainedEventIdCounter
• MerkleDistributor
• Valor
• Staking
• Revenue
• Vesting

Add storage gaps to the upgradable contracts.

Orderly Team: The issue was resolved in commit 8dae135.

Guardian Team: Storage gaps and used storage slots should add up to 50 which is not the case for most of the current contracts.

grant only validates if the array params match in length, but there are some crucial validations that should be enforced to correctly grant tokens to a holder:

• Validate that the start and cliff times are in the future.
• Ensure that the cliff time is not before the start time.
• Ensure that the amounts being granted are greater than zero.
• duration > 0
• cliffTime < startTime + duration

Additionally, when the owner creates a regrant, it will add the new amount to the exiting grant, but it will overwrite the timestamps and durations. Setting a shorter duration can allow the user to claim all tokens immediately.

Consider adding the validations above to correctly create a token grant.

Orderly Team: Resolved.

The TREASURE_UPDATER_ROLE can call Valor.dailyUsdcNetFeeRevenue to report for USDC revenue by using a signed message.

However, there are no checks if the signature has already been used which allows for one signature to be reused unlimited times.

Add a nonce to the signature, hash it and check if it's been already used.

Orderly Team: The issue was resolved in commit be65ca2.

Guardian Team: data.timestamp was added to the signature in an attempt to fix M-03. However, signatures can still be reused as the same timestamp can just be passed. Validate whether the signature was used prior.

orderTokenOft is set in the OmnichainLedgerV1 initialize function, but admin can use setOrderTokenOft to update this token address at a later stage. There are a couple of issue that arise when performing this update:

• LedgerOCCManager does not contain an admin function to update the orderTokenOft address. If

address is updated on OmnichainLedgerV1 and not in LedgerOCCManager, users will still be able to stake with the old token address.

• LedgerOCCManager will hold orderTokenOft tokens sent from vault chains. If the token address is

updated, any withdraw or claim will fail as the contract does not have balance of the new token.

• When users request vesting claims, the unclaimedOrderAmount will be sent to the orderCollector.

In case admin updates the orderTokenOft, user claims will be DoS'ed as the contract may not have sufficient amount of the new token to transfer the collector.

Avoid changing the orderTokenOft address. Alternatively, add setOrderTokenOft to the LedgerOCCManager contract or always read the updated address from the OmnichainLedgerV1. Be aware that same amount of OFT tokens should me minted as the previous OFT token balance in LedgerOCCManager to avoid issues with the stakes and vests.

Orderly Team: The issue was resolved in commit c643f97.

ProxyLedger allows users to stake ORDER or esORDER. When the isEsOrder flag is set to true, users will effectively stake esORDER on the OmnichainLedgerV1 contract, but will also be charged ORDER tokens, due to the fact that vaultSendToLedger will deduct the staked amount from the user balance.

Therefore, user will end up with an esORDER stake, and ORDER tokens will be sent to the LedgerOCCManager contract. Although user will be able to unstake, vest and re-claim the ORDER tokens, this is an unexpected behavior as esORDER staking should not be triggered directly.

Additionally, if future upgrades give more reward weight to esORDER staking, users will be able to game the system by creating an esORDER stake with ORDER tokens.

Always use LedgerToken.ORDER in the ProxyLedger stake() and remove the isEsOrder param.

Orderly Team: The issue was resolved in commit edebe01.

The intended batch flow is as follows: 1. Users redeem valor in batch A. 2. Batch A finishes. 3. A trusted role marks the batch as claimable.

Whenever a user redeems their valor or claim USDC, _collectUserRevenueForClaimableBatch is called to increase their withdrawable amount by adding the withdrawable amounts of the first claimable batch.

If there are users that redeem their valor in between steps 2 and 3 from above (so batch A is finished and batch B is active) when they claim USDC when batch B becomes claimable they will be given the reward from only the first claimable batch even though they are entitled to rewards from two batches.

Accumulate rewards for all claimable batches when claiming

Orderly Team: The issue was resolved in commit d73c7ed.

The Revenue contract tracks the amount of USDC tokens required per batch id, for users to redeem. It uses the fixedValorToUsdcRateScaled to calculate the USDC amount based on the batch valor amount.

This USDC amount calculation is missing the VALOR_TO_USDC_RATE_PRECISION correction, so the value returned is greater than expect, thus invalid. Additionally, if the batch id is not claimable yet, the fixedValorToUsdcRateScaled value is not set (value 0), so all amounts will be 0 due to the multiplication.

Add a division by VALOR_TO_USDC_RATE_PRECISION to fix the units.

Orderly Team: The issue was resolved in commit d73c7ed.

setTotalUsdcInTreasure was meant to be called by the DEFAULT_ADMIN_ROLE and not the TREASURE_UPDATER_ROLE, as the latter will use dailyUsdcNetFeeRevenue which validates signatures and checks timestamps.

Update the access control modifier in setTotalUsdcInTreasure to use onlyRole(DEFAULT_ADMIN_ROLE)

Orderly Team: The issue was resolved in commit 6828c6a.

A user may observe that redeeming valor in batch A is quite profitable so they initiate a redeemValor request. However, due to their transaction taking quite a long time to be executed and delivered to the Orderly Network, their redeem request may now end up in batch B. In batch B the redeeming terms may not be as good as the user desired.

For example, a lot of valor accumulated without much profit or the admin called Valor.setTotalUsdcInTreasure to decrease the usdc. The user will not be able to stop the transaction and in result, they will redeem at undesirable rate.

Add an additional batchId parameter to the redeemValor function and revert the tx if it doesn't match the current batchId.

Orderly Team: Users will be warned about this behavior.

According to the comments in MerkleDistributor: Contract is pausable by owner. It allows to pause claiming rewards.

However, claimRewards doesn't have the whenNotPaused modifier. This is most likely because claimRewards calls function updateRoot which implements it. However, the call to updateRoot is conditional - it happens only if there is an upgrade possible. Otherwise, claiming is still possible -even in paused state.

Add the whenNotPaused modifier to the claimRewards function.

Orderly Team: The issue was resolved in commit 3be4fb4.

In order to make a batch claimable, admins need to update the totalUsdcInTreasure using dailyUsdcNetFeeRevenue. If the USDC amount added has 6 decimal precision (as most USDC tokens), then the user will receive a 6 decimal amount when the ClaimUsdcRevenueBackward payload is executed. bool success = IERC20(usdcAddr).transfer(message.receiver, message.tokenAmount);

The protocol plans to allow claims in multiple chains, using both native USDC or OFT wrappers. The issue is that OFT tokens have 18 decimals by default. Therefore, users will receive less tokens than expected when using OFT wrappers (If the protocol launches on BSC, the pegged USDC token has 18 decimals).

Additionally, as the ProxyLedger will transfer native USDC tokens, it should use safeTransfer instead.

Create the OFT token wrapper for USDC that will be used in the vault chains, overriding decimals to use 6 decimals instead of 18. Ensure that admin updates daily USDC revenue with correct decimals.

Orderly Team: Only Order token will be OFT wrapped.

Vestings are linearly unlocked after a cliff period during the release duration. Claimable balance of a vesting is calculated by subtracting the remaining balance from the total balance.

However, remaining balance of a vesting is calculated using the mulDiv formula from Math Library and this formula rounds down by default. Rounding down the remaining balance means rounding up the claimable balance.

Total claimable amounts will not be affected from this but intermediary claims will give users slightly more tokens than it should.

Roundings should be in favour of the protocol. Use the mulDiv function with selective rounding option from the same library.

Orderly Team: Acknowledged.

Owner has a right to recall a grant and this function returns unclaimed tokens to the owner from the holder. Returning unclaimed tokens:

1. Creates unfair situations between holders. Let’s assume there are two different holders with exact

same cliff and release duration parameters but one of them claimed unlocked part of his vesting and the other didn’t claim anything yet. Recalling from both of these holders results in different user balances.

1. Creates a surface for frontrunning attacks. Users can frontrun the recall and claim their vestings’

claimable portion just before the recall.

Consider adding a new functionality to recall only the remaining portion of a vest alongside recall.

Orderly Team: Acknowledged.

LedgerOCCManager, OCCManager (and potentially OmnichainLedgerV1) are expected to hold ORDER / USDC tokens which are burned or transferred when users claim.

The issue lies with the lack of a withdraw function for Owner to recover these tokens from the contract. Owner may wish to do this during a migration to a new contract. Without a withdraw function, these tokens may become permanently stuck in the contract.

Implement a withdraw function for the contracts which are expected to hold ERC-20 tokens.

Orderly Team: Will consider add withdraw or do it during upgrades.

ProxyLedger, OmnichanLedgerV1 and LedgerOCCManager are all implementation contracts designed to be called through a proxy contract. However, the initialize function can be directly called by anyone.

This would allow a malicious actor to set storage variables such as lzEndpoint and owner on the implementation contract. While no direct risks were found, it is general good practice to prevent initialize from being called.

Use disableInitializer in a constructor. See OpenZeppelin’s guide for more details.

Orderly Team: The issue was resolved in commit cdb53b6.

Users earn valor and collect USDC using their valors. The amount of USDC to collect is determined based on USDC to valor rate and this rate is fixed for every batch by the admin. However, fixing the rate for a batch is done a few days after the batch is finished (It is expected to be 2-3 days after), and the rate at that time is used.

Revenue earned during these days, which should be allocated for the current batch, are allocated for the previous batch. Ideally, the rate at the time when a batch is finished should be used for fairness in between batches. Also, there will be unfair distribution between batches if the fixing time is not exactly the same for every batch.

Consider using the rate exactly at the time when a batch is finished and also consider using smart contract automation systems like Chainlink Keeper.

Orderly Team: The current sequence should avoid the problem.

The error message in vaultRecvFromLedger for PayloadDataType.WithdrawOrderBackward is "InvalidClaimRewardBackward". The contract also has error messages alluding the OrderlyBox, but this is not the correct contract and won't help debugging transactions.

Correct the error message to "InvalidWithdrawOrderBackward" and update the revert string contract.

Orderly Team: The issue was resolved in commit c643f97.

MerkleDistributorL1 and OmnichainLedgerV1 contracts use safeTransfer for ERC20 transfers but ProxyLedger and OCCManager contracts use regular transfer.

Consider using safeTransfer and safeTransferFrom in mentioned contracts too.

Orderly Team: The issue was resolved in commit 4cabfc4.

Comments above the buildOCCMessage function in ProxyLedger contract explain payloadType params. However, some of these comments are not correct and they are misleading. In the comment, RedeemValor is incorrectly matched with enum 6 while it should have been 9. ClaimUsdcRevenue is also not enum 7 but it is 10. Furthermore, remaining functions are not mentioned in the comments.

Consider updating the Natspec comment.

Orderly Team: The issue was resolved in commit 6b49009.

_msgPayload and _options variables in LedgerOCCManager:L114-115 and OCCManager:104-105 are not used in production but used only for testing purposes.

Also, the commented line that has been used for testing in LedgerOCCManager:179 left in the production.

Consider removing these lines from production code.

Orderly Team: Acknowledged.

ValorPerSecondExceedsMaxValue custom error in Valor contract, VestingPeriodIsOutOfRange and DepositNotEnough custom errors in Vesting contract are defined but never used.

Remove unused errors.

Orderly Team: The issue was resolved in commit 45179fb.

There is a typo in L103 of the MerkleDistributorL1 contract. “But it there…” should be “But if there…”.

Update the comment in the mentioned line.

Orderly Team: The issue was resolved in commit 45179fb.

The claim function in the Vesting contract doesn't follow the CEI pattern - it first transfers tokens to the claimer and only after that updates their balance in the mapping.

If in the future this contract is used for other tokens with hooks functionality, an attacker can hijack the execution flow when claiming and drain the funds in the contract.

Follow the CEI pattern by first updating the claimedBalances and then transferring the tokens.

Orderly Team: Resolved.

All Ownable contracts allow the owner to renounce their ownership. This can leave the contracts in an unexpected state and hinder the functioning of the protocol.

Consider utilizing Ownable2Step.

Orderly Team: This operation will be carefully checked.

MerkleDistributorL1.withdraw lets the admin withdraw the unclaimed funds after the distribution ends. However, an admin can at any time update the merkle root with endTimestamp = startTimestamp + 1 and immediately withdraw the funds.

Consider adding a required gap between startTimestamp and endTimestamp in proposeRoot.

Orderly Team: Acknowledged.

Users will start a claimUsdcRevenue transaction from the ProxyLedger but the contract might not have enough USDC balance. Therefore, users will effectively trigger claim on ledger chain, but transaction can't be completed on vault chain.

Consider validating the claimed amount against the contract USDC balance before triggering the claim transaction.

Orderly Team: Orderly is responsible to deposit enough usdc first.

ProxyLedger.claimReward accepts isEsOrder parameter and sets the token of the payload to either ORDER or esORDER. This is redundant because users claim different distributions which are already set for a given token.

Consider removing the isEsOrder parameter and use the LedgerToken.PLACEHOLDER in the payload instead.

Orderly Team: The issue was resolved in commit 5b102ad.

The OCCManager sets the msg.sender as the recipient for the LayerZero refunds when sending a tx from Vault -> Ledger. This means contracts with no receive function will not be able to send messages whenever there is a refund happening.

Either document that smart contracts interacting with the system must be able to receive funds, or let the user pass a refund parameter and use it instead of msg.sender.

Orderly Team: Will inform our users.

After the lock period and linear periods ends, users should be able to claim 100% of esOrderAmount. Although, if the amount is an odd number, the total claimed will be 1 wei less, due to solidity rounding: return _vestingRequest.esOrderAmount / 2 + (_vestingRequest.esOrderAmount * vestedTime) / vestingLinearPeriod / 2;

Consider returning _vestingRequest.esOrderAmount when vestedTime > vestingLinearPeriod.

Orderly Team: The issue was resolved in commit 7141809.

The protocol intends to use multiple chains as vault chains. One of this chains is Polygon known for having many reorgs in the blockchain. These reorgs occur when an alternative version of the blockchain gains consensus, effectively rewriting a part of the blockchain’s transaction history.

The following scenario may occur in a block reorg:

• claim rewards backward message is sent to Polygon
• mints ORDER tokens to ProxyLedger in one tx
• lzCompose tx sends ORDER tokens to the user. - A reorg may occur, discarding the ORDER mint tx,

but including the ORDER transfer tx. Users will receive double amount ORDER tokens when the lzCompose is re-submitted

Consider documenting the issue and monitoring as impact may be limited with LayerZero default configurations.

Orderly Team: Will set larger block confirmations for polygon.

A user may send a claimReward request and pays for all fees, but the merkle root undergoes update before their transaction is executed. Therefore, user will pay message fees but won't be able to claim the rewards. They will need to send a second transaction with the new merkle root in order to claim the rewards.

Consider documenting the issue so the users are aware of this.

Orderly Team: Will be added to documentation and FAQ.