Audit name:

[SCA] Aviator / SkyBridge / Jun2024

Date:

Aug 21, 2024

Table of Content

Introduction

Audit Summary

System Overview

Risks

Findings

Appendix 1. Severity Definitions

Appendix 2. Scope

Disclaimer

Introduction

We express our gratitude to the Aviator team for the collaborative engagement that enabled the execution of this Smart Contract Security Assessment.

SkyBridgeTM by Aviator Technologies, LLC is a custom bridge and launchpad solution between the Ethereum Layer-1 and the Base Layer-2 networks that supports any arbitrary ERC-20 or ERC-721 token. SkyBridge is built on the existing Optimism bridge, using it wherever possible to ensure the most secure transfer of funds. Most calls mirror the calls in the Optimism bridge, except for the fast bridging functionality. We also augmented the basic transfer functions in the L1 contract to collect fees to fund the liquidity pool used in L1 to enable fast transfers.

  • Document

    Name
    Smart Contract Code Review and Security Analysis Report for Aviator
    Audited By
    David Camps Novi, Viktor Lavrenenko
    Approved By
    Ataberk Yavuzer
    Changelog
    11/07/2024 - Preliminary Report; 21/08/2024 - Final Report
    Platform
    Base, Ethereum
    Language
    Solidity
    Tags
    Bridge, Signatures, EIP712, ERC721, ERC20, MinimalProxy

Audit Summary

26Total Findings
19Resolved
7Accepted
0Mitigated

The system users should acknowledge all the risks summed up in the risks section of the report

Documentation quality

  • Functional requirements are provided.

  • Technical description is partially provided:

    • The purpose of SkybridgeERC20 and SkyBridgeERC721 are not described in the documentation.

    • The project's design architecture is missing.

    • Sytem roles roles are partially described in the documentation.

Code quality

  • Best practices are not followed: F-2024-4229.

  • The development environment is configured.

  • Outdated solidity version: F-2024-4229.

  • Unused code present: F-2024-4219, F-2024-4226.

Test coverage

Code coverage of the project is 55.24% (branch coverage).

  • Deployment and basic user interactions are not covered with tests.

  • SkyBridge factories and tokens have no tests.

  • Test coverage is low in L1AviBridge and L2AviBridge, and particularly low in AviERC721Bridge.

  • Negative cases coverage is missed.

  • Interactions by several users are not tested thoroughly.

System Overview

SkyBridge is a bridge solution with the following contracts:

  • SkybridgeERC20  — upgradeable ERC-20 token, which is adopted from the OptimismMintableERC20. It has the minting and the burning functionality which is controlled by the bridge contract. It is minted or burned during the bridging operation of the IOptimismMintableERC20 tokens.

  • SkyBridgeERC721 - upgradeable ERC721 token, which is Optimism representation of an Ethereum-based token. It has the minting and burning functionality which is controlled. by the bridge contract. It is minted or burned during the bridging operation of the IOptimismMintableERC721 tokens.

  • SkybridgeERC20Factory - Factory contract for creating and deploying SkybridgeERC20 tokens.

  • SkyBridgeERC721 - Factory contract for creating OptimismMintableERC721 contracts.

  • L1AviBridge - The L1AviBridge is responsible for transfering ETH and ERC20 tokens between L1 and L2.

  • L1AviERC721Bridge - is a contract which works together with the L2AviERC721Bridge to make it possible to transfer ERC721 tokens from Ethereum to Optimism. This contract acts as an escrow for ERC721 tokens deposited into L2.

  • LiquidityPool - this contract is utilized to provide a central place to hold funds for fast bridging and the fees from Layer 1 to Layer 2 transfers.

  • L2AviBridge - the contract is responsible for transfering ETH and ERC20 tokens between L1 and L2. In the case that an ERC20 token is native to L2, it will be escrowed within this contract. If the ERC20 token is native to L1, it will be burnt.

  • L2AviERC721Bridge - is a contract which works together with the L1AviERC721Bridge to make it possible to transfer ERC721 tokens from Base to Ethereum. This contract acts as an escrow for ERC721 tokens deposited into L1.

  • AviPredeploys -  a library that contains hardcoded addresses.

  • AviBridge - a base abstract contract, for the L1 and L2 standard ERC20 bridges. It handles the core bridging logic, including escrowing tokens that are native to the local chain and minting/burning tokens that are native to the remote chain.

  • AviERC721Bridge - is. a base contract for the L1 and L2 ERC721 bridges.

