Closed c4-submissions closed 1 year ago
0xA5DF marked the issue as duplicate of #439
0xA5DF marked the issue as high quality report
alcueca changed the severity to 2 (Med Risk)
alcueca marked the issue as satisfactory
alcueca marked the issue as selected for report
alcueca marked issue #348 as primary and marked this issue as a duplicate of 348
alcueca marked the issue as not selected for report
Lines of code
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1204-L1217 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L930-L944
Vulnerability details
Impact
BranchBridgeAgent
andRootBridgeAgent
contracts implement thelzReceive
function that is required to receive messages from LayerZero relayer. Since it is dangerous to receive messages from unknown contracts, contracts can be securely connected by implementing the Trusted Remote validation scheme.Both contracts implement a faulty validation that makes all messages from LayerZero fail, which puts users' funds at risk.
Proof of Concept
How Are Users' Funds Lost?
Before we delve into proving the issue with the
lzReceive
functions within theBranchBridgeAgent
andRootBridgeAgent
contracts, let's initially assume they always revert. This approach allows us to first comprehend the risks posed to users' funds if the assumption is correct. To illustrate, I will you go through a step-by-step interaction flow, with a description of the key steps involved.1. User Calls
callOutAndBridge
In the source chain,
callOutAndBridge
caches thedepositNonce
, creates apayload
, and then calls_createDeposit
:https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L224C1-L224C118
_createDeposit
increments the globaldepositNonce
and locks tokens into the Local Port:https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L824C5-L824C5
bridgeOut
calls_bridgeOut
and transfers funds from user:https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L525-L533
Execution is handled to
_createDeposit
, which stores the deposit and handles execution back tocallOutAndBridge
.callOutAndBridge
then performs the LayerZero call:https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L227
The user transaction is executed in the source chain and the deposit message is sent accross chains.
2.
RootBridgeAgent
'slzReceive
FailsIn the destination chain,
lzReceive
is called and callslzReceiveNonBlocking
, which implements the modifierrequiresEndpoint
: https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L423C1-L439C78For now, we are assuming this modifier will always revert execution. So, execution reverts and no state changes happens in the destination chain.
3. User calls
retrieveDeposit
Noticing the failure, user calls
retrieveDeposit
in the source chain:https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L422-L431
retrieveDeposit
performs a LayerZero call toRootBridgeAgent
with the flag0x08
to retrieve the deposit; execution is finished in the source chain.In the destination chain,
lzReceive
is called and reverts again. This means the fallback call is never executed:https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L708-L714
_peformFallbackCall
is responsible to call theBranchBridgeAgent
with the flag0x04
to register that the deposit has failed, but this never happens.4. User Calls
redeemDeposit
Since the deposit status was never changed from
STATUS_SUCCESS
, the call toredeemDeposit
will revert:https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L439
User's funds are now stuck.
Why it Happens
When a LayerZero message is received, it triggers the execution of the
lzReceive
functions within the affected contracts. Following this, these contracts initiate the execution of their respectivelzReceiveNonBlocking
functions:https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L578-L590
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L423-L439
After additional checks, both modifiers reveal the same bug when attempting to validate the remote caller:
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L941-L943
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1210-L1215
Please, note that the constant
PARAMS_ADDRESS_SIZE
is defined and assigned inBridgeAgentConstants
with the value20
.The issue arises from the fact that the
_srcAddress
parameter, initially passed to thelzReceive
function, is in the Trusted Remote format. This format is a combination of the packed bytes of theremoteAddress + localAddress
. Consequently, both modifiers inadvertently check thelocalAddress
instead of the intendedremoteAddress
. Consequently, this check consistently results in a revert.Code Evidence
As referenced earlier, the Trusted Remote documentation provides a clear explanation of the Trusted Remote format, which consists of combining the
remoteAddress
andlocalAddress
in this order. Additionally, it outlines where it is used. Despite the documentation, we can also substantiate the issue through code analysis.This is how the message library
UltraLightNodeV2
generates thepathData
(lzReceive
's_srcAddress
):https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/UltraLightNodeV2.sol#L111
Using this value,
receivePayload
is invoked, ultimately leading to the execution of thelzReceive
function within the destination contract:https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/Endpoint.sol#L100-L125
Runtime Evidence
We can also collect recent production runtime evidence. To illustrate, consider a transaction from a LayerZero relayer that we can trace using the online Transaction Tracer at this link: Transaction Tracer Link.
In this transaction, you can search for the string
srcAddress=0x3082cc23568ea640225c2467653db90e9250aaa0
, which corresponds to the remote address on source chain ID110
.By using this value, you can search for
0x3082cc23568ea640225c2467653db90e9250aaa0
and verify that it is indeed the initial portion of thepathData
or_srcAddress
parameter passed to thelzReceive
function:So, consolidating all the crucial information together, we can determine the source and destination for this LayerZero message:
Source Chain ID:
110
(Arbitrum)0x3082cc23568ea640225c2467653db90e9250aaa0
Destination Chain ID:
102
(BNB)0xf7DE7E8A6bd59ED41a4b5fe50278b3B7f31384dF
An Executable Proof of Concept
We can create an executable proof of concept by using the contract
LZEndpointMock.sol
, provided by the LayerZero project themselves.Please note that, for the sake of brevity, this proof of concept focuses solely on testing the
BranchBridgeAgent
contract. The underlying concept being tested applies to theRootBridgeAgent
as well.For this quick proof of concept, just clone the LayerZero's
solidity-examples
repository into the/lib
directory:Now create a test file (e.g.
LAyerZeroAuditTest.t.sol
) within the/test/ulysses-omnichain
directory with the following content:testLzSendCorrectPath
Execute this test with:
Since the revert does not bubble up, we can examine the transaction traces to observe that
BranchBridgeAgent::lzReceiveNonBlocking
reverted withLayerZeroUnauthorizedCaller()
.This occurred because
send
assembled the path using the Trusted Remote format, specificallyremoteAddress + localAddress
, which differs from the expectation ofBranchBridgeAgent
.testLzSendWithWrongPath
Now execute
testLzSendWrongPath
:LayerZeroMock
reverts withdestination LayerZero Endpoint not found
because the path that was passed is in the wrong format (i.e.localAddress + remoteAddress
).testLzReceiveWithCorrectPath
Finally, execute
testLzReceiveWithCorrectPath
:When calling
lzReceiveNonBlocking
directly with the correct Trusted Remote format, it reverts with theLayerZeroUnauthorizedCaller()
error.Tools Used
Manual: code editor, Foundry's
forge
.Recommended Mitigation Steps
Basic Mitigation
To quickly mitigate the issue, this code diff can be applied to
RootBridgeAgent.sol
:And this one to
BranchBridgeAgent.sol
:Next, address the failing tests and ensure they conform to the correct format.
Going Further
Implement the Trusted Remote Format
Rather than manipulating indexes in the
_srcAddress
parameter passed tolzReceive
, I recommend considering a simpler solution. A promising approach could be to follow a pattern closer to that of theLzApp
example:https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/lzApp/LzApp.sol#L44-L49
You can introduce some adjustments related to the utilization of
_srcChainId
and impose a limit of40
bytes on_srcAddress
to align with the required specifications regarding supported chains. However, the key point to emphasize is simplifying therequire
check by directly comparing the entire_srcAddress
with the storedtrustedRemote
(s).Use
lzEndpointMock
or Similar for TestingThe provided proof of concept is simple, almost a hack, aimed at conveying the fundamental concept. However LayerZero's
LZEndPointMock
can be incorporated into the test harness, enabling the simulation of more complex scenarios. In its current state, the tests are themselves callinglzReceive
with the wrong path format, as exemplified by the following:https://github.com/code-423n4/2023-09-maia/blob/main/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol#L402
By employing a more centralised testing solution like
LZEndPointMock
, this bug coud have been prevented.Assessed type
en/de-code