We audited the across-protocol/contracts repository.
The scope consisted of five parts, listed below.
In scope were the following files, audited at commit 5a0c67c:
contracts/
├── chain-adapters/
│ ├── Router_Adapter.sol
│ ├── ForwarderBase.sol
│ ├── Ovm_Forwarder.sol
│ ├── Arbitrum_Forwarder.sol
│ └── l2/
│ ├── WithdrawalHelperBase.sol
│ ├── Ovm_WithdrawalHelper.sol
│ └── Arbitrum_WithdrawalHelper.sol
└── libraries/
└── CrossDomainAddressUtils.sol
In addition, we audited all the changes made to the contracts/SpokePool*.sol files in PR #629 until commit 6e86b70.
In scope were the following files, audited at commit 5a0c67c:
contracts/
└── chain-adapters/
├── ZkStack_Adapter.sol
└── ZkStack_CustomGasToken_Adapter.sol
In scope were the changes made to the following files in PR #639 until commit 7641fbf:
contracts/
├── SpokePool.sol
├── erc7683/
│ ├── ERC7683Across.sol
│ ├── ERC7683OrderDepositor.sol
│ └── ERC7683OrderDepositorExternal.sol
└── interfaces/
└── V3SpokePoolInterface.sol
In scope were the changes made to the following files in commit 108be77:
contracts/
├── SpokePool.sol
├── erc7683/
│ ├── ERC7683.sol
│ ├── ERC7683Across.sol
│ ├── ERC7683OrderDepositor.sol
│ └── ERC7683OrderDepositorExternal.sol
└── interfaces/
└── V3SpokePoolInterface.sol
In scope were the changes made to the following files in PR #646 until commit 51c45b2 and PR #647 until commit d4416cd:
contracts/
├── WorldChain_SpokePool.sol
└── chain-adapters/
└── WorldChain_Adapter.sol
Across is an intent-based, cross-chain bridging protocol that allows users to quickly transfer their tokens between different blockchains. For more details on how the protocol works, please refer to one of our previous audit reports.
Until now, the Across protocol only supported blockchains that communicated with Ethereum directly, such as L2 networks. In order to communicate with SpokePools deployed on other blockchains, the HubPool deployed on Ethereum used adapters which contained the logic allowing it to send cross-chain messages. However, it was not able to communicate with blockchains which communicate with Ethereum indirectly, via L2 networks. In this audit report, such blockchains will be referred to as L3 blockchains, and the newest contracts added to the repository introduce support for them.
In order to support L3 blockchains, two sets of contracts have been added:
Forwarder contracts are meant to be deployed on L2 blockchains, in between Ethereum and L3 blockchains. They are designed to pass all the messages received from Ethereum to target L3 blockchains. Each forwarder contract is able to handle communication with many different L3 blockchains. As such, it is sufficient to deploy only one of them for each L2.
The communication between Ethereum and forwarder contracts is possible by using Router_Adapter contracts. The Router_Adapter is a special type of adapter that wraps the messages designed for SpokePools so that they are first passed to the forwarders on L2s and then sent to the final targets on L3s. It is necessary to deploy one Router_Adapter on Ethereum per each supported L3 blockchain. This design allows for treating L3 blockchains like L2 blockchains inside the HubPool as the Router_Adapter contracts handle the entire L3-relevant logic and have exactly the same interface as existing L2 adapters.
While the forwarder contracts enable L1->L3 communication, the Withdrawal helper contracts allow for communication in the opposite direction. They are responsible for passing the tokens that they receive from L3 blockchains to the HubPool on Ethereum.
Two new adapters have been introduced: ZkStack_Adapter and ZkStack_CustomGasToken_Adapter. ZkStack_Adapter provides support for the ZkStack blockchains that use ETH as the gas token. On the other hand, ZkStack_CustomGasToken_Adapter has been designed to work with the remaining ZkStack blockchains. Both contracts make it possible to send custom messages and transfer tokens to the L2 targets, similar to the existing L2 adapters.
Previously, the deposit IDs inside the SpokePool contract were calculated as the total number of deposits made. However, this design did not allow the relayers to perfectly predict the relay hashes that they would have to provide in order to fill deposits. This is because each deposit ID could change if other deposits had been made before it.
The current design allows for predicting deposit IDs by allowing the depositors to specify the nonce. This nonce is then used to create a deterministic deposit ID utilizing the keccak256 hash function. The depositors are responsible for not reusing the same nonces, which could lead to deposit ID collisions inside the SpokePools.
New changes have been proposed for the ERC-7683 standard. In this regard, two new order types have been introduced: GaslessCrossChainOrder, designed to be created off-chain, and OnchainCrossChainOrder, which could be utilized directly on-chain. Structs representing both types can contain the implementation-specific data which is then used in order to construct the ResolvedCrossChainOrder struct, containing the information required for the order fillers. This struct must be emitted in an event whenever any type of order is opened.
The changes introduced to the contracts in this part of the scope implement the new requirements of the ERC-7683 standard.
World Chain implements Circle's bridged USDC standard, which allows for upgrading the bridged USDC to the native USDC emitted by Circle, in the future. Because of this, both L1 and L2 standard bridges cannot be used for USDC deposits and withdrawals.
The pull requests in this part of the scope make use of the special USDC bridges deployed on Ethereum and World Chain in order to correctly bridge USDC tokens between the HubPool and the WorldChain_SpokePool.
The Across protocol depends on many different external components, such as bridges and messaging mechanisms between different blockchains. Moreover, this audit has been restricted only to a part of the entire codebase. As a result, the audit was conducted under certain trust assumptions.
Throughout the audit, we assumed that all the contracts that the in-scope contracts interact with work correctly. In particular, we assumed that the bridges work as expected and correctly bridge assets between blockchains, and that view functions invoked on the HubPool and SpokePool contracts, such as tokenBridges, remoteL1Tokens, and poolRebalanceRoute return correct results. We also assumed that only assets supported both by the bridges and the target blockchains would be bridged.
Moreover, we assumed that both the L1 adapters used by the Router_Adapter and the L2 adapters used by the forwarder contracts work as expected. In particular, we assumed that they correctly implement the AdapterInterface, correctly validate provided token pairs to relay, and correctly bridge tokens and send messages across blockchains.
We also assumed that all the contracts would only be deployed on the blockchains that they are designed for. For instance, the Ovm_WithdrawalHelper contract will not work correctly on OVM blockchains for which there exist tokens that require custom logic for bridging, different from the one contained in the _bridgeTokensToHubPool function of the Ovm_SpokePool contract. For example, this is the case with Optimism and Blast. As such, we assumed that Ovm_WithdrawalHelper would not be deployed on such blockchains.
Furthermore, it was assumed that every ZkStack chain being used was configured properly, which, in particular, means that the l2TransactionBaseCost function used for estimating the transaction cost returns correct values. It includes situations where a custom gas token has a custom number of decimals, in which case we assume that the base token nominator and base token denominator parameters are configured correctly so that proper scaling is applied when estimating the amount of L2 gas tokens to be charged as gas expenditure. It is also worth noting that the current implementation of the adapters always uses the shared bridge exposed in the BridgeHub. It might be possible for certain tokens to require a bridge to be used that is different from the shared bridge. We assumed that such tokens would not be bridged using existing adapters and that only tokens supported both by the shared bridge and by the target L2 networks would be bridged.
It was also assumed that all the in-scope contracts would be initialized and configured correctly. In particular, it is assumed that the parameters related to gas calculations on ZkStack blockchains, such as L2_GAS_LIMIT and L1_GAS_TO_L2_GAS_PER_PUB_DATA_LIMIT, would be set to such values that allow for the correct execution of all transactions on each supported blockchain.
Multiple newly introduced privileged roles are also within the scope of this audit:
ERC7683OrderDepositorExternal contract. This account is capable of changing the ERC-7683 destination settler used by the ERC7683OrderDepositor contract in order to emit ERC-7683 fill instructions.On top of the above, there are other privileged roles present in the contracts that have been left out of the scope of this engagement but can influence the in-scope contracts (e.g., the owner of the HubPool). For the entire list of the privileged roles present in the Across protocol, please refer to our past audits.
Ultimately, we assume that all the entities having the roles mentioned above will act responsibly, and in the best interest of the protocol and its users.
setDestinationSettlerThe ERC7683OrderDepositorExternal contract contains the setDestinationSettler function which provides a mapping of chain ID to that chain's settler contract address. This value is accessed through the _destinationSettler function of the same contract and is used by the inherited ERC7683OrderDepositor contract when constructing the fill instructions inside the _resolveFor function.
The issue is that the setDestinationSettler function has no access control and can be changed to any arbitrary address by any account. Consequently, a malicious user could set the destinationSettler address to a malicious address which is used in constructing the fill instructions. The filler on the destinationChain would need to give token approvals to the destinationSettler to execute the fill call. A malicious destinationSettler would be thus able to steal funds from the filler.
Since the ERC7683OrderDepositorExternal contract already inherits the Ownable contract, consider adding the onlyOwner modifier to its setDestinationSettler function.
Update: Resolved in pull request #733 at commit 8942780.
fill Function Performs Malformed CallThe fill function of the SpokePool contract is meant to adhere to the IDestinationSettler interface, as dictated by the latest update to the ERC-7683 specifications. The fill function is meant to internally call the fillV3Relay function in order to process the order data, and it does so by making a delegatecall to its own fillV3Relay function, passing abi.encodePacked(originData, fillerData) as the parameter.
However, the fillV3Relay function accepts two parameters, having the repaymentChainId as the second parameter. Since the call is constructed using encodeWithSelector, which is not type-safe, the compiler does not complain about the missing parameter. As an incorrect number of parameters is passed, the call to fillV3Relay will always revert when trying to decode the input parameters, breaking the entire execution flow. Moreover, the input data is encoded with abi.encodePacked the use of which is discouraged, especially when dealing with structs and dynamic types like arrays.
Consider using encodeCall instead of encodeWithSelector to ensure type safety, and providing the parameters required by the fillV3Relay function separately. In addition, consider explicitly making the SpokePool contract inherit from the IDestinationSettler interface as required by the ERC-7683 standard.
Update: Resolved in pull request #744 at commit 9f54455.
The WithdrawalHelperBase and ForwarderBase contracts are designed to be deployed on L2s and assist in moving tokens and messages to and from L3 chains. These contracts are inherited by the chain-specific contracts, currently designed for Arbitrum and OVM-based blockchains. However, none of these contracts handle ETH transfers correctly. This is because they do not contain the receive function, which causes any attempt to transfer ETH to these contracts to fail.
In the case of forwarder contracts, the lack of the receive function means that WETH transfers, which rely on unwrapping before bridging such as the transfers made through the Optimism_Adapter, will fail, leaving the ETH in the bridge until the contract can be upgraded. In the case of withdrawal helper contracts, the lack of the receive function implies that they will not be capable of unwrapping WETH in an attempt to transfer it to Ethereum. Additionally, they will not be capable of receiving ETH bridged from L3s. Moreover, while the withdrawal helper contracts contain token-bridging logic, they do not support bridging ETH. This means that even if they were capable of receiving ETH and ETH was bridged to them from L3s, it could not be routed to L1.
Consider adding a receive function to the ForwarderBase and WithdrawalHelperBase contracts to facilitate incoming ETH transfers and the unwrapping of WETH tokens during bridging. As the contracts do not support bridging ETH directly, the receive function should include logic to ensure that incoming ETH is handled correctly and can be sent on to the target chain.
Update: Resolved in pull request #725, at commit 705a276 by adding the receive function to forwarder and withdrawal helper contracts. Moreover, both contracts allow to transfer ETH out of them by wrapping it in case when WETH transfer is requested. The team stated:
We went with this approach so that we can keep the same format as our L1 adapters for L2-L3 bridging, and as our L2 spoke pools for L2-L1 withdrawals.
relayTokens Calls Made By Forwarders May FailThe Router_Adapter contract could be used as an adapter by the HubPool contract in order to send messages or tokens to the L3 blockchains. In order to send tokens to L3, this contract sends two messages to the intermediary L2 blockchain: the first one is simply a call to relayTokens which will send a specified amount of tokens to a relevant forwarder contract on L2, and the second one is a call to relayMessage which will execute the relayTokens function on the forwarder contract on L2 upon arrival. This way, a forwarder contract on L2 will be instructed to send the received tokens to L3 soon after it receives them.
However, there is no guarantee that the messages sent to the forwarder contract on L2 will be delivered in the same order that they were sent. In particular, some tokens are being sent to L2 using different channels than the ones used by messages. For example, in the case of Arbitrum, the USDC token will be bridged through the CCTP protocol, but the messages will be passed through the Arbitrum Inbox contract which is completely independent of CCTP. This may cause some messages instructing forwarder contracts to relay tokens to L3s to fail on L2s as the tokens may arrive at the L2 after an attempt to send them to L3.
Consider caching failed messages inside forwarders so that they could be re-executed by anyone in the future, possibly many at once, in a batch.
Update: Resolved in pull request #664, at commit d3e790f by caching all the messages inside the relayTokens function. It is now possible to execute cached token transfers by calling the executeRelayTokens function. Calls can be grouped together by using the multicall function of the Multicaller contract.
The ForwarderBase contract contains several functions which could only be invoked by cross-chain messages originating from crossDomainAdmin. These functions include the setCrossDomainAdmin and updateAdapter functions. The ForwarderBase contract and contracts inheriting from it are expected to be communicated with via the Router_Adapter contract deployed on L1.
However, Router_Adapter does not provide a way to call any functions of a forwarder contract other than relayMessage and relayTokens. This means that if it ever becomes necessary to change crossDomainAdmin or to change the adapter used by the forwarder contract deployed on L2, it can be only done by replacing Router_Adapter with another adapter which provides such functionality.
Consider implementing logic that allows for calling other privileged functions of forwarder contracts through the Router_Adapter contract. Alternatively, consider implementing dedicated adapters which would enable calling these privileged functions from the HubPool.
Update: Resolved in pull request #665. The team stated:
We are still deciding on how exactly we will communicate with the
ForwarderBaseandWithdrawalHelpercontracts, but, for now, we think that upgrades to these contracts will be rare. With that in mind, we have a few approaches to fix this issue:- Under the assumption that we only really need to call admin functions upon initialization of a new L3, we can call
setCrossChainContractsin the hub pool to map the L3's chain ID to the L2 forwarder address/withdrawal helper address and a relevant adapter (e.g.OptimismAdapter). This connection can then be used to configure the forwarder/withdrawal helper, after which we callsetCrossChainContractswith the L3's chain ID mapping to the correct adapter/spoke pool pair.- If we need to establish a connection (temporary or persistent), we may also want to call
setCrossChainContractsto map some function of the L3 chain ID to the forwarder/withdrawal helper contract. Otherwise (for temporary connections only). If we only need to send a single message to an L2 contract, we could also temporarily halt communication to the L3 spoke pool by callingsetCrossChainContractswith the L3 chain ID and the corresponding L2 contract.While we are still thinking of what approach we want to take, here is a PR which contain two "admin adapters" corresponding to the forwarder and withdrawal helper. These are essentially special cases of the router adapter.
Edit: One last update here, the PR we provided is a special case of a router adapter which will just communicate with the forwarder/withdrawal helper; however, we've noticed that we don't really need to have this adapter since we can just reuse other deployed adapters to send messages to the forwarder/withdrawal helper. That is, the two bullet points above still stand; just instead of using a separate admin adapter to send these messages, we can
setCrossChainContractswith the network adapter (e.g.Arbitrum_Adapter, Optimism_Adapter, etc) instead of this new admin adapter.
ERC-7683The newest changes to the ERC-7683 standard redefined the types of some variables. Particularly, the variables storing the chain ID now have the uint64 type instead of uint32, and some addresses are now stored in the bytes32 type. However, the variables declared inside the AcrossOrderData struct still have the old types. In particular, the destinationChainId member is of type uint32 and the recipient member is of the address type.
Consider changing the types of all affected variables so that they are in compliance with the latest version of the ERC-7683 standard.
Update: Resolved in pull request #746 at commit 9eebe3d.
The ERC7683OrderDepositorExternal contract implements the _deposit function to finalize the creation of an Across V3 deposit. To do so, the function calls the safeIncreaseAllowance function on the inputToken specified in the order details. This mechanism will work with any token under the assumption that the entire allowance will be spent by the SpokePool in the depositV3 function call. The safeIncreaseAllowance function is also used in the ZkStack_Adapter and the ZkStack_CustomGasToken_Adapter contracts, along with some other adapters like the ZkSync_Adapter which are out of scope for this audit.
However, if for any reason, the entire allowance is not used after the approval, any further attempt to safeIncreaseAllowance with tokens that prohibit any approval change from non-zero to non-zero values, like USDT, will ultimately fail. As an example of a real impact, the second example of issue M08 will likely produce a scenario in which subsequent calls with USDT as the custom gas token will fail, thus blocking the entire ZkStack_CustomGasToken_Adapter's functionality.
Consider using the forceApprove function of the SafeERC20 library to be compatible with tokens that revert on approvals from non-zero to non-zero values.
Update: Resolved in pull request #734 at commit ea59869.
The _computeETHTxCost function of the ZkStack_Adapter contract is used to estimate transactions' cost on L2. Whenever a message is sent from L1 to L2, this estimated transaction cost is transferred out of the HubPool to the native L1 inbox. Any excess value is supposed to be refunded to the L2_REFUND_ADDRESS on L2, which is expected to be an address under the control of the Across team. However, tx.gasprice, which is used for transaction cost estimation, is a parameter that can be manipulated by the initiator of the transaction. This opens up an attack vector whereby a malicious user can inflate the tx.gasprice in order to transfer ETH from the HubPool to an L2 network.
In order to perform the attack, the attacker could invoke the executeRootBundle function of the HubPool, causing the HubPool to call the relayMessage function of the adapter. Since tx.gasprice is directly used for the required gas fee calculation, the attacker can set it to a value for which the estimated fee will be equal to the entire HubPool's ETH balance. HubPool will then transfer the ETH to L2. A similar attack could be used in order to transfer a custom gas token from the HubPool using the ZkStack_CustomGasToken_Adapter contract.
While it is normally very expensive for an attacker to inflate tx.gasprice parameter for the entire transaction (as they have to cover the gas fee), they can receive back almost the entire invested ETH amount if they are a validator for the block in which the attack is executed.
Consider limiting the maximum tx.gasprice which can be used for gas fee calculation inside the _computeETHTxCost and the _pullCustomGas functions.
Update: Resolved in pull request #742 at commit dc4337c by limiting the maximum tx.gasprice which can be used for gas fee calculation.
permitWitnessTransferFromThe PERMIT2_ORDER_TYPE variable stores the witness data type string, which is supposed to be passed as the witnessTypeString parameter to the permitWitnessTransferFrom function of the Permit2 contract. As such, this variable is supposed to define the typed data that the witness parameter passed to that function was hashed from. However, it instead specifies that the witness parameter has been hashed from the CrossChainOrder type, whereas in reality, it was hashed from the GaslessCrossChainOrder type.
Moreover, the witness parameter specified is incorrect as the orderDataType member of the GaslessCrossChainOrder struct is not taken into account when calculating it. The same is true for the exclusiveRelayer and depositNonce members of the AcrossOrderData struct, which are not included in the calculation of its hash. Furthermore, the CROSS_CHAIN_ORDER_TYPE variable used to create the witness contains an incorrect encoding of the GaslessCrossChainOrder struct as the originChainId member is specified to be of type uint32 instead of uint64.
Consider correcting the errors described above in order to maintain compliance with EIP-712 and Permit2.
Update: Resolved in pull request #745 at commit b1b5904 and at commit 98c761e. The team stated:
There have been some changes to ERC7682 during the audit, so these fixes are split between two commits. The first commit (and the attached PR) addresses the first paragraph and all of the second paragraph except for
depositNonceand swappingoriginChainIdto a uint64. ThedepositNoncehas since been removed, and the origin chains now match as a result of the second commit.
ZkStack_CustomGasToken_Adapter Will FailIn order to bridge tokens from Ethereum to a ZkStack blockchain using a custom gas token, the relayTokens function of the ZkStack_CustomGasToken_Adapter contract can be used. In case the token to bridge is WETH, the token is first converted to ETH, and then that ETH is bridged using the requestL2TransactionTwoBridges function of the BridgeHub contract. The requestL2TransactionTwoBridges function then calls the bridgeHubDeposit function of the second bridge with the ETH amount specified by the caller.
However, the bridgeHubDeposit function requires that the deposit amount specified equals 0 in case when ETH is bridged, yet it is specified as a nonzero amount inside the relayTokens function of the adapter. This will cause any attempt to bridge WETH to L2 to revert.
In cases where WETH is being bridged, consider setting the amount to be used in the second bridge's calldata to 0.
Update: Resolved in pull request #743 at commit 0bdad5b.
The relayTokens function of the ZkStack_Adapter is intended to facilitate transfers from the HubPool on the Ethereum Mainnet to ZkStack chains, having ETH as the gas token, via the BridgeHub. The relayTokens function of the ZkStack_CustomGasToken_Adapter contract enables this functionality for chains with a custom gas token.
For ZkStack_Adapter, when transferring WETH, the relayTokens function unwraps the WETH into ETH and sends this amount to the BridgeHub as part of the requestL2TransactionDirect call to the BridgeHub contract. For ETH transfers, the requestL2TransactionDirect function checks that the value sent along with the call is equal to the mintValue of the request. However, in the relayTokens function, the value that is sent is the amount + txBaseCost while mintValue is only the txBaseCost.
The requestL2TransactionDirect function requires the mintValue field to be equal to the msg.value of the call, but the mintValue will always only be set as the txBaseCost, meaning that the check will always fail. In addition, the l2Value for the requestL2TransactionDirect is fixed at 0 inside the contract, meaning that the l2Contract will never receive any ETH. Consequently, transfers of WETH to ZkStack chains that use ETH as a base token cannot succeed.
There is a similar issue present in the relayTokens function for ZkStack chains with custom gas tokens. In cases where the token to be bridged is the gas token, only the amount needed to cover the transaction cost will be transferred as the amount argument of the relayTokens function is ignored.
Consider amending the implementation in accordance with the guidelines for L1 to L2 bridging on ZkStack chains. For the ZkStack_Adapter and ZkStack_CustomGasToken_Adapter contracts, this will require setting the mintValue as the total value that is transferred with the call to requestL2TransactionDirect, which is txBaseCost + amount. The l2Value should also be amended to reflect the value to be transferred to the l2Contract.
Update: Resolved in pull request #739 at commit 8a05161.
crossDomainAdmin Will Prohibit Pending OperationsThe ForwarderBase contract has the setCrossDomainAdmin function which changes the crossDomainAdmin state variable. This variable is used to give access control to almost all functions within the contract, including those that are meant to be used to complete L1->L3 message passing and asset transfers. When calling setCrossDomainAdmin, it is assumed that there are no outstanding operations that need to be passed to another chain. This is because when operations arrive at L2, the original sender of those is the old crossDomainAdmin and will be blocked from continuing further.
In order to raise awareness about such behavior, consider documenting this edge case in the setCrossDomainAdmin function.
Update: Resolved in pull request #729 at commit 4ba3439.
Orders inside the ERC7683OrderDepositor contract may be resolved either by the _resolve function or by the _resolveFor function. Both functions receive an order and convert it to the ResolvedCrossChainOrder struct, which contains the minReceived member of the Output[] type. However, inside both _resolve and _resolveFor functions, the minReceived member is initialized using the block.chainId cast to uint32, although the Output.chainId member is of type uint64. This means that the code will revert for blockchains with chainID not fitting into uint32, although it should work for all blockchains with chain IDs lower than type(uint64).max.
Consider casting block.chainId to uint64 instead of uint32 when initializing ResolvedCrossChainOrder.minReceived.
Update: Resolved in pull request #736 at commit eee4a75. The team stated:
Since the audit commit hash, there has been more suggestions for ERC7683, so some of the fields to structs like
ResolvedCrossChainOrderhave changed. For example, now, the chain ID is represented as a uint256 (see this). In short, the casting has now just been removed altogether in the proposed PR.
In order to bridge ERC-20 tokens to L2s, the ZkStack_Adapter and ZkStack_CustomGasToken_Adapter contracts first approve the relevant amount of tokens to the SHARED_BRIDGE and then invoke the requestL2TransactionTwoBridges function of the Bridgehub contract, where they specify the second bridge to be used as BRIDGE_HUB.sharedBridge(). The requestL2TransactionTwoBridges function then calls the bridgehubDeposit function on the second bridge, which transfers tokens from the contract which initially called the Bridge Hub.
However, the token approval given by the adapters is always for the immutable SHARED_BRIDGE address, yet the BRIDGE_HUB.sharedBridge() address, specified as the second bridge, returns the current value of the sharedBridge variable. Although it is unlikely that the variable will change in the future, it is nonetheless possible, and if that happens, none of the ZkStack adapters will be able to bridge tokens as the allowance will be given to the previous sharedBridge address.
Consider removing the SHARED_BRIDGE variable and always accessing the sharedBridge variable through BRIDGE_HUB.sharedBridge().
Update: Acknowledged, not resolved. The decision has been made to redeploy the adapters in case when the sharedBridge variable changes and not to call BRIDGE_HUB.sharedBridge() in order to save gas. BRIDGE_HUB.sharedBridge() calls have been replaced with the SHARED_BRIDGE variable accesses in the commit 3d260d7 in order to reduce gas cost further. The team stated:
This is true, but we think this may be one of the few where we may want to have this behavior. This is because we call these adapters often, and since they are deployed on L1, they can become fairly expensive. For this reason, it is particularly important to minimize the gas cost whenever possible, and this is one such shortcut we take. [...] Particularly, in the event the bridge _does change, the adapter calls would just revert, and we would need to redeploy. To be clear, if the shared bridge does change, we will need to deploy a new adapter. The hope is that in the long run, the cost of redeploying will be cheaper than making an extra call on the adapter for each new transaction._
The _setCrossDomainAdmin internal function of the ForwarderBase contract is called inside the setCrossDomainAdmin external function that is only callable by the admin. However, both functions implement the same logic and emit the same events.
Consider removing duplicated logic from the external function's body and keep the logic and event emission inside the internal definition.
Update: Resolved in pull request #728 at commit 1ecbb3f.
The updateAdapter function of the ForwarderBase contract allows for a new destination chain ID to be linked with the appropriate adapter for cross-chain forwarding. However, should a chain become unsupported at some time in the future, the target of the chainId cannot be set to address(0), nor can it be deleted from the chainAdapters mapping.
Consider adding logic to remove an adapter for a given destination chain.
Update: Resolved in pull request #728 at commit b621adf.
The AcrossDestinationFillerData struct declared in ERC7683Across.sol is not used anywhere in the codebase.
To improve the overall clarity, intentionality, and readability of the codebase, consider either using or removing any currently unused structs.
Update: Resolved in pull request #744 at commit 9f54455 by using the AcrossDestinationFillerData struct in the fill function of the SpokePool contract.
Throughout the codebase, multiple instances of misleading documentation were identified:
L2_TARGET contract implements the AdapterInterface, but in reality, it implements the ForwarderInterface._crossDomainAdmin parameter of the constructor.ZkStack_Adapter and ZkStack_CustomGasToken_Adapter contracts is incorrect as it points to a nonexistent location.Consider correcting the aforementioned comments to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #728 at commit 702f21e.
The SafeERC20 library has been imported in the WithdrawalHelperBase contract. This library is used for IERC20 functions, but there are no such functions in this abstract contract. Since this library has also been imported in the derived Arbitrum_WithdrawalHelper and Ovm_WithdrawalHelper contracts, it may be removed from WithdrawalHelperBase.sol without consequence.
Consider removing the unused import of the SafeERC20 library from the WithdrawalHelperBase contract.
Update: Resolved in pull request #728 at commit 98100c3.
Throughout the codebase, multiple instances of functions lacking proper docstrings were identified. One example is the _bridgeTokensToHubPool function, declared inside WorldChain_SpokePool.sol.
Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #647 at commit 08534da.
Within the ForwarderBase.sol file, the chainAdapters state variable lacks an explicitly declared visibility.
For improved code clarity, consider always explicitly declaring the visibility of state variables, even when the default visibility matches the intended visibility.
Update: Resolved in pull request #728 at commit 467a207.
Throughout the codebase, multiple opportunities for comment improvement were identified:
Consider implementing the aforementioned comment improvement suggestions in order to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #728 at commit 30b1ecb and in pull request #646 at commit f39418a.
Throughout the codebase, multiple instances of events without indexed parameters were identified:
SetXDomainAdmin event in WithdrawalHelperBase.solZkStackMessageRelayed event in ZkStack_Adapter.solZkStackMessageRelayed event in ZkStack_CustomGasToken_Adapter.solTo improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.
Update: Resolved in pull request #728 at commit e13dd1f.
Throughout the codebase, multiple opportunities for improved naming were identified:
CROSS_CHAIN_ORDER_TYPE variable could be renamed to GASLESS_CROSS_CHAIN_ORDER_TYPE.CROSS_CHAIN_ORDER_EIP712_TYPE variable could be renamed to GASLESS_CROSS_CHAIN_ORDER_EIP712_TYPE.CROSS_CHAIN_ORDER_TYPE_HASH variable could be renamed to GASLESS_CROSS_CHAIN_ORDER_TYPE_HASH.exclusivityDeadline parameter of the _callDeposit function could be renamed to exclusivityPeriod in order to be consistent with the AcrossOrderData struct. We also recommend ensuring that the code in the SpokePool contract is consistent with this change (i.e., the parameter is indeed treated as a period of time and not as a timestamp).Consider renaming the variables specified above to improve code readability.
Update: Resolved in pull request #728 at commit 03a0d1a.
The ERC7683Across.sol file name does not match the ERC7683Permit2Lib library name.
To make the codebase easier to understand for developers and reviewers, consider renaming the file to match the library name.
Update: Resolved in pull request #728 at commit 3df9450.
In the ZKStack adapters, address(1) is used to represent ETH when it is used as the gas token. In both contracts, this value is used several times and could be declared as a constant, similar to the ETH_TOKEN_ADDRESS constant which is used in the Bridgehub contract.
In order to improve readability of the codebase, consider declaring address(1) as a constant with a descriptive name.
Update: Resolved in pull request #728 at commit 813bf95.
In order to clearly identify the Solidity version with which the contracts will be compiled, pragma directives should be fixed and consistent across file imports. The Ovm_WithdrawalHelper.sol file has the pragma directive pragma solidity ^0.8.0; and imports the WithdrawalHelperBase.sol file, which has a different pragma directive - ^0.8.19.
The intention seems to be to fix the version to be lower than v0.8.20, which is where the PUSH0 opcode has been introduced. However, ^0.8.19 will allow any version greater than or equal to that (and lower than v0.9.0) to be used. In addition, the Arbitrum_WithdrawalHelper contract has a comment that states that Arbitrum only supports v0.8.19, but the referenced documentation states differently and indeed shows that Arbitrum now supports PUSH0 opcode.
Consider reviewing the pragma directives to make them consistent. If there is any reason to believe that the version should be less than the v0.8.20, use <= instead of ^.
Update: Resolved in pull request #728 at commit c335be2.
The ERC7683OrderDepositor contract does implement the IOriginSettler interface declared in ERC-7683. However, there are several inconsistencies between the parameters' names originally specified in the interface and the names used in the ERC7683OrderDepositor contract:
fillerData parameter's name of the openFor function does not match the name from the IOriginSettler interface (originFillerData).resolve and resolveFor functions should be named in the IOriginSettler as they are present in the implementation.Consider making the interface and implementation consistent with each other in order to improve code readability.
Update: Partially resolved in pull request #728 at commit 88ae26a. The team stated:
We ended up only addressing the first bullet point of this issue. The motivation for this is that we want
resolveandresolveForto not define a return variable on the interface level. The commit attached addresses the first point, but not the second point.
The predictability of the relay hashes enables the fillers to fill the deposits on target chains before they are created on origin chains. When a deposit is created on the origin chain, the current block timestamp is validated, such that the deposit has to be made with the fill deadline in the future.
However, it is possible to have a scenario in which a pre-fill has happened for a deposit, but the deposit has not been made until the fill deadline has passed. It could for example happen as a result of a high blockchain congestion or a blockchain halt. In such a case, it will not be possible to create a deposit anymore, which results in a loss of assets for the pre-filler.
Update: The team resolved this in pull request #870 at commit 3b21fea, allowing to make deposits after the fill deadline has passed. As a side effect for the fix, it is now possible to create a deposit which has not been pre-filled, after the fill deadline. It would result in temporary transfer of assets from the depositor, but the assets will be refunded afterwards.
Communication from L1 to L3 will require the use of adapter contracts on the intermediary L2. The Across team has stated that the adapter contracts for these calls will be based on the current adapter contracts, but there are key differences that should be kept in mind.
The Router_Adapter contract enables the sending of cross-chain messages from L1 to L2 and then on to L3. For example, in the Arbitrum_Adapter contract, the relayMessage and relayTokens functions include the required gas for L2 execution inside the function logic. This presupposes that the calling contract, the HubPool on L1, holds enough ETH to cover this gas cost. This is enforced by a minimum balance check inside the relayMessage and the relayTokens functions.
However, on the L2, the forwarder contract is the caller to the adapter contract. The relayMessage function of the ForwarderBase contract does not have a check to ensure that the required amount of the gas token is present to perform the L2 to L3 call. Furthermore, there does not currently appear to be any automated logic present in the protocol to provide the forwarder contracts with the assets they may need in order to pay for gas for L3 transactions.
In light of the above, it is recommended to ensure that the adapter contracts on L2 are tailored to the target L3, taking into account the target chain's gas token and bridging logic. It is also recommended to ensure that the forwarder contracts always have enough assets to be able to successfully execute both relayMessage and relayTokens functions.
In order to enhance the quality and security of the codebase, it is necessary to implement a comprehensive testing suite. This should include both unit tests, which test each component in isolation, and integration tests, which ensure that the interactions between different parts of the system and between the system and external components lead to the desired outcomes. Integration tests can be implemented by forking a blockchain at a specific block and interacting with the contracts already deployed and configured, such as bridges. Throughout the audit, multiple issues were identified which indicated that the current testing suite is not sufficient.
Consider implementing a thorough testing suite for the codebase. This will help ensure better code quality and greatly reduce the number of issues present in the codebase in the future.
The audited codebase introduced support for L3 blockchains in the Across protocol and implemented new changes as laid out in the ERC-7683 standard. In addition, several new adapters have been added, introducing support for new blockchains and modifying the logic related to calculating the IDs of deposits inside SpokePools.
Given the complexity of the protocol and the number of external components it depends on, we believe that the codebase would highly benefit from implementing integration tests, which would allow for the identification of many errors related to improper use of bridges and messaging mechanisms. We believe that the majority of issues with medium and higher severity that were identified during this engagement could have been easily detected during development by a proper integration testing suite which goes beyond mocking external components. Moreover, given that the ERC-7683 standard is currently undergoing modifications, and it is likely that new changes will be introduced to it after the audit, we recommend ensuring that the code adheres to the final version of the standard.
The Risk Labs team has been consistently helpful throughout the engagement, promptly answering all our questions and thoroughly explaining the protocol's details.