Reach out for an audit or to learn more about Macro
or Message on Telegram

Connext A-2

Security Audit

July 12, 2023

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Connext's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from April 24, 2023 to May 26, 2023.

The purpose of this audit is to review the source code of certain Connext Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 7 - - 7
Medium 8 1 - 7
Low 6 1 - 5
Code Quality 15 2 - 13
Informational 1 1 - -

Connext was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within the monorepo repository:

./packages/deployments/contracts/contracts/messaging/

Contract SHA256
./Conductor.sol

55f1767dd806c2ee2767e093ba8a02f011d7f1bcb1d9750ecf4a8e4756d3e76a

./MerkleTreeManager.sol

b1dc10d58c5da9720418633a8b71454d3c938fd006c05eeb0d4d0084fb600f40

./RootManager.sol

fce2ea45afbf33b2b4937344269b1364184f0e4a7d46a987fa24b6d2061397a7

./connectors/Connector.sol

eccf9a33b5d0f170003c0a343e68693c16eb4d499654b5ed177fe787a224d923

./connectors/ConnectorManager.sol

78a29659b3f55e4ac2f5b6a0f489048ea552c77b8371530a21bc255c6c5515f5

./connectors/GasCap.sol

b27b0e5f079af9c5f1539d1ba2d2984cad515192a0fc9a02095a1b979612e7b5

./connectors/HubConnector.sol

0d138e14b7b8c80b54463580155c834af445580a838c70cb86a13a20ff1ef226

./connectors/SpokeConnector.sol

d3fc564c6a3d71ebb5a110d55f19779212dd43b7bd11deeafeba02af04162c33

./connectors/mainnet/MainnetSpokeConnector.sol

6ec3570640c1d2d92b0861ba795133d47ea761af72e39ddd324ce43603c1b5c0

./connectors/optimism/BaseOptimism.sol

8ba3dc99a2bb0fa668fdec812a3c35115c21ae78207c1e0056b7b72e9507960e

./connectors/optimism/OptimismHubConnector.sol

87e76de475254cc776735d75e0f6e5ffa9bec0ecb414f5d7f360ffa6a36f9ea8

./connectors/optimism/OptimismSpokeConnector.sol

68f46da81aafe83fa8ac40468a5dfab6f186e608592824d162f0f65c51a5c734

./connectors/optimism/lib/Types.sol

adb10e8b8a71ddb405b378943f0ca19511a2c510ad6a1ff5bd61e6c16f80ddd1

./connectors/polygonzk/BasePolygonZk.sol

0de1ee567a7f6e3e27e6d1cc09e6dfb3e335c8f1336507b4d3f605b46229f2a6

./connectors/polygonzk/PolygonZkHubConnector.sol

c12fec532345fb4a431cd6777e40b8c66489140368c8169a845cdb855e1b116d

./connectors/polygonzk/PolygonZkSpokeConnector.sol

ce3bffbdc6484edfde1d0942686b9b63a23c82a8772f64b7a5c266127827aaea

./connectors/zksync/ZkSyncHubConnector.sol

71d172e0f55002e86483ec25a6f185b08d979ba993dc8ae02daa4a0c58698550

./connectors/zksync/ZkSyncSpokeConnector.sol

a267b84fb21d40a969d358a26e2fa792f40514abe79f0ffbc48f312566f26f33

./connectors/consensys/ConsensysBase.sol

a046f94fc66da4485bed9c5c184d3cbbc540eb7074d814b2c4ea0ced6effb885

./connectors/consensys/ConsensysHubConnector.sol

612632f9788ab29a14f297ea9333a869b5a92302a2d1f2ff94375206375abb1f

./connectors/consensys/ConsensysSpokeConnector.sol

2e57de75f96a740450c6458255fc2dfdb4ddfad95b84b40579a02b4517e43d27

./interfaces/IConnector.sol

18e69eed4b7226e8650fa078fab7cc47600f4bdd24652ffb2e38d23c3e7fc5a6

./interfaces/IConnectorManager.sol

34583a69beb6430e1d3a563e2672bbb64e7316d8eb25ca2bbd62868b8cd77303

./interfaces/IHubConnector.sol

2bf30b5788b487ddaa1e44333f999aa16ba71e7ab5ef4c476e3905bd3f00a53a

./interfaces/IMessageRecipient.sol

