Layerzero V2 Review Log

I was engaged by LayerZero to do a time bounded security review of a part of the contracts comprising their V2 deployment. The review took place over two weeks and from commit hash 80e93e7. Layerzero is a bridge protocol and my review was focused on the components in the directories: protocol/contracts and messagelib/contracts in the layerzero monorepo. This encompasses the contracts for core message passing component and also libraries related to parsing the messages which are passed. This review does not generally comment on the bridging mechanism itself but rather the fidelity of the scoped contracts to their intended purpose.

Findings

Presented in chronological order:

• Silent failure in the safeCall function family can cause corrupted data: The functions safeCall and safeStaticCall silently truncate returndata to prevent against an attack class called a 'returndatabomb', however they do not indicate to the caller of the function that an error has occurred. This can cause corruption of memory as a contract which interacts with the message passing system gets less data than the encoding would indicate, or in the worst case corruption of memory items such as replacing the end of a bytes array with zero when it is copied from returndata.
  • Severity Estimate: Low [Impact: Medium, Likelihood: Low]
  • Remediation Recommendation: The call should be marked as a failure if not all of its return data can be loaded.

Notes:

• The isSendLib and isReceiveLib modifiers in the MessageLibManager both use a check that the value returned from the a call IMessageLib(_lib).messageLibType() is not equal to their corresponded expected inverse ie isSendLib checks that the returned value is not MessageLibType.Receive. If a library ever either intentionally uses a 3rd state or unintentionally returns a value besides 1 or 0 the isSendLib and isReceiveLib will both be true breaking assumptions that send/receive is mutually exclusive. I recommend changing the checks to be that the returned enum is equal to the expected value.
• The DVN library's execute function has a return data bomb vector. So if there is a place where these are required to be executed in order then it could block execution if a malicious contract is called.
• Seems like slight overkill to have a allow list check on a view function in this.
• The outbound function allows overflows of the new outbound nonce but the inbound functions don't allow overflows so in the very unlikely case somebody could make 2^64 messages it could generate a valid message the backend would not be able deliver. Also the quote function deviates from this behavior and will revert despite the send function succeeding
• The Executor counts the values sent to native drop targets even if those calls revert, this leads to funds potentially being stuck in the Executor till being claimed by the admin. This could be fixed by changing this line to being a conditional, but its not fully clear who a refund should be made to in the context of this contract.