Maia DAO - Ulysses - Black_Box_DD's results

Lines of code

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

Vulnerability details

Impact

The Virtual Account holds user balances and users for operating and interacting with Dapps on Arbitrum chain. Since it acts as a Wallet it holds assets. The payableCall() function acts similar as call() function, the Issue here it's missing the requiresApprovedCaller Modifier.

Thus, anyone can call payableCall() function and execute anything without approved by the User who owns that Virtual Account.

Proof of Concept

1. The user creates a Virtual Account. Assume that the user holds 10000 USDC tokens on a Virtual account contract.
2. Then a malicious user calls payableCall() function will following Values:

_call.target = USDC Token Address
_call.callData = approve(attacker_address, 10000)
_call.value = 0

3. Now the malicious user has to be approved 10000 USDC and able to Steal from the User's Virtual Account contract.

Vulnerable Snippet

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

Tools Used

Manual Review

Recommended Mitigation Steps

Add requiresApprovedCaller modifier to payableCall() function

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

Assessed type

Access Control

No duplicate check while adding tokens to the strategyTokens

Permalink: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L362

In the BranchPort.sol, the function addStrategyToken() lacks duplicate check while adding token into strategyTokens Array

Duplicate token Addresses can pushed multiple times to the strategyTokens Array.

Recommendation:

Add Duplicate check if token Address already added to the Array.

 if(isStrategyToken[_token]) revert ("AlreadyAddedStrategyToken");

    function addStrategyToken(address _token, uint256 _minimumReservesRatio) external override requiresCoreRouter {
        if (_minimumReservesRatio >= DIVISIONER || _minimumReservesRatio < MIN_RESERVE_RATIO) {
            revert InvalidMinimumReservesRatio();
        }

    +   if(isStrategyToken[_token]) revert AlreadyAddedStrategyToken();
        strategyTokens.push(_token);
        getMinimumTokenReserveRatio[_token] = _minimumReservesRatio;
        isStrategyToken[_token] = true;

        emit StrategyTokenAdded(_token, _minimumReservesRatio);
    }