Across Protocol OFT Integration Differential Audit

OpenZeppelin Security
 | July 3, 2025

Table of Contents

Summary

Type
Cross-Chain
Timeline
From 2025-05-19
To 2025-05-23
Languages
Solidity
Total Issues
32 (11 resolved, 4 partially resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
1 (0 resolved)
Medium Severity Issues
7 (4 resolved, 1 partially resolved)
Low Severity Issues
13 (1 resolved, 3 partially resolved)
Notes & Additional Information
11 (6 resolved)

Scope

OpenZeppelin performed a differential audit of the across-protocol/contracts repository at commit c5d75410 against base commit c88ac8ad. Specifically, the changes highlighted in this diff were the main subject of this audit.

In addition, the following two pull requests were also audited:

In scope were the following files:

 contracts
├── AdapterStore.sol
├── AlephZero_SpokePool.sol
├── Arbitrum_SpokePool.sol
├── Ethereum_SpokePool.sol
├── Linea_SpokePool.sol
├── Ovm_SpokePool.sol
├── PolygonZkEVM_SpokePool.sol
├── Polygon_SpokePool.sol
├── Scroll_SpokePool.sol
├── SpokePool.sol
├── Succinct_SpokePool.sol
├── Universal_SpokePool.sol
├── ZkSync_SpokePool.sol
├── chain-adapters
│   ├── Arbitrum_Adapter.sol
│   └── Universal_Adapter.sol
├── libraries
│   ├── OFTTransportAdapter.sol
│   └── OFTTransportAdapterWithStore.sol
└── interfaces
    ├── SpokePoolInterface.sol
    └── V3SpokePoolInterface.sol

System Overview

Across is a cross-chain bridging protocol built around a central HubPool contract on Ethereum and per-chain SpokePool contracts deployed on supported networks. When a user initiates a cross-chain transfer, a Relayer (filler) observes the intent and fronts liquidity on the destination chain, enabling fast settlement. A network of Dataworkers submits proofs of these fill events, which the protocol uses to verify their validity and calculate the corresponding Relayer's reimbursement. Upon successful verification, the protocol does not reimburse the Relayer on the destination chain directly. Instead, the Relayer can decide where it wants to be refunded. The HubPool contract governs cross-chain settlement, maintains the global state, and performs final token reimbursements and accounting.

These changes introduce support for bridging LayerZero OFT (Omnichain Fungible Token) assets such as USDT0 by integrating LayerZero's messaging standard into Across's Transport Layer. The main bridging logic is found in the OFTTransportAdapter contract, which is employed in both directions:

  1. L1->L2 transfers: On the L1 side, the selected Adapters (e.g., Arbitrum_Adapter) inherit from the OFTTransportAdapterWithStore contract, which uses the global AdapterStore contract to map tokens to their corresponding OFT messengers.

  2. L2->L1 transfers: On the L2 side, each SpokePool contract (e.g., Arbitrum_SpokePool) inherits the OFTTransportAdapter contract via the base SpokePool contract and maintains its own local token-messenger mapping through a setter/getter pattern.

In both flows, the _transferViaOFT function ensures correct LayerZero fee quoting, applies a maximum allowed fee FEE_CAP, and invokes the OFT messenger's send function to relay tokens across chains. This integration allows Across to wrap LayerZero's OFT standard for cross-chain transfers within its own economic model of Relayers, fill proofs, and settlement via the Ethereum-based HubPool contract.

Note that the bridging mechanism described above is not used for sending user assets. As stated earlier, these are delivered by Relayers who front liquidity on the destination chain. Rather, bridging via the newly introduced _transferViaOFT function is part of the protocol's internal accounting that is used to rebalance liquidity across SpokePool contracts and the HubPool contract, and to reimburse Relayers by bridging tokens from L2 to the L1 HubPool contract. In special cases where a Relayer fills a transfer on L1 (e.g., for an L2->L1 user request), the reimbursement occurs entirely on L1 without using the aforementioned function.

Pull request #1031 removes legacy functionality that provided the option to set the address of outputToken to the zero address. This used to indicate that the output token is to be interpreted as the equivalent of the input token on the destination chain. The motivation for removing this functionality is the inability to determine the equivalent token for some chains. Previously the equivalence was determined through pool rebalance routes, which used to connect all supported tokens. However this is no longer the case i.e., not all tokens have a connecting pool rebalance route.

Pull request #1032 adds support for EIP-7702 delegated wallets to receive ETH, as opposed to wrapped tokens (e.g., WETH). The latter is the default token type for regular smart contracts.

Security Model and Trust Assumptions

During the audit, the following trust assumptions were identified:

SpokePool Admin

The SpokePool admin is trusted to behave honestly and responsibly. Specifically:

  • The SpokePool admin is trusted to correctly map the messenger to the token contract address, specifically through the setOftMessenger function, which allows for overwriting the values in the oftMessengers mapping without thorough validation. The risk that a malicious or honest-but-negligent admin may map the OFT messenger to the wrong token address or even to a non-token address is covered by this trust assumption.
  • It is implicitly trusted that the SpokePool contract's admin will not change the token mapping of the messenger through the setOftMessenger function while tokens are being transferred through that method and the transaction has not been mined at the origin. In such a scenario, the transfer might revert at best or cause inconsistencies.
  • The SpokePool contract's admin has full control over the relaying configuration. Specifically, it is possible to end up with a configuration in which USDC tokens are relayed over the OFT bridge (instead of over CCTP). In addition, even after the OFT bridge has been configured to relay USDC tokens, they may still get relayed over the CCTP bridge if the latter gets enabled in the meantime.

AdapterStore Admin

There is a trust assumption that the admin of AdapterStore is honest. Specifically, it is assumed that they set the IOFT messengers to valid IOFT addresses in the AdapterStore contract.

LayerZero Backend

There is implicit trust in the LayerZero backend implementation:

  • The call to the send method in the messenger returns two structs: MessagingReceipt and OFTReceipt. Of those, only the latter is checked, trusting that the operation is successful on the LayerZero side. In particular, this trust covers concerns in a scenario in which the OFT layer returns a receipt, but message delivery still fails at the destination, with the potential result of a loss of funds.
  • The quoteSend method from the LayerZero backend, called in the _transferViaOFT function call of the OFTTransportAdapter contract, is trusted to behave "properly". Specifically, it is trusted that LayerZero will not unreasonably increase the fee returned by quoteSend, for instance, to a value close to the OFT_FEE_CAP with the goal of extracting maximum economic profit from the relaying process, thereby potentially stalling execution by making it economically infeasible.

EIP-7702 Delegation Wallets

The following trust assumptions were made regarding EIP-7702 Delegations Wallets:

  • The protocol may not be compatible with implementations of EIP-7702 wallets, as these may contain logic that might not work with the current expected flow.
  • The protocol treats EIP-7702 delegated wallets as EOAs that receive ETH as opposed to regular smart contracts receiving ERC-20 tokens. Delegated wallets may not have the fallback or receive functions implemented, which will prevent them from receiving ETH. Similarly, if the wallets do not implement respective assets' hooks (such as the ones found in ERC-1155 or ERC-721) then these will not be able to receive those assets, which will result in the transaction being reverted.
  • In view of deviated expectations after two iterations of calls, it cannot be assumed that EIP-7702 delegated wallets will not change their implementation.
  • Depending on a wallet's signature mechanism, it may be possible to have replayability attacks on different chains from those wallets.
  • As opposed to a regular EOA, sending ETH or assets with hooks could initiate a reentrancy point in the protocol.

General Trust Assumptions

The following general trust assumptions were made as part of the audit:

  • If a token listed to be bridged over OFT supports pausability, it could lead to message-related issues similar to the high-severity issue (H-01) described in this report.
  • It is assumed that there is a single distinct messenger per token. If this assumption is violated, finality might be broken.
  • It is assumed that the admin is diligent and honest and never sets a wrong endpoint/chainID. If that happens, funds can be locked.
  • The protocol relies on Dataworkers and the optimistic oracles working correctly when it comes to keeping track of refunds and rebalances.

Security Model

The _unwrapwrappedNativeTokenTo function of the SpokePool contract uses the isContract function from the OpenZeppelin contracts library to check if an address corresponds to an EOA account. However, the isContract function is part of an older version of the library (4.x) and has been removed in the latest one (5.x). The use of isContract is strongly discouraged in view of the known risks listed in the library documentation (cf. sections marked "IMPORTANT"). It is worth mentioning that this also applies to the introduction of the EIP-7702 EOA wallets that behave like a contract.

Integration

The following are some important details that should be kept in mind during future integrations:

  • The _transferViaOFT function makes an external call to the messenger's send method, where the messenger is mapped to a given LayerZero-supported token. In principle, such an external call may open the door to reentrancy attacks. Currently, this is not an issue since the _transferViaOFT function gets called by functions protected by the nonReentrant modifier. However, in future integrations, it is crucial to be aware of this fact and address it by adding the non-reentrancy feature in the _transferViaOFT function.
  • In the Ethereum_SpokePool contract, the second argument of the __SpokePool_init function, which is supposed to be _crossDomainAdmin, is set to be the same as the third argument (_withdrawalRecipient). The documentation states that "crossDomainAdmin is unused on this contract". However, it is understood that there is an assumption in play here: for the function in question, the HubPool contract will always be both the withdrawal recipient and the admin. In future integrations, it is recommended to keep in mind this assumption and verify that it holds. Otherwise, that address might get extra permissions that it was not meant to have.
  • Pull request #1031 removes deprecated functionalities, which include the usage of the zero address in the outputToken token to signal equivalence to the one provided by the router. This should be kept in mind for preserving consistency in future, and past, integrations.

Design Choices

  • The OFTTransportAdapter contract's implementation restricts the usage of the extra options, composable messages, and commands from LayerZero. If certain transfers or assets do need to use them, the outcome might not be the expected one.
  • The AdapterStore contract allows for using different messenger types as keys for the crossChainMessengers mapping. However, all adapters that can currently use the AdapterStore contract are not allowed to select such type as its value is hardcoded. This means that the admin can set messengers whose type does not match the OFT_MESSENGER one, but messengers will not be used. Moreover, they can remain unnoticed until they can perform a malicious action in the future.
  • Not all the adapters and SpokePool contracts that are part of the introduced changes will allow for the usage of OFT Transport, even though they inherit the logic. These contracts use hardcoded values that, while not directly restricting the OFT Transport functionality, render it useless. However, it is worth mentioning that the attack surface could further be reduced by taking the OFT Transport logic out of the base contract (SpokePool).
 

High Severity

Failed Messenger Can Render the Canonical Methods Useless

The Arbitrum_Adapter and Arbitrum_SpokePool contracts implement a new else if conditional statement branch to make use of the _transferViaOFT function from the OFTTransportAdapter contract. To enter into this branch, the only requirement is for the messenger associated with that token to not be zero.

The call stack on the HubPool contract's side is: executeRootBundle -> _sendTokensToChainAndUpdatePooledTokenTrackers -> relayTokens -> _transferViaOFT. In this call sequence, if the messenger throws an error in its internal operation when sending the message inside the innermost call to the _transferViaOFT function, the reversion will be propagated all the way up to the _sendTokensToChainAndUpdatePooledTokenTrackers function and the assets will not be sent. Not only that, since the assets are sent in the same call as the message with the refund roots, the roots will also not be sent to the respective SpokePool contract. As the bridging mechanisms are checked sequentially, the Arbitrum Gateway, which comes after the OFT Transport method, will not be able to be used as a replacement, meaning that there will be no way of transferring those funds until either the messenger resolves the conflicts or the admin role removes it from the oftMessengers mapping, possibly resulting in a stall situation until it gets resolved.

In order to prevent a scenario in which assets need to be moved into or out of Arbitrum (e.g., there are insufficient assets) when the OFT messenger is currently not working, consider replacing these patterns in all the affected contracts to allow for bypassing a failing messenger so that the canonical alternative can be used. Alternatively, consider using a try-catch mechanism to prevent the propagation of the revert that could also affect the bridging of the root hashes.

Update: Acknowledged, not resolved. The team stated:

This is indeed an important situation, which will not, however, be mitigated by try - catching.

The scenario we imagine is OFT transfer breaking (messenger freezing the route) for some OFT token, say USDT. As discussed in Slack, USDT is not bridgeable in a meaningful way to L2s via canonical bridges. If the OFT messenger for that route is shut down / frozen, there's indeed no way for our system to rebalance USDT to that destination chain because OFT is supposed to be the only available bridge option for USDT.

If a situation like this ever happens though, it's crucial that the engineering team knows about it immediately. We can not just silently try catch, because system will not continue to operate correctly without the rebalance required by it (e.g. we're trying to repay relayers on Arbitrum with USDT that's not in Spoke's balance -> that won't work). The mitigation scenario we envision is the following:

1. We deploy a new ArbitrumAdapter with commented _transferViaOft line commented out, allowing bundle to execute.

2. At this point, any USDT relayer refund leaves on Arbitrum will fail to execute and will page us a lot. We deal with this by modifying the executor (probably) to suppress this noise.

3. Once OFT is back unfrozen for this path, we use relaySpokePoolAdminFunction in conjunction with something like Arbitrum_SendTokensAdapter.sol (change it's code to OFT logic) to backfill the missing USDT tokens on L2 (that were not executed properly during 1).

Otherwise, if OFT route is never unfrozen, we have no way to repay the relayers on Arbitrum. Summing this up, doing anything other than sending the funds through the OFT adapter should require manual intervention. (i.e. no try-catch and drop the sending of funds).

Medium Severity

Insufficient Validation of Sent Amount in OFTTransportAdapter

In the OFTTransportAdapter contract, the _transferViaOFT function gets the OFTReceipt output containing two elements: the amount sent at origin and the amount received at destination. The current implementation only validates the amount received at destination, ensuring that it matches the input amount specified by the user. However, this approach overlooks the validation of the amount sent, potentially increasing the attack surface.

Due to the lack of validation of values sent at origin, a scenario might materialize whereby the messenger could take more assets at origin, deposit the correct lesser amount at destination, and pass the check. LayerZero's documentation on the _debit function states that "In NON-default OFT, amountSentLD could be 100, with a 10% fee, the amountReceivedLD amount is 90, therefore amountSentLD CAN differ from amountReceivedLD.". The current implementation tries to mitigate this through the forceApprove call method associated with the token. Still, if its implementation allows flexible or greater values than the amount sent, the outcome would rely on the assumption that the messenger does not take more than needed.

Consider imposing restrictions on the values sent at origin to reduce the attack surface and prevent situations such as the aforementioned ones. In addition, consider validating that the tokens used through the OFT messenger do not present a behavior deviation or edge cases when using the approval functionality.

Update: Resolved in pull request #1027.

Compromised Messengers Cannot Be Removed

In the SpokePool contract, the _setOftMessenger function allows for adding or updating an OFT messenger address for a particular token. However, if the current messenger is compromised or needs to be taken down, there is no functionality that would allow the admin to remove it from the available messengers (without replacing it with a new one).

Furthermore, it is not possible to use the _setOftMessenger function to set it to zero, as there is a validation done over its token that would revert if the messenger is set to the zero address or to an address that does not have the token method implemented or to an address that does not match the passed one. Since temporarily setting a (compromised) messenger address to a dummy contract that implements the token method could also be dangerous as it might not stop the whole OFT flow, the admin might not be able to react fast enough and stop using it.

Consider implementing a method that would allow the admin to remove a messenger from the storage.

Update: Resolved in pull request #1034. The team stated:

Added ability for admin to always set messenger to zero address.

OFT Transfer Might Revert Due to Non-Zero ZRO Token Fee Quote

The OFTTransportAdapter contract implements the _transferViaOFT function, meant to interact with the OFT messenger and send the funds with that method. To do so, it first quotes the fees, which are returned as native or ZRO token fees. Later, this same output is used as part of the message.

Even though the OFTTransportAdapter contract instructs that it will pay the fees in native tokens, there is no validation that the value for fees in ZRO tokens is zero, meaning that it will be passed once again to the messenger. This means that if the implementation of the messenger outputs both quotes at the same time, it might not recognize with which asset it will be paid, resulting in a reversion if it tries to get paid with ZRO. As seen in an example from LayerZero, in the case of paying in native tokens, the lzTokenFee parameter is set to zero.

In order to prevent unexpected outcomes and reversions, especially if the messenger deviates in behavior, consider asserting that the returned lzTokenFee is zero when quoting for the cost. Furthermore, consider adding more scenarios to the mocked contracts to validate proper integration with protocols.

Update: Resolved in pull request #1029. The team stated:

Added a zero-check for lzTokenFee and added some fee negative-scenario tests (partially addressing M-04).

Insufficient Test Coverage

Throughout the codebase, and in particular in the added changes, multiple instances of insufficient test coverage were identified:

  • There are different test suites implemented at the same time, namely Hardhat and Foundry. Maintaining different suites instead of having all under the same one adds friction and error-proneness, and increases the cost for the developer to keep it secure.
  • Similar contracts in the protocol use different test suites. In particular, Universal contracts rely on Foundry, while the Arbitrum contracts rely on Hardhat. Standardizing the tests would allow for testing similar contracts under the same cases, which could be beneficial when finding edge cases or bugs.
  • New additions only add 5 single positive overall cases to the suite, leaving many other edge cases untested and not asserting any negative situations.
  • The values used for testing the outgoing fees have been set to zero, as a result of which the whole fee protection is bypassed.
  • Contracts are not fuzzed to find edge cases that could be used for exploits.
  • There is a lack of integration with the LayerZero protocol, which requires analyzing its caveats, edge cases, and behaviors, and testing the project under such conditions.

Insufficient testing, while not a specific vulnerability, implies a high probability of additional undiscovered vulnerabilities and bugs. It also exacerbates multiple interrelated risk factors in a complex code base. This includes a lack of complete, implicit specification of the functionality and exact expected behaviors that tests normally provide, which increases the chances of correctness issues being missed. It also requires more effort to establish basic correctness and reduces the effort spent exploring edge cases, thereby increasing the chances of missing complex issues.

Moreover, the lack of repeated automated testing of the full specification increases the chances of introducing breaking changes and new vulnerabilities. This applies to both previously audited code and future changes to currently audited code. Underspecified interfaces and assumptions increase the risk of subtle integration issues which testing could reduce by enforcing an exhaustive specification.

To address these issues, consider implementing a comprehensive multi-level test suite. Such a test suite should comprise contract-level tests with 95%-100% coverage, per chain/layer deployment, and integration tests that test the deployment scripts as well as the system as a whole, along with per chain/layer fork tests for planned upgrades. Crucially, the test suite should be documented in such a way that a reviewer can set up and run all these test layers independently of the development team. Some existing examples of such setups can be suggested for use as reference in a follow-up conversation. In addition, consider merging all the test suites into a single one for better maintenance. Implementing such a test suite should be of very high priority to ensure the system's robustness and reduce the risk of vulnerabilities and bugs.

Update: Partially resolved in pull request #1038. The team stated:

Addressed these 2 points:

- New additions only add 5 single positive overall cases to the suite, leaving many other edge cases untested and not asserting any negative situations.

- The values used for testing the outgoing fees have been set to zero, as a result of which the whole fee protection is bypassed.

Added multiple new local (unit) test-cases (fee ones were added as part of the fix for issue M-03) as well as a fork test for sending USDT via a Universal_Adapter. A fork test can be run with this command:

NODE_URL_1=<your-ethereum-rpc-url> forge test --match-path test/evm/foundry/fork/UniversalAdapterOFT.t.sol

OFT Transfers Revert if Chains Have Different Local Decimals

In the OFTTransportAdapter contract, the _transferViaOFT function checks if the expected value received at the end matches the input passed at origin. In LayerZero, the default value for the local decimals is 18, but it can be changed. In such a scenario, the amountReceivedLD value will be expressed in local decimals at destination and it will not match the _amount input expressed in local decimals at origin. Consequently, the transfer will revert, and movement using the OFT mechanism in that combination will get stalled. Note that the movement will work if the decimals on both chains are the same.

Consider taking into account the difference in decimals on both chains and performing the conversions when validating the received amount.

Update: Acknowledged, not resolved. Assets that implement different decimals on source and destination, and therefore deviate from the default implementation, might override the OFTAdapter._debit() and OFTCore._debitView() functions, causing the reversion of the validations in the _transferViaOFT function. OFT implementations whose decimals are the same might not present this issue.

The team stated:

_transferViaOft flow: We're interacting with OFTAdapter on chain. Here's default implementation of that. We're calling (call is to OFTAdapter which inherits OFTCore):

OFTCore.send() -> OFTAdapter._debit() -> OFTCore._debitView()

This call chain starts here. Within _debitView, which is the default OFT implementation, we see this:

amountSentLD = _removeDust(_amountLD); amountReceivedLD = amountSentLD;

Amounts received and sent are both in the local decimals of the *source chain(, so decimal discrepancies will not be a problem in the default OFT implementation. USDT0 uses the same underlying logic (although their contracts are upgradeable).

All in all, we don't expect to support tokens with non-standard decimal implementation in _debit() nor do we expect OApps to override this function in terms of decimals logic. What we might expect some OApps do is maybe override _debit in terms of adding extra fees, we won't be able to support those just yet.

Inconsistent Use of the __gap Variable

Throughout the codebase, multiple instances where the __gap variable is being used inconsistently were identified, which could cause security and operational problems.

The number of reserved slots does not seem to follow a particular pattern. For example, it has been set as 1000 in cases where no local storage variables have been defined, but it is also 1000 in cases where local storage variables have been defined. This means that the sum of the reserved slots and the used-up slots (i.e., used by the definition of storage variables) does not add up to a common value, sometimes greatly exceeding the value of the rest of the contracts. This is, for instance, the case with the SpokePool contract which defines multiple storage variables but also keeps 997 slots reserved using the __gap variable, resulting in the total value of storage slots being greater than 1000. The importance of keeping a common value is that it helps check whether a possible collision could happen between contracts, in particular when they inherit each other.

In addition, not having proper documentation for keeping track of which slot represents which storage variable could cause a mismatch between the slots used and the variables defined. This might push the storage layout of the inheritance and cause storage collisions. Moreover, it has been observed that, in the past, changes have been made to the __gap variable that increased its length while removing a storage variable. Such changes are dangerous as they can create situations in which a new variable that is added after such a change might not have an empty value and could be used in an exploit.

Consider reviewing the whole protocol to assert that the slots are being used as planned, comprehensively documenting the variables and their slots in the respective __gap variable. Additionally, from now onward, consider standardizing the value that will be used for future contracts.

Update: Resolved in pull request #1039. The team stated:

Updated __gap documentation.

EIP-7702 EOA Accounts' Treatment Could Result in Reentrancy

When the to address is detected as a EIP-7702 EOA wallet by the _is7702DelegatedWallet function when sending ETH or WETH, the _unwrapwrappedNativeTokenTo function from the SpokePool contract will first convert the WETH into ETH and then send it to the wallet with a low level .call call. As such, this low level call does not limit the gas to 2300, being able to perform more complex operations.

Also, as the EIP-7702 EOA wallets could use any implementation, it might be possible that their receive or fallback method implement malicious logic to reenter the protocol when it is not expected.

Although several functions are protected against reentrancy, consider reducing the attack vector in the protocol by treating the EIP-7702 EOA wallets as contracts by sending WETH. Alternately, consider reducing the gas stipend for the low level .call call so it cannot perform any storage change nor external call to another contract.

Update: Acknowledged, not resolved. The team stated:

The fill functionality already supports external calls with unlimited gas budgets at the same point in the code. From our perspective, any ETH call to a 7702 wallet could also trigger a callback into the contract via the handleV3AcrossMessage callback right after with no state changes in between. We cannot think of any cases where re-entrancy would be a problem in one case, but not the other.

For the protocol to function with its current feature set, it must be resilient to re-entrancy, in general. If the protocol is not resilient to re-entrancy, then we think that should be addressed in a way that covers all cases, not just the ETH to 7702 case.

We do not think these changes will impact the security of the protocol, so we acknowledge, but choose not to make the suggested changes.

Low Severity

Lack of Validation On Linked Messengers

In the SpokePool contract, there is no standard way of checking whether a contract supports specific interfaces or features. In particular, when adding a messenger, the single check being performed is whether the input address matches its token function against the one passed. As the token function is pretty common in a diverse set of contracts, a contract could be mistakenly passed that implements a token function and retrieves the particular token, but is not an IOFT type of contract. In such a scenario, the assignment will pass but will result in unexpected situations.

In the Ethereum ecosystem, more thorough validation can be accomplished if a contract is compliant with EIP-165. An interface is a collection of functions that define a set of behaviors. By implementing a standard, contracts can expose a function to query whether they support specific interfaces, making it easier for other contracts to understand their capabilities. Since messengers might be upgraded and new functionality can be added to existing contracts, this standardization would also allow new versions to ensure backward compatibility.

Furthermore, the IOFT interface does define the interface ID, which could be used to complement such validation when setting the messengers. Also, note that in the AdapterStore contract, the messengers added through any of the functions are not being checked as they are in the SpokePool contract.

Consider defining and implementing a standard interface detection mechanism in modules that interact with external contracts to improve their overall usability, safety, and efficiency by providing a consistent way for contracts to query and identify the features they support. Moreover, consider performing the same verification in the AdapterStore contract to be consistent with the messengers' additions.

Update: Partially resolved in pull request #1033. The team stated:

Added part of suggested change: IOFT validation on AdapterStore.sol side via calling .token(). This is meant to be human-error protection more than anything. Implementing EIP-165 calls was a bit too much to add.

Misleading Documentation

Throughout the codebase, multiple instances of misleading documentation were identified:

  • In the Universal_Adapter contract, the inline documentation states that the relayTokens function "only uses the CircleCCTPAdapter to relay USDC tokens to CCTP enabled L2 chains". While this is partially true for that token, the new implementation also allows for using the OFT method.
  • A comment in the AlephZero_SpokePool contract states that " Arbitrum only supports v0.8.19". However, on Arbiscan, newer versions are also listed as being supported.

Consider updating the documentation to reflect the current behavior of the functionality.

Update: Partially resolved in pull request #1030. Only the Universal_Adapter contract has been updated.

Logic Not Fully Deprecated

The SpokePool contract implements the basics of keeping track of the respective OFT messengers against tokens. It also inherits the OFTTransportAdapter contract, to which it passes the respective input parameters from the constructor function. Currently, only the Arbitrum-based contracts (cf. AlephZero) and Universal SpokePool and adapters make use of such OFT functionality. This means that on any other chain, these constructor parameters are set to zero.

This approach raises the following issues:

  • Even though the _transferViaOFT function is not called in the implementation of such adapters or SpokePools, and that possibly the messenger and/or the token might not be set, the usage of zero values in the OFT_DST_EID and OFT_FEE_CAP parameters might increase the attack surface due to how they are being used as checks. This could give rise to potential problems in a future upgrade where the flow could reach such a call and make it part of an exploit.
  • The admin can still add/modify the messengers' mapping at will, as these functionalities are not blocked when the contract does not need the OFT feature. This could be a potential attack vector for future operations, in which a current admin could pass a certain messenger attached to a token while the chain does not support the OFT functionality, to then have it ready in the future once that changes.

Consider restricting the functionality of the OFT inherited by the adapters and the SpokePools when they are not being used to prevent the aforementioned scenarios and reduce the attack surface.

Update: Acknowledged, not resolved. The team stated:

Having SpokePool inherit OFTTransportAdapter allows us to add OFT functionality to more spokes down the line more easily. What is more, OFT_FEE_CAP is meant to be protecting from sending an overly excessive fee. It's not responsible for protection against other scenarios. Set to 0, it still fulfills it's role. Protection against incorrectly setting OFT messengers etc. depends on admin being honest.

Abstract Contracts Allow Direct Modification of State Variables

The internal and public state variables in abstract contracts allow them to be directly modified by child contracts. This may break the expected properties for the state variables and limit off-chain monitoring capabilities due to the lack of event emissions for changes to the variables.

Specifically, in SpokePool.sol, the SpokePool abstract contract contains the oftMessengers state variable which is public. Moreover, as the oftMessengers mapping is only being accessed by setters and getters inside the SpokePool contract, there is no need for keeping such high visibility on the child contracts.

Consider using private visibility for state variables in abstract contracts. In addition, consider creating internal functions for updating these variables which emit appropriate events and verifying if the desirable conditions are met.

Update: Acknowledged, not resolved. The team stated:

The proposed solution brings with it a limitation of not being able to see the set messengers on e.g. etherscan. While possible to mitigate by adding an additional public getter on the SpokePool, we feel like SpokePool code is already complex and adding code like this can overwhelm someone trying to understand it more. We'd like to keep the current visibility of oftMessengers

Missing Zero-Address Checks

When operations with address parameters are performed, it is crucial to ensure the address is not set to zero. Setting an address to zero is problematic because it has special burn/renounce semantics. This action should be handled by a separate function to prevent accidental loss of access during value or ownership transfers.

Throughout the codebase, there are multiple instances where operations are missing a zero address check:

Consider adding a zero address check before assigning a state variable.

Update: Acknowledged, not resolved. The team stated:

After internal discussion, we think that zero-address checks here are a bit of overkill because all the functions mentioned are only callable by admin (except for _adapterStore case, but that's contract creation, and this contract can only be used within the system after admin action)

Missing Docstrings

Throughout the codebase, multiple instances of missing docstrings were identified. Particularly, in the following files:

  • AdapterStore.sol
  • Arbitrum_Adapter.sol
  • Arbitrum_SpokePool.sol
  • OFTTransportAdapter.sol
  • OFTTransportAdapterWithStore.sol
  • Ovm_SpokePool.sol
  • PolygonZkEVM_SpokePool.sol
  • Polygon_SpokePool.sol
  • Scroll_SpokePool.sol
  • SpokePool.sol
  • Succinct_SpokePool.sol
  • Universal_Adapter.sol
  • Universal_SpokePool.sol
  • ZkSync_SpokePool.sol

Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API, events, storage variables, and constants. 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: Partially resolved at commit 84f87be. The team stated:

Added docstrings to:

- AdapterStore.sol - OFTTransportAdapter.sol - OFTTransportAdapterWithStore.sol

to keep changes to the OFT scope.

Floating Pragma

Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.

Throughout the codebase, multiple instances of floating pragma directives were identified:

Consider using fixed pragma directives.

Update: Acknowledged, not resolved. The team stated:

After internal discussion, we concluded that moving to a fixed pragma is not something we'd like to do as a part of this audit. Maybe in the future! Thanks for the suggestion.

Different Pragma Directives Are Used

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.

Throughout the codebase, multiple instances of varying pragma directives were identified:

Consider using the same, fixed pragma directive across all files.

Update: Acknowledged, not resolved. The team stated:

After internal discussion, we concluded that moving to a fixed pragma is not something we'd like to do as a part of this audit. Maybe in the future! Thanks for the suggestion.

Adapter Implementation Could Be Misused

The Arbitrum_Adapter contract on L1 is being used by the HubPool contract which delegateCalls its execution. All the parameters passed to the constructor are being used to set immutable variables, meaning that these can later be used during the delegateCall. However, this also creates the possibility for someone to use the adapters as an entry point to the system instead of going through the HubPool contract since all the parameters are also set there. Even though the Arbitrum_Adapter contract should not have any assets under regular conditions, if a user mistakenly sends assets to it, another actor might take the opportunity to relay these assets to a personal address.

In order to prevent the aforementioned case, consider enforcing that the implementation cannot be used when being called directly.

Update: Acknowledged, not resolved. The team stated:

Adapter implementation is not supposed to hold any assets. We might consider this problem in the future, but not as a part of this audit.

Protocol Cannot Overcome OFT Fee Increase Without Upgrading

To prevent sending tokens through the OFT messaging system with unreasonable fees, the OFTTransportAdapter contract imposes a cap on the fees, and they are stored as an immutable parameter. This contract is being used for both the adapters and the SpokePool contracts. However, if messaging fees exceed the cap, all OFT transfers will fail. As a result of the immutable nature of the cap, there is no straightforward way of increasing it further to resolve the problem. This would necessitate deploying a new adapter and setting it with the setCrossChainContracts function from the HubPool contract, or deploying a new SpokePool contract with the new value and perform an upgrade.

Since both of the above ways to address the fees exceeding the cap might render the protocol unusable during cost fluctuations, consider implementing functionalities to modify the cap. Alternatively, consider thoroughly documenting a contingency plan to resolve such a situation as fast as possible once it emerges.

Update: Acknowledged, not resolved. The team stated:

"This would necessitate deploying a new adapter and setting it with the setCrossChainContracts function from the HubPool contract, or deploying a new SpokePool contract with the new value and perform an upgrade." - this is our standard way of updating these params, yes.

We have a high cap of 1 ETH set for our deployments, so we don't expect that to be a problem. What is more, the failing OFT transfer will not make the system unusable. It might delay the execution of the bundle or a leaf, which might delay relayer repayments. User deposits and fills will still work as expected so we have time to react to this problem if it arises.

Transfers of Fee-On-Transfer Tokens Will Revert

The _transferViaOFT function of the OFTTransportAdapter contract allows for using the OFT transport between chains. To assert that the assets have arrived correctly, the function implements a check that compares the amount sent from one chain with the amount received on the other chain. However, if the underlying token charges fees on transfers, the two amounts will differ (i.e., the input _amount will not match the output amountReceivedLD).

Consider implementing the necessary logic to allow for the transfer of fee-on-transfer tokens. Alternatively, consider documenting the fact that fee-on-transfer tokens cannot be used with OFT transfers when the fees are enabled.

Update: Acknowledged, not resolved. The team stated:

Yes, that's fine. We plan on only supporting reputable tokens without fees on transfer.

LayerZero's Dust Removal Might Revert OFT Transfers

In the _transferViaOFT function of the OFTTransportAdapter contract, the final verification checks that the amount of assets passed as input matches the amount that will be received at destination. However, when invoking the messenger during the transfer, the _removeDust function in LayerZero will be called, which will remove all those digits from the input amount that cannot be represented by the sharedDecimals value (by default, 6). This means that if the _amount input has a leftover after the integer division of the decimalConversionRate value, then the amountReceivedLD value will be lesser than the _amount value by that dust, so the two will not match.

Relatedly, there is an inline comment that states " Setting minAmountLD equal to amountLD protects us from any changes to the sent amount due to internal OFT contract logic, e.g. _removeDust", referring to the two values being the same and equal to the amount sent, the _amount input. However, it is worth noting that this measure does not offer greater protection against dust removal since the latter is accomplished through the final check on the oftReceipt output. Indeed, this check only employs the amount sent (_amount) and not the minimum amount, but even if validation with the minimum amount is not performed, the dust removal protection would still be caught by the final check with the oftReceipt output. Even though the amount is being passed by the Dataworker when submitting the bundle, it should be noted that the input amount could be converted before sending it through the OFT messenger so that it does not contain any dust in the first place, minimizing the cases of reversion.

Consider implementing the necessary calculation and conversion to prevent dust from reverting the transaction. In particular, the amount could be rounded up to the next precision given by the decimalConversionRate value to prevent sending fewer assets than those passed by the Dataworker.

Update: Acknowledged, not resolved. The team stated:

It's a design decision we went with: we added this rounding requirement to the relevant UMIP as a requirement for correctness of a bundle. So dataworker is responsible for providing correct decimals. Otherwise the bundle is deemed incorrect If there's a bug in dataworker code, OFT send will indeed revert; that's desired behavior.

Function Selectors On Deprecated Functions Are Not Locked

Pull request 1031 removes already announced deprecated functionalities, such as the depositDeprecated_5947912356 and depositFor functions, alongside their _deposit internal function.

However, as these entry points are removed, future versions of the codebase might introduce new functions that could have the same function selector as the removed ones. In such case, a protocol that might have used the deprecated functionalities could now call to the new ones with unexpected outcomes.

Similarly, if a fallback function is used in the future, depending on its implementation, it might take the calldata of protocols using the old deprecated functionalities with similar results.

In order to prevent the reuse of the deprecated function selectors, consider keeping the public functions' declaration, without their original definition, and reverting the calls to them.

Update: Resolved in pull request #1048 at commit 0e262bf.

Notes & Additional Information

Zero-Address Validation Bypass in SpokePool

The _setOftMessenger function of the SpokePool contract performs a check to validate that the proposed OFT messenger contract is the appropriate one for the given token. However, there is a scenario whereby if the messenger has not set its token, an admin is able to link the messenger to the zero address as the _token parameter. Even though consecutive calls to that zero-address token would fail in the OFT transfer flow, it is recommended to reduce the attack surface of edge cases.

Consider validating that the token address input is not the zero address at all times.

Update: Acknowledged, not resolved. The team stated:

That's fine, we trust the admin not to do this.

Typographical Errors in Documentation and Comments

Typographical errors reduce readability and may cause misunderstandings. Throughout the codebase, multiple instances of typographical errors were identified:

  • In OP_Adapter.sol, the documentation in line 41 states "Desination", whereas it should be "Destination".
  • In OFTTransportAdapter.sol, the documentation in line 54 states "trasnfer", whereas it should be "transfer".

Consider correcting any instances of typographical errors and using spell-checking tools to avoid their recurrence.

Update: Resolved in pull request #1035.

Possible Duplicate Event Emissions

When a setter function does not check if the value has changed, it creates a possibility for spamming events which indicate that the value has changed even when it has not. Spamming the same values repeatedly can potentially confuse off-chain clients.

In the SpokePool contract, when setting the new messenger, the new values can be identical to the ones in storage, allowing for triggering the same event multiple times by setting the same value.

Consider adding a check that reverts the transaction if the value being set is identical to the existing one.

Update: Acknowledged, not resolved. The team stated:

We rely on admin to not produce duplicate "set events".

Redundant Getter Function

When state variables use public visibility in a contract, a getter method for the variable is automatically included.

In the SpokePool contract, the _getOftMessenger function is redundant because the oftMessengers state variable already has a getter.

To improve the overall clarity, intent, and readability of the codebase, consider removing any redundant getter functions or documenting the reasons to keep them.

Update: Acknowledged, not resolved. The team stated:

We feel like operating with raw oftMessengers mapping from a child contract is more dangerous (easier for a programmer to make a mistake) than using a getter.

Function Visibility Overly Permissive

Throughout the codebase, multiple instances of functions having unnecessarily permissive visibility were identified:

  • The _setOftMessenger function in the SpokePool contract with internal visibility could be limited to private.
  • The setOftMessenger function in the SpokePool contract with public visibility could be limited to external, as it must be called by the admin.

To better convey the intended use of functions and to potentially realize some additional gas savings, consider changing a function's visibility to be only as permissive as required.

Update: Resolved in pull request #1041.

Non-Explicit Imports

The use of non-explicit imports in the codebase can decrease code clarity and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity file or when inheritance chains are long.

Throughout the codebase, multiple instances of non-explicit/global imports were identified:

Following the principle that clearer code is better code, consider using the named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.

Update: Resolved in pull request #1036.

Multiple Contract Declarations Per File

Within AdapterStore.sol, multiple contracts or libraries have been declared. These are the MessengerTypes library and the AdapterStore contract.

Consider separating the contracts into their own files to make the codebase easier to understand for developers and reviewers.

Update: Acknowledged, not resolved. The team stated:

With MessengerTypes lib being so small, I feel like it improves readability rather than subtracts from it.

Missing Named Parameters in Mappings

Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means that mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.

Throughout the codebase, multiple instances of mappings without named parameters were identified:

Consider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.

Update: Resolved in pull request #1040.

Lack of Security Contact

Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.

Throughout the codebase, multiple instances of contracts missing a security contact were identified:

Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Update: Resolved in pull request #1037.

Custom Errors in require Statements

Since Solidity version 0.8.26, custom error support has been added to require statements. Initially, this feature was only available through the IR pipeline. However, Solidity 0.8.27 extended its support to the legacy pipeline as well.

Throughout the codebase, multiple instances where if-revert statements could be replaced with require statements were identified:

For conciseness and gas savings, consider replacing if-revert statements with require statements.

Update: Acknowledged, not resolved. The team stated:

Tried to address this, but OFTTransportAdapter is inherited by many other contracts and the new version requirement was not trivial to fix. E.g. "stack too deep" errors appeared. Might consider moving to 0.8.27 in the future for require with custom errors!

Unused Errors Due To Deprecated Logic

After the removal of the _deposit function from the SpokePool contract, the InvalidRelayerFeePct and MaxTransferSizeExceeded errors are no longer in use.

Consider removing the unused errors.

Update: Resolved in pull request #1047 at commit 2de2d50.

Conclusion

The reviewed code introduces an OFT LayerZero bridging mechanism to the Across protocol in order to enhance the existing setup. To do so, the OFTTransportAdapter contract has been introduced, implementing the necessary parsing to interact with the OFT messengers. Due to the difference in how Adapters and the SpokePool contract interact, the AdapterStore and the OFTTransportAdapterWithStore contracts have been added to be used as a beacon for the rest of the Adapters during the delegatecall in the HubPool contract. On the SpokePool side, an internal mapping has been added to the SpokePool contracts in order to keep track of such linkage between tokens and the respective messengers. Currently, only the Universal, Arbitrum, and AlephZero adapters will make use of this OFT feature, while the rest of the adapters and SpokePools were either left unmodified or have been hardcoded with null values to prevent the OFT functionality's usage.

In addition, pull requests #1031 and #1032 were added into the scope. These removed legacy functionality related to the ability to set the address of the output token to the zero address and added support for transferring ETH to EIP-7702 delegated wallets.

Overall, the addition of the OFT feature appears to be sound. That said, it may still benefit from adding more test suits, enforcing a stronger deprecation of the flow in the chains that currently will not be used, and documenting some of the design choices made in the implementation. Furthermore, a roadmap for the upcoming changes in this feature would complement the documentation to better explain the potential roadblocks. The changes introduced by pull request #1031 are well-motivated. As for #1032, concerns were listed as to its compliance with the EIP-7702 standard, specifically in the treatment of delegated EOAs as regular EOAs, as opposed to regular contracts.

The changes in scope rely on multiple trust assumptions, which were duly listed, along with some integration points to keep in mind in future integrations, in this report.

The Risk Labs team has been very helpful throughout the engagement, answering all of the audit team's questions promptly and in great detail.

Request Audit

 

Related Posts