cd9c247436a3baeca59201b1cab92367da901b383b7fbeacb48fb6f9acb8b421

./interfaces/IOutbox.sol

2e06d4c06aee3e8208780812acb796a1e1745e5fc0cdd9dfc98f69b91a80ca07

./interfaces/IRootManager.sol

d1dcb24dae1549066036e00c9eac5293411dd899177295f6f401031e624af3be

./interfaces/ambs/optimism/IOptimismPortal.sol

41cff48edb04834553bec3921283b5dccaa7e76a8249b462e88f5186875bc751

./interfaces/ambs/optimism/OptimismAmb.sol

1ef13616e80e6337a0dc0c72be02e9ae47f5b39b451ccfc36b4b2f04a1d3cdf1

./interfaces/ambs/polygonzk/IBridgeMessageReceiver.sol

a8eb4926df3031341dcd299a0e974b61ba60d9564e982e27b58c47a17f3e46fd

./interfaces/ambs/polygonzk/IPolygonZkEVMBridge.sol

29f13a887856de236c8b20ab84ec0f36afdb3827e2d097c179db91a46d9ee475

./interfaces/ambs/zksync/IL1Messenger.sol

d4f97d76ce1e8ec0eeedf67c4c85f0d14310b5da026e606d49c8a975a294c896

./interfaces/ambs/zksync/IZkSync.sol

ecd5f295053f40ebc1c3db09005735098fb492ab756cc228fb822de31136f1e6

./libraries/DomainIndexer.sol

19fca7174da361d51a0343ac8a2931db0aacdf8f268a30334d4709cbdee4e73a

./libraries/SnapshotId.sol

93c200bbbe70554ca25da16e42f008273ecc85a27cf1b033d902dd50b93b84a9

Snapshot Library Improvement (PR4060)

./packages/deployments/contracts/contracts/core/connext/helpers/

Contract SHA256
./RootManager.sol

94bc1020cca4f13b5d94594561052614dac2f011c1083f18235762d0e0d05560

./connectors/SpokeConnector.sol

1a44e10256c8f0dc4a78d077b2f432100ff5a9843c3c79b02aa7102ef9487be6

./libraries/SnapshotId.sol

e661b537559a3c3a24ad4d47616677ed4d1b3002ef412d9da5e02b05b3c44e08

Specific changes (PR4061) for the Relayer Proxy contracts

./packages/deployments/contracts/contracts/core/connext/helpers/

Contract SHA256
./RelayerProxy.sol

28036f04d0f3596e3611900e9b87e74f506ef391f64e003c16a45f29864445d3

./RelayerProxyHub.sol

f503fedd08b41aff76d2ca534f1e5a7944ea111d7c7bce8ffa0511bad76fcc51

And, specifically, we audited the following contracts within the chain-abstraction-integration repository:

./contracts/

Contract SHA256
./destination/xreceivers/AuthForwarderXReceiver.sol

b9885798adf17358867522790b9310c3229e24e6b836b105440e620666a7bd75

./destination/xreceivers/ForwarderXReceiver.sol

b847b153ba7ab456a5a7aa6a7d9679ecfda6ca63cff284d1b7746231b385034e

./destination/xreceivers/Swap/SwapForwarderXReceiver.sol

71d08a6f9dacde6cad8f71082ea8bb7e32d6548c98754642cc7f27b3c5093ffe

./origin/Swap/SwapAndXCall.sol

6807a4f014d9bf7f5577580d12dc2fae30335d10448797ed03f0254415cfd064

./shared/Swap/OneInch/OneInchUniswapV3.sol

34b1adcd7bfa2c54ef58fe93799ee97d166a02169377ef9edd84c9d7dee44d86

./shared/Swap/Pancakeswap/PancakeV3Swapper.sol

94fed22e19824f9af01b7f0a63550629298e24047b5afc9233fcff082d6b4ddb

./shared/Swap/SwapAdapter.sol

74d10828789e61b84573729dbdac6ccb16877e140ceeeed3f41d11e5fad2ac4b

./shared/Swap/Uniswap/UniV2Swapper.sol

34e33e21ed36fd2c6e420eef1b21f41c012aca983add519fccddd8845f877269

./shared/Swap/Uniswap/UniV3Swapper.sol

888581b0e5260e71d535b6afa502926ad7d5883319fc140ffa8355a64310c02a