Privileged roles

  • The SkybridgeERC20 has one privileged role, which is BRIDGE:

    • BRIDGE can mint and burn SkybridgeERC20 tokens

  • The SkyBridgeERC721 has one privileged role, which is BRIDGE:

    • BRIDGE can safeMint and burn SkyBridgeERC721 tokens

  • Abstract AviBridge has the following roles:

    • DEFAULT_ADMIN_ROLE, which can:

      • change the recipient of the flat fee via setFlatFeeRecipient()

      • change the flat fee via setFlatFee()

      • add and remove admins via addAdmin() and removeAdmin()

      • add and remove accounts from the PAUSER_ROLE via addPauser() and removePauser()

      • change the address of the backendUser via setBackend()

    • onlyOtherBridge, which can:

      • finalize the bridging operations via finalizeBridgeETH() and finalizeBridgeERC20()

  • Abstract AviERC721Bridge has the following roles:

    • DEFAULT_ADMIN_ROLE, which can:

      • change the flat fee via setFlatFee()

      • add and remove admins via addAdmin() and removeAdmin()

      • add and remove accounts from the PAUSER_ROLE via addPauser() and removePauser()

      • change the address of the backendUser via setBackend()

    • onlyEOA addresses, which can:

      • use bridgeERC721()

  • The L1AviBridge contract inherits from the abstract AviBridge's functionality and has the additional privileged roles:

    • DEFAULT_ADMIN_ROLE, which can:

      • set the bridging fee via setBridgingFee()

      • set the address of the other bridge via setOtherBridge()

      • set the address of the AVI token via setAviTokenAddress()

    • onlyPauserOrAdmin, which can:

      • pause the bridge's functionality via setPaused()

    • onlyEOA, which can:

      • transfer eth via receive() function

      • use the bridgeETH() functionality to bridge the ETH tokens

      • use the bridgeERC20() functionality to bridge ERC20 tokens

  • The L1AviERC721Bridge contract inherits from the AviERC721Bridge's functionality and has the additional privileged roles:

    • DEFAULT_ADMIN_ROLE, which can:

      • set the address of the other bridge via setOtherBridge()

      • finalize the bridging of ERC721 via finalizeBridgeERC721()

    • onlyPauserOrAdmin, which can:

      • pause the bridge's functionality via setPaused()

  • The L2AviBridge contract inherits from the AviBridge's functionality and has the additional privileged roles:

    • DEFAULT_ADMIN_ROLE, which can:

      • add and remove the allowed tokens via addAllowedToken() and removeAllowedToken() functions

    • onlyEOA, which can:

      • initiate the withdrawal from L2 to L1 via withdraw() or receive()

  • The L2AviERC721Bridge contract inherits from the AviERC721Bridge's functionality and has the additional privileged roles:

    • DEFAULT_ADMIN_ROLE, which can:

      • finalize the bridging of ERC721 via finalizeBridgeERC721()

  • The LiquidityPool has the following roles:

    • DEFAULT_ADMIN_ROLE:

      • can add and remove admins via addAdmin() and removeAdmin() functions

      • can withdraw ERC20 and ETH via withdraw functions

      • add and remove the bridge role via addBridge() and removeBridge() functions.

    • BRIDGE_ROLE:

      • can send ETH and ERC20 tokens via sendETH() and sendERC20() functions.

Risks

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.

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.

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.

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.

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.

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 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.

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.

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.

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.

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.

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.

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.

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

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.

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.

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.

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.

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.

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

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.

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.

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.

The project's contracts AviBridge and AviERC721Bridge 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.

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.

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.

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

Findings

Code
Title
Status
Severity
F-2024-4250
Incorrect Signature Validation Will Revert Execution
Fixed

High
F-2024-4214
Uninitialized flatFeeRecipient Leads to Loss of Fees
Fixed

High
F-2024-4262
Non-Standard ERC20 Tokens Result in Loss of Funds
Fixed

Low
F-2024-4260
Users Lose Their Fees If The LiquidityPool is Empty
Fixed

Low
F-2024-4254
Incorrect EIP712 Constructor Name
Fixed

Low
F-2024-4225
Lack of Refund Functionality Leads To Loss Of Funds
Fixed

Low
F-2024-4217
Public AccessControl Functionality Allows Bypassing of Checks
Fixed

Low
F-2024-4213
Missing Zero Address Validation Can Lead To Loss of Funds
Fixed

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

Observation
F-2024-4271
Bridge Contract Should have Specific Access Control
Fixed

Observation
1-10 of 26 findings

Appendix 1. Severity Definitions

When auditing smart contracts, Hacken is using a risk-based approach that considers Likelihood, Impact, Exploitability and Complexity metrics to evaluate findings and score severities.

Reference on how risk scoring is done is available through the repository in our Github organization:

  • Severity

    Critical

    Description

    Critical vulnerabilities are usually straightforward to exploit and can lead to the loss of user funds or contract state manipulation.

    Severity

    High

    Description

    High vulnerabilities are usually harder to exploit, requiring specific conditions, or have a more limited scope, but can still lead to the loss of user funds or contract state manipulation.

    Severity

    Medium

    Description

    Medium vulnerabilities are usually limited to state manipulations and, in most cases, cannot lead to asset loss. Contradictions and requirements violations. Major deviations from best practices are also in this category.

    Severity

    Low

    Description

    Major deviations from best practices or major Gas inefficiency. These issues will not have a significant impact on code execution, do not affect security score but can affect code quality score.

Appendix 2. Scope

The scope of the project includes the following smart contracts from the provided repository:

Contracts in Scope

factories
SkyBridgeERC20.sol - factories/SkyBridgeERC20.sol
SkyBridgeERC20Factory.sol - factories/SkyBridgeERC20Factory.sol
SkyBridgeERC721.sol - factories/SkyBridgeERC721.sol
SkyBridgeERC721Factory.sol - factories/SkyBridgeERC721Factory.sol
IOptimismMintableERC721.sol - factories/IOptimismMintableERC721.sol
IOptimismMintableERC20.sol - factories/IOptimismMintableERC20.sol
L1
L1AviBridge.sol - L1/L1AviBridge.sol
L1AviERC721Bridge.sol - L1/L1AviERC721Bridge.sol
LiquidityPool.sol - L1/LiquidityPool.sol
L2
L2AviBridge.sol - L2/L2AviBridge.sol
L2AviERC721Bridge.sol - L2/L2AviERC721Bridge.sol
libraries
AviPredeploys.sol - libraries/AviPredeploys.sol
universal
AviBridge.sol - universal/AviBridge.sol
AviERC721Bridge.sol - universal/AviERC721Bridge.sol

Disclaimer