Connext Amarok contest - slywaters's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L347 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L249 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L304 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/PortalFacet.sol#L98 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1011

Vulnerability details

Impact

An unexpected revert could cause failures on the part of bridges and portals to repay loans.

Proof of Concept

OpenZeppelin's safeApprove() function will revert if the account has already been approved and subsequent calls pass a non-zero value.

safeApprove() is used in one instance in the codebase in the AssetLogic.sol contract:

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

It is called within a private function _swapAssetOut() which is used by the public function swapFromLocalAssetIfNeededForExactOut().

  function swapFromLocalAssetIfNeededForExactOut(
    address _asset,
    uint256 _amount,
    uint256 _maxIn
  )
    internal
    returns (
      bool,
      uint256,
      address
    )
  {
    AppStorage storage s = LibConnextStorage.connextStorage();


    // Get the token id
    (, bytes32 id) = s.tokenRegistry.getTokenId(_asset);


    // If the adopted asset is the local asset, no need to swap
    address adopted = s.canonicalToAdopted[id];
    if (adopted == _asset) {
      return (true, _amount, _asset);
    }


    return _swapAssetOut(id, _asset, adopted, _amount, _maxIn);
  }

This public function is called from the contracts PortalFacet.sol and BridgeFacet.sol.

File: 2022-06-connext/contracts/contracts/core/connext/facets/PortalFacet.sol
98:    (bool success, uint256 amountIn, address adopted) = AssetLogic.swapFromLocalAssetIfNeededForExactOut(

File: 2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol
1011:    (bool swapSuccess, uint256 amountIn, address adopted) = AssetLogic.swapFromLocalAssetIfNeededForExactOut(

These functions both handle functionality pertaining to loan repayments: _reconcileProcessPortal() in BridgeFacet.sol and repayAavePortal() in PortalFacet.sol.

Given that safeApprove() will revert when it is called more than once with a non-zero value argument, the use of this function could seriously impact the availability of these features and result in negative financial impacts for users.

Tools Used

Reading source code

Recommended Mitigation Steps

Replace calls to safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() where appropriate. If this cannot be done, use safeApprove(0) if the allowance will be changed.

References

• safeApprove() source code. Note that it is deprecated.
• Similar issue in a previous code4rena audit

Developer notes should be removed from production code

Comments in production code should not contain developer discussion or notes about known bugs or problems. These issues should be tracked elsewhere and resolved before being deployed.

Comments of this kind can also indicate potential avenues of attack for an adversary.

The following lines are affected:

2022-06-connext/contracts/contracts/core/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?
2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol:492:      // TODO: do we want to store a mapping of custodied token balances here?
2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol:579:      // TODO: do we need to keep this
2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol:594:    // FIXME: routers can repay any-amount out-of-band using the `repayAavePortal` method
2022-06-connext/contracts/contracts/core/connext/helpers/Executor.sol:7:// TODO: see note in below file re: npm
2022-06-connext/contracts/contracts/core/connext/libraries/LibConnextStorage.sol:303:  // BridgeFacet (cont.) TODO: can we move this

Gas Report

Variables do not need to be initialized with 'empty' values such as 0, false, or address(0)

Uninitialized variables by default contain a value equivalent to 0: uints are initialized to 0; bools to false; addresses to address(0).

Explicitly assigning these values to variables when they are declared increases gas costs while providing no funciton.

e.g. change this code:

uint256 var = 0;

to

uint256 var;

For more information, please consult the following resources:

Tips and Tricks to Save Gas and Reduce Bytecode Size

The following lines of code are affected:

2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol:68:  uint16 public constant AAVE_REFERRAL_CODE = 0;
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:415:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/facets/VersionFacet.sol:16:  uint8 internal immutable _version = 0;
2022-06-connext/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol:176:    for (uint256 i = 0; i < tokenAddresses.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/Multicall.sol:16:    for (uint256 i = 0; i < calls.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol:81:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/Encoding.sol:22:    for (uint8 i = 0; i < 10; i += 1) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1014:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1019:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1039:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1055:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:205:    for (uint256 i = 0; i < xp.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:254:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:268:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:289:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:300:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:302:      for (uint256 j = 0; j < numTokens; j++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:344:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:405:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:425:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:558:    for (uint256 i = 0; i < balances.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:591:    for (uint256 i = 0; i < balances.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:844:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:869:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:924:    for (uint256 i = 0; i < amounts.length; i++) {
2022-06-connext/contracts/contracts/core/relayer-fee/libraries/RelayerFeeMessage.sol:81:    for (uint256 i = 0; i < length; ) {
2022-06-connext/contracts/contracts/core/shared/Version.sol:9:  uint8 public constant VERSION = 0;

Unchecked increment can be used in for-loop

Newer versions of the Solidity compiler will check for integer overflows and underflows automatically. This provides safety but increases gas costs.

When an unsigned integer is guaranteed to never overflow, the unchecked feature of Solidity can be used to save gas costs.

A common case for this is for-loops using a strictly-less-than comparision in their conditional statement, e.g.:

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

In cases like this, the maximum value for length is 2**256 - 1. Therefore, the maximum value of i is 2**256 - 2 as it will always be strictly less than length.

This example can be replaced with the following construction to reduce gas costs:

for (uint i = 0; i < length; i = unchecked_inc(i)) {
    // do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
    unchecked {
        return i + 1;
    }
}

For more information, consult the following resources:

Solidity gas optimizations

Solidity docs: underflows, overflows, and unchecked

The following lines of code are affected:

2022-06-connext/contracts/contracts/core/connext/facets/DiamondLoupeFacet.sol:31:    for (uint256 i; i < numFacets; i++) {
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:415:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol:176:    for (uint256 i = 0; i < tokenAddresses.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/Multicall.sol:16:    for (uint256 i = 0; i < calls.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol:81:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:104:    for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:129:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:147:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:162:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1014:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1019:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1039:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1055:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:205:    for (uint256 i = 0; i < xp.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:254:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:268:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:289:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:300:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:302:      for (uint256 j = 0; j < numTokens; j++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:344:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:405:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:425:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:558:    for (uint256 i = 0; i < balances.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:591:    for (uint256 i = 0; i < balances.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:844:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:869:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:924:    for (uint256 i = 0; i < amounts.length; i++) {

Replace postfix increment (var++) with prefix increment (++var)

Using ++i costs less gas than using i++. In the context of a for-loop, gas is saved on each iteration.

The following lines of code are affected:

2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol:613:          i++;
2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol:684:          i++;
2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol:799:            i++;
2022-06-connext/contracts/contracts/core/connext/facets/DiamondLoupeFacet.sol:31:    for (uint256 i; i < numFacets; i++) {
2022-06-connext/contracts/contracts/core/connext/facets/RelayerFacet.sol:144:        i++;
2022-06-connext/contracts/contracts/core/connext/facets/RelayerFacet.sol:168:        i++;
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:415:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol:176:    for (uint256 i = 0; i < tokenAddresses.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/Multicall.sol:16:    for (uint256 i = 0; i < calls.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol:81:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:104:    for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:129:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:134:      selectorPosition++;
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:147:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:153:      selectorPosition++;
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:162:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1014:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1019:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1039:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1055:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:205:    for (uint256 i = 0; i < xp.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:254:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:268:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:289:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:300:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:302:      for (uint256 j = 0; j < numTokens; j++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:344:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:405:    for (uint256 i = 0; i < numTokens; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:425:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:558:    for (uint256 i = 0; i < balances.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:591:    for (uint256 i = 0; i < balances.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:844:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:869:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:924:    for (uint256 i = 0; i < amounts.length; i++) {
2022-06-connext/contracts/contracts/core/relayer-fee/libraries/RelayerFeeMessage.sol:85:        i++;

Array length can be cached

In the context of a for-loop that iterates over an array, it costs less gas to cache the array's length in a variable and read from this variable rather than use the arrays .length property. Reading the .length property for on the array will cause a recalculation of the array's length on each iteration of the loop which is a more expensive operation than reading from a stack variable.

For example, the following code:

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

should be changed to:

uint length = arr.length;
for (uint i; i < length; ++i) {
	// ...
}

Note that in the second case, the length of the array must not change during the loop's execution.

For more information, see the following resource:

Solidity gas optimizations

The following lines of code are affected:

2022-06-connext/contracts/contracts/core/connext/facets/RelayerFacet.sol:140:    for (uint256 i; i < _transferIds.length; ) {
2022-06-connext/contracts/contracts/core/connext/facets/RelayerFacet.sol:164:    for (uint256 i; i < _transferIds.length; ) {
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:415:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol:176:    for (uint256 i = 0; i < tokenAddresses.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/Multicall.sol:16:    for (uint256 i = 0; i < calls.length; i++) {
2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol:81:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:104:    for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:129:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:147:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:162:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1014:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1019:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1039:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1055:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:205:    for (uint256 i = 0; i < xp.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:558:    for (uint256 i = 0; i < balances.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:591:    for (uint256 i = 0; i < balances.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:844:    for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:869:      for (uint256 i = 0; i < pooledTokens.length; i++) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:924:    for (uint256 i = 0; i < amounts.length; i++) {

"calldata" can be used instead of "memory" to reduce gas costs

Both memory and calldata allow a developer to manage variables within the scope of a function. memory is mutable which makes it a more flexible tool; however, it costs additional gas when compared with calldata. calldata is non-modifiable but cheaper. Therefore it is recommended to use calldata for function parameters when they will not be modified.

For more information, see the following resources: Solidity Documentation recommending the use of calldata

Solidity — Storage vs Memory vs Calldata

The following function calls can be changed to use calldata instead of memory:

2022-06-connext/contracts/contracts/core/connext/helpers/Executor.sol:113:  function execute(ExecutorArgs memory _args) external payable override onlyConnext returns (bool, bytes memory) {
2022-06-connext/contracts/contracts/core/connext/helpers/LPToken.sol:21:  function initialize(string memory name, string memory symbol) external initializer returns (bool) {
2022-06-connext/contracts/contracts/core/connext/helpers/Multicall.sol:13:  function aggregate(Call[] memory calls) public returns (uint256 blockNumber, bytes[] memory returnData) {
2022-06-connext/contracts/contracts/core/connext/libraries/ConnextMessage.sol:146:  function formatTokenId(TokenId memory _tokenId) internal pure returns (bytes29) {
2022-06-connext/contracts/contracts/core/connext/libraries/LibCrossDomainProperty.sol:157:  function parseDomainAndSenderBytes(bytes memory _property) internal pure returns (bytes29) {
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:286:  function getD(uint256[] memory xp, uint256 a) internal pure returns (uint256) {

Use strict less-than/greather-than comparisons rather than less-than-or-equal/greater-than-or-equal

When compiled, Solidity code using the >= or <= comparison operators in fact executes two separate checks: one for 'is-equal-to' and a second for 'is-greater-than/is-less-than'. By contrast, using > or < performs only one check. Therefore code that is written to use strict comparison operators is more gas-efficient.

If this change is applied, be sure to update the relevant variables being evaluated. For clarity, it is also advised to rename the variables to make this change explicit, e.g. renaming a variable from MINIMUM to MINIMUM_PLUS_ONE.

The following lines are affected:

2022-06-connext/contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol:151:    if ((block.timestamp - s._routerOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol:184:    if ((block.timestamp - s._assetOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol:222:    if ((block.timestamp - s._proposedOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol:246:    if ((block.timestamp - s._proposedOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/connext/facets/RoutersFacet.sol:434:    if (block.timestamp - s.routerPermissionInfo.proposedRouterTimestamp[router] <= _delay)
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:108:    if (index >= s.swapStorages[canonicalId].pooledTokens.length) revert StableSwapFacet__getSwapToken_outOfRange();
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:132:    if (index >= s.swapStorages[canonicalId].balances.length)
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:408:    if (_pooledTokens.length <= 1 || _pooledTokens.length > 32)
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:431:    if (_a >= AmplificationUtils.MAX_A) revert StableSwapFacet__initializeSwap_aExceedMax();
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:432:    if (_fee >= SwapUtils.MAX_SWAP_FEE) revert StableSwapFacet__initializeSwap_feeExceedMax();
2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol:433:    if (_adminFee >= SwapUtils.MAX_ADMIN_FEE) revert StableSwapFacet__initializeSwap_adminFeeExceedMax();
2022-06-connext/contracts/contracts/core/connext/helpers/BridgeToken.sol:130:    require(block.timestamp <= _deadline, "ERC20Permit: expired deadline");
2022-06-connext/contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol:178:    if ((block.timestamp - _routerOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol:220:    if ((block.timestamp - _assetOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol:258:    if ((block.timestamp - _proposedOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol:282:    if ((block.timestamp - _proposedOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/connext/helpers/SponsorVault.sol:208:      amountIn = currentBalance >= amountIn ? amountIn : currentBalance;
2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol:125:    require(block.timestamp <= deadline, "Deadline not met");
2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol:75:    require(_pooledTokens.length > 1, "_pooledTokens.length <= 1");
2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol:76:    require(_pooledTokens.length <= 32, "_pooledTokens.length > 32");
2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol:90:      require(decimals[i] <= SwapUtils.POOL_PRECISION_DECIMALS, "Token decimals exceeds max");
2022-06-connext/contracts/contracts/core/connext/libraries/AmplificationUtils.sol:84:    require(block.timestamp >= self.initialATime.add(1 days), "Wait 1 day before starting ramp");
2022-06-connext/contracts/contracts/core/connext/libraries/AmplificationUtils.sol:85:    require(futureTime_ >= block.timestamp.add(MIN_RAMP_TIME), "Insufficient ramp time");
2022-06-connext/contracts/contracts/core/connext/libraries/AmplificationUtils.sol:92:      require(futureAPrecise.mul(MAX_A_CHANGE) >= initialAPrecise, "futureA_ is too small");
2022-06-connext/contracts/contracts/core/connext/libraries/AmplificationUtils.sol:94:      require(futureAPrecise <= initialAPrecise.mul(MAX_A_CHANGE), "futureA_ is too large");
2022-06-connext/contracts/contracts/core/connext/libraries/AssetLogic.sol:333:      if (_maxIn >= ipool.calculateSwapInv(tokenIndexIn, tokenIndexOut, _amountOut)) {
2022-06-connext/contracts/contracts/core/connext/libraries/AssetLogic.sol:342:      if (_amountIn <= _maxIn) {
2022-06-connext/contracts/contracts/core/connext/libraries/MathUtils.sol:19:    return (difference(a, b) <= 1);
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1007:    require(maxBurnAmount <= v.lpToken.balanceOf(msg.sender) && maxBurnAmount != 0, ">LP.balanceOf");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1035:    require(tokenAmount <= maxBurnAmount, "tokenAmount > maxBurnAmount");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1071:    require(newAdminFee <= MAX_ADMIN_FEE, "Fee is too high");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:1084:    require(newSwapFee <= MAX_SWAP_FEE, "Fee is too high");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:198:    require(tokenAmount <= xp[tokenIndex], "Withdraw exceeds available");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:554:    require(amount <= totalSupply, "Cannot exceed total supply");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:649:      require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:662:    require(dy >= minDy, "Swap didn't result in min tokens");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:697:    require(dy <= self.balances[tokenIndexTo], "Cannot get more than pool balance");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:703:    require(dx <= maxDx, "Swap needs more than max tokens");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:717:      require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:750:    require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:756:    require(dy >= minDy, "Swap didn't result in min tokens");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:784:    require(dy <= self.balances[tokenIndexTo], "Cannot get more than pool balance");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:790:    require(dx <= maxDx, "Swap didn't result in min tokens");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:890:    require(toMint >= minToMint, "Couldn't mint min requested");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:916:    require(amount <= lpToken.balanceOf(msg.sender), ">LP.balanceOf");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:925:      require(amounts[i] >= minAmounts[i], "amounts[i] < minAmounts[i]");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:954:    require(tokenAmount <= lpToken.balanceOf(msg.sender), ">LP.balanceOf");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:961:    require(dy >= minAmount, "dy < minAmount");
2022-06-connext/contracts/contracts/core/promise/libraries/PromiseMessage.sol:136:    if (_len <= LENGTH_RETURNDATA_START) {
2022-06-connext/contracts/contracts/core/relayer-fee/libraries/RelayerFeeMessage.sol:99:    return _len >= MIN_CLAIM_LEN && (_len - TRANSFER_IDS_START) % TRANSFER_ID_LEN == 0;
2022-06-connext/contracts/contracts/core/shared/ProposedOwnable.sol:128:    if ((block.timestamp - _proposedOwnershipTimestamp) <= _delay)
2022-06-connext/contracts/contracts/core/shared/ProposedOwnable.sol:152:    if ((block.timestamp - _proposedOwnershipTimestamp) <= _delay)

Split require() statements using && into separate statements

Using && operations within require() statements increases gas costs. Separate these conditions into separate require() calls.

The following require() statements should be refactored:

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

Use custom errors rather than revert strings

Custom errors can be coded in Solidity in order to provide a user-friendly message while also saving on gas costs. It is preferable to use these errors instead of hard-coding strings into revert() statements.

For more information, consult the following resources:

• Solidity docs: errors and the revert statement
• Solidity revert with custom error explained with example

The following lines are affected:

2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:113:        revert("LibDiamondCut: Incorrect FacetCutAction");
2022-06-connext/contracts/contracts/core/connext/libraries/LibDiamond.sol:236:          revert("LibDiamondCut: _init function reverted");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:275:    revert("Approximation did not converge");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:320:    revert("D does not converge");
2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol:432:    revert("Approximation did not converge");