Maia DAO - Ulysses - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. _createSettlementMultiple() creates payload using bytes1(0x02) always regardless of the value of _hasFallbackToggled. As a result, in effect, _hasFallbackToggled is always treated as if it was false. That means, the fallback function can never be called!

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

_createSettlementMultiple() is supposed to create different payloads based on _hasFallbackToggled.

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

However the function uses _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02) instead of _hasFallbackToggled ? bytes1(0x82) : bytes1(0x02) to create the payload. The former actually will always use 0x02 since bytes1(0x02) & 0x0F = 0x02!. As a result, even when _hasFallbackToggled is true, it will be treated as false. That means, the fallback function can never be called!

The following POC shows that bytes1(0x02) & 0x0F is equal to bytes1(0x02). Thus, _hasFallbackToggled is treated as if it was always false.

function testSameValue() public
{
     assertEq(bytes1(0x02) & 0x0F, bytes1(0x02)); 
}

Tools Used

VScode

Recommended Mitigation Steps

change bytes1(0x02) & 0x0F to bytes1(0x82) when creating the payload:

function _createSettlementMultiple(
        uint32 _settlementNonce,
        address payable _refundee,
        address _recipient,
        uint16 _dstChainId,
        address[] memory _globalAddresses,
        uint256[] memory _amounts,
        uint256[] memory _deposits,
        bytes memory _params,
        bool _hasFallbackToggled
    ) internal returns (bytes memory _payload) {
        // Check if valid length
        if (_globalAddresses.length > MAX_TOKENS_LENGTH) revert InvalidInputParamsLength();

        // Check if valid length
        if (_globalAddresses.length != _amounts.length) revert InvalidInputParamsLength();
        if (_amounts.length != _deposits.length) revert InvalidInputParamsLength();

        //Update Settlement Nonce
        settlementNonce = _settlementNonce + 1;

        // Create Arrays
        address[] memory hTokens = new address[](_globalAddresses.length);
        address[] memory tokens = new address[](_globalAddresses.length);

        for (uint256 i = 0; i < hTokens.length;) {
            // Populate Addresses for Settlement
            hTokens[i] = IPort(localPortAddress).getLocalTokenFromGlobal(_globalAddresses[i], _dstChainId);
            tokens[i] = IPort(localPortAddress).getUnderlyingTokenFromLocal(hTokens[i], _dstChainId);

            // Avoid stack too deep
            uint16 destChainId = _dstChainId;

            // Update State to reflect bridgeOut
            _updateStateOnBridgeOut(
                msg.sender, _globalAddresses[i], hTokens[i], tokens[i], _amounts[i], _deposits[i], destChainId
            );

            unchecked {
                ++i;
            }
        }

        // Prepare data for call with settlement of multiple assets
        _payload = abi.encodePacked(
-            _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),
+            _hasFallbackToggled ? bytes1(0x82): bytes1(0x02),

            _recipient,
            uint8(hTokens.length),
            _settlementNonce,
            hTokens,
            tokens,
            _amounts,
            _deposits,
            _params
        );

        // Create and Save Settlement
        // Get storage reference
        Settlement storage settlement = getSettlement[_settlementNonce];

        // Update Setttlement
        settlement.owner = _refundee;
        settlement.recipient = _recipient;
        settlement.hTokens = hTokens;
        settlement.tokens = tokens;
        settlement.amounts = _amounts;
        settlement.deposits = _deposits;
        settlement.dstChainId = _dstChainId;
        settlement.status = STATUS_SUCCESS;
    }

Assessed type

Math

QA1. setAddress() fails to delete the getGlobalTokenFromLocal[oldL][_srcChainId] getLocalTokenFromGlobal[oldG][_srcChainId]. As a result there might be two global tokens that are mapped to one local token. For example, suppose we had L1 <-> G1, and now we need to set L2 <-> G2, then we need to set L1 so that it points to zero; otherwise, both L1 and L2 will point to G2.

