search

code-423n4 / 2023-09-maia-findings

25 stars 17 forks source link

Any ETH sent to `ArbitrumBranchBridgeAgent.callOut*()` functions will be lost #226

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L195 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L209 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L231 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L262 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L276 https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/BranchBridgeAgent.sol#L306

Vulnerability details

Impact

Users will face permanent loss of their funds because there is no mechanism in place to transfer these funds anywhere. Specifically, the RootBridgeAgentExecutor lacks any function for withdrawing its balance.

Proof of Concept

In the case of the basic BranchBridgeAgent deployed on other chains, the msg.value sent to the callOut*() functions is treated as a gas fee, efficiently managed by LayerZero. It is sent alongside the call to lzEndpoint, and after the cross-chain call, it is returned to the user as intended.

However, in the scenario involving the ArbitrumBranchBridgeAgent, the msg.value ends up with the RootBridgeAgentExecutor (as evident from the execution logs below), and unfortunately, it is never returned to the user. This results in the loss of user funds.

PoC

//SPDX-License-Identifier: AGPL-3.0-only
pragma solidity ^0.8.16;

import "../ulysses-omnichain/RootTest.t.sol";

contract AuditTest is RootTest {
    address bob = address(2047);

    MockERC20 auditERC20 = new MockERC20("underlying token", "UNDER", 18);

    function test_audit_noMsgValueReturn() public {
        arbitrumCoreRouter.addLocalToken(address(auditERC20), GasParams(0, 0));

        hevm.deal(bob, 1 ether);
        auditERC20.mint(bob, 1 ether);
        hevm.startPrank(bob);

        DepositInput memory depositInput = DepositInput({
            hToken: rootPort.getLocalTokenFromUnderlying(address(auditERC20), rootChainId),
            token: address(auditERC20),
            amount: 0.5 ether,
            deposit: 0.5 ether
        });

        auditERC20.approve(address(arbitrumPort), 0.5 ether);

        // The msg.value will be lost on RootBridgeAgentExecutor
        arbitrumCoreBridgeAgent.callOutAndBridge{value: 0.1 ether}(payable(bob), "", depositInput, GasParams(0, 0));
    }
}
    ├─ [342819] ArbitrumBranchBridgeAgent::callOutAndBridge{value: 100000000000000000}(0x00000000000000000000000000000000000007ff, 0x, (0xABB32f1fFb9f90E0D914A4cc12cf986aAdB08555, 0xcB5Ea74a0483B47A6e6560D4182C2d54c3C4805a, 500000000000000000 [5e17], 500000000000000000 [5e17]), (0, 0))
    │   ├─ [27238] ArbitrumBranchPort::bridgeOut(0x00000000000000000000000000000000000007ff, ERC20hTokenRoot: [0xABB32f1fFb9f90E0D914A4cc12cf986aAdB08555], MockERC20: [0xcB5Ea74a0483B47A6e6560D4182C2d54c3C4805a], 500000000000000000 [5e17], 500000000000000000 [5e17])
    │   │   ├─ [20494] MockERC20::transferFrom(0x00000000000000000000000000000000000007ff, ArbitrumBranchPort: [0x369Ff55AD83475B07d7FF2F893128A93da9bC79d], 500000000000000000 [5e17])
    │   │   │   ├─ emit Transfer(from: 0x00000000000000000000000000000000000007ff, to: ArbitrumBranchPort: [0x369Ff55AD83475B07d7FF2F893128A93da9bC79d], amount: 500000000000000000 [5e17])
    │   │   │   └─ ← true
    │   │   └─ ← ()
    │   ├─ [55] RootBridgeAgent::receive{value: 100000000000000000}()
    │   │   └─ ← ()
    │   ├─ [116769] RootBridgeAgent::lzReceive(42161 [4.216e4], 0x, 0, 0x0200000003abb32f1ffb9f90e0d914a4cc12cf986aadb08555cb5ea74a0483b47a6e6560d4182c2d54c3c4805a00000000000000000000000000000000000000000000000006f05b59d3b2000000000000000000000000000000000000000000000000000006f05b59d3b20000)
    │   │   ├─ [114717] RootBridgeAgent::lzReceiveNonBlocking(ArbitrumBranchBridgeAgent: [0x7985B5c887210B302b064cF58998BA567D4379fb], 42161 [4.216e4], 0x, 0x0200000003abb32f1ffb9f90e0d914a4cc12cf986aadb08555cb5ea74a0483b47a6e6560d4182c2d54c3c4805a00000000000000000000000000000000000000000000000006f05b59d3b2000000000000000000000000000000000000000000000000000006f05b59d3b20000)
    │   │   │   ├─ [80715] RootBridgeAgentExecutor::executeWithDeposit{value: 100000000000000000}(CoreRootRouter: [0xB79FC86962b787A122c8f563baEc3cB4D50309F1], 0x0200000003abb32f1ffb9f90e0d914a4cc12cf986aadb08555cb5ea74a0483b47a6e6560d4182c2d54c3c4805a00000000000000000000000000000000000000000000000006f05b59d3b2000000000000000000000000000000000000000000000000000006f05b59d3b20000, 42161 [4.216e4])
    │   │   │   │   ├─ [77999] RootBridgeAgent::bridgeIn(CoreRootRouter: [0xB79FC86962b787A122c8f563baEc3cB4D50309F1], (3, 0xABB32f1fFb9f90E0D914A4cc12cf986aAdB08555, 0xcB5Ea74a0483B47A6e6560D4182C2d54c3C4805a, 500000000000000000 [5e17], 500000000000000000 [5e17]), 42161 [4.216e4])
    │   │   │   │   │   ├─ [765] RootPort::isLocalToken(ERC20hTokenRoot: [0xABB32f1fFb9f90E0D914A4cc12cf986aAdB08555], 42161 [4.216e4]) [staticcall]
    │   │   │   │   │   │   └─ ← true
    │   │   │   │   │   ├─ [713] RootPort::getLocalTokenFromUnderlying(MockERC20: [0xcB5Ea74a0483B47A6e6560D4182C2d54c3C4805a], 42161 [4.216e4]) [staticcall]
    │   │   │   │   │   │   └─ ← ERC20hTokenRoot: [0xABB32f1fFb9f90E0D914A4cc12cf986aAdB08555]
    │   │   │   │   │   ├─ [780] RootPort::getGlobalTokenFromLocal(ERC20hTokenRoot: [0xABB32f1fFb9f90E0D914A4cc12cf986aAdB08555], 42161 [4.216e4]) [staticcall]
    │   │   │   │   │   │   └─ ← ERC20hTokenRoot: [0xABB32f1fFb9f90E0D914A4cc12cf986aAdB08555]
    │   │   │   │   │   ├─ [72769] RootPort::bridgeToRoot(CoreRootRouter: [0xB79FC86962b787A122c8f563baEc3cB4D50309F1], ERC20hTokenRoot: [0xABB32f1fFb9f90E0D914A4cc12cf986aAdB08555], 500000000000000000 [5e17], 500000000000000000 [5e17], 42161 [4.216e4])
    │   │   │   │   │   │   ├─ [69189] ERC20hTokenRoot::mint(CoreRootRouter: [0xB79FC86962b787A122c8f563baEc3cB4D50309F1], 500000000000000000 [5e17], 42161 [4.216e4])     
    │   │   │   │   │   │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: CoreRootRouter: [0xB79FC86962b787A122c8f563baEc3cB4D50309F1], amount: 500000000000000000 [5e17])
    │   │   │   │   │   │   │   └─ ← true
    │   │   │   │   │   │   └─ ← ()
    │   │   │   │   │   └─ ← ()
    │   │   │   │   └─ ← ()
    │   │   │   ├─ emit LogExecute(depositNonce: 3, srcChainId: 42161 [4.216e4])
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   └─ ← ()
    └─ ← ()

