Aviator

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.