Connext Amarok contest - zzzitron's results

Connext Amarok QA report

Low

Missing initializer in TokenRegistry

• TokenRegistry.sol:69-71

  function setLocalDomain(uint32 domain) public {
    _local = domain;
  }

The setLocalDomain in TokenRegistry is public with no modifier initializer. Related information is used later to decide whether _deployToken or not in ensureLocalToken as well as in the library AssetLogic, but due to the limited time, could not figure out ways to exploit this.

Non-critical

Two ProposedOwnableUpgradeables

• ProposedOwnable.sol:176
• ProposedOwnableUpgradeable.sol:26 There are two ProposedOwnableUpgradeable contracts. One is in the helpers directory and the other is in the shared/ProposedOwnable.sol which is inheriting ProposedOwnable. The ProposedOwnableUpgradeable from helpers is seemingly not used within this repo.

setContractOwner function zero address check in LibDiamond

• LibDiamond.sol:54-59 The setContractOwner does not have zero address check and should not have zero address check as it will be used in the ProposedOwnableFacet to renounceOwnership. So, setContractOwner doubles as set and renounce the ownership. It may be safer to separate those usages. For example, this setContractOwner was used in the test implementation of Connext without zero address check. If it is the intended usage of this library for the future, it may be safeguarded in the library level.

Misleading comment in ProposedOwnableFacet and others

• ProposedOwnableFacet:234
• ProposedOwnable.sol:140
• ProposedOwnableUpgradeable.sol:270

  /**
   * @notice Transfers ownership of the contract to a new account (`newOwner`).
   * Can only be called by the current owner.
   */
  function acceptProposedOwner() public onlyProposed {

The acceptProposedOwner can only be called by the proposed owner, yet the comment says it can only be called by the current owner. Also ProposedOwnableFacet ProposedOwnableUpgradeable has a Natspec, with the title ProposedOwnable.

Out of Scope

TODO in the UpgradeBeaconController

• UpgradeBeaconController.sol:17-18

// TODO: upgrade to `ProposedOwnable`
contract UpgradeBeaconController is Ownable {

TODO comment mentions to upgrade to ProposedOwnable, however, unlike the openzeppelin's contract, the ProposedOwnable in this repos does not implement a constructor, presumably because it was going to be used as an implementation of a proxy. But the UpgradeBeaaconController to function as a controller, it needs owner. If a new contract named ProposedOwnable was going to be written, it may be confusing because it will only be distinguished by the import path.