QA Report

[code423n4]

Missing Equivalence Checks in Setters

Severity: Low
Context: PortalFacet.sol#L57-L69, ConnextPriceOracle.sol#L142-L180

Description:
Setter functions are missing checks to validate if the new value being set is the same as the current value already set in the contract. Such checks will showcase mismatches between on-chain and off-chain states.

Recommendation:
Add in the additional checks to validate if the new value being set is the same as the current value already set in the contract.

Missing Time locks

Severity: Low
Context: PortalFacet.sol#L57-L69

Description:
None of the onlyOwner functions that change critical protocol addresses/parameters appear to have a timelock for a time-delayed change to alert: (1) users and give them a chance to engage/exit protocol if they are not agreeable to the changes (2) team in case of compromised owner(s) and given them a chance to perform incident response.

Recommendation:
Add a timelock to these functions for a time-delayed change to alert users and protect against possible malicious changes by compromised owners(s).

Lack of Event Emission For Critical Functions

Severity: Low
Context: PortalFacet.sol#L57-L69

Description:
Several functions update critical parameters that are missing event emission. These should be performed to ensure tracking of changes of such critical parameters.

Recommendation:
Add events to functions that change critical parameters.

Unneeded Use of SafeMath.sol

Severity Informational
Context: ConnextPriceoracle.sol, AmplificationUtils.sol, SwapUtils.sol

Description:
Since Solidity 0.8, the overflow/underflow check is implemented on the language level - it adds the validation to the bytecode during compilation.

You don't need the SafeMath library for Solidity 0.8+. It will just perform the same validation twice (one on the language level and one in the library).

Recommendation:
Remove the use of SafeMath library.

Unindexed Event Parameters

Severity Informational
Context: AssetFacet.sol#L22-L62, BridgeFacet.sol#L179-L195, RoutersFacet.sol#L98-L124, ConnextPriceOracle.sol#L65-L69, SponsorVault.sol#L70-L113

Description:
Parameters of certain events are expected to be indexed so that they’re included in the block’s bloom filter for faster access. Failure to do so might confuse off-chain tooling looking for such indexed events.

Recommendation:
Add the indexed keyword to event parameters that should include it.

TODOs && FIXMEs Left In The Code

Severity: Informational
Context: BridgeFacet.sol#L492, BridgeFacet.sol#L579, BridgeFacet.sol#L594, BridgeFacet.sol#L1027, Executor.sol#L7, LibConnextStorage.sol#L303

Description:
There should never be any TODOs in the code when deploying.

Recommendation:
Finish the TODOs before deploying.

Spelling Errors

Severity: Informational
Context: BridgeFacet.sol#L86 (crosschain => cross chain), BridgeFacet.sol#L105 (crosschain => cross chain), BridgeFacet.sol#L125 (crosschain => cross chain), BridgeFacet.sol#L134 (crosschain => cross chain), BridgeFacet.sol#L148 (crosschain => cross chain), BridgeFacet.sol#L265 (bridgable => bridgeable), BridgeFacet.sol#L450-L451 (asseted => asset?), BridgeFacet.sol#L774 (addressess => addresses), BridgeFacet.sol#L1059 (a sthe => as the), ProposedOwnableFacet.sol#L143 (sournce => source), ProposedOwnableFacet.sol#L163 (sournce => source), ProposedOwnableFacet.sol#L176 (sournce => source), RelayerFacet.sol#L35 (rlayer => relayer), RoutersFacet.sol#L290 (crosschain => cross chain), RoutersFacet.sol#L575 (specicfied => specified), Executor.sol#L17 (callabale => callable), LPToken.sol#L41 (everytime => every time), ProposedOwnableUpgradeable.sol#L170 (sournce => source), ProposedOwnableUpgradeable.sol#L198 (sournce => source), ProposedOwnableUpgradeable.sol#L212 (sournce => source), PromiseRouter.sol#L50 (incomming => incoming), PromiseRouter.sol#L266 (crosschain=> cross chain), PromiseRouter.sol#L278 (crosschain=> cross chain), LibConnextStorage.sol#L23 (crosschain=> cross chain), LibConnextStorage.sol#L26 (crosschain=> cross chain), LibConnextStorage.sol#L278 (briding => bribing), LibCrossDomainProperty.sol#L35 (identifer => identifier)

Description:
Spelling errors in comments can cause confusion to both users and developers.

Recommendation:
Check all misspellings to ensure they are corrected.

Too Recent of a Pragma

Severity Informational
Context: All Contracts

Description:
Using too recent of a pragma is risky since they are not battle tested. A rise of a bug that wasn't known on release would cause either a hack or a need to secure funds and redeploy.

Recommendation:
Use a Pragma version that has been used for sometime. I would suggest 0.8.4 for the decrease of risk and still has the gas optimizations implemented.