Similar problem for setLocalAddress().

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L239-L256

Mitigation: function setAddresses( address _globalAddress, address _localAddress, address _underlyingAddress, uint256 _srcChainId ) external override requiresCoreRootRouter { if (_globalAddress == address(0)) revert InvalidGlobalAddress(); if (_localAddress == address(0)) revert InvalidLocalAddress(); if (_underlyingAddress == address(0)) revert InvalidUnderlyingAddress();

•    address oldG = getGlobalTokenFromLocal[_localAddress][_srcChainId];
  

•    address oldL = getLocalTokenFromGlobal[_globalAddress][_srcChainId];
  

•    address oldU = getUnderlyingTokenFromLocal[_localAddress][_srcChainId];
  

•    delete isGlobalAddress[oldG];
  

•    delete getGlobalTokenFromLocal[oldL][_srcChainId];
  

•    delete getLocalTokenFromGlobal[oldG][_srcChainId];
  

•    delete getLocalTokenFromUnderlying[oldU][_srcChainId];
  

•    delete getUnderlyingTokenFromLocal[_localAddress][_srcChainId];
  
    isGlobalAddress[_globalAddress] = true;
    getGlobalTokenFromLocal[_localAddress][_srcChainId] = _globalAddress;
    getLocalTokenFromGlobal[_globalAddress][_srcChainId] = _localAddress;
    getLocalTokenFromUnderlying[_underlyingAddress][_srcChainId] = _localAddress;
    getUnderlyingTokenFromLocal[_localAddress][_srcChainId] = _underlyingAddress;
  
    emit LocalTokenAdded(_underlyingAddress, _localAddress, _globalAddress, _srcChainId);
  

  }

GA2:

A. Any comments for the judge to contextualize your findings I have focused on to ensure 1) the state transition of each component is properly performed; 2) funds have been transferred properly from one component to another without messing up.

B. Approach taken in evaluating the codebase

1. Understand the interaction among the triple (RootPort, RootBridgeAgent, and coreRootRouter) and the triple (BranchPort, BranchBridgeAgent, and CoreBranchRouter).
2. Understand the interaction between RootBridgeAgent and BranchBridgeAgent via Layerzero messaging as follows:

RootBridgeAgent callout: 0x00: callout 0x01/0x81: callOutAndBridge(createSettlement) 0x02/0x82: calloutAndBridgeMultiple(_createSettlementMultiple) 0x03: retrieveSettlement 0x04: _performFallbackCall

RootBridgeAgeent.lzReceiveNonBlocking will route call to RootBridgeAgentExecutor or local calls: 0x00: executeSystemRequest 0x01: executeNoDeposit 0x02: executeWithDeposit 0x03: executeWithDepositMultiple 0x04: executeSignedNoDeposit 0x05/0x85: executeSignedWithDeposit 0x06/0x86: executeSignedWithDepositMultiple 0x07/0x87: _performRetrySettlementCall 0x08: _performFallbackCall 0x09: set getSettlement[nonce].status = STATUS_FAILED

BranchBridgeAgent callout: 0x00: calloutSystem 0x01: callOut 0x02: callOutAndBridge 0x03: callOutAndBridgeMultiple 0x04: calloutSigned 0x05/0x85: callOutSignedAndBridge 0x06/0x86: callOutSignedAndBridgeMultiple 0x07/0x87: retrySettlement 0x08: retrieveDeposit 0x09: _performFallbackCall

BranchBridgeAgent. lzReceiveNonBlocking will route calls to BranchBridgeAgentExecutor or local calls: 0x00: executeNoSettlement 0x01/0x81: executeWithSettlement 0x02/0x82: executeWithSettlementMultiple 0x03: _performFallbackCall 0x04: Fallback change getDeposit[nonce].status = STATUS_FAILED;

C. Architecture recommendations

1. The complexity is greatly increased by RootBridgeAgentExecutor and BranchBridgeAgentExecutor. As a result, many interactions between RootBridgeAgent and other components have to go through RootBridgeAgentExecutor. The same is true for BranchBridgeAgentExecutor.

