Maia DAO - Ulysses - Tendency's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchBridgeAgent.sol#L112-L118

Vulnerability details

Impact

When a user calls RootBridgeAgent::retrySettlement function, to retry a failed settlement call to a root chain branch bridge agent(ArbitrumBranchBridgeAgent contract), msg.value(sent in native token) is passed as parameter in the internal call to _performRetrySettlementCall function.

    function retrySettlement(
        uint32 _settlementNonce,
        address _recipient,
        bytes calldata _params,
        GasParams calldata _gParams,
        bool _hasFallbackToggled
    ) external payable override lock {
                       
                       //SNIP

        // Perform Settlement Retry
        _performRetrySettlementCall(
            _hasFallbackToggled,
            settlementReference.hTokens,
            settlementReference.tokens,
            settlementReference.amounts,
            settlementReference.deposits,
            _params,
            _settlementNonce,
            payable(settlementReference.owner),
            _recipient,
            settlementReference.dstChainId,
            _gParams,
            msg.value // <-- HERE
        );
    }

In _performRetrySettlementCall function, native gas tokens are sent to the local branch bridge agent(Arbitrum branch bridge agent contract):

    function _performRetrySettlementCall(
        bool _hasFallbackToggled, 
        address[] memory _hTokens,
        address[] memory _tokens,
        uint256[] memory _amounts,
        uint256[] memory _deposits,
        bytes memory _params,
        uint32 _settlementNonce,
        address payable _refundee,
        address _recipient,
        uint16 _dstChainId,
        GasParams memory _gParams,
        uint256 _value
    ) internal {
        
                // SNIP

            // Check if call to local chain
        } else {
            //Send Gas to Local Branch Bridge Agent
            callee.call{value: _value}("");
            //Execute locally
            IBranchBridgeAgent(callee).lzReceive(0, "", 0, payload);
        }
    }

Assuming the execution fails at a point in ArbitrumBranchBridgeAgent, where fallback is toggled to true, ArbitrumBranchBridgeAgent::_performFallbackCall function is called. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L747

The problem here is, on fallback, _performFallbackCall function, does not refund the excess/remaining native gas deposit to the user:

    function _performFallbackCall(address payable, uint32 _settlementNonce) internal override {
        //Sends message to Root Bridge Agent
        IRootBridgeAgent(rootBridgeAgentAddress).lzReceive( 
            rootChainId, "", 0, abi.encodePacked(bytes1(0x09), _settlementNonce)
        );
    }

It's important to note that in other branch bridge agents, users do receive a refund of their excess native gas deposit from Layer Zero when a similar situation occurs. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L785-L795

Proof of Concept

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchBridgeAgent.sol#L112-L118

Tools Used

Manual Review

Recommended Mitigation Steps

Consider refunding users back their native gas tokens when performing a fallback call due to a prior failed execution in ArbitrumBranchBridgeAgent:

    function _performFallbackCall(address payable refundee, uint32 _settlementNonce) internal override {
        //perform a refund
  ++    require(refundee.call{value: address(this).balance}(""), "Refund failed");     
        IRootBridgeAgent(rootBridgeAgentAddress).lzReceive( 
            rootChainId, "", 0, abi.encodePacked(bytes1(0x09), _settlementNonce)
        );
    }

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

Impact

Current implementation assumes users will input the right gas param when they intend to make a cross chain call, which wouldn't always be the case. Through out the contract implementation, calls from branch bridge agents to Root bridge agent and back passes user Gas params to layer zero without any verification.

A malicious user can take advantage of this vulnerability to DoS the protocol, by making calls in BranchBridgeAgents that delivers to RootBridgeAgent with very little gas amount that guarantees a Out-Of-Gas(OOG) revert in RootBridgeAgent::lzReceive function, blocking communication between BranchBridgeAgents and RootBridgeAgents

Using BranchBridgeAgent::callOutSignedAndBridge function to illustrate this issue, here is the function call route: BranchBridgeAgent::callOutSignedAndBridge -->ILayerZeroEndpoint::send --> Relayer --> UltraLightNodeV2::validateTransactionProof --> ILayerZeroEndpoint::receivePayload --> RootBridgeAgent::lzReceive

ILayerZeroEndpoint::receivePayload function

    function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external override receiveNonReentrant {
                        //SNIP
                        
        // block if any message blocking
        StoredPayload storage sp = storedPayload[_srcChainId][_srcAddress];
        require(sp.payloadHash == bytes32(0), "LayerZero: in message blocking");

        try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
            // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
            storedPayload[_srcChainId][_srcAddress] = StoredPayload(uint64(_payload.length), _dstAddress, keccak256(_payload));
            emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
        }
    }

The user passed in gas limit at the source chain BranchBridgeAgent, from the gas param(GasParams.gasLimit) is used as the gas required for the computation at the destination chain(Root bridge agent contract). The problem here is, if the call fails at the destination chain(Root bridge agent contract) lzReceive function, LayerZero switches to blocking mode, thereby blocking all calls from the source chain Branch agent to the Root chain agent. This attack could have been prevented if sufficient gas amount is provided to ensure the call delivers to RootBridgeAgent::lzReceiveNonBlocking function, since the revert happens at lzReceive function, layer Zero catches the error and then stores the payload.

        // block if any message blocking
        StoredPayload storage sp = storedPayload[_srcChainId][_srcAddress];
        require(sp.payloadHash == bytes32(0), "LayerZero: in message blocking");

The attacker can repeat this for all supported branch bridge agents, subsequently rendering the protocol unusable.

ATTACK PATH

The attacker calls BranchBridgeAgent::callOutSignedAndBridge with a gas limit of 1 GasParams(1, 1) The Relayer delivers the transaction with the specified gas at the destination chain. The transaction gets verified at LayerZero endpoint contracts then forwarded to RootBridgeAgent::lzReceive function. Note: Layer zero calls RootBridgeAgent::lzReceive with the exact gas input from GasParams(1 gas).

When RootBridgeAgent::lzReceive function is called with only 1 gas, down the execution, the call reverts due to out-of-gas error, this results in layer zero catching and then storing the payload, thereby blocking the communication channel between the source branch bridge agent and Root bridge agent.

Proof of Concept

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

https://vscode.blockscan.com/arbitrum-one/0x3c2269811836af69497e5f486a85d7316753cf62

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a check in the contract that ensures users input Gas Params are within a certain threshold. The set threshold should be enough to ensure the gas limit always gets the call to lzReceiveNonBlocking Rigorous testing should be done before using this value, the max possible payload size should be considered.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L936-L944 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1212 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L423 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L434

Vulnerability details

Impact

