- December 12, 2025

OpenZeppelin Security

OpenZeppelin Security

OpenZeppelin Security

OpenZeppelin Security

Security Audits

Summary

Type: Cross-Chain
Timeline: From 2025-11-11 → To 2025-11-12
Languages: Solidity

Findings
Total issues: 7 (5 resolved)
Critical: 0 (0 resolved) · High: 0 (0 resolved) · Medium: 0 (0 resolved) · Low: 1 (0 resolved)

Notes & Additional Information
6 notes raised (5 resolved)

Scope

OpenZeppelin audited the changes introduced in the following pull requests:

In scope were the following files:

 contracts
└── src
    └── SP1Helios.sol

 contracts
├── SpokePool.sol
└── interfaces
    └── SpokePoolInterface.sol

System Overview

The changes under review introduce a new VKEY_UPDATER_ROLE in the SP1Helios contract. Now, the heliosProgramVkey value used in the update function can be changed by a specialized actor. The same role is set to be in charge of granting and revoking the role itself.

The specialized actor is meant to be the SpokePool contract, which has been introduced with an executeExternalCall function that is able to perform an arbitrary external call, including changing the heliosProgramVkey value in the SP1Helios contract. This is done so that SpokePool can also be used for administrative tasks, while allowing the flexibility to change the heliosProgramVkey parameter in SP1Helios without the need to re-deploy the contract itself.

The logic of the _fillRelayV3 function has been refactored into two private helper functions, namely _emitFilledRelayEvent and _transferTokensToRecipient. This has been done to resolve the "stack too deep" issue in the _fillRelayV3 function that arose due to upgrading the Solidity pragma to version 0.8.30.

The refactoring extracts the event emission logic into the _emitFilledRelayEvent function, and the token transfer logic into the _transferTokensToRecipient function. Both of these functions are called sequentially from within _fillRelayV3 to maintain the same execution flow and functionality.

Privileged Roles

While the executeExternalCall function is only callable by the admin of the pool due to the onlyAdmin modifier, it is also true that it is a general-purpose function that can make arbitrary external calls. As such, it is assumed that this feature will not be abused, since it increases the attack surface for potential issues if the admin ever gets compromised.

 

Low Severity

Missing Whitelist Protection in executeExternalCall

The executeExternalCall function only checks that the target is not the zero address and that calldata has at least 4 bytes. It does not restrict which contracts or functions can be called. While it is protected by the onlyAdmin modifier, this increases the risk of accidental calls and, in general, the attack surface if the admin is compromised. As an example, the call can be made to any ERC-20 token to drain the contract, or can be a reentrant call that allows for self calls. Apart from this, the adminship of SpokePool can be used to change the state of the governed contracts.

Consider adding whitelist mappings for target addresses and function selectors, validating them in executeExternalCall, and providing admin functions to manage the whitelists. Ideally, such privileged functions should respond to an alternate mechanism (e.g., a governance structure with a Timelock delay).

Update: Acknowledged, not resolved. The team stated:

We understand the argument for a whitelist, but we feel this overhead is more likely to cause delays in responding quickly to an active issue in the protocol than it is to protect against a mistake or attack on the HubPool. The admin (HubPool) already has substantial permissions to upgrade and transfer funds from the SpokePool as a part of the normal operation of the protocol. We don't view arbitrary calls as a practical increase in the privileges given to the HubPool to control or manage the Spoke. We don't see any specific reentrancy attacks or state manipulation attacks that could create an impact beyond what existing priviledges would allow.

Notes & Additional Information

Inconsistent Role Management

With the newly introduced VKEY_UPDATER_ROLE role, the docstrings in line #12 have become obsolete as the VKEY_UPDATER_ROLE is mutable. Additionally, it is now not clear why the UPDATER_ROLE is still restricted to be non-mutable, while the VKEY_UPDATER_ROLE is not.

Consider fixing the docstrings and being explicit regarding the intended behavior of the code, justifying why some roles might be non-mutable and why others might not.

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

We now use the DEFAULT_ADMIN_ROLE to manage both the VKEY_UPDATER_ROLE and the UPDATER_ROLE.

Unclear Event Emission

The HeliosProgramVkeyUpdated event is emitted with only the new value. Without the old value, off-chain tools cannot track the change history from a single event, and users cannot see what changed without querying historical events. Missing the old value makes debugging harder and limits transparency for a critical security parameter. Moreover, it is never checked that the new value is different from the current value, and thus, even if the old value is not being emitted, there might be repeated event emissions with the same value, thereby confusing off-chain indexers.

Consider updating the event to include both old and new heliosProgramVkey values and to add a check to prevent the same value from being set again.

Update: Resolved in pull request #54. No check has been added to prevent the same value from being set again.

Missing Event Emission in executeExternalCall

The executeExternalCall function does not emit events when executing external calls. As such, there is no record of which contracts the admin called and the calldata executed. This makes it hard to monitor admin behavior, investigate incidents, or provide transparency to users about executed operations.

