callOut And Bridge feature always fail with core router

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L350-L357 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L360-L367 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L370-L372 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L375-L382 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L385-L392

Vulnerability details

Impact

Core root router will always revert when user put _params parameter while bridging.

Proof of Concept

In CoreRootRouter contract, executeDepositSingle, executeDepositMultiple, executeSigned, executeSignedDepositSingle, executeSignedDepositMultiple functions will always be reverted.

/// @inheritdoc IRootRouter
  function executeDepositSingle(bytes memory, DepositParams memory, uint16)
      external
      payable
      override
      requiresExecutor
  {
      revert();
  }

  /// @inheritdoc IRootRouter
  function executeDepositMultiple(bytes calldata, DepositMultipleParams memory, uint16)
      external
      payable
      override
      requiresExecutor
  {
      revert();
  }

  /// @inheritdoc IRootRouter
  function executeSigned(bytes memory, address, uint16) external payable override requiresExecutor {
      revert();
  }

  /// @inheritdoc IRootRouter
  function executeSignedDepositSingle(bytes memory, DepositParams memory, address, uint16)
      external
      payable
      override
      requiresExecutor
  {
      revert();
  }

  /// @inheritdoc IRootRouter
  function executeSignedDepositMultiple(bytes memory, DepositMultipleParams memory, address, uint16)
      external
      payable
      override
      requiresExecutor
  {
      revert();
  }

These functions are called when bridging with _params parameter at BranchBridgeAgent contract. This is BranchBridgeAgent contract’s functions which get _params parameter.

function callOutAndBridge(
    address payable _refundee,
@>  bytes calldata _params,
    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);
}

/// @inheritdoc IBranchBridgeAgent
function callOutAndBridgeMultiple(
    address payable _refundee,
@>  bytes calldata _params,
    DepositMultipleInput 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(0x03),
        uint8(_dParams.hTokens.length),
        _depositNonce,
        _dParams.hTokens,
        _dParams.tokens,
        _dParams.amounts,
        _dParams.deposits,
@>      _params
    );

    //Create Deposit and Send Cross-Chain request
    _createDepositMultiple(
        _depositNonce, _refundee, _dParams.hTokens, _dParams.tokens, _dParams.amounts, _dParams.deposits
    );

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

/// @inheritdoc IBranchBridgeAgent
@> function callOutSigned(address payable _refundee, bytes calldata _params, GasParams calldata _gParams)
    external
    payable
    override
    lock
{
    //Encode Data for cross-chain call.
@>  bytes memory payload = abi.encodePacked(bytes1(0x04), msg.sender, depositNonce++, _params);

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

/// @inheritdoc IBranchBridgeAgent
function callOutSignedAndBridge(
    address payable _refundee,
@>  bytes calldata _params,
    DepositInput memory _dParams,
    GasParams calldata _gParams,
    bool _hasFallbackToggled
) external payable override lock {
    //Cache Deposit Nonce
    uint32 _depositNonce = depositNonce;

    //Encode Data for cross-chain call.
    bytes memory payload = abi.encodePacked(
        _hasFallbackToggled ? bytes1(0x85) : bytes1(0x05),
        msg.sender,
        _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);
}

/// @inheritdoc IBranchBridgeAgent
function callOutSignedAndBridgeMultiple(
    address payable _refundee,
@>  bytes calldata _params,
    DepositMultipleInput memory _dParams,
    GasParams calldata _gParams,
    bool _hasFallbackToggled
) external payable override lock {
    // Cache Deposit Nonce
    uint32 _depositNonce = depositNonce;

    // Encode Data for cross-chain call.
    bytes memory payload = abi.encodePacked(
        _hasFallbackToggled ? bytes1(0x86) : bytes1(0x06),
        msg.sender,
        uint8(_dParams.hTokens.length),
        _depositNonce,
        _dParams.hTokens,
        _dParams.tokens,
        _dParams.amounts,
        _dParams.deposits,
  @>    _params
    );

    // Create a Deposit and Send Cross-Chain request
    _createDepositMultiple(
        _depositNonce, _refundee, _dParams.hTokens, _dParams.tokens, _dParams.amounts, _dParams.deposits
    );

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

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

_params parameter is for custom root router. The custom router get information about what to do from _params parameter. But core root router doesn’t provide custom action for user, so if _params parameter exist, it just revert the transaction.

This is RootBridgeAgentExecutor contract functions which call router functions. They call router function only when _params parameter exist. And this would be reverted if router is core root router.

function executeWithDeposit(address _router, bytes calldata _payload, uint16 _srcChainId)
    external
    payable
    onlyOwner
{
    ...
    // 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
        );
    }
}

function executeWithDepositMultiple(address _router, bytes calldata _payload, uint16 _srcChainId)
    external
    payable
    onlyOwner
{
    ...
    // Check if there is additional calldata in the payload
    if (length > PARAMS_END_OFFSET + (numOfAssets * PARAMS_TKN_SET_SIZE_MULTIPLE)) {
        //Try to execute remote request
        IRouter(_router).executeDepositMultiple{value: msg.value}(
            _payload[PARAMS_END_OFFSET + uint256(numOfAssets) * PARAMS_TKN_SET_SIZE_MULTIPLE:], dParams, _srcChainId
        );
    }
}

function executeSignedNoDeposit(address _account, address _router, bytes calldata _payload, uint16 _srcChainId)
    external
    payable
    onlyOwner
{
    //Execute remote request
    IRouter(_router).executeSigned{value: msg.value}(_payload[PARAMS_TKN_START_SIGNED:], _account, _srcChainId);
}

function executeSignedWithDeposit(address _account, address _router, bytes calldata _payload, uint16 _srcChainId)
    external
    payable
    onlyOwner
{
    ...

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

function executeSignedWithDepositMultiple(
    address _account,
    address _router,
    bytes calldata _payload,
    uint16 _srcChainId
) external payable onlyOwner {
    ...

    // Check if there is additional calldata in the payload
    if (
        _payload.length
            > PARAMS_END_SIGNED_OFFSET
                + uint256(uint8(bytes1(_payload[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE
    ) {
        //Execute remote request
        IRouter(_router).executeSignedDepositMultiple{value: msg.value}(
            _payload[
                PARAMS_END_SIGNED_OFFSET
                    + uint256(uint8(bytes1(_payload[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE:
            ],
            dParams,
            _account,
            _srcChainId
        );
    }
}

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

If the user did not use the fallback option, they cannot call redeemDeposit immediately, but must call retrieveDeposit first to request token back.

The automatic fallback option is not provided for the callOutAndBridge, callOutAndBridgeMultiple, and callOutSigned functions, so the user must call retrieveDeposit and redeemDeposit to get their tokens back. Or the user should modify the request with retryDeposit to delete the _params parameter and resend it.

Users who have spent all their tokens bridging will have a hard time getting their tokens out because they can't pay for gas anymore.

To avoid this situation, the core branch bridge agent or core branch router should limit the length of the _params to zero.

Tools Used

Manual Review

Recommended Mitigation Steps

You can restrict _params on the core branch router, but it is not possible to prevent all of these situations because user can call the functions of the Branch bridge agent without going through the router.

One suggestion is to have the isCore setting on the Branch bridge agent or branch router, and if it is true, limit the length of the _params .

If you want to use Core contracts solely for administrative purposes, it would be better to prevent users from requesting token moves to core.

Assessed type

DoS

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #817

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid