OFT and OFT Adapter

Currently, the OFT contract allows anyone to set the address(0) as a token recipient on the destination chain. However, this is a transaction that will always revert because the OpenZeppelin's implementation does not allow minting to the 0 address. An attacker can leverage this to block the LayerZero pathway.

An example:

• The nonce on both chains is 5.
• An attacker successfully sends their minting tx and the nonce becomes 6.
• The transaction is received on the destination chain, but fails.
• Since the app uses ordered delivery, all subsequent transactions will be blocked until 6 is resolved.

As a result, an attacker can keep blocking the LayerZero pathway.

Don't allow sending cross-chain packages that mint to the address(0).

Orderly Team: The issue was resolved in commit c2e1991.

OrderOFT and OrderAdapter allow the owner to burn, nilify, skip and clear nonces. When doing so, a call to _acceptNonce is initiated.

This is problematic for:

• burning: The nonce to be burnt will always be less than the maxReceivedNonce. However, in

ordered delivery the _acceptNonce function reverts if the specified nonce is not max + 1. This will result in burning not working when ordered delivery is turned on.

• nilifying: The _acceptNonce will let us nilify only the packet with nonce = max + 1. After nilifying the

maxReceivedNonce will be set to the nilified nonce. Now even if the nilified package gets reverified, it will not be possible to execute it.

• clearing: If there is a need to clear a nonce which is bigger than maxNonce + 1 , it will be impossible

because of the ordered nonce.

In addition, these actions can be initiated by a delegate address and not by the OFTApp directly. This will cause a discrepancy between the OrderOFT and LayerZero maxReceivedNonce and the messaging will be blocked because acceptNonce will always revert from this moment on.

Do not call _acceptNonce when burning, nilifying, nor clearing. Furthermore, add a setter setMaxReceivedNonce to update the mapping as necessary and even consider passing a maxReceivedNonce argument in each of the functions which can be used to update the mapping during the execution of the owner functions.

This is because since there can be multiple in-flight packets and one of them is burned/nulled/cleared, and now the order enforcement will lead to DoS. Furthermore, a delegate can burn/nullify/clear outside the Oapp so the setter is absolutely necessary.

Orderly Team: The issue was resolved in commit e32d7c0.

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:

OFT:

• OFTAdapterUpgradable
• OFTCoreUpgradable
• OFTUpgradable
• OAppCoreUpgradable
• OAppReceiverUpgradable
• OAppSenderUpgradable
• OAppUpgradable

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.

The OrderOFT and OrderAdapter contracts are intilialized with orderedNonce flag enabled, although owner can turn this flag on and off as pleased using setOrderedNonce. When orderedNonce is set to false, messages can be executed in any order, increasing the maxReceivedNonce.

If the orderedNonce is turned on back again, there will be issues with messages that were not executed with lower nonces, as the only acceptable nonce will be maxReceivedNonce + 1

Remove the owner function to set the orderedNonce flag. Alternatively, only allow to turn it off and never be able to turn it back on again.

Orderly Team: The issue was resolved in commit e32d7c0.

OrderOFT, OrderAdapter are 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.

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.