Consider adding an event that logs the target address, calldata, and caller address whenever executeExternalCall is invoked.

Update: Resolved in pull request #1180.

Missing Zero Address Validation for vkeyUpdater in Constructor

The constructor calls _grantRole on VKEY_UPDATER_ROLE without checking if params.vkeyUpdater is address(0). This differs from the UPDATER_ROLE handling, which validates that the updater is not address(0) before granting the UPDATER_ROLE. If address(0) is granted VKEY_UPDATER_ROLE, no one can grant the role to a valid address later, since VKEY_UPDATER_ROLE is self-administered, thereby permanently locking the vkey update functionality. The contract would need to be redeployed if the vkey needs to change again.

To align with the UPDATER_ROLE validation, consider adding an address(0) check for params.vkeyUpdater before granting the VKEY_UPDATER_ROLE role.

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

This was fixed indirectly by the PR addressing N-01. address(0) is now checked and the role is left unset if the argument is set to address(0). Since the role is now mutable, being left unset will not lock the value.

Missing Return Data Size Cap in executeExternalCall

The executeExternalCall function does not limit the size of return data from external calls. When target.call(data) executes, the return data is copied into the returnData bytes memory variable, whereas copying large return data can cause transactions to run out of gas or become expensive. This is especially risky if executeExternalCall usage expands to call arbitrary external contracts, as an attacker could deploy a contract that returns extremely large data to cause a DoS of legitimate admin operations or drain gas.

Consider implementing a maximum return data size limit using assembly to cap the return data before copying it into memory.

Update: Acknowledged, not resolved. The team stated:

The intended use of executeExternalCall is to use the SpokePool as an "admin proxy" for the HubPool on destination chains, whereby it would interact with other contracts that are part of the Across system. It's not intended to be used with arbitrary contracts that could be manipulated by an attacker. We will take this DoS vector into account when constructing governance transactions in the future, though, in case there is a reason to interact with a non-protocol-governed contract.

Too Strict Visibility In Functions

The new _emitFilledRelayEvent and _transferTokensToRecipient functions of the SpokePool contracts were introduced to solve a stack too deep error in the _fillRelayV3 function. Those are defined as private while the _fillRelayV3 is defined as internal.

By making those two new functions internal, they can be reused by child contracts. Since they seem to represent primitive actions, consider whether this is something that future developments can benefit from and eventually change the private visibility to internal instead.

Update: Resolved in pull request #1179. 

Conclusion

The changes made to SP1Helios and SpokePool enable a new feature whereby the SpokePool contract can handle administrative tasks such as changing the heliosProgramVkey parameter in the SP1Helios contract, which now also features mutable roles and permissions. The _fillRelayV3 function of the SpokePool contract has been refactored into two private helper functions to resolve the "stack too deep" issue while maintaining the same functionality and execution flow.

No major issues were identified, with key recommendations given to increase the overall robustness of the codebase. 

Appendix

Issue Classification

OpenZeppelin classifies smart contract vulnerabilities on a 5-level scale:

  • Critical
  • High
  • Medium
  • Low
  • Note/Information

Critical Severity

This classification is applied when the issue’s impact is catastrophic, threatening extensive damage to the client's reputation and/or causing severe financial loss to the client or users. The likelihood of exploitation can be high, warranting a swift response. Critical issues typically involve significant risks such as the permanent loss or locking of a large volume of users' sensitive assets or the failure of core system functionalities without viable mitigations. These issues demand immediate attention due to their potential to compromise system integrity or user trust significantly.

High Severity

These issues are characterized by the potential to substantially impact the client’s reputation and/or result in considerable financial losses. The likelihood of exploitation is significant, warranting a swift response. Such issues might include temporary loss or locking of a significant number of users' sensitive assets or disruptions to critical system functionalities, albeit with potential, yet limited, mitigations available. The emphasis is on the significant but not always catastrophic effects on system operation or asset security, necessitating prompt and effective remediation.

Medium Severity

Issues classified as being of medium severity can lead to a noticeable negative impact on the client's reputation and/or moderate financial losses. Such issues, if left unattended, have a moderate likelihood of being exploited or may cause unwanted side effects in the system. These issues are typically confined to a smaller subset of users' sensitive assets or might involve deviations from the specified system design that, while not directly financial in nature, compromise system integrity or user experience. The focus here is on issues that pose a real but contained risk, warranting timely attention to prevent escalation.

Low Severity

Low-severity issues are those that have a low impact on the client's operations and/or reputation. These issues may represent minor risks or inefficiencies to the client's specific business model. They are identified as areas for improvement that, while not urgent, could enhance the security and quality of the codebase if addressed.

Notes & Additional Information Severity

This category is reserved for issues that, despite having a minimal impact, are still important to resolve. Addressing these issues contributes to the overall security posture and code quality improvement but does not require immediate action. It reflects a commitment to maintaining high standards and continuous improvement, even in areas that do not pose immediate risks.

 

Request an Audit →