Sonic Gateway Audit

OpenZeppelin Security | December 13, 2024

Table of Contents

Summary

Type
DeFi
Timeline
From 2024-09-24
To 2024-10-21
Languages
Solidity, Golang
Total Issues
25 (2 resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
1 (1 resolved)
Medium Severity Issues
3 (1 resolved)
Low Severity Issues
8 (0 resolved)
Notes & Additional Information
13 (0 resolved)

Scope

We audited the Fantom-foundation/Bridge repository at commit 558465d.

In scope were the following files:

 Bridge-1.0.0-rc.1
├── cmd
│   ├── relay
│   │   ├── main.go
│   │   ├── relay.go
│   │   └── sender.go
│   ├── voter
│   │   ├── main.go
│   │   └── voter.go
├── contracts
│   ├── interfaces
│   │   ├── IActivityMonitor.sol
│   │   ├── IMintedBurnableERC20.sol
│   │   ├── IProofVerifier.sol
│   │   ├── IProvingContract.sol
│   │   ├── ISFC.sol
│   │   ├── IStateOracle.sol
│   │   ├── ITokenDeposit.sol
│   │   ├── ITokenPairs.sol
│   │   └── IUpdateVerifier.sol
│   ├── ActivityMonitor.sol
│   ├── Bridge.sol
│   ├── DirectExitAdministrator.sol
│   ├── ExitAdministrator.sol
│   ├── MPTProofVerifier.sol
│   ├── MessageBus.sol
│   ├── MintedERC20.sol
│   ├── SFCUpdateVerifier.sol
│   ├── StateOracle.sol
│   ├── TokenDeposit.sol
│   ├── TokenPairs.sol
│   ├── UpdateManager.sol
│   └── ValidatorsRegistry.sol
└── internal
    ├── collector
    │   ├── collector.go
    │   └── signature.go
    ├── rpc
    │   ├── chainlistener.go
    │   └── transact.go
    ├── signing
    │   ├── keystore.go
    │   └── signing.go
    ├── validators
    │   └── validators.go
    └── voting
        └── voting.go

System Overview

Sonic Gateway is a protocol designed to enable users to transfer approved ERC-20 tokens between the Ethereum network and the Sonic mainnet. The protocol consists of two main components:

  1. On-Chain Smart Contracts: Deployed on both Ethereum and Sonic networks, these contracts handle the token locking, burning, and verification processes required for asset transfers.
  2. Off-Chain Inter-Blockchain Communication (IBC) Relay Network: A network of validators responsible for updating the state information between the two blockchains. They ensure that Merkle Roots that represent the current state of the bridge are accurately relayed and updated across both chains.

Bridging from Ethereum to Sonic

Users initiate a transfer by locking their ERC-20 tokens in the TokenDeposit Bridge contract on Ethereum. This action creates a pending transfer that will later be completed on the Sonic chain. The IBC Relay network then updates the Merkle Root on the Sonic chain's StateOracle contract to reflect the state that includes this pending transfer. Once the Merkle Root is updated, users can generate a Merkle Proof of their deposit and submit it to the Bridge contract on the Sonic chain. Upon verification, the Bridge contract mints the IOU tokens on Sonic, completing the transfer.

Bridging from Sonic to Ethereum

To transfer assets back to Ethereum, users burn their IOU tokens on the Sonic chain, creating a pending withdrawal. The IBC Relay network updates the Merkle Root on the Ethereum StateOracle contract that includes the state with this pending withdrawal. Users then generate a Merkle Proof of their burn tokens and submit it to the TokenDeposit contract on Ethereum. Upon successful verification, the contract releases the locked ERC-20 tokens back to the users' address on Ethereum.

Privileged Roles and Trust Assumptions

Security Model

The security of the Sonic Gateway protocol relies on a combination of properly functioning smart contracts, multisignature wallets controlling administrative roles, and the honest operation of the IBC Relay network:

UpdateManager Contract: Acts as the central point for updating the parameters and smart contracts of the bridge.

Multisignature Wallets: Hold critical administrative roles across various contracts. Compromise of these wallets could lead to unauthorized modifications, including the ability to update the smart contracts.

IBC Relay Network: Validators in the relay network are responsible for accurately updating Merkle Roots across chains. They must operate honestly and maintain consensus to prevent incorrect state updates, which could lead to fund losses or denial of service. The Sonic Gateway protocol involves several smart contracts with specific administrative roles and permissions. Proper management of these roles is crucial for the security and integrity of the bridge. The following are some of their administrative roles alongside their associated capabilities:

  • ActivityMonitor has an admin role that will be held by the UpdateManager contract. This allows for the update function to call markActivity for active validators.
  • Bridge has an admin role held by a multisig. They are able to initialize the DepositToken address after deployment via setTokenDepositAddress function. This address is not known upon deployment and this function can only be called once. The admin is also able to update the proofVerifier address via the setProofVerifier function.
  • ExitAdministrator has two roles, the DEFAULT_ADMIN_ROLE held by a multisig and the IMPLEMENTATION_ROLE held by the current implementation of the DirectExitAdministrator contract. The DEFAULT_ADMIN_ROLE will be able to grant or revoke other roles, while the IMPLEMENTATION_ROLE is able to call the withdrawWhileDead. This raises a trust concern whereby if the DEFAULT_ADMIN_ROLE is compromised, they will be able to grant the IMPLEMENTATION_ROLE to any address, allowing the compromised address to withdraw all funds locked in the TokenDeposit. Great care should be taken with the multisig holding the DEFAULT_ADMIN_ROLE to prevent such an attack from materializing.
  • StateOracle has an admin role held by the UpdateManager contract. This allows the update function to call the update function of the StateOracle.
  • TokenDeposit has an admin role held by a multisig that allows them to update the proofVerifier address via the setProofVerifier function.
  • TokenPairs has two roles, the DEFAULT_ADMIN_ROLE and the REGISTER_ROLE, both held by a multisig. The DEFAULT_ADMIN_ROLE will be able to grant or revoke other roles, while the REGISTER_ROLE will be able to call the register function to register Ethereum ERC-20 tokens to L2 tokens minted by the bridge.
  • UpdateManager has a few different roles held by a multisig. The DEFAULT_ADMIN_ROLE will be able to grant or revoke other roles. HEARTBEAT_ROLE will be able to call both the setHeartbeat and the setFastLanePadding functions. FEE_SETTER_ROLE has the ability to update the fast lane fee via the setFastLaneFee function. Lastly, the MONITOR_SETTER_ROLE will be able to call setActivityMonitor to update the address of the ActivityMonitor contract.
  • ValidatorsRegistry has an admin held by the UpdateManager contract, allowing the update function to call the setValidators function.
 

High Severity

Incorrect Update Order in UpdateManager

In the UpdateManager contract, the update function incorrectly calls setValidators on the previous updateVerifier instance instead of calling it on the newly updated one. This results in the new verifier contract being set without valid validators. Consequently, the UpdateManager contract could get bricked because when verifyUpdate is called on the new verifier, it would revert due to the absence of validators.

Consider setting the new verified address before calling setValidators to ensure that the correct instance is used.

Update: Resolved in pull request #62.

Medium Severity

User-Provided Deposit and Withdraw IDs Can Cause Collisions

The Bridge and TokenDeposit contracts rely on users to provide unique IDs for claiming and withdrawal operations, as they lack an internal mechanism to generate unique IDs. This design can result in multiple users submitting a transaction with the same ID, either by mistake or even on purpose by malicious users, who then monitor the pending transactions and submit a transaction with the same ID before the original transaction is processed. Since the contract requires each ID to be unique, after the first transaction with a specific ID, all other transactions with the same ID will fail. This can be abused to cause a denial of service for a particular account by front-running its transaction with the same ID.

Consider implementing an internal unique ID generation mechanism to prevent ID collisions and mitigate front-running vulnerabilities.

Users with Account Abstraction Wallets May Lose Bridged Funds Due to Address Mismatch

When using account abstraction wallets, users may have different addresses across chains for the same account. If a user using account abstraction deposits tokens via the bridge and later attempts to claim them on the other chain while having a different address, the transaction will revert due to an address mismatch.

When the user deposits tokens on L1 using the deposit function of the TokenDeposit contract, the deposit is recorded using a hash of the L1 address msg.sender, the token address, and the amount. On L2, when attempting to claim the tokens via the claim function of the Bridge contract, the L2 address derived from the account abstraction wallet could differ from the L1 address, causing the proof verification to fail in the verifyProof. As a result, the only way to retrieve the deposited tokens would be for the bridge to be marked as dead so that the user could call cancelDepositWhileDead. However, this would be an extremely unlikely scenario, and the user would most likely experience a permanent loss of funds.

Consider clearly documenting the fact that the bridge requires the same user address on both networks for proper functionality, which may not be the case with account abstraction wallets.

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

We fixed it by including msg.sender in the internal unique ID.

payFastLane Could Be Continuously Blocked by Exploiting selfdestruct

A malicious contract could exploit the selfdestruct opcode to send ETH directly to the UpdateManager contract, blocking calls to the payFastLane function until the next call to update. This occurs due to the strict equality check of the contract's balance having to be equal to the msg.value. Since the contract balance only returns to zero after a successful call to update, an attacker could even front-run legitimate calls to update and recover their ETH, and keep repeating the process to block the payFastLane functionality.

Consider implementing an alternative mechanism to determine whether the fast lane is in use.

Low Severity

Lack of Deregistration Mechanism for TokenPairs

The TokenPairs contract lacks a mechanism to remove registered token pairs. Without the ability to remove token pairs, the bridge may continue supporting tokens that have been renamed, are no longer relevant, have been found to have security vulnerabilities, or no longer meet the acceptance criteria for being used in the bridge.

Consider implementing a de-registration function to allow for the removal of token pairs.

Floating Pragma

Throughout the codebase, multiple instances of floating pragma directives were identified. Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.

Consider using fixed pragma directives.

Missing Zero-Address Checks

When assigning an address from a user-provided parameter, it is crucial to ensure that the address is not set to zero. Setting an address to zero is problematic because it has special burn/renounce connotations. As such, this action should be handled by a separate function to prevent accidental loss of access during value or ownership transfers.

Throughout the codebase, multiple instances of assignment operations missing zero-address checks were identified. As such, consider adding a zero address check before assigning a state variable.

Possible Precision Loss Due to Division Before Multiplication

Solidity's integer division truncates the result. Thus, performing division before multiplication can lead to precision loss. Throughout the codebase, multiple instances of multiplications that can lead to precision loss were identified:

  • The multiplication * within the SFCUpdateVerifier.sol file.
  • The multiplication * within the ValidatorsRegistry.sol file.

Performing multiplication before division is generally better to avoid loss of precision. As such, consider performing multiplication before division.

extendValidatorsArray Can Create Gaps in validatorAddress Array

In the ValidatorsRegistry contract, the extendValidatorsArray function used in setValidators can introduce gaps in the validatorAddress array. When setValidators is called by the update function in the UpdateManager contract, it invokes the first if condition when validatorAddress.length is less than or equal to the newValidators.id. In such cases, extendValidatorsArray is called with newValidators.id + 1. This can lead to an issue where, for example, if the array currently holds 5 addresses with IDs 1–5, and a new validator has ID 10, the array will be extended to include the new validator but will leave uninitialized address(0) gaps in between.

Consider modifying the array extension mechanism to prevent uninitialized gaps in the validatorAddress array.

Infinite Loop in Subscription Retry Mechanism

In the Listen function of ChainWatcher, when a subscription error occurs, it immediately attempts to resubscribe without any delay or back-off mechanism. If there is a network error or any other persistent error, it can lead to a loop of unsubscribing and attempting to subscribe. Continuous retrying without delay can lead to unnecessary CPU usage and network strain, potentially degrading performance.

Consider implementing an exponential back-off strategy for retry attempts.

Quorum Calculation Rounds Down, Allowing Insufficient Validator Approval

In the getQuorumSignatures function, the quorum is calculated as two-thirds of the total validator weight. However, the calculation uses integer division which rounds down any fractional result. When totalWeight is not divisible by three, this rounding down leads to a quorum that is lower than two-thirds of the totalWeight. For example, if the totalWeight is 10, two-thirds would be approximately 6.66, which should round up to 7. However, the current implementation rounds it down to 6.

Consider adjusting the quorum calculation to ensure that it correctly rounds up to the nearest integer when necessary.

Updating Too Many Validator in a Single Update May Cause Out-of-Gas Error

The LookupValidatorsChanges function is responsible for returning a list of validators whose addresses or weights have changed between two blocks. This list is then used to create a transaction to update the validators.

If a lot of validators are modified between blocks, the size of the transaction required to process these updates may exceed the gas limit, resulting in an out-of-gas error and transaction failure. Such a failure could prevent the validator set from being updated correctly, potentially causing delays or disruptions in network operation.

To avoid exceeding gas limits, consider implementing a cap on the number of validators processed per update transaction. Alternatively, consider introducing a mechanism to batch updates across multiple transactions.

Notes & Additional Information

Lack of Length Check on Proof Items in MPTProofVerifier

The MPTProofVerifier contract does not perform a length check on proofItems before accessing its elements. The verifyProof function assumes that the proofItems array contains at least two elements to retrieve the account and storage proofs. Without validating the length of the proofItems array, an empty or insufficiently populated proof could result in an out-of-bounds array access error.

Consider adding a check with a custom error to ensure that the proof array has the correct structure before proceeding with verification.

Missing Length Check for Public Key in SFCUpdateVerifier

In the SFCUpdateVerifier contract, the validatorAddress function does not validate that the public key retrieved from ISFC(sfc).getValidatorPubkey(index) is at least 66 bytes long. If the length of the public key is less than 66 bytes, this access results in an out-of-bounds memory error, leading to a revert or undefined behavior.

Consider adding a check to ensure that the public key is at least 66 bytes long before proceeding with the substring extraction. This will help prevent out-of-bounds access and improve contract robustness.

Improper Node ID Handling in MerkleTrie Library

The MerkleTrie library from Optimism that is utilized by the MPTProofVerifier contract deviates from the Ethereum Merkle Patricia trie specification in the way that it handles node IDs for nodes with RLP-encoded lengths greater than or equal to 32 bytes. Specifically, the _getNodeID function returns the decoded bytes instead of the Keccak-256 hash of the RLP encoding for such nodes.

While this implementation error could theoretically allow an attacker to craft node ID collisions and forge inclusion proofs, exploiting this vulnerability in practice is currently computationally infeasible due to the collision resistance of the Keccak-256 hash function.

To ensure correctness and future-proofing, consider raising this issue with the library maintainers to align the implementation with the Ethereum specification.

Missing Docstrings

Many of the contracts and functions in the Sonic codebase lack documentation. This hinders reviewers' understanding of the code's intention, which is fundamental to correctly assessing not only security, but also correctness of the code. In addition, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, and the values returned and the events emitted.

Consider thoroughly documenting all functions (and their parameters) that are part of the contracts' public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

Move Checks to Fail Earlier

A few functions have checks that could be performed earlier in the function body. This is so that in case of check failure, the function fails early and users save gas. Two checks were identified to have this issue:

  • The "Already withdrawn" check performed in the withdraw function can be moved to just after getting the withdrawHash.
  • The "No minted token for given token" check performed in the mintedToken function can be moved to the start of the function.

Consider implementing the aforementioned suggestions so that the functions fail earlier and save users gas.

Lack of Security Contact

Throughout the codebase, there are contracts that do not have a 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.

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.

Missing Named Parameters in Mappings

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.

Throughout the codebase, multiple instances of mappings without named parameters were identified:

Consider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.

Non-Explicit Imports

The use of non-explicit imports throughout the codebase decreases code clarity and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity file or when inheritance chains are long.

Following the principle that clearer code is better code, consider using the named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.

Unnecessary Casts

Throughout the codebase, multiple instances of unnecessary casts were identified:

To improve the overall clarity and intent of the codebase, consider removing any unnecessary casts.

Variables 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. Throughout the codebase, multiple instances of variables that could be made immutable were identified:

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.

Lack of Indexed Event Parameters

Throughout the codebase, multiple instances of events missing indexed parameters were identified:

To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.

Use calldata Instead of memory

When dealing with the parameters of external functions, it is more gas-efficient to read their arguments directly from calldata instead of storing them to memory. calldata is a read-only region of memory that contains the arguments of incoming external function calls. This makes using calldata as the data location for such parameters cheaper and more efficient compared to memory. Thus, using calldata in such situations will generally save gas and improve the performance of a smart contract.

Throughout the codebase, multiple instances where function parameters should use calldata instead of memory were identified:

  • In SFCUpdateVerifier.sol, the None parameter
  • In ValidatorsRegistry.sol, the newValidators parameter

Consider using calldata as the data location for the parameters of external functions to optimize gas usage.

Functions Updating State Without Event Emissions

Throughout the codebase, multiple instances of functions updating the 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. 

Conclusion

The audited codebase, which includes both on-chain smart contracts and off-chain components, allows bridging specific ERC-20 tokens between the Ethereum network and the Sonic mainnet. Validators play a critical role by voting on the next state root hash and block height to be transmitted between the blockchains, ensuring the security of cross-chain operations.

Throughout the audit, we identified several security issues and provided actionable recommendations to address them. In addition, we suggested best practices to enhance the overall robustness and readability of the codebase.

The Sonic Gateway team was responsive and collaborative throughout the audit process. The project documentation was clear and comprehensive, which greatly facilitated our understanding of the system's architecture and functionalities. However, we note that the codebase would benefit from more detailed inline documentation to assist future developers and auditors in navigating the code.

 

Request Audit

 

Related Posts