Virtual account lacks access control

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The Virtual Account lacks access control on a function that allows arbitrary calls. This enables anyone to take any assets contained within the account.

Proof of Concept

The Virtual account has the requiresApprovedCaller modifier to prevent use from non-approved actors:

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

However, the payableCall function is missing that modifier:

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

        // Finally, make sure the msg.value = SUM(call[0...i].value)
        if (msg.value != valAccumulator) revert CallFailed();
    }

Since this function performs arbitrary calls with arbitrary msg.values, this allows anyone to take all assets from the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the requiresApprovedCaller modifier to the payableCall function (just like all other critical functions have it)

Assessed type

Other

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-sponsor]

0xBugsy (sponsor) confirmed

[alcueca]

I also found this one, for the record. My reward will be distributed among the wardens.

[c4-judge]

alcueca marked issue #885 as primary and marked this issue as a duplicate of 885

[c4-judge]

alcueca marked the issue as satisfactory