Connext Amarok contest - c3phas's results

FINDINGS

Open todos left in the code File: BridgeFacet.sol line 492

      // TODO: do we want to store a mapping of custodied token balances here?

File: BridgeFacet.sol line 579

      // TODO: do we need to keep this

File: BridgeFacet.sol line 594

    // FIXME: routers can repay any-amount out-of-band using the `repayAavePortal` method

File: BridgeFacet.sol line 1027

    // TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?

Commented code should be deleted

File:DiamondLoupeFacet.sol line 20-23

  // struct Facet {
  //     address facetAddress;
  //     bytes4[] functionSelectors;
  // }

TYPO

File:ProposedOwnableFacet.sol line 163

    // Contract as sournce of truth

sournce instead of source

File:ProposedOwnableFacet.sol line 176

    // Contract as sournce of truth

sournce instead of source

File: ProposedOwnableUpgradeable.sol line 170, see also line 198, line 212

    // Contract as sournce of truth

File: RelayerFacet.sol line 35

   * @notice Emitted when a rlayer is added or removed from whitelists

rlayer instead of relayer

Get rid of safemath as the over/underflow checks are in built in solidity 0.8+

We can save on some gas by getting rid of safemath library and using unchecked blocks for arithmetic operations that can never overflow/underflow SafeMath is no longer needed starting with Solidity 0.8. The compiler now has built in overflow checking.

If we get rid of safemath we can optimize some functions to save on gas. Since the compiler will check for under/overflows by default, we can disable this checks for arithmetic operations that are guaranteed to never over/underflow. This would include the following

File:AmplificationUtils.sol line 15

  using SafeMath for uint256;

File:SwapUtils.sol line 20

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block

