01: PromiseRouter.initialize() doesn't initialize the ReentrancyUpgradable dependency
In your initialize() function you should also call the initialize() functions of all the other contracts you inherit. In this case, the ReentrancyGuardUpgradable one is missing.
02: Use a two-step process for critical address changes
You already do that in a couple of spots. But, some are missing. For example, the ConnextPriceOracle contract doesn't use it to change the admin address
Throughout the codebase there are multiple open TODOs.
.//contracts_forge/core/connext/facets/RoutersFacet.t.sol:53: // TODO: test modifiers? onlyRouterOwner, onlyProposedRouterOwner, etc.
.//contracts_forge/core/connext/facets/BridgeFacet.t.sol:468: // TODO: For some reason, balance isn't changing. Perhaps the vm.mockCall prevents this?
.//contracts_forge/core/connext/facets/BridgeFacet.t.sol:1310: // TODO: fails if destination domain does not have an xapp router registered
.//contracts_forge/core/connext/facets/AssetFacet.t.sol:147: // TODO: test_adminFunctions__onlyOwner ??
.//contracts_forge/core/connext/facets/RelayerFacet.t.sol:44: // TODO: onlyRelayerFeeRouter
.//contracts_forge/Connext.t.sol:352: // TODO Correctly calculate the message
.//contracts/core/connext/facets/BridgeFacet.sol:496: // TODO: do we want to store a mapping of custodied token balances here?
.//contracts/core/connext/facets/BridgeFacet.sol:583: // TODO: do we need to keep this
.//contracts/core/connext/facets/BridgeFacet.sol:1031: // TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?
.//contracts/core/connext/libraries/LibConnextStorage.sol:304: // BridgeFacet (cont.) TODO: can we move this
.//contracts/core/connext/helpers/Executor.sol:7:// TODO: see note in below file re: npm
.//contracts/nomad-core/contracts/upgrade/UpgradeBeaconController.sol:17:// TODO: upgrade to `ProposedOwnable`
.//contracts/nomad-core/libs/ExcessivelySafeCall.sol:7:// TODO: Update to npm version when these changes are reflected
Report
01:
PromiseRouter.initialize()
doesn't initialize the ReentrancyUpgradable dependencyIn your
initialize()
function you should also call theinitialize()
functions of all the other contracts you inherit. In this case, the ReentrancyGuardUpgradable one is missing.Relevant code:
02: Use a two-step process for critical address changes
You already do that in a couple of spots. But, some are missing. For example, the ConnextPriceOracle contract doesn't use it to change the admin address
Relevant code:
03: Multiple open TODOs
Throughout the codebase there are multiple open TODOs.