QA Report

[code423n4]

[L-01] Immutable addresses should 0-Check

I recommend adding check of 0-address for immutable addresses. Not doing so might lead to non-functional contract when it is updated to 0-address accidentally.

Issue found at

Executor.sol
47:  constructor(address _connext) {
48:    connext = _connext;
49:  }

[L-02] abi.encodePacked should not be used with dynamic types

This is because using abi.encodePacked with dynamic types will cause a hash collisions. link: https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode I recommend using abi.encode instead.

./connext/libraries/ConnextMessage.sol:178:    return keccak256(abi.encodePacked(bytes(_name).length, _name, bytes(_symbol).length, _symbol, _decimals));

[L-03] safeApprove is Deprecated

safeApprove is deprecated. Please use safeIncreaseAllowance and safeDecreaseAllowance instead. link: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bfff03c0d2a59bcd8e2ead1da9aed9edf0080d05/contracts/token/ERC20/utils/SafeERC20.sol#L38-L45

./connext/libraries/AssetLogic.sol:347:        SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);

[N-01] Event is missing indexed fields

Each event should have 3 indexed fields if there are 3 or more fields.

Issue found at

./relayer-fee/libraries/RelayerFeeRouter.sol:50:  event Send(uint32 domain, address recipient, bytes32[] transferIds, bytes32 remote, bytes message);
./relayer-fee/RelayerFeeRouter.sol:50:  event Send(uint32 domain, address recipient, bytes32[] transferIds, bytes32 remote, bytes message);
./promise/PromiseRouter.sol:78-86:  event Send(uint32 domain, bytes32 remote, bytes32 transferId, address callbackAddress, bool success, bytes data, bytes message);
./promise/PromiseRouter.sol:99-107:  event Receive(uint64 indexed originAndNonce, uint32 indexed origin, bytes32 transferId, address callbackAddress, bool success, bytes data, bytes message);
./promise/PromiseRouter.sol:116:  event CallbackFeeAdded(bytes32 indexed transferId, uint256 addedFee, uint256 totalFee, address caller);
./promise/PromiseRouter.sol:123:  event CallbackExecuted(bytes32 indexed transferId, address relayer);
./connext/facets/BridgeFacet.sol:75-82:  event XCalled(bytes32 indexed transferId, XCallArgs xcallArgs, XCalledEventArgs args, uint256 nonce, bytes message, address caller);
./connext/facets/BridgeFacet.sol:93-100:  event Reconciled(bytes32 indexed transferId, uint32 indexed origin, address[] routers, address asset, uint256 amount, address caller);
./connext/facets/BridgeFacet.sol:114-121:  event Executed(bytes32 indexed transferId, address indexed to, ExecuteArgs args, address transactingAsset, uint256 transactingAmount, address caller);
./connext/facets/BridgeFacet.sol:129:  event TransferRelayerFeesUpdated(bytes32 indexed transferId, uint256 relayerFee, address caller);
./connext/facets/BridgeFacet.sol:139-144:  event ForcedReceiveLocal(bytes32 indexed transferId, bytes32 indexed canonicalId, uint32 canonicalDomain, uint256 amount);
./connext/facets/BridgeFacet.sol:153:  event AavePortalMintUnbacked(bytes32 indexed transferId, address indexed router, address asset, uint256 amount);
./connext/facets/BridgeFacet.sol:162:  event AavePortalRepayment(bytes32 indexed transferId, address asset, uint256 amount, uint256 fee);
./connext/facets/BridgeFacet.sol:171:  event AavePortalRepaymentDebt(bytes32 indexed transferId, address asset, uint256 amount, uint256 fee);
./connext/facets/BridgeFacet.sol:179:  event SponsorVaultUpdated(address oldSponsorVault, address newSponsorVault, address caller);
./connext/facets/BridgeFacet.sol:187:  event PromiseRouterUpdated(address oldRouter, address newRouter, address caller);
./connext/facets/BridgeFacet.sol:195:  event ExecutorUpdated(address oldExecutor, address newExecutor, address caller);
./connext/facets/PortalFacet.sol:30:  event AavePortalRouterRepayment(address indexed router, address asset, uint256 amount, uint256 fee);
./connext/facets/RoutersFacet.sol:65:  event RouterAdded(address indexed router, address caller);
./connext/facets/RoutersFacet.sol:72:  event RouterRemoved(address indexed router, address caller);
./connext/facets/RoutersFacet.sol:103:  event MaxRoutersPerTransferUpdated(uint256 maxRoutersPerTransfer, address caller);
./connext/facets/RoutersFacet.sol:110:  event LiquidityFeeNumeratorUpdated(uint256 liquidityFeeNumerator, address caller);
./connext/facets/RoutersFacet.sol:117:  event RouterApprovedForPortal(address router, address caller);
./connext/facets/RoutersFacet.sol:124:  event RouterUnapprovedForPortal(address router, address caller);
./connext/facets/RoutersFacet.sol:133-139:  event RouterLiquidityAdded(address indexed router, address local, bytes32 canonicalId, uint256 amount, address caller);
./connext/facets/RoutersFacet.sol:149:  event RouterLiquidityRemoved(address indexed router, address to, address local, uint256 amount, address caller);
./connext/facets/RelayerFacet.sol:25:  event RelayerFeeRouterUpdated(address oldRouter, address newRouter, address caller);
./connext/facets/RelayerFacet.sol:32:  event RelayerAdded(address relayer, address caller);
./connext/facets/RelayerFacet.sol:39:  event RelayerRemoved(address relayer, address caller);
./connext/facets/RelayerFacet.sol:48:  event InitiatedClaim(uint32 indexed domain, address indexed recipient, address caller, bytes32[] transferIds);
./connext/facets/RelayerFacet.sol:56:  event Claimed(address indexed recipient, uint256 total, bytes32[] transferIds);
./connext/facets/AssetFacet.sol:27:  event WrapperUpdated(address oldWrapper, address newWrapper, address caller);
./connext/facets/AssetFacet.sol:35:  event TokenRegistryUpdated(address oldTokenRegistry, address newTokenRegistry, address caller);
./connext/facets/AssetFacet.sol:44:  event StableSwapAdded(bytes32 canonicalId, uint32 domain, address swapPool, address caller);
./connext/facets/AssetFacet.sol:55:  event AssetAdded(bytes32 canonicalId, uint32 domain, address adoptedAsset, address supportedAsset, address caller);
./connext/facets/AssetFacet.sol:62:  event AssetRemoved(bytes32 canonicalId, address caller);
./connext/facets/ProposedOwnableFacet.sol:52:  event RouterOwnershipRenunciationProposed(uint256 timestamp);
./connext/facets/ProposedOwnableFacet.sol:54:  event RouterOwnershipRenounced(bool renounced);
./connext/facets/ProposedOwnableFacet.sol:56:  event AssetOwnershipRenunciationProposed(uint256 timestamp);
./connext/facets/ProposedOwnableFacet.sol:58:  event AssetOwnershipRenounced(bool renounced);
./connext/libraries/AmplificationUtils.sol:17:  event RampA(uint256 oldA, uint256 newA, uint256 initialTime, uint256 futureTime);
./connext/libraries/AmplificationUtils.sol:18:  event StopRampA(uint256 currentA, uint256 time);
./connext/libraries/LibDiamond.sol:69:  event DiamondCutProposed(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata, uint256 deadline);
./connext/libraries/LibDiamond.sol:81:  event DiamondCutRescinded(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata);
./connext/libraries/LibDiamond.sol:92:  event DiamondCut(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata);
./connext/libraries/SwapUtils.sol:25:  event TokenSwap(address indexed buyer, uint256 tokensSold, uint256 tokensBought, uint128 soldId, uint128 boughtId);
./connext/libraries/SwapUtils.sol:26-32:  event AddLiquidity(address indexed provider, uint256[] tokenAmounts, uint256[] fees, uint256 invariant, uint256 lpTokenSupply);
./connext/libraries/SwapUtils.sol:33:  event RemoveLiquidity(address indexed provider, uint256[] tokenAmounts, uint256 lpTokenSupply);
./connext/libraries/SwapUtils.sol:34-40:  event RemoveLiquidityOne(address indexed provider, uint256 lpTokenAmount, uint256 lpTokenSupply, uint256 boughtId, uint256 tokensBought);
./connext/libraries/SwapUtils.sol:41-47:  event RemoveLiquidityImbalance(address indexed provider, uint256[] tokenAmounts, uint256[] fees, uint256 invariant, uint256 lpTokenSupply);
./connext/libraries/SwapUtils.sol:48:  event NewAdminFee(uint256 newAdminFee);
./connext/libraries/SwapUtils.sol:49:  event NewSwapFee(uint256 newSwapFee);
./connext/helpers/ProposedOwnableUpgradeable.sol:62:  event RouterOwnershipRenunciationProposed(uint256 timestamp);
./connext/helpers/ProposedOwnableUpgradeable.sol:64:  event RouterOwnershipRenounced(bool renounced);
./connext/helpers/ProposedOwnableUpgradeable.sol:66:  event AssetOwnershipRenunciationProposed(uint256 timestamp);
./connext/helpers/ProposedOwnableUpgradeable.sol:68:  event AssetOwnershipRenounced(bool renounced);
./connext/helpers/ConnextPriceOracle.sol:65:  event NewAdmin(address oldAdmin, address newAdmin);
./connext/helpers/ConnextPriceOracle.sol:66:  event PriceRecordUpdated(address token, address baseToken, address lpToken, bool _active);
./connext/helpers/ConnextPriceOracle.sol:67:  event DirectPriceUpdated(address token, uint256 oldPrice, uint256 newPrice);
./connext/helpers/ConnextPriceOracle.sol:68:  event AggregatorUpdated(address tokenAddress, address source);
./connext/helpers/ConnextPriceOracle.sol:69:  event V1PriceOracleUpdated(address oldAddress, address newAddress);
./connext/helpers/SponsorVault.sol:73:  event ConnextUpdated(address oldConnext, address newConnext, address caller);
./connext/helpers/SponsorVault.sol:78:  event RateUpdated(uint32 originDomain, Rate oldRate, Rate newRate, address caller);
./connext/helpers/SponsorVault.sol:83:  event RelayerFeeCapUpdated(uint256 oldRelayerFeeCap, uint256 newRelayerFeeCap, address caller);
./connext/helpers/SponsorVault.sol:88:  event GasTokenOracleUpdated(address oldOracle, address newOracle, address caller);
./connext/helpers/SponsorVault.sol:93:  event TokenExchangeUpdated(address token, address oldTokenExchange, address newTokenExchange, address caller);
./connext/helpers/SponsorVault.sol:98:  event ReimburseLiquidityFees(address token, uint256 amount, address receiver);
./connext/helpers/SponsorVault.sol:103:  event ReimburseRelayerFees(uint256 amount, address receiver);
./connext/helpers/SponsorVault.sol:108:  event Deposit(address token, uint256 amount, address caller);
./connext/helpers/SponsorVault.sol:113:  event Withdraw(address token, address receiver, uint256 amount, address caller);

