Maia DAO - Ulysses - rvierdiiev'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 called by anyone, which means that all funds can be stolen.

Proof of Concept

When user bridges tokens from any chain, then they are bridged to the user's virtual account. So eventually funds are inside this wallet.

VirtualAccount is going to be permissioned, so it can be only called by owner or approved routers.

The problem is that VirtualAccount.payableCall function doesn't use that requiresApprovedCaller modifier. As result anyone can call function and steal everything that is inside.

Tools Used

VsCode

Recommended Mitigation Steps

Use same requiresApprovedCaller modifier to restrict calling.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L940-L946

Vulnerability details

Impact

RootBridgeAgent._performFallbackCall will not work for arbitrum branch bridge

Proof of Concept

In case if user would like to receive result about failing tx on source chain, then he provides _hasFallbackToggled param. In that case, RootBridgeAgent will send falback message back and mark deposit as STATUS_RETRIEVE. STATUS_RETRIEVE means that it will be not possible to retry this deposit on root chain. In order to send fallback _performFallbackCall function is used. And this function will just call LZ.send to the destination.

This will mark deposit as failed on initial chain. Once it's marked as failed, then user can redeem deposit. So user will likely use fallback, when he wants to try do some actions and be able to redeem once it's failed.

ArbitrumBranchBridgeAgent and RootBridgeAgent are both on same chain. That's why they don't speak through the LZ. But in case RootBridgeAgent._performFallbackCall doesn't check, that caller is from same chain and uses LZ to send fallback. As result, function will always fail, as it will request LZ to relay to current chain. Because of that users on arbitrum branch will not have ability to use fallback functionality.

Tools Used

VsCode

Recommended Mitigation Steps

In case if destination chain is same as local, then call arbitrum bridge agent directly.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L423-L431

Vulnerability details

Impact

Airdropped amount is not refunded to the user in case if call has reverted

Proof of Concept

When user initiates deposit for example, then he provides gas params, which are used by LZ. Version 2 is used here, because it's needed to top up root bridge agent with native token, so it can initiate another LZ call back. So in case if user would like to execute some action on root chain and then bridge out(create settlement), then user will airdrop root bridge agent thorugh the LZ.

Now, let's check how LZ txs are executed on RootBridgeAgent. RootBridgeAgent.lzReceive will always succeed in case if it's not called from same chain(not through LZ). This is done to not allow user to replay LZ tx, once it was tried to be executed.

RootBridgeAgent.lzReceive function calls lzReceiveNonBlocking function. And this function calls _execute function. Now we have 2 version of this function. One will revert in case if call didn't succeed and another will send fallback, if requested by user.

_performFallbackCall function will send all funds to LZ, which means that user will receive refund, but all other calls, when fallback was not requested will not send refunds to the user. This funds will be left inside contract after the call.

Example. 1.User initiates deposit and also would like to do some swapping on root chain, so he would like to withdraw other token at the end. 2.User provides LZ gas params to top up root bridge agent, so it can then send settlement. So user pays for it. 3.Swap on some exchange platform has failed, because of slippage, so call reverted. 4.Airdropped amount was not refund to user and left in the root bridge agent. Someone else used it later. 5.User is able to retry deposit, but he should to provide new amount to LZ to airdrop root bridge agent again.

Tools Used

VsCode

Recommended Mitigation Steps

Make payload for every function have encoded refundee address(as first param maybe) and then decode it and send all left funds to the refundee if call has failed. This can be done inside execute function. So in case if it's call without fallback or fallback was not requested and call has failed, then refund all contract's balance.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L1212-L1214

Vulnerability details

Impact

Root bridge agent will not be able to communicate with branch bridge agents. User can lose funds.

Proof of Concept

Bridges communicate with each other using LZ messages. requiresEndpoint modifier is used to check if call is authorized.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L1204-L1217

    modifier requiresEndpoint(address _endpoint, uint16 _srcChain, bytes calldata _srcAddress) virtual {
        if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();


        if (_endpoint != getBranchBridgeAgent[localChainId]) {
            if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();


            if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();


            if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
                revert LayerZeroUnauthorizedCaller();
            }
        }
        _;
    }

In case if _endpoint is bridge agent from arbitrum, then no more checks are done. Otherwise, modifier checks that its indeed LZ called function and then it checks, that caller from source chain is allowed bridge agent for that chain.

In order to get source chain caller, function does _srcAddress[PARAMS_ADDRESS_SIZE:], which will take last 20 bytes of _srcAddress. This is incorrect and in reality, function fetched destination instead of caller, which will be address(this) in context of modifier. When LZ packs _srcAddress, then first is source chain caller and 2 is destination. When protocol imitates LZ in tests, then they pack destination first. Because of that their tests work.

I believe that the reason why protocol fetched source address in such way is this LZ example, where they use 20 bytes offset.

But the difference here is that example uses assembly and with this 20 bytes offset they just want to skip length of bytes array. But protocol uses calldata array and they don't should handle length.

This simple functions can be run in remix. Using encode function you can get bytes array. And 2 other functions will fetch source address using LZ example and protocol example. You will see, that assembly function will return first address, while calldata version will return second address.

