When using BaseBranchRouter as a router on the 'Arbitrum' branch, we are unable to invoke the 'callOutAndBridge' function.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L95-L100 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L167-L168 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L224 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L824 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L134

Vulnerability details

Impact

All branches have one core router and several subsidiary branches. The core branches handle system calls, and users can create deposits using subsidiary branches. The function utilized for creating deposits is the callOutAndBridge function. However, in the Arbitrum branch, this function is not available for use. This restriction has system-wide implications because the Arbitrum branch should function similarly to other local branches.

Proof of Concept

When a user attempts to make a deposit in other branches, the local hTokens are transferred from the user to the local branch port and subsequently burned. However, in Arbitrum, there is no differentiation between local and global hTokens. Consequently, they are sent directly to the root port. When a user invokes the callOutAndBridge function in a branch, it involves the transfer of hTokens and underlying tokens from the user to the router. Additionally, the router approves the local port to access and utilize these tokens. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L95

_transferAndApproveToken(_dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L167-L168

_hToken.safeTransferFrom(msg.sender, address(this), _amount - _deposit);
ERC20(_hToken).approve(_localPortAddress, _amount - _deposit);

_token.safeTransferFrom(msg.sender, address(this), _deposit);
ERC20(_token).approve(_localPortAddress, _deposit);

Then we call the 'callOutAndBridge' function within the bridge agent. Within this function, a deposit is created. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L224

_createDeposit(_depositNonce, _refundee, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L824

IPort(localPortAddress).bridgeOut(msg.sender, _hToken, _token, _amount, _deposit);

We've now reached the _bridgeOut function in ArbitrumBranchPort. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L134

IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);

Here, the _depositor is the router, and ArbitrumBranchPort is authorized to access these tokens, but not RootPort. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L301

_hToken.safeTransferFrom(_from, address(this), _amount);

Consequently, we are unable to bridge these tokens to the RootPort from _depositor.

Add test below to test/ulysses-omnichain/ArbitrumBranchTest.t.sol and run test. You could see that the test will be failed.

    function testCallOutAndBridge() public {
        testAddLocalTokenArbitrum();
        address _user = address(this);
        arbitrumNativeToken.mint(_user, 150 ether);
        // Approve Tokens
        arbitrumNativeToken.approve(address(localPortAddress), 50 ether);
        // Call deposit to port
        arbitrumMulticallBridgeAgent.depositToPort(
            address(arbitrumNativeToken),
            50 ether
        );

        require(
            MockERC20(arbitrumNativeToken).balanceOf(_user) == 100 ether,
            "_user has 100 underlying tokens"
        );
        require(
            MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_user) ==
                50 ether,
            "_user has 50 global(local) tokens"
        );

        arbitrumNativeToken.approve(
            address(arbitrumMulticallRouter),
            100 ether
        );
        ERC20hTokenRoot(newArbitrumAssetGlobalAddress).approve(
            address(arbitrumMulticallRouter),
            50 ether
        );

        DepositInput memory depositInput = DepositInput({
            hToken: address(newArbitrumAssetGlobalAddress),
            token: address(arbitrumNativeToken),
            amount: 150 ether,
            deposit: 100 ether
        });
        GasParams memory gasParams = GasParams(0.5 ether, 0.5 ether);
        // Get some gas.
        hevm.deal(address(this), 1 ether);

        // test will be failed
        arbitrumMulticallRouter.callOutAndBridge{value: 1 ether}(
            "",
            depositInput,
            gasParams
        );
    }

Tools Used

Recommended Mitigation Steps

Change this

IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);

with

_localAddress.safeTransferFrom(_depositor, address(this), _amount - _deposit);
ERC20(_localAddress).approve(rootPortAddress, _amount - _deposit);
IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(address(this), _localAddress, _amount - _deposit);

You can observe that the test will pass successfully. It's important to note that when users attempt to use the callOutAndBridge function within the bridge agent, they allow the local port to access their funds, just as in other branches.

Assessed type

Token-Transfer

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[0xA5DF]

Leaving open for sponsor to comment

[c4-sponsor]

0xLightt (sponsor) confirmed

[c4-sponsor]

0xBugsy marked the issue as disagree with severity

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[alcueca]

No funds at risk. Medium at best, since there is a workaround (since this is also a router contract, users can always interact directly with the bridgeAgent and approve funds to be spent by the correct Port themselves (approve rootPort instead of ArbitrumBranchPort)

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[alcueca]

From the sponsor:

we believe it is accurate. we need to increase the allowance/approve to the root port in arbitrum base branch routers.

[0xBugsy]

Addressed at https://github.com/Maia-DAO/2023-09-maia-remediations/commit/cbdd6e25ce9c4190282eb6ebf8f3c1425d1236aa