Maia DAO - Ulysses - minhtrng's results

Lines of code

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

Vulnerability details

Impact

The Virtual Account lacks access control on a function that allows arbitrary calls. This enables anyone to take any assets contained within the account.

Proof of Concept

The Virtual account has the requiresApprovedCaller modifier to prevent use from non-approved actors:

    modifier requiresApprovedCaller() {
        if (!IRootPort(localPortAddress).isRouterApproved(this, msg.sender)) {
            if (msg.sender != userAddress) {
                revert UnauthorizedCaller();
            }
        }
        _;
    }

However, the payableCall function is missing that 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();
    }

Since this function performs arbitrary calls with arbitrary msg.values, this allows anyone to take all assets from the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the requiresApprovedCaller modifier to the payableCall function (just like all other critical functions have it)

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The protocol uses LayerZeros Airdrop mechanism to send gas to BridgeAgents which they need to pay for subsequential cross-chain-messages. If the transaction on the receiver fails, this airdropped gas will remain in the BridgeAgent and can be used up by the next caller.

Proof of Concept

The use of the Airdrop mechanism can be seen for example in BranchBridgeAgent._performCall:

    function _performCall(address payable _refundee, bytes memory _payload, GasParams calldata _gParams)
        internal
        virtual
    {
        ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
            ...
            abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)
        );
    }

An example that shows an attempt of using the airdropped gas to perform a follow up call is in BranchBridgeAgent._performFallBackCall:

ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}(...

This call happens after the execution of another call has failed, for example _executeWithSettlement:

function executeWithSettlement(address _recipient, address _router, bytes calldata _payload)
	external
	payable
	onlyOwner
{
	...
	} else {
		_recipient.safeTransferETH(address(this).balance);
	}
}

As can be seen, the function attempts to send back the remaining native gas to the recipient. If the execution fails, the fallback is triggered. However the fallback too can fail, if the gas balance of the bridge agent is not enough to cover the relayer costs (looking at LayerZero code -> UltraLightNodeV2.send gets called by the Endpoint). In this case, the airdropped gas is not send anywhere and can be used up by the next caller.

Tools Used

Manual Review

Recommended Mitigation Steps

At lzReceive (which can never fail due to a low level call, with additional safety), the contract balance could be sent to a refundee or recipient in case of failure:

    function lzReceive(uint16, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
        address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
        );

        //check if !success and transfer address(this).balance to the recipient (has to be determined from payload)
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When encoding a payload for settlement of multiple tokens, the fallback flag is not set when it should be. This will cause no fallback to be triggered even though the user has paid enough to cover the additional costs that are required.

Proof of Concept

In RootBridgeAgent._createSettlementMultiple the function ID which contains the fallback flag as first bit is encoded like this:

_hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),

This bitwise AND of 0x02 with 0x0F will yield 0x02 (same as the no fallback case). Hence no fallback will be triggered in case of failure on the branch chain:

function lzReceiveNonBlocking(address _endpoint, bytes calldata _srcAddress, bytes calldata _payload)
    public
    override
    requiresEndpoint(_endpoint, _srcAddress)
{
    //Save Action Flag
    bytes1 flag = _payload[0] & 0x7F; //remove leftmost bit
    ...
    } else if (flag == 0x02) {
    ...
    _execute(
        _payload[0] == 0x82, //hasFallback
    ...

Minimal PoC with foundry to showcase 0x02 & 0x0F == 0x02:

pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract BitOperationsTest is Test {

    function testBitwiseAnd() public{
        assert(bytes1(0x02) & 0x0F == 0x02);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Looking at how RootBridgeAgent._createSettlement determines the first byte:

_hasFallbackToggled ? bytes1(0x81) : bytes1(0x01),

This should be done analoguously for _createSettlementMultiple

_hasFallbackToggled ? bytes1(0x82) : bytes1(0x02),

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The source address of LayerZero messages is validated on a wrong part of the calldata, which will cause all cross-chain-messages to fail on a live deployment.

Proof of Concept

The receivers of cross-chain-messages BranchBridgeAgent and RootBridgeAgent both perform validation of the sender in their respective requiresEndpoint modifiers:

// RootBridgeAgent
if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
    revert LayerZeroUnauthorizedCaller();
}

//BranchBridgeAgent
if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();

Both try to extract the sender address from the last 20 bytes of the _srcAddress (which consists of 2 EVM addresses, so 40 bytes in total). The issue is that the address of the sender on the source chain is contained in the first 20 bytes not the last.

During development the assumption was made that whatever path gets send, will also be the path when receiving. Looking at BranchBridgeAgent._performCall, the following parameter is passed:

ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
    ...
    rootBridgeAgentPath,
    ...
);

Here, rootBridgeAgentPath is the 40 bytes combination of remoteAddress and localAddress mentioned above:

rootBridgeAgentPath = abi.encodePacked(_rootBridgeAgentAddress, address(this));

It is easy to think that since address(this) occupies the last 20 bytes here, that the path given to the receiver will also be in the same order.

However, LayerZero actually swaps the addresses as the path is always semantically meant to be comprised of remoteAddress first and then localAddress, relative to the current chain (see docs: Sending on L0 and Trusted remotes on L0 + Github example)

Proving on a real on-chain transaction that this is what happens:

cross-chain tx on layerzeroscan

This link shows a cross-chain message from Arbitrum to Avalanche. Looking at the decoded transaction traces (using Tenderly, links below), we can see that the path used in lzSend is 0x9d1b1669c73b033dfe47ae5a0164ab96df25b944352d8275aae3e0c2404d9f68f6cee084b5beb3dd while the path received in lzReceive is 0x352d8275aae3e0c2404d9f68f6cee084b5beb3dd9d1b1669c73b033dfe47ae5a0164ab96df25b944 (first and last 20 bytes swapped)

lzSend on Arbitrum

lzReceive on Avalanche

The reason why this has not been caught in the unit tests is because unit tests can't integrate with LayerZero, so the encoding has been done manually, but in the wrong order, as can be seen in CoreRootBridgeAgentTest.t.sol:

function encodeSystemCall(
    address payable _fromBridgeAgent,
    address payable _toBridgeAgent,
    ...
) private {
    ...
    // since we are on the target chain already, _from is the remote and should be packed first
    RootBridgeAgent(_toBridgeAgent).lzReceive{gas: _gasParams.gasLimit}(
            _srcChainIdId, abi.encodePacked(_toBridgeAgent, _fromBridgeAgent), 1, inputCalldata
        );
}

To get an executable POC, swap _toBridgeAgent and _fromBridgeAgent when encoding and run any test that uses encodeSystemCall. The tests will fail, works the same with the other encode* functions. (Executable Github Gist for reference)

Tools Used

Manual Review, Tenderly, Layerzeroscan

Recommended Mitigation Steps

In the requiresEndpoint modifiers, get the sender from the first 20 bytes, not the last:

// RootBridgeAgent
if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[0:PARAMS_ADDRESS_SIZE])))) {
    revert LayerZeroUnauthorizedCaller();
}

//BranchBridgeAgent
if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[0:20])))) revert LayerZeroUnauthorizedCaller();

Also adjust the encode* functions in the tests to reflect that.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L100-L105

Vulnerability details

Impact

The callOutAndBridge flow causes funds to be locked if no additional payload is send.

Proof of Concept

In RootBridgeAgentExecutor.executeWithDeposit the function ends if the payload is not longer than 128 bytes:

//router is recipient of funds
_bridgeIn(_router, dParams, _srcChainId);

if (_payload.length > PARAMS_TKN_SET_SIZE) {
    //Execute remote request
    IRouter(_router).executeDepositSingle{value: msg.value}(
        _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
    );
}
//END

At this point however, the funds that were bridged in were send to the router, where they remain.

Tools Used

Manual review

Recommended Mitigation Steps

Revert, if the router is not called (payload.length <= PARAMS_TKN_SET_SIZE)

Assessed type

Other