Security Audit
July 12, 2023
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
96d5d965081d8391466b9d0ff4b9a8bebcf131b0
0152ca9966abc3f183404ba78ae84fb480cbb9fa
12def02d68c4f6779e2c209549245607bef2e40b
ac7932652d506615a58a9f21e2819117ecd4428d
Specifically, we audited the following contracts within the monorepo repository:
./packages/deployments/contracts/contracts/messaging/
Contract | SHA256 |
---|---|
./Conductor.sol |
|
./MerkleTreeManager.sol |
|
./RootManager.sol |
|
./connectors/Connector.sol |
|
./connectors/ConnectorManager.sol |
|
./connectors/GasCap.sol |
|
./connectors/HubConnector.sol |
|
./connectors/SpokeConnector.sol |
|
./connectors/mainnet/MainnetSpokeConnector.sol |
|
./connectors/optimism/BaseOptimism.sol |
|
./connectors/optimism/OptimismHubConnector.sol |
|
./connectors/optimism/OptimismSpokeConnector.sol |
|
./connectors/optimism/lib/Types.sol |
|
./connectors/polygonzk/BasePolygonZk.sol |
|
./connectors/polygonzk/PolygonZkHubConnector.sol |
|
./connectors/polygonzk/PolygonZkSpokeConnector.sol |
|
./connectors/zksync/ZkSyncHubConnector.sol |
|
./connectors/zksync/ZkSyncSpokeConnector.sol |
|
./connectors/consensys/ConsensysBase.sol |
|
./connectors/consensys/ConsensysHubConnector.sol |
|
./connectors/consensys/ConsensysSpokeConnector.sol |
|
./interfaces/IConnector.sol |
|
./interfaces/IConnectorManager.sol |
|
./interfaces/IHubConnector.sol |
|
./interfaces/IMessageRecipient.sol |
|
./interfaces/IOutbox.sol |
|
./interfaces/IRootManager.sol |
|
./interfaces/ambs/optimism/IOptimismPortal.sol |
|
./interfaces/ambs/optimism/OptimismAmb.sol |
|
./interfaces/ambs/polygonzk/IBridgeMessageReceiver.sol |
|
./interfaces/ambs/polygonzk/IPolygonZkEVMBridge.sol |
|
./interfaces/ambs/zksync/IL1Messenger.sol |
|
./interfaces/ambs/zksync/IZkSync.sol |
|
./libraries/DomainIndexer.sol |
|
./libraries/SnapshotId.sol |
|
Snapshot Library Improvement (PR4060)
./packages/deployments/contracts/contracts/core/connext/helpers/
Contract | SHA256 |
---|---|
./RootManager.sol |
|
./connectors/SpokeConnector.sol |
|
./libraries/SnapshotId.sol |
|
Specific changes (PR4061) for the Relayer Proxy contracts
./packages/deployments/contracts/contracts/core/connext/helpers/
Contract | SHA256 |
---|---|
./RelayerProxy.sol |
|
./RelayerProxyHub.sol |
|
And, specifically, we audited the following contracts within the chain-abstraction-integration repository:
./contracts/
Contract | SHA256 |
---|---|
./destination/xreceivers/AuthForwarderXReceiver.sol |
|
./destination/xreceivers/ForwarderXReceiver.sol |
|
./destination/xreceivers/Swap/SwapForwarderXReceiver.sol |
|
./origin/Swap/SwapAndXCall.sol |
|
./shared/Swap/OneInch/OneInchUniswapV3.sol |
|
./shared/Swap/Pancakeswap/PancakeV3Swapper.sol |
|
./shared/Swap/SwapAdapter.sol |
|
./shared/Swap/Uniswap/UniV2Swapper.sol |
|
./shared/Swap/Uniswap/UniV3Swapper.sol |
|
./shared/Swap/interfaces/IPancakeSmartRouter.sol |
|
./shared/Swap/interfaces/ISmartRouter.sol |
|
./shared/Swap/interfaces/ISwapper.sol |
|
./shared/Swap/interfaces/IWETH9.sol |
|
./shared/Swap/libraries/BytesLib.sol |
|
Click on an issue to jump to it, or scroll down to see them all.
addBypass()
is enabled to be bypassable
directSwapperCall
allows arbitrary calls to any contract
exactSwap()
does not send any value to swapETH
call
swap()
function does not consider _fromAsset == _toAsset
OneInchUniswapV3
can’t receive native assets
OneInchUniswapV3
incorrect swapETH
implementation
exactSwap()
may get lost
swap()
may get lost
swapAndXCall()
may get lost
ZkSyncHubConnector
fee refunds will be sent to L2 address of RootManager
OneInchUniswapV3
may get lost
block.timestamp
allows pending transactions to be maliciously executed
addSwapper()
and removeSwapper()
do not check for the current state values nor emit events
_setupAndSwap()
references incorrect variable
l2GasPerPubdataByteLimit
is hardcoded
ForwarderXReceiver
and AuthForwarderXReceiver
_originSender
address when it is added instead of when it used
allowedSwappers
assignment
removeOrigin()
renounceOwnership
logic is not consistent across contracts
SwapAndXCall
functions do not document address(0)
treatment
We quantify issues in three parts:
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. |
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:
_sendMessage()
function when encodedData.length
is not 0encodedData.length
is not 32_sendMessage()
when encodedData.length
is not 0_getGasFromEncoded()
when encodedData.length
is not 32_sendMessage()
when encodedData.length
is not 96As 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:
finalizedOptimisticAggregateRoot
variable in _optimisticPropagate()
and _slowPropagate()
functions, orpropagate()
and finalizeAndPropagate()
function to relayers only.addBypass()
is enabled to be bypassable
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:
delay()
) the addBypass
function using the addBypass.selector
and address
of the conductor as _target
.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:
_target
in addBypass
.directSwapperCall
allows arbitrary calls to any contract
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:
_swapper
parameter value to entries in the allowedSwappers
mapping.exactSwap()
does not send any value to swapETH
call
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:
msg.value
to the swapETH
call.swap()
function does not consider _fromAsset == _toAsset
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:
if
statements and sending the assets back to the caller.OneInchUniswapV3
can’t receive native assets
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:
receive()
function.OneInchUniswapV3
incorrect swapETH
implementation
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:
WETH9
as an intermediate swap, similar to the other swapper contracts or,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:
proposeAggregateRootCooldown
is never set, and there are no means to update itlastProposeAggregateRootAt
is never setAggregateRootCooldownChanged
event is missing, which is necessary to have consistent implementationAs a result, relayers are able to propose aggregate roots with any frequency without restriction.
Remediations to Consider:
RelayerProxyHub
contract functionality to have proper cooldown functionality for proposing aggregate root, orexactSwap()
may get lost
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:
exactSwap()
function, ormsg.value
is non-zero.swap()
may get lost
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:
swap()
function, ormsg.value
is non-zero.swapAndXCall()
may get lost
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:
_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.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:
swapAndXCall()
function that uses the output asset for paying the relayer fee, or_setupAndSwap()
function into two separate functions to appropriately handle edge cases for each higher-level variant of the swapAndXCall()
.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:
SafeERC20
and its safeTransfer()
wrapper for a more secure transfer of ERC20 tokens.ZkSyncHubConnector
fee refunds will be sent to L2 address of RootManager
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:
_refundAddress
parameter with an address owned by Connext to use these potential refunded assets.OneInchUniswapV3
may get lost
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:
msg.value
is equal to the _amountIn
if _fromAsset == address(0)
.block.timestamp
allows pending transactions to be maliciously executed
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:
deadline
parameter to all functions which perform a swap on the user's behalf, such as swap()
and swapETH()
.It is difficult to decide
deadline
on the destination side. BecausexReceive
function is called byconnext
contract via relayer.
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:
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:
originRegistry[_originDomain]
is already set and reverting if that is the case. Otherwise, add a new origin.addSwapper()
and removeSwapper()
do not check for the current state values nor emit events
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.
_setupAndSwap()
references incorrect variable
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:
amountOut
instead of _amountIn
.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:
X
weeks/days after the delay
).l2GasPerPubdataByteLimit
is hardcoded
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:
_l2GasPerPubdataByteLimit
on the client side and using it in the function parameters.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.
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.
ForwarderXReceiver
and AuthForwarderXReceiver
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);
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);
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
.
_originSender
address when it is added instead of when it used
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.
In the AuthForwarderXReceiver
contract, consider calling addOrigin()
in the constructor while adding the initial set of allowed origins instead of reimplementing the same functionality.
allowedSwappers
assignment
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.
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.
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.
removeOrigin()
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);
}
_signature
param in proposeAggregateRootKeep3r()
SnapshotRootSaved
eventexactSwap()
and directSwapperCall()
ISwapper:swap()
xReceive
, prepareAndForward
, _prepare
, _forwardFunctionCall
functionsxReceive
, prepareAndForward
, _prepare
, _forwardFunctionCall
functions_prepare
swap()
swap()
swap()
renounceOwnership
logic is not consistent across contracts
Although renounceOwnership
is disabled in all current contracts, there are different logic implementations:
rennounceOwnership
error.Consider following a consistent implementation approach in all contracts.
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.
SwapAndXCall
functions do not document address(0)
treatment
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()
.
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.
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.