Mitigation: combine RootBridgeAgentExecutor into RootBridgeAgent; combine BranchBridgeAgentExecutor into BranchBridgeAgent.

2. The communication between between CoreRootRouter and RootBridgeAgent can be direct instead of via RootPort. For example, consider the flow CoreRootRouter._syncBranchBridgeAgent() -> rootPortAddress.syncBranchBridgeAgentWithRoot() -> _rootBridgeAgent.syncBranchBridgeAgent(_newBranchBridgeAgent, _branchChainId). The complexity can be reduced if CoreRootRouter._syncBranchBridgeAgent() calls _rootBridgeAgent.syncBranchBridgeAgent(_newBranchBridgeAgent, _branchChainId) directly.

D. Codebase quality analysis Consider optimize the following five flows by removing RootBridgeAgentExector in the path. The RootBridgeAgentExecutor should call CoreRootRouter directly instead of through RootBridgeAgentExecutor.

1. BranchBridgeAgent.calloutAndBridge() -> _performCall() -> lzEndpointAddress).send() -> RootBridgeAgent.lzreceive() -> excessivelySafeCall() -> lzReceiveNonBlocking() -> RootBridgeAgentExecutor.executeWithDeposit() -> CoreRootRouter.executeDepositSingle() -> revert()

2. BranchBridgeAgent.calloutAndBridgeMultiple() -> _performCall() -> lzEndpointAddress).send() -> RootBridgeAgent.lzreceive() -> excessivelySafeCall() -> lzReceiveNonBlocking() -> RootBridgeAgentExecutor.executeWithDepositMultiple() -> CoreRootRouter.executeDepositMultiple() -> revert()

3. BranchBridgeAgent.calloutSigned() -> _performCall() -> lzEndpointAddress).send() -> RootBridgeAgent.lzreceive() -> excessivelySafeCall() -> lzReceiveNonBlocking() -> RootBridgeAgentExecutor.executeSignedNoDeposit() -> CoreRootRouter.executeSigned() -> revert()

4. BranchBridgeAgent.calloutSignedAndBridge() -> _performCall() -> lzEndpointAddress).send() -> RootBridgeAgent.lzreceive() -> excessivelySafeCall() -> lzReceiveNonBlocking() -> RootBridgeAgentExecutor.executeSignedWithDeposit() -> CoreRootRouter.executeSignedDepositSingle() -> revert()

5. BranchBridgeAgent.calloutSignedAndBridgeMultple() -> _performCall() -> lzEndpointAddress).send() -> RootBridgeAgent.lzreceive() -> excessivelySafeCall() -> lzReceiveNonBlocking() -> RootBridgeAgentExecutor.executeSignedWithDepositMultiple() -> CoreRootRouter.executeSignedDepositMultiple() -> revert()

E. Centralization risks

F. Mechanism review

1. CoreRootRouter should focus on the functionality of "routing". Since the functionality of _addLocalToken, _setLocalToken, _syncBranchBridgeAgent are implemented in BranchBridgeAgent.sol, these internal functions should be moved to c.sol. Currently, these internal functions just unwrap the payload and then wrap another payload to call BranchBridgeAgent.sol - a waste of time and only make the code harder to manage. Simplicity is the key for security.

G. Systemic risks

1. BranchBridgeAgent_performFallbackCall() will send the whole balance of the native token in the contract back to the refundee.This amount might be more than what the refundee sent. One should keep track of the amount sent by the refundee so that we will only send back what we owe to the refundee. Otherwise, it is possible for the refundee to steal funds from the contract.

2. Similarly, RootBridgeAgent. _performFallbackCall() will send the whole balance of the native token in the contract back to the refundee.This amount might be more than what the refundee sent. One should keep track of the amount sent by the refundee so that we will only send back what we owe to the refundee. Otherwise, it is possible for the refundee to steal funds from the contract.

Time spent:

100 hours