SkyBridge

SkyBridge Audit Report for Dummies

Stixil

Sep 15, 2024 — 12 min read

psst . . . hey you! yeah, you!!

Findings Explained

Now that we have completed our second audit with Hacken, we are incredibly proud to share that over these two revisions, there have been no critical issues, and all security issues have been resolved! (🎉) As a part of these audits, Hacken also reports various non-security suggestions called "Observations" that developers have the choice to accept or change.

Sometimes referred to as "Info" or "Informational" findings, these items do not affect the security of any audited code, and Hacken does not assign a severity level to these items in their methodology.

There were eighteen observational-only findings reported by our auditors, with eleven being fixed and seven being accepted.

Why Accepted and Not Fixed?

In the end, there were seven observations that our development team acknowledged and chose to leave in place. Most of these are a result of leftover code from our implementation of the Optimism Standard Bridge. As mentioned earlier, these are merely code observations—none of these issues are related to security and can be safely ignored.

The list of accepted findings below have been taken directly from the Audit report:

List of Accepted Findings

F-2024-4273 - Mismatch Between Off-Chain Messages and Function Selectors

This is how off-chain messages and function selectors are handled on the Standard Optimism Bridge which we are built upon, so we elected to leave this as is.

F-2024-4255 - Incorrect Bridge Address Initialization

This is intentional, as we cannot assign an address to OTHER_BRIDGE in the constructor until both bridges are deployed.

F-2024-4238 - Code Style Inconsistency

The Standard Optimism bridge contains these same inconsistencies from multiple collaborators, so we elected to leave it as is.

F-2024-4237 - onlyEOA Modifier Does Not Prevent Contracts From Calling the Bridge's Functionality

This is included directly from our implementation of the Standard Optimism Bridge, so we elected to leave it as is. SkyBridge does not restrict access from other Smart Contracts (Gnosis Safe, Third-Party Routers, etc.)

F-2024-4229 - Old Solidity Version May Result in Unsafe Code

This is a file with simple constants only and was not updated at the time of auditing. This has since been updated.

F-2024-4223 - Deprecated Optimism Predeploys Address Causing Unexpected Results

The Optimism Standard Bridge still uses this internally, so we elected to leave it as is.

F-2024-4222 - Usage of receive() Function for Bridging may Revert

This is included directly from our implementation of the Standard Optimism Bridge, so we elected to leave it as is. SkyBridge does not restrict access from other Smart Contracts (Gnosis Safe, Third-Party Routers, etc.)

Risks Explained

Hacken has identified several potential risks associated with SkyBridge that they believe users should be aware of, and we agree! Many of the identified risks are inherent in all bridges, including the bridge's ability to pause transactions and future upgrade plans for the protocol.

No protocol is without risks, and users should be aware of all potential risks associated with any platform they use to handle their assets. Let's quickly go over a few common risks that have been outlined in detail by Hacken.

Access Control

Many smart contracts will have an owner or admin address that can call special restricted functionality. Theoretically, a malicious admin user could wreak havoc on a trusted protocol.

In our case, the admin is able to set bridge and backend addresses, set fee addresses, update bridging fees, pause bridging for users, set the AVI address on L2, and manage other access roles. As previously documented in our Flightpaper, SkyBridge's only admin role account is a 5-7 multi-sig smart contract by Gnosis Safe. You can find the address of this multi-sig at skybridge-admin.eth. The 7 signers of this smart contract are listed and linked below:

All signers are KYC'd by SolidProof, with the founder and company KYC'd by Coinbase and registered with Fincen in the state of New Mexico.

Additionally, a special pauser role exists in SkyBridge which is granted to Hacken Extractor for emergency response in case of an exploit. Only the pauser and admin roles are able to pause the bridge.

Upgradeable Contracts

Proxy contracts are a type of smart contract which utilize “implementations” to point to a new contract version. Implementations can only be changed by the proxy contract’s admin, and allows for contracts to be upgraded at any time. Skybridge utilizes proxy contracts to allow for bug fixing, introducing new functionality, or to otherwise change bridge behavior. Theoretically, however, a malicious admin could push unsafe code in a proxy upgrade.

Hacken Risks Report

Below is a list of every risk identified by Hacken, as well as our reasonings and responses. As always, we invite you to ask any questions in our Discord or Telegram communities if you would like any further information or clarification!

The architecture of some contracts including L1AviBridge, L1AviERC721Bridge, L2AviBridge, LiquidityPool, L2AviERC721Bridge, SkyBridgeERC20Factory and SkyBridgeERC721Factory relies on administrative keys for critical operations. Centralized control over these keys presents a significant security risk, as compromise or misuse can lead to unauthorized actions or loss of funds. It is recommended to use a multi-signature wallet with a proper minimal signatures threshold.

This is currently a necessary feature in SkyBridge and is extensively documented in our Whitepaper and online documentation.

The contracts L1AviBridge and L2AviBridge provide users with the capability to specify a custom gasLimit for the transfer of Ether or ERC20 tokens. However, if the gasLimit is set incorrectly, it poses a risk that transactions may become indefinitely stuck in pending status or be reverted.

