LayerZero Upgradeable OApp/OFT Contracts Security Review Report

The __OAppCore_init() initializer will be called twice in __OFTCore_init() as __OApp_init() also contains a call to it.

function __OFTCore_init(address _delegate) internal onlyInitializing {
    __OAppCore_init(_delegate); // 1
    __OApp_init(_delegate); // <--
    __OAppPreCrimeSimulator_init();
    __OAppOptionsType3_init();
}

function __OApp_init(address _delegate) internal onlyInitializing {
    __OAppCore_init(_delegate); // 2
    __OAppReceiver_init();
    __OAppSender_init();
}

We recommend to remove the call __OAppCore_init(_delegate); in __OFTCore_init().

There are open TODO's in code where a simulation PreCrimeUpgradeable::simulate() may revert due to the enforced nonce order on line 198 in PreCrimeUpgradeable.sol.

The same TODO also exists in PreCrime.sol.

TODO's may refer to open issues that should be resolved.

// TODO ??
// Wont the nonce order not matter and enforced at the OApp level? the simulation will revert?
 
// the following packet's nonce add 1 in order
if (packet.origin.nonce != ++nonce) revert PacketUnsorted();

Consider resolving the TODO's.

Since OAppUpgradeable doesn't directly inherit OAppCoreUpgradeable, the OAppUpgradeable::__OApp_init() initializer shouldn't call __OAppCore_init() and __OAppReceiver_init()/__OAppSender_init() together, accordingly with the best practices from the following resources:

• Using with Upgrades - OpenZeppelin Docs
• Why keep "__Ownable_init" beside "__Ownable_init_unchained"?

function __OApp_init(address _delegate) internal onlyInitializing {
    __OAppCore_init(_delegate);
    __OAppReceiver_init();
    __OAppSender_init();
}

function __OAppReceiver_init() internal onlyInitializing {}
function __OAppReceiver_init_unchained() internal onlyInitializing {}

function __OAppSender_init() internal onlyInitializing {}
function __OAppSender_init_unchained() internal onlyInitializing {}

It is recommended to

• replace __OAppReceiver_init()/__OAppSender_init() with __OAppReceiver_init_unchained()/__OAppSender_init_unchained() in __OApp_init()
• add the call to __OAppCore_init() in __OAppReceiver_init()/__OAppSender_init()

In the contract PreCrimeUpgradeable.sol, the implementations of preCrimePeers() and getPreCrimePeers() are identical, with the only difference being that preCrimePeers() is an internal function, while getPreCrimePeers() is external.

By consolidating these functions into a single public function, we can eliminate redundancy and reduce deployment gas costs.

function getPreCrimePeers() external view returns (PreCrimePeer[] memory) {
    PreCrimeStorage storage $ = _getPreCrimeStorage();
    return $.preCrimePeers;
}

function preCrimePeers() internal view returns (PreCrimePeer[] storage) {
    PreCrimeStorage storage $ = _getPreCrimeStorage();
    return $.preCrimePeers;
}

Consider removing the function preCrimePeers() and changing the visibility of getPreCrimePeers() to public.

- function getPreCrimePeers() external view returns (PreCrimePeer[] memory)
+ function getPreCrimePeers() public view returns (PreCrimePeer[] memory){
    PreCrimeStorage storage $ = _getPreCrimeStorage();
    return $.preCrimePeers;
}
 
- function preCrimePeers() internal view returns (PreCrimePeer[] storage) {
-     PreCrimeStorage storage $ = _getPreCrimeStorage();
-     return $.preCrimePeers;
- }

According to the Solidity style guide:

Constants should be named using all uppercase letters with underscores separating words (e.g., MAX_BLOCKS, TOKEN_NAME, TOKEN_TICKER, CONTRACT_VERSION).

Currently, the constants OAppCoreStorageLocation, OAppOptionsType3StorageLocation, etc., use mixed case, which may cause confusion when reading the code.

bytes32 private constant OAppCoreStorageLocation = 0x72ab1bc1039b79dc4724ffca13de82c96834302d3c7e0d4252232d4b2dd8f900;

bytes32 private constant OAppOptionsType3StorageLocation = 0x8d2bda5d9f6ffb5796910376005392955773acee5548d0fcdb10e7c264ea0000;

bytes32 private constant OAppPreCrimeSimulatorStorageLocation = 0xefb041d771d6daaa55702fff6eb740d63ba559a75d2d1d3e151c78ff2480b600;

bytes32 private constant PreCrimeStorageLocation = 0x56af11d8938063b7ae95a808f88d40c589be3bbf4cb6facdbc642a7b38e01f00;

bytes32 private constant OFTCoreStorageLocation = 0x41db8a78b0206aba5c54bcbfc2bda0d84082a84eb88e680379a57b9e9f653c00;

Rename these constants to use all uppercase letters with underscores for better alignment with the recommended naming convention.

In line 310 of the OFTCoreUpgradeable._lzReceive() function, the comment /* the index of the composed message */ is intended to clarify the purpose of the third input parameter 0. However, since the comment is positioned after the third comma, it may mistakenly appear to reference the fourth parameter, composeMsg, potentially confusing the reader.

endpoint.sendCompose(toAddress, _guid, 0, /* the index of the composed message*/ composeMsg);

Consider modifying line 310 of the contract OFTCoreUpgradeable to:

- endpoint.sendCompose(toAddress, _guid, 0, /* the index of the composed message*/ composeMsg);
+ endpoint.sendCompose(toAddress, _guid, 0 /* the index of the composed message*/, composeMsg);

The decimals() function is not a part of the ERC-20 standard, and was added later as an optional extension:

"OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present."

Some ERC20 tokens may not support the decimals() function, therefore it's unsafe to cast all tokens to this interface.

constructor(
    address _token,
    address _lzEndpoint
) OFTCoreUpgradeable(IERC20Metadata(_token).decimals(), _lzEndpoint) {
    innerToken = IERC20(_token);
}

Let the token decimals be passed in as a parameter or consider using some function safeDecimals() like this:

https://github.com/boringcrypto/BoringSolidity/blob/c73ed73afa9273fbce93095ef177513191782254/contracts/libraries/BoringERC20.sol#L49-L55