Before a message is delivered in a destination chain, Layer Zero UltraLightNodeV2::validateTransactionProof function is called, towards the end of this function, we can observe that pathData is an encodePacked of the packet source address(from source chain) and destination address(In delivery/destination chain), LayerZero EndPoint::receivePayload function is then called afterwards.

    function validateTransactionProof(uint16 _srcChainId, address _dstAddress, uint _gasLimit, bytes32 _lookupHash, bytes32 _blockData, bytes calldata _transactionProof) external override {
        // retrieve UA's configuration using the _dstAddress from arguments.
        ApplicationConfiguration memory uaConfig = _getAppConfig(_srcChainId, _dstAddress);

        // assert that the caller == UA's relayer
        require(uaConfig.relayer == msg.sender, "LayerZero: invalid relayer");

        LayerZeroPacket.Packet memory _packet;
        uint remoteAddressSize = chainAddressSizeMap[_srcChainId];
        require(remoteAddressSize != 0, "LayerZero: incorrect remote address size");
        {
            // assert that the data submitted by UA's oracle have no fewer confirmations than UA's configuration
            uint storedConfirmations = hashLookup[uaConfig.oracle][_srcChainId][_lookupHash][_blockData];
            require(storedConfirmations > 0 && storedConfirmations >= uaConfig.inboundBlockConfirmations, "LayerZero: not enough block confirmations");

            // decode
            address inboundProofLib = inboundProofLibrary[_srcChainId][uaConfig.inboundProofLibraryVersion];
            _packet = ILayerZeroValidationLibrary(inboundProofLib).validateProof(_blockData, _transactionProof, remoteAddressSize);
        }

        // packet content assertion
        require(ulnLookup[_srcChainId] == _packet.ulnAddress && _packet.ulnAddress != bytes32(0), "LayerZero: invalid _packet.ulnAddress");
        require(_packet.srcChainId == _srcChainId, "LayerZero: invalid srcChain Id");
        // failsafe because the remoteAddress size being passed into validateProof trims the address this should not hit
        require(_packet.srcAddress.length == remoteAddressSize, "LayerZero: invalid srcAddress size");
        require(_packet.dstChainId == localChainId, "LayerZero: invalid dstChain Id");
        require(_packet.dstAddress == _dstAddress, "LayerZero: invalid dstAddress");

        // if the dst is not a contract, then emit and return early. This will break inbound nonces, but this particular
        // path is already broken and wont ever be able to deliver anyways
        if (!_isContract(_dstAddress)) {
            emit InvalidDst(_packet.srcChainId, _packet.srcAddress, _packet.dstAddress, _packet.nonce, keccak256(_packet.payload));
            return;
        }

        bytes memory pathData = abi.encodePacked(_packet.srcAddress, _packet.dstAddress);
        emit PacketReceived(_packet.srcChainId, _packet.srcAddress, _packet.dstAddress, _packet.nonce, keccak256(_packet.payload));
        endpoint.receivePayload(_srcChainId, pathData, _dstAddress, _packet.nonce, _gasLimit, _packet.payload);
    }

In receivePayload function, the encodePacked of the source and destination address is then sent along other parameters to the destination address lzReceive function.

    function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external override receiveNonReentrant {
  
                           // SNIP
                           
        try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
            // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
            storedPayload[_srcChainId][_srcAddress] = StoredPayload(uint64(_payload.length), _dstAddress, keccak256(_payload));
            emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
        }
    }

Assuming the call is coming from the RootBridgeAgent contract to a BranchBridge Agent, where: source chain address = RootBridgeAgent address Destination chain address = BranchBridgeAgent address

BranchBridgeAgent::lzReceive function, further makes an excessivelySafeCall to BranchBridgeAgent::lzReceiveNonBlocking

    function lzReceive(uint16, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
        address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
        );
    }

In lzReceiveNonBlocking function, the modifier requiresEndpoint, is meant to ensure the only caller to this function is the contract itself, the function also checks if the passed in endpoint equals the stored endpoint address. The function also ensures the passed in _srcAddress(the encoded path data) equals 40, which makes sense since 20 bytes + 20 bytes gives 40 bytes. The only problem here is, the function attempts to get the source Address by making a slice from the 20th byte forward, which isn't right. Remember: source chain address = RootBridgeAgent address Destination chain address = BranchBridgeAgent address encodePacked(RootBridgeAgent address, BranchBridgeAgent address). EncodePacked will store the bytes of the first, then just at the very end, pad the next. Therefore, the slice address(uint160(bytes20(_srcAddress[20:]))) returns the destination address instead of the source address. Note: RootBridgeAgent::requiresEndpoint also implements this wrongly. https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1212

Maia team, couldn't catch this error via their testing suite, since even in tests, the encoding is wrong. They encoded (source, destination) as (bAgent, rootBridgeAgentAddress), so the modifier in BranchBridgeAgent, gets the rootBridgeAgentAddress as the source address(this doesn't fail, since they are returning the last 20 bytes, which will be the root bridge agent for BranchBridgeAgent and branch bridge agent for RootBridgeAgent case), when in reality the bridge agent::lzReceive function will be called by layer zero with (rootBridgeAgentAddress, bAgent).

