Sonic Opera Native Token Bridge Audit

OpenZeppelin Security | December 9, 2024

Table of Contents

Summary

Type
DeFi
Timeline
From 2024-11-13
To 2024-11-15
Languages
Solidity
Total Issues
14 (14 resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
0 (0 resolved)
Medium Severity Issues
2 (2 resolved)
Low Severity Issues
3 (3 resolved)
Notes & Additional Information
9 (9 resolved)

Scope

We audited the Fantom-foundation/Opera-Bridge repository at commit 730e10b3.

In scope were the following files:

 contracts/
├── Errors.sol
└── OperaBridge.sol
ignition/
├── modules
│   ├── builder.ts
│   ├── opera.ts
│   └── sonic.ts
└── params.json

System Overview

The Opera Bridge

The OperaBridge contract is designed to facilitate the two-way bridging of native tokens exclusively between the Opera and Sonic networks, with each network acting as the peer chain for the other.

Deposits can only be made by externally owned accounts (EOAs) on either network via msg.value. Upon deposit, the OperaBridge contract records the transaction by emitting events that include the depositor's address, the amount (after deducting a fee), and a unique incrementing deposit ID.

On the peer network, a validator is responsible for packaging deposits in sequential batches to resolve them. Validators provide signatures that authorize the OperaBridge contract to transfer equivalent native tokens of the peer network to the designated EOAs.

Security Model and Trust Assumptions

The same contract will be deployed on both networks, meaning that the privileged roles will apply to both. These privileged roles possess the power to control all funds in the respective opera bridges. Below are the details of the trust assumptions.

Privileged Roles

Default Admin Role

The default admin role can set a minimal deposit value and the signature threshold for resolving deposits. More importantly, this role can set the reserve balance and draining address at any time. This setup presents a certain centralization risk, as the default admin role could arbitrarily lower the reserve balance and change reserveDrain to a controlled address, immediately draining all funds from the bridge contract.

Validator Role

The validator role can sign deposits of any amount and any deposit ID for any recipient. There is no explicit on-chain component to track if the deposit has been bridged from one chain to another correctly. Thus, one trusts the validator's role to relay the correct information across the chain. In the case where the exchange rate of one base token to another is involved, the validator role would need to perform the conversion with its oracle of choice.

Pauser Role

The pauser role can pause and unpause deposits into the OperaBridge.

We assume that the accounts in charge of the above roles and actions always act in the intended way. Hence, any attacks or vulnerabilities targeting this part of the system were not considered throughout this audit.

Deployment Script

In addition to auditing the smart contracts, the deployment script using Hardhat Ignition was reviewed. This was done to ensure that the script's configurations adhere to best practices and are correctly implemented. The review focused on the following components:

  • builder.ts: Responsible for defining the deployment logic, including setting up contract parameters and making necessary calls to initialize the deployed contract.
  • opera.ts: Handles the deployment of the OperaModule by utilizing the builder.ts script and defining the parameters specific to the Opera chain.
  • sonic.ts: Manages the deployment of the SonicModule, mirroring the logic in opera.ts but tailored for the Sonic chain.

Additionally, the values included in params.json were verified, as they serve as inputs for contract deployment and initialization.

As a result of the analysis, one medium-severity issue was identified:

  • Incorrect Chain ID Values in opera.ts and params.json

The issue is described in detail in the following sections of this report.

 

Medium Severity

Missing Chain ID and Contract Address in Message Hash

The _verifySignatures() function constructs msgHash using only the chain ID (peer chain), batch ID, total amount, and deposits. This makes signatures potentially replayable in different bridge contracts if they are all deployed on the same chain or if the contract is redeployed. This may allow the signatures to be replayed in a fork situation. For example, if an Ethereum to Sonic chain bridge is deployed on Ethereum in the future with the same signature structure, the signatures on the Opera bridge can be replayed on Ethereum, leading to stolen funds.

Note that for the signatures to match, the batchId and lastDepositId must remain synchronized between the two chains. However, an attacker could attempt to manipulate these values. If the lastDepositId is lagging, the attacker could spam deposits to increase it. Conversely, if the batchId is lagging, the attacker could spam deposits with a delay between each deposit to force them into separate batches.

Consider adding the block.chainid and address(this) to the msgHash struct or, ideally, implementing the complete EIP-712 structured data signing.

Update: Resolved in pull request #22. The Sonic team stated:

The intended destination chain ID and processing contract address was added into the message hash as suggested. The origin chain ID has already been encoded into the batch ID as shown by the test cases and is now explicitly confirmed by _isValidBatchSequence(), so this parameter was dropped from the message hash as being redundant.

Incorrect Chain ID Values in opera.ts and params.json

In opera.ts, the sonicChainId parameter is incorrectly set to 250 (0xFA) instead of the correct value 64165 (0xFAA5) for the Sonic test network. Similarly, in the params.json file, the operaChainId parameter is set to 64165 (0xFAA5) instead of the correct value 250 (0xFA). This discrepancy can cause deployments to fail or result in the contract interacting with the wrong chain.

Consider ensuring that the chain IDs are consistent and accurate at the moment of deployment. In addition, consider updating the sonicChainId in opera.ts to 64165 (0xFAA5) if using the test net and then to the correct Sonic mainnet chain ID when it is launched. Similarly, also change the operaChainId in params.json to 250 (0xFA) in order to avoid misconfiguration and deployment failures.

Update: Resolved in pull request #9 at commit dad2148. The Sonic team stated:

A new Chain ID will be used for the Sonic chain. The 0xFAA5 is a temporary testnet. The deployment configuration will need to be updated again before the actual deployment on the target environment, so we did not update it yet. Yet, the current configuration was indeed incorrect and was updated accordingly by the linked pull request.

Low Severity

Potential Impact of EIP-7702 on EOA Assumption

This issue arises in the context of the planned EIP-7702 update, which introduces the ability to set code for EOAs. While EIP-7702 is not yet live, it is expected to be implemented in early 2025 along with the Pectra update on Ethereum. With EIP-7702, code can be added to an EOA after deposit() has been called on one chain and before resolve() is called on the other. As such, this may impact chains where Opera Bridge is deployed.

Any account code added to the withdrawal adddress can cause the subcall to revert, specifically due to insufficient gas forwarded for the execution. This will prevent such accounts from receiving their withdrawal. Consider adopting a pull pattern instead of the current push model, where withdrawable balances are stored for each address, allowing users to claim their tokens individually. Alternatively, consider tracking failed withdrawals in storage so they can be retried later, ensuring that all users have a path to successful withdrawals.

Update: Resolved in pull request #21. The Sonic team stated:

The EIP-7702 is not relevant for the target deployment environment and expected time span of the bridge. Its sole purpose is to allow current Opera users to move their native token balance to Sonic. We do not expect the planed EIP-7702 to have any impact on the operations during this time period. Yet, we recognize the importance of future-proofing the project to preserve its potential value. The push pattern is one of the main requirements. Users should be able to obtain an initial native token balance to operate on the new Sonic blockchain without having to claim. The alternative recommended storing of failed deposits were added instead. Users now have the ability to claim these failed deposits to a different address as long as they can initiate the claim from the original recipient EOA.

Missing Verification for Total Sum of User Withdrawals

The _batchProcess function does not verify whether the sum of user withdrawals matches total. If the actual sum of withdrawals is greater than total, the balance check becomes ineffective, potentially skipping users and consuming their withdrawal amounts without transferring tokens.

As an additional security measure, consider tallying the individual withdrawal amounts in _batchProcess and ensuring that they equal total before proceeding. If the sums do not match, the transaction should revert to prevent unintended fund discrepancies.

Update: Resolved in pull request #10. The Sonic team stated:

The total batch amount is now built during processing and confirmed after with the originally provided value to revert the whole transaction if it doesn't match. We did not want to spend additional gas re-iterating the deposits in advance to reject the batch before processing starts. This would have to be added if the current solution is found inadequate. The total sum, being part of the message hash, is expected to be confirmed by validators. The validators' agreement on the message content is the enforced imperative, so this value calculation was moved off-chain where it is cheap. Still, this additional check improves reliability of the solution on the chain level.

Fee Can Change During Deposit

The deposit fee might change between the user signing the deposit() transaction and the transaction getting accepted on-chain. In such a case, the user could end up paying more fees than intended.

Consider adding an expected fee parameter to the deposit() function and reverting if the actual fee is different from the expected fee.

Update: Resolved in pull request #11. The Sonic team stated:

We don't expect the fee to change regularly. Yet, an update may result in an unexpected deduction from the swapped amount. We added the suggested parameter as proposed. A possible deposit revert will need to be handled by the UI/client app accordingly.

Notes & Additional Information

Variable Could Be immutable

If a variable is only ever assigned a value from within the constructor of a contract or during compile time, then it could be declared as immutable. The peerChainID state variable could be immutable.

To better convey the intended use of variables and to potentially save gas, consider adding the immutable keyword to variables that are only set in the constructor.

Update: Resolved in pull request #12.

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 and interfaces without 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 #13.

Incorrect Order of Modifiers

Function modifiers should be ordered as follows: visibility, mutability, virtual, override, and then custom modifiers. The receive function in OperaBridge.sol has an incorrect order of modifiers.

To improve the project's overall legibility, consider reordering the modifier order of functions as recommended by the Solidity Style Guide.

Update: Resolved in pull request #15.

File and Interface Name Mismatch

The Errors.sol file name does not match the BridgeErrors interface name.

To make the codebase easier to understand for developers and reviewers, consider renaming the files to match the interface names.

Update: Resolved in pull request #16. The Sonic team stated:

The linked pull request renames the BridgeErrors file as suggested.

Incomplete Docstrings

Within OperaBridge.sol, multiple instances of incomplete docstrings were identified:

Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of a contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

Update: Resolved in pull request #14. The Sonic team stated:

The pull request adds missing parameters' description as suggested.

Variable Name Suggestion

In OperaBridge.sol, the _lastDeposit name can be renamed to _lastResolvedDeposit to avoid confusion in relation to lastDepositId.

Update: Resolved in pull request #17. The Sonic team stated:

The variable was renamed as suggested.

Inefficient Use of uint16 Type

The OperaBridge contract uses uint16 for loop counters and return values in _processBatch() and _verifySignatures() functions. Given Solidity's type checking, this results in unnecessary overflow checks during incrementation since uint16 can overflow, unlike uint256 which is optimized by the compiler. Specifically, the inefficient usage appears in the loop counter i and validator count valid in _verifySignatures(), loop counter i in _processBatch(), and the signature count return value from _verifySignatures() used in comparison with signatureThreshold.

Consider changing all instances of uint16 to uint256 in both functions and their return types to optimize gas consumption and remove unnecessary safety checks.

Update: Resolved in pull request #18. The Sonic team stated:

All usages of uint16 type were changed to uint256 as suggested.

Unused Error

Unused code can negatively affect the clarity of the codebase. The DepositsDisabledSince error is not used.

Consider removing any unused errors to improve the readability and maintainability of the codebase.

Update: Resolved in pull request #19. The Sonic team stated:

Unused error declaration was removed.

Functions Are Updating the State Without Event Emissions

Within OperaBridge.sol, multiple instances of functions updating state without an event emission were identified:

Consider emitting events whenever there are state changes to make the codebase less error-prone and improve its readability.

Update: Resolved in pull request #20. The Sonic team stated:

State update events were added as recommended.

 
 

Conclusion

The Opera Bridge is intended for two-way base token bridging between the Opera and Sonic networks. Generally, we recommend adopting a pull pattern instead of a push pattern when it comes to base token transfers, relying less on the Validator role for deposit accounting, and for exchanges to be more transparent and decentralized in the future.

Request Audit

 

Related Posts