QA Report

[code423n4]

Low

Front-running init

To initialize contract parameters, most contracts employ an init pattern (rather than a constructor). They are vulnerable to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attack unless they are enforced to be atomic with contact deployment via deployment script or factory contracts.

Many init routines (such as DiamondInit.init) lack an explicit event emission, making it difficult to monitor such scenarios.

Affected source code:

• DiamondInit.sol#L36

Ownable / Pausable

The contract OwnerPausableUpgradeable is Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while paused should be avoided.

Affected source code:

• OwnerPausableUpgradeable.sol#L13

Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

If the owner calls the following methods, it does not verify that the proposed address is valid.

Affected source code:

• ProposedOwnable.sol#L123
• BridgeToken.sol#L202
• ConnextPriceOracle.sol#L168

Outdated compiler

The pragma version used are:

pragma solidity >=0.6.11;
pragma solidity 0.8.14;
pragma solidity >=0.7.6;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

Outdated packages

The packages used are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "4.5.0",
"@openzeppelin/contracts-upgradeable": "4.5.2",

Lack of checks

Check for address(0) during constructor, otherwise it could lead to bad initialization, bad implementations, or bad admin changes.

Affected source code without checking address(0):

• DiamondInit.sol#L42
• PromiseRouter.sol#L146
• PromiseRouter.sol#L155
• RelayerFeeRouter.sol#L81
• RelayerFeeRouter.sol#L90
• XAppConnectionClient.sol#L28
• XAppConnectionClient.sol#L40
• NomadFacet.sol#L26
• LibDiamond.sol#L54
• ConnextPriceOracle.sol#L77
• Executor.sol#L48
• SponsorVault.sol#L127

Wrong comments

Router who took out the credit, and router is not used:

• PortalFacet.sol#L126

Asserts caller is the router owner (if set) or the router itself, the router is only checked if there are no owner:

• RoutersFacet.sol#L156

Todo's left in code

TODO usually mean something still have to be checked of done. This could lead to vulnerabilities if not verified.

Affected source code:

• BridgeFacet.sol#L492
• BridgeFacet.sol#L579

And this seems more important, because routers can repay any-amount out-of-band using the repayAavePortal method or by interacting with the aave contracts directly:

• BridgeFacet.sol#L594-L595

Non Critical

Use encode instead of encodePacked for hashig

Use of abi.encodePacked in ConnextMessage is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

keccak256(abi.encodePacked(bytes(_name).length, _name, bytes(_symbol).length, _symbol, _decimals));:

• ConnextMessage.sol#L178

Double emits

The methods pause and unpause of ProposedOwnableFacet.sol#L253-L261 contract does not check that the contract is already paused or not, it could produce double emits and dApps could be affected.

Codding style

• The file OZERC20.sol contains a contract with another name, ERC20. This is a terrible practice that reduces the readability and auditability of the code.

[jakekidd]

Outdated compiler lacking examples?

OZERC20 is just an open zeppelin ERC20 implementation.