code-423n4 / 2022-06-connext-findings

1 stars 0 forks source link

QA Report #232

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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 `ProposedOwnableUpgradeable`s

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.

ecmendenhall commented 2 years ago

The Low finding here ("Missing initializer in TokenRegistry") is a duplicate of #54.

jakekidd commented 2 years ago

TokenRegistry out of scope

0xleastwood commented 1 year ago

First part is incorrect: See #72.