./shared/Swap/interfaces/IPancakeSmartRouter.sol

b232a10c70916913d8763c7c096d6e02d72789f5d2946bb06afc0244a1872210

./shared/Swap/interfaces/ISmartRouter.sol

cce7f213b6c7e44eef721560bfbd1ef99501b7fb7655fb090794119400aa6b54

./shared/Swap/interfaces/ISwapper.sol

d7066a42cd77c48b2ee6e96d7dfd8115d8b6df3c65e9eab5744d41830f82bccc

./shared/Swap/interfaces/IWETH9.sol

b991bc8a69dd21e4eb5e7599457972f17abf0bdca8f4357bca059f7620393099

./shared/Swap/libraries/BytesLib.sol

5656aba4deaa3d2e68d27b62f8bdc437e8026fa44f2ec502097185a2ed8b6571

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

RootManager.propagate susceptible to DoS

Topic
Griefing
Status
Impact
High
Likelihood
High

In the RootManager contract, propagate() function is responsible for distributing aggregate roots to all connectors for corresponding domains. Implementation of this function does not revert when interaction with a specific connector fails. This prevents a single inaccessible chain from causing a complete system halt. Particular functionality was introduced to fix a previously reported issue in Spearbit audit https://github.com/spearbit-audits/connext-nxtp/issues/34.

When propagate() is called, a set of initial validation checks is performed. If these checks are successful lastPropagatedRoot value will be set in _sendRootToHubs(). In the rest of propagate() function, even if interaction with all connectors fails, propagate() function will not revert. In this case, the lastPropagatedRoot value will remain set. This means that follow-up invocations of propagate() function will not be possible until _aggregateRoot is updated. This is indeed the original intention of this code.

// Sanity check: make sure we are not propagating a redundant aggregate root.
require(_aggregateRoot != lastPropagatedRoot, "redundant root");
lastPropagatedRoot = _aggregateRoot;

However, propagate() function can be invoked by anyone. Since it doesn’t have any access control limits. As a result, a malicious actor can invoke this function with inappropriate arguments. For example, by providing encoding data of unexpected length. This will cause most, if not all, connectors to revert when invoked. For example, processing will fail for:

  • PolygonHubConnector in _sendMessage() function when encodedData.length is not 0
  • ZKSyncHubConnector when encodedData.length is not 32
  • BaseMultichain in _sendMessage() when encodedData.length is not 0
  • GnosisBase in _getGasFromEncoded() when encodedData.length is not 32
  • ArbitrumHubConnector in _sendMessage() when encodedData.length is not 96

As a result of malicious actor actions, aggregate roots won’t be propagated to destination domains. The system may recover if a new root is added and propagate() is called with proper inputs. However, an attacker may front-run these calls with their own invocation with adverse inputs.

Remediations to Consider:

  1. Implement per domain check and take into account that the same scenario is possible due to the reset of finalizedOptimisticAggregateRoot variable in _optimisticPropagate() and _slowPropagate() functions, or
  2. Restrict access to propagate() and finalizeAndPropagate() function to relayers only.
H-2

Trust model affected when addBypass() is enabled to be bypassable

Topic
Protocol Design
Status
Impact
Spec Breaking
Likelihood
Medium

The Conductor contract serves as an upgrade timelock for the messaging layer; it enables the owner to queue transactions and execute them after a delay period. Additionally, the addBypass/removeBypass functions can be utilized to skip the execution delay for a specific _target and _selector. These bypass whitelisted functions must undergo the same queue and delay period before their addition.

However, the addBypass function allows the owner to execute any arbitrary function call without queueing and waiting for the corresponding delay by:

  • Queue and execute (after delay()) the addBypass function using the addBypass.selector and address of the conductor as _target.
  • Assemble and executeWithBypass a transaction array with:
    • addBypass(anyTarget, anySelector)
    • anyTarget.anySelector()

Since the logic checks if the target/selector is bypassed and executes it in order, the owner can add any _target and _selector atomically in one executeWithBypass call.

Remediations to Consider:

  • Disallowing the address of the conductor as _target in addBypass.
H-3

directSwapperCall allows arbitrary calls to any contract

Topic
Trust Model
Status
Impact
High
Likelihood
Medium

SwapAdapter is a shared contract that allows swapping assets by implementing adapters for each exchange that can be used both in the origin and destination chain.

However, in the directSwapperCall function, there are no checks on whether the _swapper address is a swapper implementation. Allowing anyone to execute any arbitrary call on any address with the contract’s context. Although the contracts are not intended to hold any assets, any future integration implementation built on top of SwapAdapter contract will inherit this dangerous function, essentially acting as a public backdoor entry point.

Remediations to Consider:

  • Consider restricting _swapper parameter value to entries in the allowedSwappers mapping.
H-4

exactSwap() does not send any value to swapETH call

Topic
Wrong implementation
Status
Impact
High
Likelihood
Medium

In SwapAdapter.sol, exactSwap() function calls with _fromAsset equal to address(0) will perform the specific logic for native asset in the intended Swapper contract, swapETH():

function exactSwap(
  address _swapper,
  uint256 _amountIn,
  address _fromAsset,
  address _toAsset,
  bytes calldata _swapData // comes directly from API with swap data encoded
) external payable returns (uint256 amountOut) {
  ...

  if (_fromAsset == address(0)) {
    amountOut = ISwapper(_swapper).swapETH(_amountIn, _toAsset, _swapData);
  } else {
    ...
    }
    ...
  }
}

However, no value is sent to this call, making this function to revert always in the swapper if:

  • _fromAsset == address(0),
  • _amountIn > 0 and,
  • _fromAsset != _toAsset.

Remediations to Consider:

  • Send the corresponding msg.value to the swapETH call.
H-6

swap() function does not consider _fromAsset == _toAsset

Topic
Use Cases
Status
Impact
High
Likelihood
Medium

In the UniV2Swapper contract, if the swap function is called with an equal asset in _fromAsset and _toAsset the tokens will not be processed and will be stuck in the swapper contract, returning a zero amountOut. This behavior is not consistent with the other swapper implementations.

Remediations to Consider:

  • Consider adding this case to the if statements and sending the assets back to the caller.
H-7

OneInchUniswapV3 can’t receive native assets

Topic
Protocol Design
Status
Addressed
Impact
High
Likelihood
Medium
PR closed after noticing that new functionality is not required taking into account H-8 change

The current OneInchUniswapV3 implementation does not implement a receive() or fallback() function, disallowing it to receive any native assets and reverting any attempt of swap to native assets. This will happen to any swap calls where _toAsset is equal to address(0).

Remediations to Consider:

  • Consider adding a receive() function.
H-8

OneInchUniswapV3 incorrect swapETH implementation

Topic
Protocol Design
Status
Impact
High
Likelihood
Medium

In the OneInchUniswapV3 swapper contract, the swapETH() function is incorrectly implemented and is not consistent with the other swapper implementations that use WETH as an intermediate swap for native assets. The oneInchUniRouter.uniswapV3Swap() call (line 102) does not send any value to the call. Another point to take into account is that the uniswapV3Swap() function is not payable and therefore doesn’t accept ETH transfers.

Remediations to Consider:

M-1

Incomplete cooldown feature for proposing aggregate root

Topic
Spec
Status
Impact
Medium
Likelihood
Medium

The RelayerProxyHub contract contains functionality meant to prevent more frequent invocation of proposeAggregateRoot(). This is based on using lastProposeAggregateRootAt and proposeAggregateRootCooldown variables.

However, this functionality does not work since:

  1. proposeAggregateRootCooldown is never set, and there are no means to update it
  2. lastProposeAggregateRootAt is never set
  3. corresponding AggregateRootCooldownChanged event is missing, which is necessary to have consistent implementation

As a result, relayers are able to propose aggregate roots with any frequency without restriction.

Remediations to Consider:

  1. Update RelayerProxyHub contract functionality to have proper cooldown functionality for proposing aggregate root, or
  2. Remove this functionality if it is unnecessary
M-2

Native assets sent to exactSwap() may get lost

Topic
Protocol Design
Status
Impact
High
Likelihood
Low

In the SwapAdapter contract, exactSwap() is a wrapper function for performing ERC20 token and native asset swaps in the interaction with allowlisted swappers.

In case native assets are incorrectly sent together with ERC20 token assets, native assets may get lost since they will not be accounted for.

Remediations to Consider:

  • Remove the payable modifier from the exactSwap() function, or
  • Add a guard to prevent further processing if msg.value is non-zero.
M-3

Native assets sent to swap() may get lost