function testCalldata(bytes calldata path) public view returns(address ) {
        return address(uint160(bytes20(path[20:])));
    }

    function testMemory(bytes memory path) public view returns(address result) {
        assembly {
            result := mload(add(path, 20))
        }
    }

    function encode() public view returns(bytes memory result) {
        address a = address(1);
        address b = address(2);
        bytes memory scrBytes = abi.encodePacked(a);
        bytes memory path = abi.encodePacked(scrBytes, b);
        return path;
    }

Impact of this is severe. Actually protocol will not work. All communication between root bridge and branch bridges will not work, except Arbitrum branch.

Also, it's possible that new chain will be added with already deployed tokens on it. So user who will bridge those tokens, will lose deposit, as message will not be executable on root bridge agent.

Tools Used

VsCode

Recommended Mitigation Steps

In order to get source address use _srcAddress[:PARAMS_ADDRESS_SIZE]

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/ArbitrumCoreBranchRouter.sol#L168-L170 https://github.com/code-423n4/2023-09-maia/blob/main/src/CoreBranchRouter.sol#L140-L146

Vulnerability details

Impact

ArbitrumCoreBranchRouter.executeNoSettlement can't handle setCoreBranchRouter function.

Proof of Concept

ArbitrumCoreBranchRouter extends CoreBranchRouter and overrides its executeNoSettlement function. CoreBranchRouter.executeNoSettlement can handle 0x07 function, but ArbitrumCoreBranchRouter.executeNoSettlement can't.

I believe this is because, previously CoreBranchRouter.executeNoSettlement also didn't handle 0x07 function and when support for it was added, then ArbitrumCoreBranchRouter was not updated with new function.

Tools Used

VsCode

Recommended Mitigation Steps

Add support of 0x07 function.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

RootPort.setCoreBranchRouter doesn't have enough validation. As result new bridge can be set, that will have access to the funds, but will not be able to communicate with RootBridgeAgent.

Proof of Concept

When new bridge is added to the chain, then this bridge is marked as known inside RootBridgeAgent. As result, only 1 known bridge per chain can call messages to the RootBridgeAgent.

RootPort.setCoreBranchRouter can be called by owner in order to add router and bridge to the branch chain.

This function does nothing on Root side, but adds router and bridge on branch side. As result new router is added and new bridge is added. And this bridge now has access to the port funds, which means that users can start brigding with it.

The problem is that in case if this bridge was not inside RootBridgeAgent.getBranchBridgeAgent[chain], then messages from this bridge will not be accepted, so they will just revert and noone will be able to replay them.

While this function is fully controlled by owner, incorrect usage of it will make big problems, so it's better to add additional check.

Tools Used

VsCode

Recommended Mitigation Steps

I believe that RootPort.setCoreBranchRouter function should check that provided bridge is inside RootBridgeAgent.getBranchBridgeAgent[chain]. Only then it should allow to proceed.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L681-L695 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L938-L948

Vulnerability details

Impact

User always overpay for fallback call

Proof of Concept

In case if user would like to receive fallback call for it deposit, then he can provide _hasFallbackToggled param as true. This means that in case if this deposit will fail in root bridge agent, then fallback will be called to the original chain.

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L938-L948

    function _performFallbackCall(address payable _refundee, uint32 _depositNonce, uint16 _dstChainId) internal {
        //Sends message to LayerZero messaging layer
        ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}(
            _dstChainId,
            getBranchBridgeAgentPath[_dstChainId],
            abi.encodePacked(bytes1(0x04), _depositNonce),
            payable(_refundee),
            address(0),
            ""
        );
    }

As you can see _adapterParams of layer zero send function is provided as empty, which means that default gas amount will be used for such call. Currently it's 200_000 gas.

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

Abstract: Every transaction costs a certain amount of gas. Since LayerZero delivers the destination transaction when a message is sent it must pay for that destination gas. A default of 200,000 gas is priced into the call for simplicity.

But if you will look how fallback is handled on BranchBridgeAgent then you will see, that it will never use such big amount of gas.

Put this into BranchBridgeAgentTest.

function testFallbackGasAmount() public {
        // Encode Fallback message
        bytes memory fallbackData = abi.encodePacked(bytes1(0x04), uint32(1));

        // Call 'Fallback'
        vm.prank(lzEndpointAddress);
        uint256 gasStart = gasleft();
        bAgent.lzReceive(rootChainId, abi.encodePacked(bAgent, rootBridgeAgentAddress), 1, fallbackData);
        console2.log("gas used: ", gasStart - gasleft());
    }

[PASS] testFallbackGasAmount() (gas: 44011)
Logs:
  gas used:  35288

As you can see user overpays more than 150_000 of gas, which can be not cheap as branch can be ethereum.

Tools Used

VsCode

Recommended Mitigation Steps

You can calculate approx gas amount to call fallback(for example 50_000) and set it as variable in the root brodge agent that can be changed(so each fallback call will use that amount of gas). Or you can allow users to provide gas amount for fallback, which is worse as they will also need to pay for this info to be relayed to root chain.

Assessed type

Error