Tools Used

Manual review, Forge

Recommended Mitigation Steps

Pass the msg.value back to the gas recipient (msg.sender in most cases). It's better to copy behavior of LayerZero to not make special exceptions in the execution of calls on Arbitrum.

Assessed type

ETH-Transfer

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

0xBugsy commented 1 year ago

What is being described is incorrect, the msg.value is passed in order to be spent by the attached router being called, this can be used, for example to start Layer Zero cross-chain messages.

If this were to revert this revert would be triggered and a refund would take place: https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/RootBridgeAgent.sol#L430

As a result the issue being described does not occur.

c4-sponsor commented 1 year ago

0xBugsy (sponsor) disputed

ustas-eth commented 1 year ago

What is being described is incorrect, the msg.value is passed in order to be spent by the attached router being called, this can be used, for example to start Layer Zero cross-chain messages.

If this were to revert this revert would be triggered and a refund would take place: https://github.com/code-423n4/2023-09-maia/blob/ffbe532c6f5224a55ce099b4016bd8806bdbc913/src/RootBridgeAgent.sol#L430

As a result the issue being described does not occur.

Try to add console.logs at the end of the test suit to see the balances after the call:

console2.log("Bob balance", bob.balance); // 0.9 ETH
console2.log("Executor balance", coreBridgeAgent.bridgeAgentExecutorAddress().balance); // 0.1 ETH

The funds sent to arbitrumCoreBridgeAgent by Bob are now located on RootBridgeAgentExecutor without being refunded.

0xBugsy commented 1 year ago

I see what is happening now, you are sending 0 calldata.

This is a duplicate of #685

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

alcueca marked the issue as grade-a