Due to the current implementation, when the right encodpacked is sent to the lzReceive function from layer zero, the function lzReceiveNonBlocking will always revert. This will make the whole contracts inoperable.

Proof of Concept

I have written a simple test to show that the called internal function within the modifier reverts when called with the right encodpacked. I had to add a getter function in order to be able to catch the revert more easily. I added the below function to BranchBridgeAgent contract.

    function requiresEndpointt(address _endpoint, bytes calldata _srcAddress) external view {
        _requiresEndpoint(_endpoint, _srcAddress);

    }

Add this test to /test/ulysses-omnichain/BranchBridgeAgentTest.t.sol

    function testFunc() public {
        vm.startPrank(address(bAgent));
        vm.expectRevert();
        //this was initially bAgent.requiresEndpointt(lzEndpointAddress, abi.encodePacked(bAgent, rootBridgeAgentAddress));
        bAgent.requiresEndpointt(lzEndpointAddress, abi.encodePacked(rootBridgeAgentAddress, bAgent));
    }

Tools Used

Manual Review, Layer Zero Contracts

Recommended Mitigation Steps

Change BranchBridgeAgent::_requiresEndpoint function to:

    function _requiresEndpoint(address _endpoint, bytes calldata _srcAddress) internal view virtual {
        //Verify Endpoint
        if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();
        if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();

        //Verify Remote Caller
        if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();
    ++  if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[:20])))) revert LayerZeroUnauthorizedCaller();
    }

Also, change RootBridgeAgent::requiresEndpoint modifier to:

    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();
            }
        }
        _;
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

In ArbitrumBranchBridgeAgent contract, users are expected to retry settlement from the root environment, but the function retrySettlement is still available and callable in ArbitrumBranchBridgeAgent contract

   function retrySettlement(uint32, bytes calldata, GasParams[2] calldata, bool) external payable override lock {}

The effect of this is, when users do interact with this function(many will, since they can in other Branch bridge agent contracts), they will lose their native gas deposit.

Proof of Concept

I have included a simple test to show this: Add the below test to /test/ulysses-omnichain/ArbitrumBranchTest.t.sol file

    function testRetrySettlement() public {
        
        address _user = address(this);

        // Get some gas.
        hevm.deal(_user, 10 ether);
        console2.log("User Balance Before: ", _user.balance);
        console2.log("Branch Balance Before: ", address(arbitrumMulticallBridgeAgent).balance);

                //Gas Params
        GasParams[2] memory gasParams = [GasParams(0.5 ether, 0.5 ether), GasParams(0.5 ether, 0.5 ether)];

        arbitrumMulticallBridgeAgent.retrySettlement{value: 6 ether}(1, "", gasParams, true);
                console2.log("User Balance Before: ", _user.balance);
        console2.log("Branch Balance Before: ", address(arbitrumMulticallBridgeAgent).balance);

    }

You can execute the test by running: forge test -vv --match-path test/ulysses-omnichain/ArbitrumBranchTest.t.sol --match-test testRetrySettlement

When you run the test, you will get the below output:

Running 1 test for test/ulysses-omnichain/ArbitrumBranchTest.t.sol:ArbitrumBranchTest
[PASS] testRetrySettlement() (gas: 24663)
Logs:
  User Balance Before:  10000000000000000000
  Branch Balance Before:  0
  User Balance Before:  4000000000000000000
  Branch Balance Before:  6000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 16.89ms

From the above test we can see that when a user do call ArbitrumBranchBridgeAgent::retrySettlement function, in the hope of retrying a failed settlement call, their native gas deposit will be lost, without any settlement or a possible gas refund.

Tools Used

Manual Review

Recommended Mitigation Steps

If retrySettlement function shouldn't be accessed from ArbitrumBranchBridgeAgent contract, then consider reverting on call.

    function retrySettlement(uint32, bytes calldata, GasParams[2] calldata, bool) external payable override lock {
        revert();
    }

Assessed type

Payable