Across Protocol SVM Solidity Audit

Table of Contents

Summary

Type

DeFi

Timeline

From 2024-12-12

To 2024-12-20

Languages

Solidity

Total Issues

11 (10 resolved)

Critical Severity Issues

1 (1 resolved)

High Severity Issues

0 (0 resolved)

Medium Severity Issues

0 (0 resolved)

Low Severity Issues

5 (4 resolved)

Notes & Additional Information

3 (3 resolved)

Scope

We targeted the across-protocol/contracts repository with three different goals:

1) At commit 7f0cedb in the master branch, we reviewed the ERC-7683 implementation with a focus on the predictable relay hashes feature that was first introduced in pull request #639.

2) At commit 401e24c of the svm-dev branch, we audited the changes made to the following files:

 contracts/
├── SpokePool.sol
├── SpokePoolVerifier.sol
├── SwapAndBridge.sol
├── erc7683/
│   ├── ERC7683OrderDepositor.sol
│   ├── ERC7683OrderDepositorExternal.sol
│   ├── ERC7683Permit2Lib.sol
├── external/interfaces/
│   ├── CCTPInterfaces.sol
├── interfaces/
│   ├── V3SpokePoolInterface.sol
├── libraries/
│   ├── AddressConverters.sol
│   ├── CircleCCTPAdapter.sol
├── permit2-order/
│   ├── Permit2Depositor.sol

3) At pull request #805 we reviewed the changes as part of the fix review process of this audit.

System Overview

The Protocol enables instant token transfers multiple blockchain networks. At the core of the protocol is the HubPool contract on the Ethereum mainnet, which serves as the central liquidity hub and cross-chain administrator for all contracts within the system. This pool governs the SpokePool contracts deployed on various networks that either initiate token deposits or serve as the final destination for transfers. A more detailed overview of the Protocol and its various components can be found in our previous reports.

The audited changes mainly center around the added compatibility with the Solana network. To achieve this, the following changes have been made:

• Many address data types have been converted to bytes32 to make them more general and compatible with Solana.
• A repaymentAddress parameter has been added to the fill functions so that relayers can specify different addresses.
• There was an edge case in which tokens with blacklists might prevent refund-related transfers from being executed. For this reason, transfers within a bundle that revert are now accounted into a new mapping which allows relayers to retrieve refunds that could not be transferred as part of a normal relayer refund call.

Adding compatibility for the Solana network also comprises other pull requests and files. However, for this audit, we exclusively focused on the changes required to the existing Solidity files mentioned in the scope above.

Critical Severity

Anyone Can Lock Relayer Refunds and Contract Can Be Drained

The claimRelayerRefund function is meant to give relayers the option to claim outstanding repayments. This can happen in different edge cases, like blacklists not allowing token transfers. In such cases, the relayer can call this function and specify a different refundAddress to claim their funds.

The function first reads the current outstanding refund from the relayerRefund mapping using the l2TokenAddress and msg.sender keys. If this value is greater than 0 then it is transferred to the refundAddress and the appropriate event is emitted.

Before transferring out the tokens, the mapping value is set to 0 for correct accounting. However, the key used to reset the mapping value is refundAddress and not msg.sender. This opens the door for any relayer with a small refund amount to zero out any other relayer's refund by specifying their refundAddress, effectively making the relayers lose their refunds. In addition, since the original mapping value is never reset, a malicious relayer can exploit this by repeatedly calling the function to drain the entire balance of the l2TokenAddress contract.

Consider using the proper msg.sender key instead of refundAddress to correctly set the refund amount to zero.

Update: Resolved in pull request #826. The code correctly resets the mapping value now.

Low Severity

Function Can Be Declared external

The claimRelayerRefund function of the SpokePool contract is declared as public but can be defined as external instead.

In order to improve overall code quality and to improve gas consumption, consider changing the function visibility to external.

Update: Resolved in pull request #827.

_destinationSettler Can Return Zero Address

In the _resolveFor function of the ERC7683OrderDepositor contract, the value returned from the internal _destinationSettler function is not validated to be non-zero. The _destinationSettler function returns the settler contract for the destination chain. If this value has not been set, the default value will be returned and passed to the fillInstructions field of the resolvedOrder parameter.

Consider adding the requirement that the _destinationSettler value used in the FillInstruction struct of the resolved order cannot be the zero address.

Update: Resolved in pull request #834.

Incorrect Right Shift in AddressConverters Library

The toAddress function of the AddressConverters library shifts right by 192 bits. However, it should shift right by 160 bits instead to correctly check that the upper 96 bits of _bytes32 are actually empty.

Consider updating the toAddress function so that it shifts right by 160 bits instead.

Update: Resolved in pull request #828.

Repeated Function

The _toBytes32 internal function of the ERC7683OrderDepositor contract has the same logic as the imported toBytes32 function of the AddressConverters library. Both the _toBytes32 and toBytes32 functions are used throughout the ERC7683OrderDepositor contract, which lessens code clarity.