This is correct, as general user interaction sets a gasLimit when interacting with SkyBridge contracts through the web UX. gasLimit is left customizable for third-party developer integrations.

The protocol's contracts, including L1AviERC721Bridge and AviBridge, incorporate functionality that can be paused, such as finalizeBridgeERC721(), finalizeBridgeETH(), and finalizeBridgeERC20(). Pausing these finalization functions creates a risk that deposit or withdrawal transactions may not revert, potentially leading to the loss of users' funds.

Pausable functionality was introduced in between our two security audits as an emergency response in the event of an exploit. Pausing can only be called by the admin and pauser roles. The pauser role is granted to Hacken Extractor, Hacken’s post-deployment security solution for attack detection and prevention, for emergency intervention if an exploit response is triggered.

Hardcoded addresses in the AviPredeploys library and the OTHER_BRIDGE address from the L2AviBridge contract, which cannot be changed after its initialization, create a risk of issues if one of the aforementioned addresses will be changed in the future, but the Bridge contracts will not have any setter mechanism to update it.

We have elected to use upgradeable proxy addresses for L1AviBridge, L2AviBridge, L1AviERC721Bridge, L2AviERC721Bridge, and LiquidityPool to allow changes of implementation for major updates, bug-fixes, or other necessary changes for protocol features and security. These changes are only permissible with a 5-7 approval from the admin smart contract address.

The protocol is relying on the out-of-scope and off-chain functionality to handle its operations, which creates a risk of introducing new security concerns, due to the fact that this part of the project is out-of-scope, hence was not deeply reviewed within the current security assessment: CrossDomainMessenger, SafeCall, OptimismMintableERC20, Predeploys, IOptimismMintableERC721, ILegacyMintableERC20.

These are contracts inherited from Optimism and have been previously audited extensively.

The SkyBridge team is planning to add new chains to the current SkyBridge solution in the future, which can potentially introduce new security risks to the system.

The introduction of new Optimism-based chains—and later on non Optimism-based chains—will add increased cross-chain compatibility for the SkyBridge token standard. These additions will be third-party audited and tested before mainnet implementation.

The SkyBridgeERC20 token contract contains the function burn() to burn users' tokens. However, it lacks the _spendAllowance() function call, which means that the bridge calling SkyBridgeERC20::burn() can burn users' tokens without their approval. It creates a security risk that the BRIDGE will contain the address of other entity not related to the bridge, which will be able to burn users' tokens at its will.

This is intentional, as a SkyBridgeERC20 token contains a reference to its bridge address [see IOptimismMintableERC20.sol for the standard Optimism implementation] which enables minting and burning of an ERC20 token on L2. Burning is only called when a token is withdrawn from a L2 chain, and minting is only called when a token is deposited to an L2 chain.
While it is possible that BRIDGE could theoretically point to an incorrect bridge address and allow burning or minting of a single token, such an address would not be able to arbitrarily burn any tokens from said user address.
The internal bridge referenced in SkyBridgeERC20 is set on deployment and cannot be changed.

The project's contracts SkybridgeERC20 and SkyBridgeERC721 are upgradable, allowing the administrator to update the contract logic at any time. While this provides flexibility in addressing issues and evolving the project, it also introduces risks if upgrade processes are not properly managed or secured, potentially allowing for unauthorized changes that could compromise the project's integrity and security.

We have elected to use upgradeable proxy addresses for L1AviBridge, L2AviBridge, L1AviERC721Bridge, L2AviERC721Bridge, and LiquidityPool to allow changes of implementation for major updates, bug-fixes, or other necessary changes for protocol features and security. These changes are only permissible with a 5-7 approval from the admin smart contract address.

The testing of various contracts and workflows within the project is insufficient, leading to low reliability. It is crucial to emphasize that comprehensive testing is an essential component of any project. Failing to achieve complete test coverage poses significant risks and reduces the project's reliability.

Due to unit test coverage no longer being a factor in Hacken's audit scoring methodology, we elected to work on test coverage outside of the audit instead of during it.

The address of the flatFeeRecipient in the L1AviBridge contract should be an independent address according to the project's documentation. However, this address is setup to the same address as the LiquidityPool in the contract constructor(). As a consequence, the function _initiateETHDeposit() performs two calls to the same address, duplicating the transactions. Furthermore, the malicious address can be set as a flatFeeRecipient or LIQUIDITY_POOL, which can create additional security risks to the system.

This is correct as the flatFeeRecipient address defaults to LiquidityPool, however this value must be manually set to the correct address by the admin shortly after deployment.
SkyBridge is deployed with a false pause state of true, therefore this is always changed and verified before any transactions can take place.

The protocol has no methods to track users' assets and allow tokens to be retrieved in case of failure. There can be several reasons why the bridge operation may not succeed on the destination chain, and assets become stuck: native tokens being sent to a contract that cannot handle them, transactions not meeting the requirements criteria in the destination chain, lack of liquidity in the liquidity pool.