[N-02] Unnecessary use of named returns

Several function adds return statement even thought named returns variable are used. Remove unnecessary named returns variable to improve code readability. Also keeping the use of named returns or return statement consistent through out the whole project if possible is recommended.

Issue found at StableSwap.sol (remove returns variable availableTokenAmount)

291:   function calculateRemoveLiquidityOneToken(uint256 tokenAmount, uint8 tokenIndex)
292:     external
293:     view
293:     override
294:     returns (uint256 availableTokenAmount)
295:   {
296:     return swapStorage.calculateWithdrawOneToken(tokenAmount, tokenIndex);
297:   }

[N-03] nonReentrant modifier should occur before all other modifiers

This is best practice to protect against reentrancy in other modifiers.

./connext/facets/BridgeFacet.sol:279:  function xcall(XCallArgs calldata _args) external payable whenNotPaused nonReentrant returns (bytes32) {
./connext/facets/BridgeFacet.sol:411:  function execute(ExecuteArgs calldata _args) external whenNotPaused nonReentrant returns (bytes32) {

[N-04] Should Resolve TODOs before Deployment

Questions/Issues in the code should be resolved before the deployment.

Issue found at

./connext/facets/BridgeFacet.sol:492:      // TODO: do we want to store a mapping of custodied token balances here?
./connext/facets/BridgeFacet.sol:579:      // TODO: do we need to keep this
./connext/facets/BridgeFacet.sol:1027:    // TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?
./connext/libraries/LibConnextStorage.sol:303:  // BridgeFacet (cont.) TODO: can we move this
./connext/helpers/Executor.sol:7:// TODO: see note in below file re: npm