File: BridgeFacet.sol line 1099-1100

      if (availableAmount > backUnbackedAmount) {
        // Repay the whole transfer and a partial amount of fees
        portalFee = availableAmount - backUnbackedAmount;

The line availableAmount - backUnbackedAmount; should be wrapped in an unchecked block since it cannot underflow due to the check on line 1099 that ensures that availableAmount is greater than backUnbackedAmount

We can modify the above as follows:

      if (availableAmount > backUnbackedAmount) {
        // Repay the whole transfer and a partial amount of fees
        portalFee = unchecked { availableAmount - backUnbackedAmount; }

Other Instances to modify File:PortalFacet.sol line 147

      uint256 missing = total - amount;

The above cannot underflow due to the check on line 146 which ensures that total is greater than amount before performing the arithmetic operation.

File:PortalFacet.sol line 150

        _feeAmount -= missing;

The above cannot underflow due the check on line 148 which ensures that _feeAmount is greater than missing before performing the arithmetic operation.

File:PortalFacet.sol line 153

        missing -= _feeAmount;

The above line would only be executed if the check on line 148 fails. That check ensure that _feeAmount is greater than missing which means if this check fails then missing is greater than _feeAmount therefore our arithmetic operation missing -= _feeAmount; would not underflow.

For loops Increments can be unchecked to save gas.

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop.

e.g Let's work with a sample loop below to demonstrate how to use unchecked blocks

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code

File: DiamondLoupeFacet.sol line 31

  function facets() external view override returns (Facet[] memory facets_) {
    LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
    uint256 numFacets = ds.facetAddresses.length;
    facets_ = new Facet[](numFacets);
    for (uint256 i; i < numFacets; i++) {
      address facetAddress_ = ds.facetAddresses[i];
      facets_[i].facetAddress = facetAddress_;
      facets_[i].functionSelectors = ds.facetFunctionSelectors[facetAddress_].functionSelectors;
    }
  }

The above function can be modified to the following

  function facets() external view override returns (Facet[] memory facets_) {
    LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
    uint256 numFacets = ds.facetAddresses.length;
    facets_ = new Facet[](numFacets);
    for (uint256 i; i < numFacets;) {
      address facetAddress_ = ds.facetAddresses[i];
      facets_[i].facetAddress = facetAddress_;
      facets_[i].functionSelectors = ds.facetFunctionSelectors[facetAddress_].functionSelectors;
      unchecked {
          i++;
        }
    }
  }

Other instances File: ConnextPriceOracle.sol line 176

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

File: LibDiamond.sol line 104

    for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {

Something similar to my proposal for the above has already been implemented on contract BridgeFacet.sol at line 610-614

      for (uint256 i; i < pathLen; ) {
        s.routerBalances[routers[i]][token] += routerAmt;
        unchecked {
          i++;
        }

See line 683,line 798 to for another implementation of the same.

Cache the length of arrays in loops ~6 gas per iteration

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

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File:RelayerFacet.sol line 130-146

  function initiateClaim(
    uint32 _domain,
    address _recipient,
    bytes32[] calldata _transferIds
  ) external whenNotPaused {
    // Make sure the transferIds length is greater than 0.
    // This is to make sure a valid relayer is calling this function.
    if (_transferIds.length == 0) revert RelayerFacet__initiateClaim_emptyClaim();


    // Ensure the relayer can claim all transfers specified.
    for (uint256 i; i < _transferIds.length; ) {
      if (s.transferRelayer[_transferIds[i]] != msg.sender)
        revert RelayerFacet__initiateClaim_notRelayer(_transferIds[i]);
      unchecked {
        i++;
      }
    }

From the above function we can see that _transferIds.length is being read first when checking whether it's value is 0 and then read again inside the for loop. The length should be cached and used instead of repeatedly fetching it from the array.

Other instances File:RelayerFacet.sol line 164

    for (uint256 i; i < _transferIds.length; ) {

File:StableSwapFacet.sol line 415

    for (uint8 i = 0; i < _pooledTokens.length; i++) {

File: ConnextPriceOracle.sol line 176

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

File: StableSwap.sol line 75-81

    require(_pooledTokens.length > 1, "_pooledTokens.length <= 1");
    require(_pooledTokens.length <= 32, "_pooledTokens.length > 32");
    require(_pooledTokens.length == decimals.length, "_pooledTokens decimals mismatch");


    uint256[] memory precisionMultipliers = new uint256[](decimals.length);


    for (uint8 i = 0; i < _pooledTokens.length; i++) {

Other than being repeatedly read inside the the for loop, _pooledTokens.length is also being read three times in the require statement . Caching this variable would save alot of gas as it would not have to be checked up everytime.

File: LibDiamond.sol line 104

    for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {

In the above we should cache _diamondCut.length

File:SwapUtils.sol line 205

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

Other than being repeatedly read inside the the for loop, xp.length is also being read in the require statement at line 191 and line 202. Caching this variable would save alot of gas as it would not have to be checked up everytime. See line 247 to see how a good impelementation would look.

File:SwapUtils.sol line 558 File:SwapUtils.sol line 591 File:SwapUtils.sol line 844 File:SwapUtils.sol line 869 File:SwapUtils.sol line 924 File:SwapUtils.sol line 1014 File:LibDiamond.sol line 147 File:LibDiamond.sol line 162

Splitting require() statements that use && saves gas - 8 gas per &&

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).

File: StableSwap.sol line 84

        require(tokenIndexes[address(_pooledTokens[i])] == 0 && _pooledTokens[0] != _pooledTokens[i],"Duplicate tokens");

The above should be modified to:

  require(tokenIndexes[address(_pooledTokens[i])] == 0 "Duplicate tokens");
  require(_pooledTokens[0] != _pooledTokens[i],"Duplicate tokens");

Other instances File:AmplificationUtils.sol line 86

    require(futureA_ > 0 && futureA_ < MAX_A, "futureA_ must be > 0 and < MAX_A");

File: SwapUtils.sol line 397

    require(tokenIndexFrom < numTokens && tokenIndexTo < numTokens, "Tokens must be in pool");

File: SwapUtils.sol line 493

    require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range");

File: SwapUtils.sol line 524

    require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range");

File: SwapUtils.sol line 1007

    require(maxBurnAmount <= v.lpToken.balanceOf(msg.sender) && maxBurnAmount != 0, ">LP.balanceOf");

Proof The following tests were carried out in remix with both optimization turned on and off

    require ( a > 1 && a < 5, "Initialized");
    return  a + 2;
}

Execution cost 21617 with optimization and using && 21976 without optimization and using &&

After splitting the require statement

    require (a > 1 ,"Initialized");
    require (a < 5 , "Initialized");
    return a + 2;
}

Execution cost 21609 with optimization and split require 21968 without optimization and using split require

No need to initialize variables with their default values

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas. It costs more gas to initialize variables to zero than to let the default of zero be applied

File: ConnextPriceOracle.sol line 176

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

Other instances to modify File:SwapUtils.sol line 205

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

File:SwapUtils.sol line 254 File:SwapUtils.sol line 268 File:SwapUtils.sol line 558 File:SwapUtils.sol line 591

For the loops my suggestion would be to modify them as follows

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

Comparisons: != is more efficient than > in require

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) For uints the minimum value would be 0 and never a negative value. Since it cannot be a negative value, then the check > 0 is essentially checking that the value is not equal to 0 therefore >0 can be replaced with !=0 which saves gas.

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs:

I suggest changing > 0 with != 0 here:

File: ConnextPriceOracle.sol line 150

    require(baseTokenPrice > 0, "invalid base token");

Something similar to my proposal has already been implemented on line 35

use shorter revert strings(less than 32 bytes)

You can (and should) attach error reason strings along with require statements to make it easier to understand why a contract call reverted. These strings, however, take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

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.

File: LibDiamond.sol line 66

    require(msg.sender == diamondStorage().contractOwner, "LibDiamond: Must be contract owner");

Other instances File: LibDiamond.sol line 113 File: LibDiamond.sol line 121 File: LibDiamond.sol line 123 File: LibDiamond.sol line 132 File: LibDiamond.sol line 139 File: LibDiamond.sol line 141 File: LibDiamond.sol line 150 File: LibDiamond.sol line 158 File: LibDiamond.sol line 161 File: LibDiamond.sol line 170 File: LibDiamond.sol line 191 File: LibDiamond.sol line 193 File: LibDiamond.sol line 224 File: LibDiamond.sol line 226

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

see Source

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 are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).