This is also the case in the Standard Optimism Bridge. It is theoretically possible for funds to become stuck in-transit, as they have previously in Optimism's Bridge. Users should be aware of the potential for loss in any bridge, including ones outside of Optimism's Superchain.
SkyBridge Supersonic withdrawals are resistant to this issue as funds are immediately credited to a user account.

Tokens can be retrieved from the LiquidityPool contract by the DEFAULT_ADMIN_ROLE via withdrawERC20() and withdrawETH(). Although this is a trusted address, the scenario is still possible and should be noted.

This is intended. In the event of a disaster recovery scenario, this allows for the bridge to be paused so that funds may be manually returned to users.

ERC721 tokens can be bridged into the protocol. Users should be aware that ERC721 are unique assets with specific properties and data, and they rely on the Aviator protocol to bridge the token features correctly.

SkyBridge creates a 1:1 peg of the L1 ERC721 token to be deployed. Standard on-chain features are preserved in this operation. Off-chain attributes and custom features rely on implementation from the NFT administrator.

The contract AviPredeploys contain hardcoded addresses that should be updated during project deployment.

SkyBridge is deployed with a false pause state of true, Therefore these addresses are always changed and verified before any transactions can take place.

Although the flat fee and the liquidity fee have a predefined value described in the documentation, the protocol has the capability to change these values.

This is correct. The admin role has the ability to change the flat fee from 0 to .005 ETH, and the liquidity fee from 0% to 10%.
The Aviator DAO has the sole authority to change the liquidity fee, which is collected for bridge liquidity and shall be used for LP rewards in a future update.

The base contracts AviBridge and AviERC721Bridge should introduce storage gaps →. When working with upgradeable contracts, it is necessary to introduce storage gaps to base contracts to allow a storage extension during upgrades. Storage gaps are a convention for reserving storage slots in a base contract, allowing future versions of that contract to use up those slots without affecting the storage layout of child contracts. Without a storage gap, the variable in the child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences for the child contracts.

This has since been fixed.

The following contracts lack the invocation of _disableInitializers in the constructor: L1AviBridge, L2AviBridge, L1AviERC721Bridge, L2AviERC721Bridge. Leaving an upgradeable smart contract uninitialized may lead to a takeover of the implementation contract, which could result in several undesired side effects.

This has since been fixed.

The state variables REMOTE_TOKEN, BRIDGE, DECIMALS and REMOTE_CHAIN_ID defined in the contracts SkyBridgeERC20 and SkyBridgeERC721 should be set as immutable to save gas.

This is a gas optimization issue and has since been fixed.

The Zero Address and Zero Value checks introduced in SkyBridgeERC721's constructor() are redundant since those addresses are originated in the corresponding factory, where they are already checked.

This redundancy, while present, does not affect bridge security.

The returned addresses newSkybridgeERC721 and newSkyBridgeERC20 should be checked not to be the Zero Address (0x0) to make sure the contract was created correctly.

SkyBridge always checks every deploy to ensure it was successfully executed before allowing users to deposit any L1 token to a new SkyBridgeERC20 L2 token.

The state variables L1AviERC721Bridge::_isPaused and AviBridge::_numAdmins will not be initialized to the defined values since they belong to an implementation contract. If said variables should have any value, they should be setup in the contract's initialization function.

SkyBridge is deployed with a false pause state of true. Therefore these addresses are always changed and verified before any transactions can take place.

Using the Ownable instead of Ownable2Step in the SkyBridgeERC20Factory and SkyBridgeERC721Factory creates a risk that the contract ownership will mistakenly be transferred to an address that cannot handle it.

There are no plans to change ownership away from the 5-7 multi-sig admin address.

If the deployment and the initialization of the implementation contracts, which are L1AviBridge, L1AviERC721Bridge, L2AviBridge and L2AviERC721Bridge, will not be handled within one transaction, it can cause the frontrunning of the initialize() functions by the attacker which will lead to unexpected system behavior.

This has been fixed by us using _disableInitializers() .

The base upgradeable contracts AviBridge and AviERC721Bridge lack the upgradeable import AccessControlUpgradeable, which creates a risk of problems in the future due to the missing gaps. Moreover, the initialize() functions of AviBridge and AviERC721Bridge should call the AccessControlUpgradeable::__AccessControl_init() function of the parent contract AccessControlUpgradeable.

This has since been fixed.

Public salt parameters of the SkyBridgeERC20Factory::createSkyBridgeERC20() and SkyBridgeERC721Factory::createSkyBridgeERC721() functions create a risk that external actors will use this data and deploy a contract to the generated address in advance.

This is not an issue per the steps done by create2 when providing a salt argument, per its documentation.

The system lacks the whitelisting for both ERC20 and ERC721 bridge operations, which can potentially break the system if malicious tokens are used.

This is intended behavior, as the protocol is unrestricted and does not impose whitelisting for token deployments.
There are expected misbehaviors with fee-on-transfer tokens and rebase tokens leading to potential accounting errors and partial loss. The end user accepts this possibility when deploying their assets using SkyBridge.

We hope you have found this guide helpful! If you have any additional questions or concerns, please reach out via Discord or Telegram. 🦊