Topic
Protocol Design
Status
Impact
High
Likelihood
Low
Fixed in updated chain-abstraction-integration

In the OneInchUniswapV3 contract, swap() is a wrapper function for performing ERC20 token swap in the interaction with the 1inch router. In addition, swap() is defined as payable even though it doesn’t handle native assets.

In case native assets are incorrectly sent together with ERC20 token assets, native assets may get lost since they will not be accounted for.

Remediations to Consider:

  • Remove the payable modifier from the swap() function, or
  • Add a guard to prevent further processing if msg.value is non-zero.
M-4

Native assets sent to swapAndXCall() may get lost

Topic
Protocol Design
Status
Impact
High
Likelihood
Low

The SwapAndXCall contract implements two swapAndXCall() functions, one which pays the relayer fee in the native asset and the second one that uses the output asset to pay for the relayer fee. However, both of these functions are payable. If users mistakenly send native assets to the swapAndXCall() function with the _relayerFee argument, these funds will get stuck in the contract.

Following edge case scenarios are not correctly handled when a variant of swapAndXCall() with _relayerFee is invoked:

  1. _fromAsset == address(0) and msg.value is greater than amountIn. In this case, the amount of native assets equal to the difference of msg.value and amountIn will remain stuck in the contract.
  2. fromAsset is a non-zero address and msg.value is also non-zero. In this case, the amount of native assets equal to the msg.value will remain stuck in the contract.

Remediations to Consider:

  • Removing the payable modifier for the swapAndXCall() function that uses the output asset for paying the relayer fee, or
  • Splitting the internal _setupAndSwap() function into two separate functions to appropriately handle edge cases for each higher-level variant of the swapAndXCall().
M-5

Unsafe transfer of ERC20 tokens

Topic
Coding Standards
Status
Impact
High
Likelihood
Medium

In ForwarderXReceiver and AuthForwarderXReceiver contracts, xReceive() function implementation contains an identical piece of code that handles the fallback case when a transfer of ERC20 tokens to provided target has failed.

if (!successfulForward) {
  IERC20(_asset).transfer(_fallbackAddress, _amount);
}

These contracts are supposed to handle generic ERC20 tokens, some of which do not follow ERC20 conventions with respect to how they report transfer failure. Some revert, as specified by the standard, but others return false.

As a result, mentioned implementation is unsafe since potentially some transfer failures will be considered successful even when they are not. In these cases, assets will remain in the custody of a particular contract instead of being transferred to the _fallbackAddress.

Remediations to Consider:

  • Use OZ’s SafeERC20 and its safeTransfer() wrapper for a more secure transfer of ERC20 tokens.
M-6

ZkSyncHubConnector fee refunds will be sent to L2 address of RootManager

Topic
Protocol Design
Status
Impact
Medium
Likelihood
Medium

In ZkSyncHubConnector, the _sendMessage internal function, used to propagate the root to the ZkSync domain, sets as the _refundAddress the msg.sender of the call:

IZkSync(AMB).requestL2Transaction{value: fee}(
  // The address of the L2 contract to call
  mirrorConnector,
  // We pass no ETH with the call
  0,
  // Encoding the calldata for the execute
  _calldata,
  // l2 Gas limit
  l2GasLimit,
  // The default l2GasPricePerPubdata
  l2GasPerPubdataByteLimit,
  // factory dependencies
  new bytes[](0),
  msg.sender
);

However, in the context of this call, msg.sender will be the RootManager contract, and since the refund will happen on the L2 after the transaction gets executed. These funds will be locked in this address.

Remediations to Consider:

  • Consider providing the _refundAddress parameter with an address owned by Connext to use these potential refunded assets.
M-7

Native assets sent to OneInchUniswapV3 may get lost

Topic
Protocol Design
Status
Impact
High
Likelihood
Low

In OneInchUniswapV3, the msg.value used in the swapETH function could be higher than the _amountIn. In this case, the amount of native assets equal to the difference of msg.value and amountIn will remain stuck in the contract.

Remediations to Consider:

  • Requiring that the msg.value is equal to the _amountIn if _fromAsset == address(0).
M-8

Default deadline value of the block.timestamp allows pending transactions to be maliciously executed

Topic
Protocol Design
Status
Acknowledged
Impact
Medium
Likelihood
Medium

