Open code423n4 opened 1 year ago
trust1995 marked the issue as primary issue
trust1995 marked the issue as satisfactory
Due to a cross-chain tx being composed of several txs on different networks this would only be feasible on arbitrum since its the only chain where both root and branch contracts coexist allowing you to nest new retrys inside the previous otherwise the nonce would be flagged as executed in execution history after the first successful run. But definitely the lock
should be added there
0xBugsy marked the issue as sponsor confirmed
To give a little further context on my reply,
1) The permissionless addition of Bridge Agent does not expose any unintended functions to Router so this part is completely intended on our behalf.
2) The core issue here really resides on the fact that the executionHistory[nonce] = true;
should be done in the Branch and Root Bridge Agents before and not after (respecting CEI) calling their respective Executor within a try-catch block.
Adding a lock can also be introduced as a safe-guard but adding that by itself we would still be able to do this attack once within the original settlement
trust1995 marked the issue as selected for report
Lines of code
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L244 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/factories/RootBridgeAgentFactory.sol#L75
Vulnerability details
RootBridgeAgent.retrySettlement()
is lacking alock
modifier to prevent reentrancy. AndRootBridgeAgentFactory.createBridgeAgent()
is missing access control. Both issues combined allows anyone to re-enterretrySettlement()
and trigger the same settlement repeatedly.Impact
An attacker can steal funds from the protocol by executing the same settlement multiple times before it is marked as executed.
Detailed Explanation
Issue #1
In
RootBridgeAgentFactory
, the privileged functioncreateBridgeAgent()
is lacking access control, which allows anyone to deploy a newRootBridgeAgent
. Leveraging that, the attacker can inject malicious RootRouter and BranchRouter that can be used to trigger a reentrancy attack inretrySettlement()
. Injection of the malicious BranchRouter is done with a separate call toCoreRootRouter.addBranchToBridgeAgent()
in CoreRootRouter.sol#L81-L116, refer to POC for actual steps.https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/factories/RootBridgeAgentFactory.sol#L75C1-L89C6
Issue #2
In
RootBridgeAgent
, theretrySettlement()
function is not protected from reentrancy with thelock
modifier. We can then re-enter this function via the injected malicious BranchRouter (Issue #1). The malicious BranchRouter can be triggered viaBranchBridgeAgentExecutor
when the attacker perform the settlement call. That will executeIRouter(_router).anyExecuteSettlement()
when additional calldata is passed in as shown in BranchBridgeAgentExecutor.sol#L110.https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L244-L252
Proof of Concept
RootTest.t.sol
.RootTest
contract withinRootTest.t.sol
.Recommended Mitigation Steps
Add
lock
modifier toRootBridgeAgent.retrySettlement()
and add access control toRootBridgeAgentFactory.createBridgeAgent()
.Assessed type
Other