OpenZeppelin performed a diff audit of the changes made to the SP1 Helios repository in pull request #2 at commit 6d554b9.
In scope were the following files:
.
├── contracts/src
│ └── SP1Helios.sol
├── primitives/src
│ ├── lib.rs
│ └── types.rs
└── program/src
└── main.rs
SP1 Helios is a ZK light client developed by Succinct that embeds Helios within the SP1 ZK VM. It is used to verify the consensus of a source chain in the execution environment of a destination chain. For example, SP1 Helios can be run on an L2 to verify Ethereum Mainnet's consensus. The process results in a ZK proof that proves that the light client successfully updated its state to the latest finality state on Ethereum as signed by the current validator set.
When executed inside SP1, Helios receives Ethereum beacon block headers, verifies that a certain state was included, checks all relevant consensus proofs (e.g., BLS signatures, sync committees, etc.), and updates its state. SP1 produces a succinct ZK proof of this execution, which is then verified by an on-chain verifier contract on the destination chain. As a result, the latter does not need to verify Ethereum's consensus directly. Instead, it verifies the SP1 proof, which attests that Helios ran correctly and saw valid Ethereum data.
The audited pull request modifies Succinct's original SP1 Helios implementation by adding storage slot proofs to the proving system. In addition, the verifier contract has been updated to accept valid L1 proofs and subsequently store the proven storage slots on L2. This update enables proving that a particular Ethereum contract holds a specific value in a given storage slot at a certain block number.
During the audit, it was assumed that the original implementation of SP1 Helios by Succinct was sound and secure. In addition, the following trust assumptions were made.
update function in the SP1Helios contract is restricted to authorized updaters:
SP1Helios contract will be managed in alignment with the community’s best interests.SP1Helios Contract Can End Up UnupdatableThe check on line 167 in the SP1Helios contract prevents updaters from updating storage slots against a head older than 1 week. If updaters fail to call the update function for a longer duration, such that the newest valid fromHead will itself be at least 1 week old, the update function will then always revert, causing the SP1Helios contract to end up in an unupdatable state.
Dependent contracts relying on the SP1Helios contract data (like oracles or bridges) can face unexpected reverts once the SP1Helios contract becomes permanently stuck. Systems assuming continuous updates would need to rely on emergency overrides or migrations, potentially breaking composability in unpredictable ways.
Consider implementing a failsafe mechanism to prevent permanent bricking. Alternatively, consider documenting the risks of the SP1Helios dependency as well as outlining possible mitigation steps to ensure dependent systems can recover gracefully.
Update: Acknowledged, not resolved. The team stated:
We've purposefully decided not to make the SP1Helios contract an Ownable contract because we want it to be "trustless" for anyone to use. Our architecture design is to encourage clients of this contract, like the UniversalSpokePool, to have failsafes in case this light client gets bricked. For example, the UniversalSpokePool has a comment here explaining what to do if the Helios contract gets bricked. This SpokePool is upgradeable and its implementation can be redeployed to point to a new Helios light client if neccessary. The SpokePool has replay protection built-in by storing all executed messages by their
messageNoncewhich can't be replayed even if the Helios contract gets redeployed, assuming the HubPoolStore doesn't also need to be upgraded and the Helios contract updates are secure and valid L1 state roots.
The SP1Helios contract inherits the AccessControlEnumerable contract to enable role-based access control mechanisms. According to the comments, the UPDATER_ROLE should be admin-less, making the updaters set immutable once deployed. As mentioned in the AccessControl contract, by default, the admin role for all roles is DEFAULT_ADMIN_ROLE, which means that only accounts with this role will be able to grant or revoke other roles.
However, the DEFAULT_ADMIN_ROLE's value is set to bytes32(0), making the call in line 141, where the UPDATER_ROLE's admin is being set to bytes32(0), superfluous. Moreover, the comment in the test in line 70 in SP1Helios.t.sol suggests that UPDATER_ROLE should be its own admin, which contradicts the previously mentioned comment in SP1Helios, that the UPDATER_ROLE should be admin less. In addition, the subsequent check in line 71 is only checking that the initialUpdater has the UPDATER_ROLE, and not that the initialUpdater is the admin of the UPDATER_ROLE.
Consider removing the _setRoleAdmin call in SP1Helios to ensure that UPDATER_ROLE has no admin. Since no address is assigned to the DEFAULT_ADMIN_ROLE, the UPDATER_ROLE's admin will remain DEFAULT_ADMIN_ROLE, with no address assigned to this role. Additionally, consider fixing the comment in the test to align with the documentation in the SP1Helios contract.
Update: Resolved in pull request #19.
Throughout the codebase, multiple instances of misleading documentation were identified:
storageValues mapping assumes that it maps from the tuple (blockNumber, contract, slot) instead of the key being computed by hashing these values.main.rs mentions the contract_trie_value_bytes and value variables. However, both of these values are unclear as to what they refer to. Consider clarifying this comment further. Additionally, consider replacing the single quotes with backticks when mentioning variables in comments.SP1Helios contract assume that the subsequent respective checks ensure that the new header has not been already set. However, the checks either ensure that the new header has not yet been set or that the new header is equal to the header set at the new head.Consider correcting the aforementioned comments to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #12.
Throughout the codebase, multiple instances of typographical errors were identified:
main.rs, "Asset" should be "Assert".SP1Helios.sol, "hasnt" should be "hasn't".SP1Helios.sol, "peroid" should be "period".Consider fixing the above-listed instances of typographical errors to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #11 at commit 31c682c.
unused_importsThe allow(unused_imports) attribute in line 1 of build.rs allows importing unused components. However, both imported components build_program_with_args and BuildArgs are used in lines 5 and 7, respectively.
Consider removing the allow(unused_imports) attribute to enhance code clarity and prevent confusion.
Update: Resolved in pull request #13.
Throughout the codebase, multiple opportunities for improved naming were identified:
SlotBehindHead error can be renamed to SlotNotAfterHead since it is thrown if the new head is behind or equal to the previous head.period variable in line 219 of the SP1Helios contract can be renamed to newPeriod.Consider renaming the variables specified above to improve code readability.
Update: Resolved in pull request #11 at commit 31c682c. The team stated:
Revert error name. Changed to
NonIncreasingHeadinstead.SlotBehindHeadwas used when we comparedpo.newHeadtoheadstored in the contract, so it made sense.SlotNotAfterHeadwould be inheriting the same semantics I feel.NonIncreasingHeadfelt more clear
In line 124 of main.rs, the clone is unnecessary since rlp_encoded_trie_account is not used later in the code. This makes it safe to pass the ownership to the verify_proof function.
Consider avoiding unnecessary cloning to improve the program's efficiency.
Update: Resolved in pull request #11 at commit e76912f.
Within SP1Helios.sol, multiple instances of missing docstrings were identified:
HeadUpdate eventSyncCommitteeUpdate eventStorageSlotVerified eventUpdaterAdded eventConsider thoroughly documenting all events (and their parameters) that are part of any contract. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #12.
In line 190 of the SP1Helios contract, the check fails if headers[po.newHead] is not empty and differs from po.newHeader. However, in line 194, headers[po.newHead] is updated regardless of whether its value was already equal to po.newHeader.
Similarly, in line 200, the check fails if executionStateRoots[po.newHead] is not empty and differs from po.executionStateRoot. Yet, in line 208, executionStateRoots[po.newHead] is updated even if it were already equal to po.executionStateRoot. In addition, the event in line 209 is always emitted when storage is updated, even if the value remains unchanged.
Consider modifying the logic to update storage and emit events only when values actually change. This would optimize gas usage and improve code clarity by avoiding redundant operations.
Update: Resolved in pull request #14 at commit 94123f5.
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.
Within SP1Helios.sol, multiple instances of mappings without named parameters were identified:
headers state variableexecutionStateRoots state variablesyncCommittees state variablestorageValues state variableConsider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.
Update: Resolved in pull request #15.
Within SP1Helios.sol, the GENESIS_VALIDATORS_ROOT and SOURCE_CHAIN_ID state variables are initialized but unused.
To improve the overall clarity and intent of the codebase, consider removing any unused state variables or clearly justify their presence in a comment.
Update: Resolved in pull request #21.
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.
The SP1Helios contract does not have a security contact.
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 #12.
Within SP1Helios.sol, the getCurrentEpoch and headTimestamp public functions have unnecessarily permissive visibility. Instead, their visibility can be limited to external.
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 #16.
require StatementsSince Solidity version 0.8.26, custom error support has been added to require statements. While initially this feature was only available through the IR pipeline, Solidity 0.8.27 extended support to the legacy pipeline as well.
The SP1Helios.sol contains multiple instances of if-revert statements that could be replaced with require statements:
if (params.updaters.length == 0) {
revert NoUpdatersProvided();
} statementif (headers[fromHead] == bytes32(0)) {
revert PreviousHeadNotSet(fromHead);
} statementif (block.timestamp - slotTimestamp(fromHead) > MAX_SLOT_AGE) {
revert PreviousHeadTooOld(fromHead);
} statementif (po.newHead <= fromHead) {
revert SlotBehindHead(po.newHead);
} statementif (currentSyncCommitteeHash != po.startSyncCommitteeHash) {
revert SyncCommitteeStartMismatch(po.startSyncCommitteeHash, currentSyncCommitteeHash);
} statementif (headers[po.newHead] != bytes32(0) && headers[po.newHead] != po.newHeader) {
revert InvalidHeaderRoot(po.newHead);
} statementif (
executionStateRoots[po.newHead] != bytes32(0)
&& executionStateRoots[po.newHead] != po.executionStateRoot
) {
revert InvalidStateRoot(po.newHead);
} statementif (syncCommittees[nextPeriod] != bytes32(0)) {
revert SyncCommitteeAlreadySet(nextPeriod);
} statementFor conciseness and gas savings, consider replacing if-revert statements with require ones.
Update: Resolved in pull request #20.
Using an outdated and unfixed version of Solidity can lead to vulnerabilities and unexpected behavior in contracts.
SP1Helios.sol has the solidity ^0.8.22 floating pragma directive. Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled. Moreover, this Solidity version is outdated.
Consider taking advantage of the latest Solidity version to improve the overall readability and security of the codebase. Regardless of which version of Solidity is used, consider pinning the version to prevent bugs due to incompatible future releases.
Update: Resolved in pull request #17.
++i) Can Save Gas in LoopsWithin SP1Helios.sol, the i++ operations [1] [2] could be optimized to save gas.
Consider using the prefix increment operator (++i) instead of the postfix increment operator (i++) in order to save gas. This optimization skips storing the value before the incremental operation, as the return value of the expression is ignored.
Update: Resolved in pull request #18.
The main function in main.rs is responsible for handling incoming proof inputs and generating a valid proof that can be later submitted to the SP1Helios contract. The proof generation process includes multiple validation steps. These include applying finality updates and verifying the execution of the state root proof.
Finality updates are verified by calling the verify_finality_update function in line 58. Among other checks, this function checks that the execution payload associated with the finality beacon block header and contained in the beacon block body is included in the SSZ Merkle trie rooted under the body_root field of the beacon block header. The execution payload itself contains the execution state root (state_root) which uniquely identifies the Ethereum global state.
The check is performed by verifying the Merkle path from the execution payload (as a leaf in the SSZ trie) to the root body_root, and it effectively binds the execution payload and the nested state root to the beacon header, thereby ensuring consistency across the consensus and execution layers.
The verification check in Helios follows the function call sequence given below:
verify_finality_update initiates the process by callingverify_generic_update, which then callsis_valid_header to validate the header structure, triggeringis_execution_payload_proof_valid for payload verification, which ultimately usesis_proof_valid which performs the final Merkle proof validation of the execution payload obtained from header.execution() against the body_root stored in header.beacon().body_root.After the finality update has been verified, it is applied to the client store via a call to apply_finality_update in line 71. The actual update of the store takes place in apply_update_no_quorum_check, and whether the function will be executed is determined by the should_apply_update flag. Specifically, the store gets updated (value of should_apply_update == true) if:
Assuming that both conditions are true, apply_update_no_quorum_check is called, which updates all fields of the store, except store.finalized_header (which also contains the execution payload, as stated above). The latter is updated at the end, but only under the condition that the update is for a slot that is strictly greater than the slot of the last finality update which the client updated to (the first part of condition 2. above).
Therefore, if we have a finality update for a slot that is equal to the slot of the last finality update stored in the store (i.e., update_finalized_slot == store.finalized_header.beacon().slot in this context), but for which we have a majority (thus satisfying condition 1.) and is also an update finalizing the next sync committee (thus satisfying condition 2.), then apply_update_no_quorum_check will be called, updating all fields of the store, except store.finalized_header. The latter will not be updated as this condition evaluates to false due to the identical slot numbers of the update and the last finalized block in the store. Consequently, the store will be updated with a header that has an invalid execution payload, thus breaking the consistency between the consensus and execution layers.
Importantly, the above scenario is not prevented in the SP1Helios verifier contract. Indeed, the verifier ensures that the slot of the update (po.newHead) is strictly greater than the last finalized slot in the store (fromHead), but does not assert that the latter is identical to po.prevHead, which is the actual last finalized slot committed to in the SP1 proof during client execution. In other words, the contract allows passing fromHead < po.newHead as input to the update function, so that this check passes in the verifier, while setting po.prevHead == po.newHead so that this condition fails in the client execution.
Consider properly binding fromHead to po.prevHead by removing it as the third input to the update function and setting fromHead = po.prevHead inside the function. In addition, on the client execution side after apply_finality_update, a check should be added to ensure that the slot of the finalized block which the client updated to is strictly greater than the initial slot prev_head which the client started the update from. This will ensure that the if-clause in apply_update_no_quorum_check has been satisfied and the execution payload has been correctly updated in the store.
Update: Resolved in pull request #11 at commit 31c682c.
The changes introduced to the SP1 Helios ZK Ethereum light client by Across Protocol in PR# 2 add new functionality by including proofs of storage slot values as part of the SP1 proof. This effectively allows the client to generate proofs for values of specific storage slots of L1 contracts and store them in the SP1Helios contract on L2. This is a desired feature in the cross-chain bridging context of the Across Protocol.
The report describes a client-reported issue that could allow invalid finality updates under specific conditions. This vulnerability made it possible for a client to successfully apply a finality update stored with an invalid execution payload, potentially leading to unintended consequences. The problem stemmed from inconsistencies across both in-scope and out-of-scope parts of the codebase. Aside from this, the implementation was found to be sound given the stated trust assumptions. Three low-severity issues were reported, along with several recommendations aimed at improving code quality and documentation. We commend the Across team for their responsiveness throughout the audit.