QA Report #232
Labels
bug
Something isn't working
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Connext Amarok QA report
Low
Missing
initializer
inTokenRegistry
The
setLocalDomain
in TokenRegistry is public with no modifierinitializer
. 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
ProposedOwnableUpgradeable
sThere are two
ProposedOwnableUpgradeable
contracts. One is in thehelpers
directory and the other is in theshared/ProposedOwnable.sol
which is inheritingProposedOwnable
. TheProposedOwnableUpgradeable
fromhelpers
is seemingly not used within this repo.setContractOwner function zero address check in
LibDiamond
The
setContractOwner
does not have zero address check and should not have zero address check as it will be used in theProposedOwnableFacet
torenounceOwnership
. So,setContractOwner
doubles as set and renounce the ownership. It may be safer to separate those usages. For example, thissetContractOwner
was used in the test implementation ofConnext
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 othersThe
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 titleProposedOwnable
.Out of Scope
TODO in the
UpgradeBeaconController
TODO comment mentions to upgrade to
ProposedOwnable
, however, unlike the openzeppelin's contract, theProposedOwnable
in this repos does not implement a constructor, presumably because it was going to be used as an implementation of a proxy. But theUpgradeBeaaconController
to function as a controller, it needs owner. If a new contract namedProposedOwnable
was going to be written, it may be confusing because it will only be distinguished by the import path.The text was updated successfully, but these errors were encountered: