1. typos

2. import not needed

Ierc20.sol can be imported from saferc20.sol take out the Ierc20.sol and import the Iec20 from safeerc20.sol https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/BridgeFacet.sol#L5 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/helpers/SponsorVault.sol#L5

best practice to remove todo comments from code it can tell an attacker inside info that wasnt meant to be send

https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/BridgeFacet.sol#L608 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/BridgeFacet.sol#L707 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/helpers/Executor.sol#L7 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L306

after calling call function you shouldn't put in a if statment because if the call fails then it just skips execution of that if statment ,logic can brake and loss of funds

instead put int a require statment like it should be to make sure the call function happens.

remove code in comments its best practice

no check on address zero it can loose of funds

https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/DiamondLoupeFacet.sol#L32 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/AssetFacet.sol#L112 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/AssetFacet.sol#L100 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/RelayerFacet.sol#L88 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/RelayerFacet.sol#L101 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/RelayerFacet.sol#L112 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/promise/PromiseRouter.sol#L155

make the erc20 not upgradeable

A change to the token semantics can break any smart contract that depends on the past behaviour. https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/PortalFacet.sol#L4

dont use charot of solidity version you can use one for testing and one for deployment and there can be a bug in deployment version.

https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol#L2

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions (those in auction contracts) do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself.

Proof of Concept

https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol#L43 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/helpers/LPToken.sol#L21 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/promise/PromiseRouter.sol#L146 https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol#L80

Recommended Mitigation Steps

Ensure atomic calls to init functions along with deployment via robust deployment scripts or factory contracts. Emit explicit events for initializations of contracts. Enfore prevention of re-initializations via explicit setting/checking of boolean initialized variables in the main init function instead of downstream function checks.

token can be more than 18 deciamls like 24 deciamls and that function will fail and it can more attack vectors for an attacker.

certain variable can fail and not get the price from dex https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L106 https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L103 https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L134

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

QA Report #193