In the UniV2Swapper and UniV3Swapper contracts, swap() and swapETH() functions feature calls to uniswapV2Router and uniswapV3Router with deadline/expiration parameter hardcoded to block.timestamp value. These functions do not allow users to configure a deadline for their underlying swaps on UniswapV2 and UniswapV3. As a consequence, this missing feature enables pending transactions to be maliciously executed at a later point.

Hardcoded deadline value of block.timestamp makes it possible for the transaction to be executed long after its expected execution time. For example, it may remain in the mempool for extended periods until it becomes interesting for block builders to include it. Also, it can be intentionally delayed to extract the maximum slippage value.

UniswapV2 and UniswapV3 offer deadline/expiration parameters in their functions as a protection mechanism. Hardcoding these parameters to block.timestamp circumvents this protection mechanism and makes this integration layer built upon UniswapV2 and UniswapV3 less secure for end users.

Remediations to Consider:

  • Introduce a configurable deadline parameter to all functions which perform a swap on the user's behalf, such as swap() and swapETH().
Response by Connext

It is difficult to decide deadline on the destination side. Because xReceive function is called by connext contract via relayer.

L-1

Incorrect event arguments for MinDisputeBlocksUpdated and DisputeBlocksUpdated

Topic
Events
Status
Impact
Low
Likelihood
High

In RootManager contract, the following events are declared:

event DisputeBlocksUpdated(uint256 previous, uint256 updated);

event MinDisputeBlocksUpdated(uint256 previous, uint256 updated);

However, when these events are emitted in setDisputeBlocks() and setMinDisputeBlocks() order of arguments is switched and incorrect.

emit MinDisputeBlocksUpdated(_minDisputeBlocks, minDisputeBlocks);
minDisputeBlocks = _minDisputeBlocks;
...
emit DisputeBlocksUpdated(_disputeBlocks, disputeBlocks);
disputeBlocks = _disputeBlocks;

Remediations to Consider:

  • Update implementation to provide correct arguments when emitting these events.
L-2

AuthForwarderXReceiver allows adding duplicated origin domains

Topic
Data Consistency
Status
Impact
Low
Likelihood
Low

In AuthForwarderXReceiver, constructor() and addOrigin() do not perform any checks on whether the _originDomain to be added is already in the originDomains array. This can cause an origin to be in the array multiple times.

In order to remove a duplicated domain and rectify an invalid state, the function removeOrigin would need to be called multiple times since the logic will only delete one value per call.

Remediations to Consider:

  • Consider checking whether the originRegistry[_originDomain] is already set and reverting if that is the case. Otherwise, add a new origin.
L-3

Functions addSwapper() and removeSwapper() do not check for the current state values nor emit events

Topic
Events
Status
Impact
Low
Likelihood
High

In the SwapAdapter contract, consider adding events to addSwapper() and removeSwapper() functions and checking if the address to be added or removed is not already in the desired state.

L-4

Allowance check in _setupAndSwap() references incorrect variable

Topic
Spec
Status
Impact
Low
Likelihood
Low
Fixed in updated chain-abstraction-integration

In the SwapAndXCall contract, function _setupAndSwap() contains an allowance check which, if satisfied, approves the max amount of _toAsset tokens to the connext contract.

if (IERC20(_toAsset).allowance(address(this), address(connext)) < _amountIn) {
  IERC20(_toAsset).approve(address(connext), type(uint256).max);
}

However, this check incorrectly references _amountIn instead of the amountOut variable, which represents the amount of _toAsset that connext uses in subsequent xcall() operations.

In a situation when the current connext allowance for _toAsset is greater or equal to _amountIn but less than amountOut, this may result in unexpected operation processing failure.

Remediations to Consider:

  • Update particular allowance check to reference the correct variable, which is amountOut instead of _amountIn.
L-5

Queued transactions do not have an expiry date for execution

Topic
Trust Model
Status
Acknowledged
Impact
Spec Breaking
Likelihood
Low

The Conductor contract timelock functionality can potentially be bypassed by queueing multiple transactions on deployment. Since there is no time limit on when these could be executed, the owner can add any arbitrary number of calls, wait for the delay(), and execute them at any given moment after the delay.

Remediations to Consider:

  • Consider adding an expiry check date to execute queued transactions (e.g. X weeks/days after the delay).
L-6

ZkSync l2GasPerPubdataByteLimit is hardcoded

Topic
Third-party Integration
Status
Impact
Medium
Likelihood
Low

The l2GasPerPubdataByteLimit is a parameter that contains the L2 gas price per each published to L1 calldata byte. In the current ZkSyncHubConnector implementation, it’s hardcoded in the _sendMessage logic function:

// The maximum amount L2 gas that the operator may charge the user for.
uint256 l2GasPerPubdataByteLimit = 800;

However, ZkSync's recommendation regarding this parameter is that this constant shouldn’t be relied on to be fixed and that every contract should provide the ability to supply the _l2GasPerPubdataByteLimit for each independent transaction.

Remediations to Consider:

  • Consider providing the _l2GasPerPubdataByteLimit on the client side and using it in the function parameters.
Q-1

ProposerAdded and ProposerRemoved events emitted when not needed

Topic
Events
Status
Acknowledged
Quality Impact
Medium

In the RootManager contract, in the addProposer() and removeProposer() functions, there is no check if the operation to be performed will result in state changes. Therefore, events like ProposerAdded and ProposerRemoved will be emitted even if no state change happened, which may confuse off-chain monitoring tools and services.

Q-2

BypassAdded and BypassRemoved events emitted when not needed

Topic
Events
Status
Acknowledged
Quality Impact
Low

In the Conductor contract, in the addBypass() and removeBypass() functions, there is no check if the operation to be performed will result in state changes. Therefore, events like BypassAdded and BypassRemoved will be emitted even if no state change happened, which may confuse off-chain monitoring tools and services.

Q-3

Missing indexed attributes for events in ForwarderXReceiver and AuthForwarderXReceiver

Topic
Events
Status
Quality Impact
Low

For easier off-chain tracking and monitoring, consider adding an indexed attribute to the _transferId argument for the following events:

event ForwardedFunctionCallFailed(bytes32 _transferId);
event ForwardedFunctionCallFailed(bytes32 _transferId, string _errorMessage);
event ForwardedFunctionCallFailed(bytes32 _transferId, uint _errorCode);
event ForwardedFunctionCallFailed(bytes32 _transferId, bytes _lowLevelData);
event Prepared(bytes32 _transferId, bytes _data, uint256 _amount, address _asset);
Q-4

Missing indexed attributes for events in AuthForwarderXReceiver

Topic
Events
Status
Quality Impact
Low

For easier off-chain tracking and monitoring, consider adding an indexed attribute:

  • for _transferId in the following events

    event ForwardedFunctionCallFailed(bytes32 _transferId);
    event ForwardedFunctionCallFailed(bytes32 _transferId, string _errorMessage);
    event ForwardedFunctionCallFailed(bytes32 _transferId, uint _errorCode);
    event ForwardedFunctionCallFailed(bytes32 _transferId, bytes _lowLevelData);
    event Prepared(bytes32 _transferId, bytes _data, uint256 _amount, address _asset);
    
  • for _originDomain in the following events

    event OriginAdded(uint32 _originDomain, address _originSender);
    event OriginRemoved(uint32 _originDomain);
    
Q-5

Incorrect error message

Topic
Error Handling
Status
Quality Impact
Medium
Fixed in updated chain-abstraction-integration

In the SwapAndXCall contract, we can see the following require statement:

require(msg.value >= _amountIn, "SwapAndXCall: msg.value != _amountIn");

The string message in this require statement is inconsistent with the code since the msg.value can be != _amountIn without reverting.

Consider changing the text to msg.value < _amountIn.

Q-6

Validate _originSender address when it is added instead of when it used

Topic
Optimization
Status
Quality Impact
Medium

In the AuthForwarderXReceiver contract, consider verifying that _originSender is not a 0 address when it is added in the addOrigin() function instead of checking it in the onlyOrigin modifier; this will ensure every origin address added in the originRegistry to be != address(0). As a result, this check won’t be necessary for every xReceive call.

Q-7

Duplicated constructor logic

Topic
Duplicated Code
Status
Quality Impact
Low

In the AuthForwarderXReceiver contract, consider calling addOrigin() in the constructor while adding the initial set of allowed origins instead of reimplementing the same functionality.

Q-8

Unnecessary allowedSwappers assignment

Topic
Unnecessary Code
Status
Quality Impact
Medium

In SwapAdapter.sol, the constructor logic assigns address(this) as an allowed swapper.

