VirtualAccount's payable function can be callable by anyone

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

VirtualAccounts are created for user specific and it holds user's financial assets. VirtualAccount contract has call and payableCall functions purpose is make this function like a EOA. So this is important that there should be important put access control for this call functions. If there is not, any malicious user can use this call function to steal asset's of virtualAccount's user.
As can be seen call function has access control which can be seen as requiresApprovedCaller modifier.

    function call(Call[] calldata calls) external override requiresApprovedCaller returns (bytes[] memory returnData) {

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

However there is no such that modifier in payableCall or any other access control so anybody can call payableCall .

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

It can be seen that there is no access control for msg.sender. Due to this lack,for example, a malicious user can give parameter as target USDT and calldata as transfer(addressOfMalUser,BalanceOfVirtualAccount). So by this way malicious user can steal users assets without revert.

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

Invalid Validation

[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