Connext Amarok contest - MiloTruck's results

QA Report

Non-Critical Issues

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps
Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

contracts/core/connext/helpers/StableSwap.sol:
 125:        require(block.timestamp <= deadline, "Deadline not met");

contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol:
 178:        if ((block.timestamp - _routerOwnershipTimestamp) <= _delay)
 220:        if ((block.timestamp - _assetOwnershipTimestamp) <= _delay)
 258:        if ((block.timestamp - _proposedOwnershipTimestamp) <= _delay)
 282:        if ((block.timestamp - _proposedOwnershipTimestamp) <= _delay)
 292:        _routerOwnershipTimestamp = block.timestamp;
 303:        _assetOwnershipTimestamp = block.timestamp;
 321:        _proposedOwnershipTimestamp = block.timestamp;

contracts/core/connext/libraries/LibDiamond.sol:
  76:        uint256 acceptance = block.timestamp + _delay;
 101:        diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] < block.timestamp,

contracts/core/connext/libraries/AmplificationUtils.sol:
  56:        if (block.timestamp < t1) {
  60:        // a0 + (a1 - a0) * (block.timestamp - t0) / (t1 - t0)
  61:        return a0.add(a1.sub(a0).mul(block.timestamp.sub(t0)).div(t1.sub(t0)));
  63:        // a0 - (a0 - a1) * (block.timestamp - t0) / (t1 - t0)
  64:        return a0.sub(a0.sub(a1).mul(block.timestamp.sub(t0)).div(t1.sub(t0)));
  84:        require(block.timestamp >= self.initialATime.add(1 days), "Wait 1 day before starting ramp");
  85:        require(futureTime_ >= block.timestamp.add(MIN_RAMP_TIME), "Insufficient ramp time");
  99:        self.initialATime = block.timestamp;
 102:        emit RampA(initialAPrecise, futureAPrecise, block.timestamp, futureTime_);
 111:        require(self.futureATime > block.timestamp, "Ramp is already stopped");
 116:        self.initialATime = block.timestamp;
 117:        self.futureATime = block.timestamp;
 119:        emit StopRampA(currentA, block.timestamp);

contracts/core/connext/facets/ProposedOwnableFacet.sol:
 151:        if ((block.timestamp - s._routerOwnershipTimestamp) <= _delay)
 184:        if ((block.timestamp - s._assetOwnershipTimestamp) <= _delay)
 222:        if ((block.timestamp - s._proposedOwnershipTimestamp) <= _delay)
 246:        if ((block.timestamp - s._proposedOwnershipTimestamp) <= _delay)
 266:        s._routerOwnershipTimestamp = block.timestamp;
 277:        s._assetOwnershipTimestamp = block.timestamp;
 293:        s._proposedOwnershipTimestamp = block.timestamp;

contracts/core/connext/facets/RoutersFacet.sol:
 420:        s.routerPermissionInfo.proposedRouterTimestamp[router] = block.timestamp;
 434:        if (block.timestamp - s.routerPermissionInfo.proposedRouterTimestamp[router] <= _delay)

contracts/core/connext/facets/StableSwapFacet.sol:
  58:        if (block.timestamp > deadline) revert StableSwapFacet__deadlineCheck_deadlineNotMet();

contracts/core/shared/ProposedOwnable.sol:
 128:        if ((block.timestamp - _proposedOwnershipTimestamp) <= _delay)
 152:        if ((block.timestamp - _proposedOwnershipTimestamp) <= _delay)
 170:        _proposedOwnershipTimestamp = block.timestamp;

safeApprove() is deprecated

With reference to SafeERC20.sol, safeApprove() is deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance().

Consider using these functions instead of safeApprove() in these instances:

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

Use scientific notation instead of exponentiation

Scientific notation should be used for clarity, such as 10e18 instead of 10**18:

contracts/core/connext/libraries/SwapUtils.sol:
  67:        // has 8, so the multiplier should be 10 ** 18 / 10 ** 8 => 10 ** 10
 104:        uint256 internal constant FEE_DENOMINATOR = 10**10;
 113:        uint256 internal constant MAX_ADMIN_FEE = 10**10;

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

contracts/core/promise/PromiseRouter.sol:
  78:        event Send(
  79:          uint32 domain,
  80:          bytes32 remote,
  81:          bytes32 transferId,
  82:          address callbackAddress,
  83:          bool success,
  84:          bytes data,
  85:          bytes message
  86:        );

  99:        event Receive(
 100:          uint64 indexed originAndNonce,
 101:          uint32 indexed origin,
 102:          bytes32 transferId,
 103:          address callbackAddress,
 104:          bool success,
 105:          bytes data,
 106:          bytes message
 107:        );

 116:        event CallbackFeeAdded(bytes32 indexed transferId, uint256 addedFee, uint256 totalFee, address caller);

contracts/core/connext/helpers/ConnextPriceOracle.sol:
  66:        event PriceRecordUpdated(address token, address baseToken, address lpToken, bool _active);
  67:        event DirectPriceUpdated(address token, uint256 oldPrice, uint256 newPrice);

contracts/core/connext/helpers/SponsorVault.sol:
  73:        event ConnextUpdated(address oldConnext, address newConnext, address caller);
  78:        event RateUpdated(uint32 originDomain, Rate oldRate, Rate newRate, address caller);
  83:        event RelayerFeeCapUpdated(uint256 oldRelayerFeeCap, uint256 newRelayerFeeCap, address caller);
  88:        event GasTokenOracleUpdated(address oldOracle, address newOracle, address caller);
  93:        event TokenExchangeUpdated(address token, address oldTokenExchange, address newTokenExchange, address caller);
  98:        event ReimburseLiquidityFees(address token, uint256 amount, address receiver);
 108:        event Deposit(address token, uint256 amount, address caller);
 113:        event Withdraw(address token, address receiver, uint256 amount, address caller);

contracts/core/connext/libraries/LibDiamond.sol:
  69:        event DiamondCutProposed(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata, uint256 deadline);
  81:        event DiamondCutRescinded(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata);
  92:        event DiamondCut(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata);

contracts/core/connext/libraries/AmplificationUtils.sol:
  17:        event RampA(uint256 oldA, uint256 newA, uint256 initialTime, uint256 futureTime);

contracts/core/connext/libraries/SwapUtils.sol:
  25:        event TokenSwap(address indexed buyer, uint256 tokensSold, uint256 tokensBought, uint128 soldId, uint128 boughtId);

  26:        event AddLiquidity(
  27:          address indexed provider,
  28:          uint256[] tokenAmounts,
  29:          uint256[] fees,
  30:          uint256 invariant,
  31:          uint256 lpTokenSupply
  32:        );

  33:        event RemoveLiquidity(address indexed provider, uint256[] tokenAmounts, uint256 lpTokenSupply);

  34:        event RemoveLiquidityOne(
  35:          address indexed provider,
  36:          uint256 lpTokenAmount,
  37:          uint256 lpTokenSupply,
  38:          uint256 boughtId,
  39:          uint256 tokensBought
  40:        );

  41:        event RemoveLiquidityImbalance(
  42:          address indexed provider,
  43:          uint256[] tokenAmounts,
  44:          uint256[] fees,
  45:          uint256 invariant,
  46:          uint256 lpTokenSupply
  47:        );

contracts/core/connext/facets/BridgeFacet.sol:
  75:        event XCalled(
  76:          bytes32 indexed transferId,
  77:          XCallArgs xcallArgs,
  78:          XCalledEventArgs args,
  79:          uint256 nonce,
  80:          bytes message,
  81:          address caller
  82:        );

  93:        event Reconciled(
  94:          bytes32 indexed transferId,
  95:          uint32 indexed origin,
  96:          address[] routers,
  97:          address asset,
  98:          uint256 amount,
  99:          address caller
 100:        );

 114:        event Executed(
 115:          bytes32 indexed transferId,
 116:          address indexed to,
 117:          ExecuteArgs args,
 118:          address transactingAsset,
 119:          uint256 transactingAmount,
 120:          address caller
 121:        );

 129:        event TransferRelayerFeesUpdated(bytes32 indexed transferId, uint256 relayerFee, address caller);

 139:        event ForcedReceiveLocal(
 140:          bytes32 indexed transferId,
 141:          bytes32 indexed canonicalId,
 142:          uint32 canonicalDomain,
 143:          uint256 amount
 144:        );

 153:        event AavePortalMintUnbacked(bytes32 indexed transferId, address indexed router, address asset, uint256 amount);
 162:        event AavePortalRepayment(bytes32 indexed transferId, address asset, uint256 amount, uint256 fee);
 171:        event AavePortalRepaymentDebt(bytes32 indexed transferId, address asset, uint256 amount, uint256 fee);
 179:        event SponsorVaultUpdated(address oldSponsorVault, address newSponsorVault, address caller);
 187:        event PromiseRouterUpdated(address oldRouter, address newRouter, address caller);
 195:        event ExecutorUpdated(address oldExecutor, address newExecutor, address caller);

contracts/core/connext/facets/RelayerFacet.sol:
  25:        event RelayerFeeRouterUpdated(address oldRouter, address newRouter, address caller);
  48:        event InitiatedClaim(uint32 indexed domain, address indexed recipient, address caller, bytes32[] transferIds);
  56:        event Claimed(address indexed recipient, uint256 total, bytes32[] transferIds);

contracts/core/connext/facets/RoutersFacet.sol:
 133:        event RouterLiquidityAdded(
 134:          address indexed router,
 135:          address local,
 136:          bytes32 canonicalId,
 137:          uint256 amount,
 138:          address caller
 139:        );

 149:        event RouterLiquidityRemoved(address indexed router, address to, address local, uint256 amount, address caller);

contracts/core/connext/facets/PortalFacet.sol:
  30:        event AavePortalRouterRepayment(address indexed router, address asset, uint256 amount, uint256 fee);

contracts/core/connext/facets/AssetFacet.sol:
  27:        event WrapperUpdated(address oldWrapper, address newWrapper, address caller);
  35:        event TokenRegistryUpdated(address oldTokenRegistry, address newTokenRegistry, address caller);
  44:        event StableSwapAdded(bytes32 canonicalId, uint32 domain, address swapPool, address caller);
  55:        event AssetAdded(bytes32 canonicalId, uint32 domain, address adoptedAsset, address supportedAsset, address caller);

contracts/core/relayer-fee/RelayerFeeRouter.sol:
  50:        event Send(uint32 domain, address recipient, bytes32[] transferIds, bytes32 remote, bytes message);

Gas Report

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

for (uint256 i = 0; i < length; ++i) {

can be changed to:

for (uint256 i; i < length; ++i) {

Consider declaring the following lines without explicitly setting the index to 0:

contracts/core/connext/helpers/ConnextPriceOracle.sol:
 176:        for (uint256 i = 0; i < tokenAddresses.length; i++) {

contracts/core/connext/helpers/StableSwap.sol:
  81:        for (uint8 i = 0; i < _pooledTokens.length; i++) {

contracts/core/connext/libraries/SwapUtils.sol:
 205:        for (uint256 i = 0; i < xp.length; i++) {
 254:        for (uint256 i = 0; i < numTokens; i++) {
 268:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 289:        for (uint256 i = 0; i < numTokens; i++) {
 300:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 302:        for (uint256 j = 0; j < numTokens; j++) {
 344:        for (uint256 i = 0; i < numTokens; i++) {
 405:        for (uint256 i = 0; i < numTokens; i++) {
 425:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 558:        for (uint256 i = 0; i < balances.length; i++) {
 591:        for (uint256 i = 0; i < balances.length; i++) {
 844:        for (uint256 i = 0; i < pooledTokens.length; i++) {
 869:        for (uint256 i = 0; i < pooledTokens.length; i++) {
 924:        for (uint256 i = 0; i < amounts.length; i++) {
1014:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1019:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1039:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1055:        for (uint256 i = 0; i < pooledTokens.length; i++) {

contracts/core/connext/facets/StableSwapFacet.sol:
 415:        for (uint8 i = 0; i < _pooledTokens.length; i++) {

contracts/core/relayer-fee/libraries/RelayerFeeMessage.sol:
  81:        for (uint256 i = 0; i < length; ) {

For-Loops: Cache array length outside of loops

Reading an array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

Consider making the following change to these lines:

contracts/core/connext/helpers/ConnextPriceOracle.sol:
 176:        for (uint256 i = 0; i < tokenAddresses.length; i++) {

contracts/core/connext/helpers/StableSwap.sol:
  81:        for (uint8 i = 0; i < _pooledTokens.length; i++) {

contracts/core/connext/libraries/LibDiamond.sol:
 104:        for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {
 129:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
 147:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
 162:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {

contracts/core/connext/libraries/SwapUtils.sol:
 205:        for (uint256 i = 0; i < xp.length; i++) {
 558:        for (uint256 i = 0; i < balances.length; i++) {
 591:        for (uint256 i = 0; i < balances.length; i++) {
 844:        for (uint256 i = 0; i < pooledTokens.length; i++) {
 869:        for (uint256 i = 0; i < pooledTokens.length; i++) {
 924:        for (uint256 i = 0; i < amounts.length; i++) {
1014:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1019:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1039:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1055:        for (uint256 i = 0; i < pooledTokens.length; i++) {

contracts/core/connext/facets/RelayerFacet.sol:
 140:        for (uint256 i; i < _transferIds.length; ) {
 164:        for (uint256 i; i < _transferIds.length; ) {

contracts/core/connext/facets/StableSwapFacet.sol:
 415:        for (uint8 i = 0; i < _pooledTokens.length; i++) {

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

For example, the code below:

for (uint256 i; i < numIterations; ++i) {  
    // ...  
}  

can be changed to:

for (uint256 i; i < numIterations;) {  
    // ...  
    unchecked { ++i; }  
}  

Consider making the following change to these lines:

contracts/core/connext/helpers/ConnextPriceOracle.sol:
 176:        for (uint256 i = 0; i < tokenAddresses.length; i++) {

contracts/core/connext/helpers/StableSwap.sol:
  81:        for (uint8 i = 0; i < _pooledTokens.length; i++) {

contracts/core/connext/libraries/LibDiamond.sol:
 104:        for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {
 129:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
 147:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
 162:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {

contracts/core/connext/libraries/SwapUtils.sol:
 205:        for (uint256 i = 0; i < xp.length; i++) {
 254:        for (uint256 i = 0; i < numTokens; i++) {
 268:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 289:        for (uint256 i = 0; i < numTokens; i++) {
 300:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 302:        for (uint256 j = 0; j < numTokens; j++) {
 344:        for (uint256 i = 0; i < numTokens; i++) {
 405:        for (uint256 i = 0; i < numTokens; i++) {
 425:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 558:        for (uint256 i = 0; i < balances.length; i++) {
 591:        for (uint256 i = 0; i < balances.length; i++) {
 844:        for (uint256 i = 0; i < pooledTokens.length; i++) {
 869:        for (uint256 i = 0; i < pooledTokens.length; i++) {
 924:        for (uint256 i = 0; i < amounts.length; i++) {
1014:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1019:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1039:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1055:        for (uint256 i = 0; i < pooledTokens.length; i++) {

contracts/core/connext/facets/StableSwapFacet.sol:
 415:        for (uint8 i = 0; i < _pooledTokens.length; i++) {

contracts/core/connext/facets/DiamondLoupeFacet.sol:
  31:        for (uint256 i; i < numFacets; i++) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

contracts/core/connext/helpers/ConnextPriceOracle.sol:
 176:        for (uint256 i = 0; i < tokenAddresses.length; i++) {

contracts/core/connext/helpers/StableSwap.sol:
  81:        for (uint8 i = 0; i < _pooledTokens.length; i++) {

contracts/core/connext/libraries/LibDiamond.sol:
 104:        for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {
 129:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
 134:        selectorPosition++;
 147:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
 153:        selectorPosition++;
 162:        for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {

contracts/core/connext/libraries/SwapUtils.sol:
 205:        for (uint256 i = 0; i < xp.length; i++) {
 254:        for (uint256 i = 0; i < numTokens; i++) {
 268:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 289:        for (uint256 i = 0; i < numTokens; i++) {
 300:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 302:        for (uint256 j = 0; j < numTokens; j++) {
 344:        for (uint256 i = 0; i < numTokens; i++) {
 405:        for (uint256 i = 0; i < numTokens; i++) {
 425:        for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
 558:        for (uint256 i = 0; i < balances.length; i++) {
 591:        for (uint256 i = 0; i < balances.length; i++) {
 844:        for (uint256 i = 0; i < pooledTokens.length; i++) {
 869:        for (uint256 i = 0; i < pooledTokens.length; i++) {
 924:        for (uint256 i = 0; i < amounts.length; i++) {
1014:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1019:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1039:        for (uint256 i = 0; i < pooledTokens.length; i++) {
1055:        for (uint256 i = 0; i < pooledTokens.length; i++) {

contracts/core/connext/facets/BridgeFacet.sol:
 332:        s.nonce += 1;
 613:        i++;
 684:        i++;
 799:        i++;

contracts/core/connext/facets/RelayerFacet.sol:
 144:        i++;
 168:        i++;

contracts/core/connext/facets/StableSwapFacet.sol:
 415:        for (uint8 i = 0; i < _pooledTokens.length; i++) {

contracts/core/connext/facets/DiamondLoupeFacet.sol:
  31:        for (uint256 i; i < numFacets; i++) {

contracts/core/relayer-fee/libraries/RelayerFeeMessage.sol:
  85:        i++;

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

contracts/core/promise/PromiseRouter.sol:
 259:        if (callbackFee > 0) {

contracts/core/promise/libraries/PromiseMessage.sol:
 142:        return _length > 0 && (RETURNDATA_START + _length) == _len;

contracts/core/connext/helpers/ConnextPriceOracle.sol:
 150:        require(baseTokenPrice > 0, "invalid base token");

contracts/core/connext/helpers/SponsorVault.sol:
 217:        if (sponsoredFee > 0) {

contracts/core/connext/helpers/StableSwap.sol:
  82:        if (i > 0) {

contracts/core/connext/libraries/LibDiamond.sol:
 121:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 139:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 158:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 226:        require(_calldata.length > 0, "LibDiamondCut: _calldata is empty but _init is not address(0)");
 232:        if (error.length > 0) {
 247:        require(contractSize > 0, _errorMessage);

contracts/core/connext/libraries/AmplificationUtils.sol:
  86:        require(futureA_ > 0 && futureA_ < MAX_A, "futureA_ must be > 0 and < MAX_A");

contracts/core/connext/libraries/SwapUtils.sol:
 369:        if (supply > 0) {
 670:        if (dyAdminFee > 0) {
 711:        if (dxAdminFee > 0) {
 765:        if (dyAdminFee > 0) {
 799:        if (dxAdminFee > 0) {
 845:        require(v.totalSupply != 0 || amounts[i] > 0, "Must supply all tokens in pool");
 965:        if (adminFee > 0) {

contracts/core/connext/facets/BridgeFacet.sol:
 293:        if (_args.params.callback == address(0) && _args.params.callbackFee > 0) {
 499:        if (_amount > 0) {
 665:        if (pathLength > 0) // make sure routers are all approved if needed

contracts/core/connext/facets/StableSwapFacet.sol:
 416:        if (i > 0) {

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

Consider changing the visibility of the following from public to internal or private:

contracts/core/connext/helpers/PriceOracle.sol:
   6:        bool public constant isPriceOracle = true;

contracts/core/connext/libraries/AmplificationUtils.sol:
  21:        uint256 public constant A_PRECISION = 100;
  22:        uint256 public constant MAX_A = 10**6;

contracts/core/connext/libraries/LibCrossDomainProperty.sol:
  38:        bytes public constant EMPTY_BYTES = hex"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff";

contracts/core/connext/facets/BridgeFacet.sol:
  68:        uint16 public constant AAVE_REFERRAL_CODE = 0;

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

contracts/core/promise/PromiseRouter.sol:
 230:        function process(bytes32 transferId, bytes calldata _message) public nonReentrant {

contracts/core/connext/facets/NomadFacet.sol:
  11:        function xAppConnectionManager() public view returns (XAppConnectionManager) {
  15:        function remotes(uint32 _domain) public view returns (bytes32) {

contracts/core/connext/facets/BridgeFacet.sol:
 199:        function relayerFees(bytes32 _transferId) public view returns (uint256) {
 203:        function routedTransfers(bytes32 _transferId) public view returns (address[] memory) {
 207:        function reconciledTransfers(bytes32 _transferId) public view returns (bool) {
 211:        function domain() public view returns (uint256) {
 215:        function executor() public view returns (IExecutor) {
 219:        function nonce() public view returns (uint256) {
 223:        function sponsorVault() public view returns (ISponsorVault) {

contracts/core/connext/facets/RelayerFacet.sol:
  70:        function transferRelayer(bytes32 _transferId) public view returns (address) {
  74:        function approvedRelayers(address _relayer) public view returns (bool) {

contracts/core/connext/facets/ProposedOwnableFacet.sol:
  76:        function routerOwnershipRenounced() public view returns (bool) {
  83:        function assetOwnershipRenounced() public view returns (bool) {
  90:        function proposed() public view returns (address) {
  97:        function proposedTimestamp() public view returns (uint256) {
 104:        function routerOwnershipTimestamp() public view returns (uint256) {
 111:        function assetOwnershipTimestamp() public view returns (uint256) {
 118:        function delay() public view returns (uint256) {
 128:        function proposeRouterOwnershipRenunciation() public onlyOwner {
 142:        function renounceRouterOwnership() public onlyOwner {
 162:        function proposeAssetOwnershipRenunciation() public onlyOwner {
 175:        function renounceAssetOwnership() public onlyOwner {
 195:        function renounced() public view returns (bool) {
 203:        function proposeNewOwner(address newlyProposed) public onlyOwner {
 217:        function renounceOwnership() public onlyOwner {
 236:        function acceptProposedOwner() public onlyProposed {
 253:        function pause() public onlyOwner {
 258:        function unpause() public onlyOwner {

contracts/core/connext/facets/RoutersFacet.sol:
 181:        function LIQUIDITY_FEE_NUMERATOR() public view returns (uint256) {
 185:        function LIQUIDITY_FEE_DENOMINATOR() public view returns (uint256) {
 193:        function getRouterApproval(address _router) public view returns (bool) {
 222:        function getProposedRouterOwner(address _router) public view returns (address) {
 231:        function getProposedRouterOwnerTimestamp(address _router) public view returns (uint256) {
 235:        function maxRoutersPerTransfer() public view returns (uint256) {
 239:        function routerBalances(address _router, address _asset) public view returns (uint256) {
 247:        function getRouterApprovalForPortal(address _router) public view returns (bool) {

contracts/core/connext/facets/VersionFacet.sol:
  20:        function VERSION() public pure returns (uint8) {

contracts/core/connext/facets/AssetFacet.sol:
  66:        function canonicalToAdopted(bytes32 _canonicalId) public view returns (address) {
  70:        function adoptedToCanonical(address _adopted) public view returns (ConnextMessage.TokenId memory) {
  78:        function approvedAssets(bytes32 _asset) public view returns (bool) {
  82:        function adoptedToLocalPools(bytes32 _adopted) public view returns (IStableSwap) {
  86:        function wrapper() public view returns (IWrapped) {
  90:        function tokenRegistry() public view returns (ITokenRegistry) {

Errors: Reduce the length of error messages (long revert strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

In these instances, consider shortening the revert strings to fit within 32 bytes, or using custom errors:

contracts/core/connext/libraries/LibDiamond.sol:
  66:        require(msg.sender == diamondStorage().contractOwner, "LibDiamond: Must be contract owner");
 121:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 123:        require(_facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)");
 132:        require(oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists");
 139:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 141:        require(_facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)");
 150:        require(oldFacetAddress != _facetAddress, "LibDiamondCut: Can't replace function with same function");
 158:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 161:        require(_facetAddress == address(0), "LibDiamondCut: Remove facet address must be address(0)");
 191:        require(_facetAddress != address(0), "LibDiamondCut: Can't remove function that doesn't exist");
 193:        require(_facetAddress != address(this), "LibDiamondCut: Can't remove immutable function");
 224:        require(_calldata.length == 0, "LibDiamondCut: _init is address(0) but_calldata is not empty");
 226:        require(_calldata.length > 0, "LibDiamondCut: _calldata is empty but _init is not address(0)");

contracts/core/connext/libraries/SwapUtils.sol:
 697:        require(dy <= self.balances[tokenIndexTo], "Cannot get more than pool balance");
 784:        require(dy <= self.balances[tokenIndexTo], "Cannot get more than pool balance");

Errors: Use multiple require statements instead of &&

Instead of using a single require statement with the && operator, using multiple require statements would help to save runtime gas cost. However, note that this results in a higher deployment gas cost, which is a fair trade-off.

A require statement can be split as such:

// Original code:
require(a && b, 'error');

// Changed to:
require(a, 'error: a');
require(b, 'error: b');

Instances where multiple require statements should be used:

contracts/core/connext/libraries/AmplificationUtils.sol:
  86:        require(futureA_ > 0 && futureA_ < MAX_A, "futureA_ must be > 0 and < MAX_A");

contracts/core/connext/libraries/SwapUtils.sol:
 397:        require(tokenIndexFrom < numTokens && tokenIndexTo < numTokens, "Tokens must be in pool");
 493:        require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range");
 524:        require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range");
1007:        require(maxBurnAmount <= v.lpToken.balanceOf(msg.sender) && maxBurnAmount != 0, ">LP.balanceOf");

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

• Cheaper deployment cost
• Lower runtime cost upon revert

Taken from Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

contracts/core/connext/helpers/ConnextPriceOracle.sol:
  72:        require(msg.sender == admin, "caller is not the admin");
 150:        require(baseTokenPrice > 0, "invalid base token");

contracts/core/connext/helpers/Executor.sol:
  57:        require(msg.sender == connext, "#OC:027");

contracts/core/connext/helpers/StableSwap.sol:
  75:        require(_pooledTokens.length > 1, "_pooledTokens.length <= 1");
  76:        require(_pooledTokens.length <= 32, "_pooledTokens.length > 32");
  77:        require(_pooledTokens.length == decimals.length, "_pooledTokens decimals mismatch");
  89:        require(address(_pooledTokens[i]) != address(0), "The 0 address isn't an ERC-20");
  90:        require(decimals[i] <= SwapUtils.POOL_PRECISION_DECIMALS, "Token decimals exceeds max");
  96:        require(_a < AmplificationUtils.MAX_A, "_a exceeds maximum");
  97:        require(_fee < SwapUtils.MAX_SWAP_FEE, "_fee exceeds maximum");
  98:        require(_adminFee < SwapUtils.MAX_ADMIN_FEE, "_adminFee exceeds maximum");
 102:        require(lpToken.initialize(lpTokenName, lpTokenSymbol), "could not init lpToken clone");
 125:        require(block.timestamp <= deadline, "Deadline not met");
 155:        require(index < swapStorage.pooledTokens.length, "Out of range");
 167:        require(address(getToken(index)) == tokenAddress, "Token does not exist");
 177:        require(index < swapStorage.pooledTokens.length, "Index out of range");

contracts/core/connext/helpers/LPToken.sol:
  35:        require(amount != 0, "LPToken: cannot mint 0");
  50:        require(to != address(this), "LPToken: cannot send to itself");

contracts/core/connext/libraries/ConnextMessage.sol:
 116:        require(isValidAction(_action), "!action");

contracts/core/connext/libraries/LibDiamond.sol:
  66:        require(msg.sender == diamondStorage().contractOwner, "LibDiamond: Must be contract owner");
 121:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 123:        require(_facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)");
 132:        require(oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists");
 139:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 141:        require(_facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)");
 150:        require(oldFacetAddress != _facetAddress, "LibDiamondCut: Can't replace function with same function");
 158:        require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
 161:        require(_facetAddress == address(0), "LibDiamondCut: Remove facet address must be address(0)");
 191:        require(_facetAddress != address(0), "LibDiamondCut: Can't remove function that doesn't exist");
 193:        require(_facetAddress != address(this), "LibDiamondCut: Can't remove immutable function");
 224:        require(_calldata.length == 0, "LibDiamondCut: _init is address(0) but_calldata is not empty");
 226:        require(_calldata.length > 0, "LibDiamondCut: _calldata is empty but _init is not address(0)");

contracts/core/connext/libraries/AmplificationUtils.sol:
  84:        require(block.timestamp >= self.initialATime.add(1 days), "Wait 1 day before starting ramp");
  85:        require(futureTime_ >= block.timestamp.add(MIN_RAMP_TIME), "Insufficient ramp time");
  86:        require(futureA_ > 0 && futureA_ < MAX_A, "futureA_ must be > 0 and < MAX_A");
  92:        require(futureAPrecise.mul(MAX_A_CHANGE) >= initialAPrecise, "futureA_ is too small");
  94:        require(futureAPrecise <= initialAPrecise.mul(MAX_A_CHANGE), "futureA_ is too large");
 111:        require(self.futureATime > block.timestamp, "Ramp is already stopped");

contracts/core/connext/libraries/SwapUtils.sol:
 191:        require(tokenIndex < xp.length, "Token index out of range");
 198:        require(tokenAmount <= xp[tokenIndex], "Withdraw exceeds available");
 248:        require(tokenIndex < numTokens, "Token not found");
 342:        require(numTokens == precisionMultipliers.length, "Balances must match multipliers");
 396:        require(tokenIndexFrom != tokenIndexTo, "Can't compare token to itself");
 397:        require(tokenIndexFrom < numTokens && tokenIndexTo < numTokens, "Tokens must be in pool");
 493:        require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range");
 524:        require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range");
 554:        require(amount <= totalSupply, "Cannot exceed total supply");
 615:        require(index < self.pooledTokens.length, "Token index out of range");
 649:        require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own");
 662:        require(dy >= minDy, "Swap didn't result in min tokens");
 697:        require(dy <= self.balances[tokenIndexTo], "Cannot get more than pool balance");
 703:        require(dx <= maxDx, "Swap needs more than max tokens");
 717:        require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own");
 723:        require(dx == tokenFrom.balanceOf(address(this)).sub(beforeBalance), "not support fee token");
 750:        require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own");
 756:        require(dy >= minDy, "Swap didn't result in min tokens");
 784:        require(dy <= self.balances[tokenIndexTo], "Cannot get more than pool balance");
 790:        require(dx <= maxDx, "Swap didn't result in min tokens");
 823:        require(amounts.length == pooledTokens.length, "Amounts must match pooled tokens");
 845:        require(v.totalSupply != 0 || amounts[i] > 0, "Must supply all tokens in pool");
 861:        require(v.d1 > v.d0, "D should increase");
 890:        require(toMint >= minToMint, "Couldn't mint min requested");
 916:        require(amount <= lpToken.balanceOf(msg.sender), ">LP.balanceOf");
 917:        require(minAmounts.length == pooledTokens.length, "minAmounts must match poolTokens");
 925:        require(amounts[i] >= minAmounts[i], "amounts[i] < minAmounts[i]");
 954:        require(tokenAmount <= lpToken.balanceOf(msg.sender), ">LP.balanceOf");
 955:        require(tokenIndex < pooledTokens.length, "Token not found");
 961:        require(dy >= minAmount, "dy < minAmount");
1005:        require(amounts.length == pooledTokens.length, "Amounts should match pool tokens");
1007:        require(maxBurnAmount <= v.lpToken.balanceOf(msg.sender) && maxBurnAmount != 0, ">LP.balanceOf");
1032:        require(tokenAmount != 0, "Burnt amount cannot be zero");
1035:        require(tokenAmount <= maxBurnAmount, "tokenAmount > maxBurnAmount");
1071:        require(newAdminFee <= MAX_ADMIN_FEE, "Fee is too high");
1084:        require(newSwapFee <= MAX_SWAP_FEE, "Fee is too high");

contracts/core/connext/facets/BaseConnextFacet.sol:
  38:        require(s._status != _ENTERED, "ReentrancyGuard: reentrant call");
 125:        require(_remote != bytes32(0), "!remote");

Unnecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

Consider declaring the following lines without explicitly setting a value:

contracts/core/connext/facets/BridgeFacet.sol:
  68:        uint16 public constant AAVE_REFERRAL_CODE = 0;

contracts/core/connext/facets/VersionFacet.sol:
  16:        uint8 internal immutable _version = 0;

contracts/core/shared/Version.sol:
   9:        uint8 public constant VERSION = 0;

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. Not defining these variables can help to reduce gas cost and contract size.

Instances include:

contracts/core/connext/libraries/LibDiamond.sol:
 131:        address oldFacetAddress = ds.selectorToFacetAndPosition[selector].facetAddress;
 164:        address oldFacetAddress = ds.selectorToFacetAndPosition[selector].facetAddress;

contracts/core/connext/libraries/AssetLogic.sol:
 388:        IStableSwap pool = s.adoptedToLocalPools[id];
 422:        IStableSwap pool = s.adoptedToLocalPools[id];

contracts/core/connext/libraries/SwapUtils.sol:
 367:        LPToken lpToken = self.lpToken;
 749:        IERC20 tokenFrom = self.pooledTokens[tokenIndexFrom];
1056:        IERC20 token = pooledTokens[i];

contracts/core/connext/facets/PortalFacet.sol:
  88:        uint256 routerBalance = s.routerBalances[msg.sender][_local]; // in local

Storage variables should be declared immutable when possible

If a storage variable is assigned only in the constructor, it should be declared as immutable. This would help to reduce gas costs as calls to immutable variables are much cheaper than regular state variables, as seen from the Solidity Docs:

Compared to regular state variables, the gas costs of constant and immutable variables are much lower. Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed.

Consider declaring these variables as immutable:

contracts/core/connext/helpers/ConnextPriceOracle.sol:
  49:        address public wrapped;

Variables declared as constant are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

contracts/core/connext/libraries/LibDiamond.sol:
  14:        bytes32 constant DIAMOND_STORAGE_POSITION = keccak256("diamond.standard.diamond.storage");

contracts/core/connext/libraries/AmplificationUtils.sol:
  22:        uint256 public constant MAX_A = 10**6;

contracts/core/connext/libraries/SwapUtils.sol:
 104:        uint256 internal constant FEE_DENOMINATOR = 10**10;
 107:        uint256 internal constant MAX_SWAP_FEE = 10**8;
 113:        uint256 internal constant MAX_ADMIN_FEE = 10**10;

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.