constructor() {
  allowedSwappers[address(this)] = true;
  allowedSwappers[uniswapSwapRouter] = true;
}

However, this mapping is checked while calling exactSwap which will call swap() on the swapper address, a function that doesn’t exist in the SwapAdapter.sol contract.

Consider removing this assignment.

Q-9

Inconsistent ownership transfer

Topic
Code Consistency
Status
Quality Impact
Low

AuthForwarderXReceiver inherits directly from the OZ Ownable contract instead of using Connext’s custom Ownable2Step contract.

Consider changing the ownable implementation of AuthForwarderXReceiver.sol to Ownable2Step to make it consistent with ForwarderXReceiver.sol.

Q-10

Unnecessary Address library usage

Topic
Unnecessary Code
Status
Quality Impact
Medium

In OneInchUniswapV3, UniV2Swapper, and UniV3Swapper contracts Address library is used for the address type. However, extra functionality from the Address library is not utilized within contract implementations. Therefore, consider removing the Address library.

Q-11

Simplify check in the removeOrigin()

Topic
Coding Standards
Status
Quality Impact
Medium

In the AuthForwarderXReceiver contract, the following check can be simplified by replacing with ==, since > state can never happen due to the previous code.

if (indexToRemove >= uint32(originDomains.length)) {
  revert ForwarderXReceiver__removeOrigin_invalidOrigin(_originDomain);
} 
Q-12

Documentation improvements

Topic
Documentation
Status
Quality Impact
High
  • RelayerProxyHub - missing natspec for _signature param in proposeAggregateRootKeep3r()
  • SpokeConnector - missing natspec for SnapshotRootSaved event
  • SwapAdapter - missing natspec for a return value in exactSwap() and directSwapperCall()
  • ISwapper - missing natspec for ISwapper:swap()
  • ForwarderXReceiver - missing natspec for a return value in xReceive, prepareAndForward, _prepare, _forwardFunctionCall functions
  • AuthForwarderXReceiver - missing natspec for a return value in xReceive, prepareAndForward, _prepare, _forwardFunctionCall functions
  • SwapForwarderXReceiver - missing natspec for a return value in _prepare
  • UniV2Swapper
    • swap() notice natspec incorrectly mentions 1inch
    • missing natspec for a return value in swap()
  • UniV3Swapper
    • swap() notice natspec incorrectly mentions 1inch
    • missing natspec for a return value in swap()
  • OneInchUniswapV3
    • missing natspec for a return value in swap()
Q-13

renounceOwnership logic is not consistent across contracts

Topic
Code Consistency
Status
Quality Impact
Medium

Although renounceOwnership is disabled in all current contracts, there are different logic implementations:

  • Conductor.sol reverts with a custom rennounceOwnership error.
  • MekleTreeManager.sol, RootManager.sol, WatcherClient.sol, WatcherManager.sol, SpokeConnector.sol, GnosisSopokeConnector.sol, MultichainSpokeConnector.sol, and OptimismSpokeConnector.sol do not revert but do not perform any state changes in the logic either.

Consider following a consistent implementation approach in all contracts.

Q-14

Inconsistent processed messages in ZkSync hub

Topic
Code Consistency
Status
Quality Impact
Medium

In ZkSyncHubConnector, processMessageFromRoot() calls to an already processed root message won’t revert. This implementation differs from Arbitrum and Optimism hubs, which revert if users attempt to reprocess a message root.

Consider making this behavior consistent across all implementations.

Q-15

SwapAndXCall functions do not document address(0) treatment

Topic
Documentation
Status
Quality Impact
High

In SwapAndXCall contract, swapAndXCall() functions do not document the special considerations on native assets.

Consider adding natspec comments explaining the possible use cases of address(0) for both _toAsset and _fromAsset in swapAndXCall().

I-1

Optimistic mode increases centralization risks

Topic
Trust Model
Status
Acknowledged
Impact
Informational

By enabling the optimistic mode, the propagation of messages to domains will completely bypass the native AMBs of each domain. A unique off-chain agent will be responsible for proposing aggregate roots and propagating them to every domain, concentrating the on-chain aggregation into one centric point.

Watchers will still execute their duties as protection against fraudulent messages but, at the moment, all the execution and safeguard controls will be centralized in Connext’s off-chain components.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Connext team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.