In order to improve the overall code quality and avoid having duplicated code, consider removing the _toBytes32 function and using the library function instead.

Update: Resolved in pull request #829. The team also removed the internal _toAddress function in favour of the equivalent function present in the same library file.

Lack of Unit Tests for ERC-7683

As the protocol expands, it is crucial to maintain a comprehensive testing suite that covers all the new features. Presently, the repository does not contain any unit tests for the ERC7683OrderDepositor and ERC7683OrderDepositorExternal contracts.

In order to improve the robustness and security of the codebase, consider implementing a comprehensive testing suite for the EIP-7683 implementation with a high degree of coverage.

Update: Acknowledged, will resolve. The team is planning to integrate changes into the testing and coverage and the issue will be resolved with them.

Notes & Additional Information

Missing Docstrings

Missing docstrings can negatively affect code clarity and maintainability. The repaymentAddress parameter is not documented in the docstrings of the fillV3RelayWithUpdatedDeposit and fillV3Relay functions of the SpokePool contract.

Consider having docstrings for all input parameters.

Update: Resolved in pull request #830.

Typographical Errors

Throughout the SpokePool contract, multiple instances of typographical errors were identified:

• In line 616, there is an unnecessary trailing *.
• In line 1297, "msgSenderand" should be "msgSender and".
• In line 1502, "revered" should be "reverted".

Consider fixing all typographical errors in order to improve the overall code clarity.

Update: Resolved in pull request #831.

Review of Pull Request #805

As discussed in the scope section, while performing the fix review of the current audit we also reviewed pull request #805. The pull request introduces changes in naming of event and functions among other small changes.

Our highlighted notes from such review were the following:

• The SpokePool contract's fill function reverted from using abi.encodeCall to using abi.encode which is not type safe. We encourage keeping the type safety in place and use encodeCall instead.
• In the SpokePool contract, the natspec for pauseDeposits should include speedUpDeposit() and not speedUpV3Deposit().
• In the new fillV3Relay, the msg.sender is taken, while on fillRelay it is a specified repaymentAddress. Consider having consistent behaviour across these functions.
• Given the conversation spiked in the pull request comments, we encourage also the team to increase test coverage focusing on backwards compatibility.

Update: Resolved, the team fixed the first two points in commit a28eb83 in the same pull request. The team also informed us that they are in the process of expanding the test suite. About the third item, the team stated:

The reason of not adding this prop is to keep the interface equivalent to the current fillV3Relay method, as defined here. in the current implementation, this method defaults to setting the filler (msg.sender) as the repayment address. this ensures this method remains backwards compatible with the previous fillV3Relay method.

Client Reported

Duplicate Fill Execution

The client reported a critical-severity issue that would allow valid duplicate fills to be executed and could lead to loss of funds from the protocol.

Deposits in Across are uniquely identified using the hash of a V3RelayData struct. The proposed changes to the codebase introduced a change to the _getV3RelayHash function. The change modified the method of calculating the hash by only including the hash of the message field contained inside the V3RelayData struct, instead of hashing the struct in its entirety.

The issue is that upgrades to the SpokePool contracts can in practice only happen asynchronously. The filledStatuses mapping uses the hash of the relay data to record the status of the order. As the method of hash derivation differs between versions this means that there will exist two valid hashes for the same order, one pre-upgrade and another post-upgrade, which means the check to see if an order has been filled already will reference a different hash and thus fail to revert for an already filled order.

This has a critical interaction with off-chain components integrated with the protocol. If the relayers or bundlers use the hashed value of the relay data as a unique identifier to keep track of deposits (instead of using the depositId of each deposit), then these actors would not be able to recognize that an order has been filled already due to the different hash values. Consequently, the protocol and/or the relayers may lose funds if an order is filled twice, as only a single deposit would have been received.

Update: Resolved. The client has fixed this issue by reverting the proposed change to hash derivation.

Missing Valid Address Check

In the deposit function of the SpokePool contract there is no check that the value provided for the bytes32 parameters are valid EVM addresses. For example, an accidental submission of an incorrectly formatted depositor address would be impossible to refund in the event of no fill.

Update: Resolved. The client resolved the issue by introducing a valid address check on the depositor parameter. We encourage the team to repeat the same on all instances where a bytes32 value is intended to be used as an address.

Conclusion

The Across protocol enables rapid token transfers between Ethereum L1 and L2 chains. Users deposit funds, relayers transfer the funds to the destination, and depositors receive the amount minus a fee. The HubPool contract on L1 manages liquidity, coordinating with the SpokePool contracts on each supported chain.

The Across protocol's Solidity contracts have now been updated to support Solana, as well as providing support for ERC-7683 orders. The changes reviewed in this diff audit show thoughtful consideration for the codebase architecture in a context where demand for support of additional chains is likely to increase. We encourage the team to expand the test suite to include specific tests for the finalized ERC-7683 implementation.

We would like to thank the Risk Labs team for their willingness to answer questions and for providing helpful context throughout the audit.