OpenZeppelin conducted a diff audit of the across-protocol/contracts repository at commit 71e990b of the solana-march-audit-2 branch.
In scope were the following pull requests:
Previously, when the recipient token account matched the relayer token account during a relayer fill, no token transfer would occur since the recipient and receiver would be the same account. In the solidity version, this was implemented by returning early, which also bypassed message handling and introduced a potential bug.
The implemented mitigation was to simply remove the early return behavior so that the messages were processed in every case. To maintain consistency with the Solidity version, the self-relay optimization in the Solana codebase was also removed.
This change removes the lower bound on the fill deadline during deposits, allowing the deadline to be in the past. This allows relayers to fill deposits optimistically, which may be desirable under some circumstances, or to avoid synchronization errors with discrepancies in block timestamps between different chains.
This change updates the naming convention throughout the codebase to match that of the Solidity counterpart. In particular, all references to "v3" have been removed.
In previous iterations of the SVM spoke program, the deposit route (i.e., the type of tokens deposited and the destination chain) had to be created by the admin and enabled. An error would be generated for attempted deposits using a route that had not been enabled. The change under review removes all logic related to creating and enabling route accounts, effectively removing all restrictions around which token types can be sent to which chains. While any route is now possible, since depositors require sending tokens to a vault account, the vault token account PDA must be initialized in order to receive tokens. A helper script is provided to initialize the vault PDAs, which can be executed by anyone.
Since all token transfers in the SVM SpokePool use SPL token transfers, native SOL must be wrapped in order to be deposited. A script demonstrating native deposits via wrapping has been added.
A deposit action to the SpokePool program is meant to pull tokens from the caller (i.e., the signer) which may be deposited on behalf of any depositor account. However, when the signer is not the depositor, a transfer_from operation is performed with the depositor token account as the source address. Such an operation would fail unless the depositor had delegated at least the input amount to the state PDA. This presents two consequences:
depositor_token_account and performing a successful deposit with arbitrary arguments other than the input token and amount. A malicious user could then submit a deposit with their own recipient address to steal depositor funds. This is possible due to the state PDA being passed as the authority and signer seed to the transfer_checked call, which, when delegated to, will pass the transfer validation logic. Note that this scenario is mitigated by the fact that native instruction batching in Solana transactions would typically involve delegation and transferring in one operation, making such a front-running scenario less likely but still possible.Consider validating the signer token account in the same way as depositor_token_account, and performing the transfer from the signer token account.
Update: Resolved in pull request #971, at commit 58f2665. The team stated:
We replaced the one “state” PDA with two distinct PDAs—one for deposit and one for fill_relay. Now users must explicitly delegate to the correct PDA before anyone can pull their tokens, restoring safe third‑party deposits and eliminating the risk of a single authority being misused to steal funds.
Pull request #939 removes route-enabling functionality in the SVM Spoke, allowing depositors to provide any input token for any destination chain, not only those with the route enabled. Previously, when a route was enabled, the Associated Token Account (ATA) of the vault for user deposits would be created. The DisabledRoute error would also be generated if a user tried to deposit to a disabled route. Since, by this point, all routes would be enabled, a user may try to submit a deposit where the vault ATA has not been initialized. This will cause the deposit transaction to fail without a descriptive error message.
Thus, if the vault has not been created, the depositor would have to initialize the PDA themselves, for example, using the createVault.ts script. However, this is not a simple process for the majority of users and it may be unclear why the deposit failed. This could cause users to abandon their deposit attempt and consider alternative bridging solutions. A more straightforward solution would be to initialize the vault during deposit using the init_if_needed flag as was previously done when the admin enabled a route.
Consider adding the init_if_needed flag or clearly documenting the desired vault creation process within the docstring for the deposit function.
Update: Resolved in pull request #957, at commit 3b8cf77. The team stated:
We added the
init_if_neededflag to thevaultaccount in thedepositmethod.
Pull request #861 renames several functions and types to remove the "V3" qualifier from the Solana spoke codebase. However, the renaming is incomplete:
HandleV3AcrossMessage struct passed to a handle_v3_across_message function, with a HANDLE_V3_ACROSS_MESSAGE_DISCRIMINATOR constant.nativeDeposit.ts mentions a "V3" deposit.Consider updating all functions and comments that mention "V3" for consistency with the rest of the code base.
Update: Partially resolved in pull request #964, at commit a78241f. The team stated:
We addressed the issue by removing the mention of "V3" from the rest of the SVM code base. The only exception to this is
handle_v3_across_messagefunction and its related structs and constants as "V3" notion is also kept in the EVM whereAcrossMessageHandlerstill has thehandleV3AcrossMessagein its interface.
Throughout the codebase, multiple instances of misleading documentation were identified:
nativeDeposit.ts states that the vault ATA is created when the route is enabled. However, route enabling has been removed in pull request #939.createVault.ts says that the script is used by the chain admin, but it can be used by anyone to create the vault PDA when necessary.Consider correcting the aforementioned comments to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #963, at commit 24017ac. The team stated:
We addressed the issue by clarifying the comments in the scripts.
The fill_relay function accepts a _relay_hash parameter but the FillRelay struct constraint calls it relay_hash (without the leading underscore).
To avoid confusion, consider maintaining consistent variable names when using the instruction constraint.
Update: Resolved in pull request #962, at commit 9f8a199. The team stated:
We addressed the issue by using the same variable names when using the
instructionconstraint.
The reviewed changes include incremental updates to the SVM Spoke. They slightly generalize the functionality and improve consistency with the EVM Spoke. No significant issues were in the codebase.
The Risk Labs team is appreciated for the extensive documentation and their forthcomingness throughout the audit period.