Connext Amarok contest - catchup's results

Floating pragma is used for DiamondInit.sol

All the contracts are using the same fixed solidity version 0.8.14, but DiamondInit.sol is using floating ^0.8.0.

Missing non-zero address checks when setting important addresses

It is a good practice to include 0 address check while updating an important address. I suggest to include a non-zero address check for the functions below.

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L158 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L163 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L168

It would be better to change setAdmin() to a two-step process

setAdmin() is called by the current admin and changes the admin address. If the newAdmin address is entered wrong for some reason, the admin role would be lost. Therefore, it is a better approach to follow a 2-step process when updating a priviliged adress. First transaction proposes the new admin address, second transaction which can only be called by the proposed address accepts the role.

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L168

Each event can have up to three indexed fields.

Many events do not have indexed fields. Consider using more indexed fields for better log filtering.

Lines of code

There are many lines, some of which are: https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L65-L69 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/AssetFacet.sol#L27 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/AssetFacet.sol#L35 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/AssetFacet.sol#L44

Missing non-zero address checks for token transfers

Tokens would be burned if sent to zero address accidentally. Therefore, it is a good practice to include non-zero address checks for token transfers.

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/AssetLogic.sol#L145 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/SponsorVault.sol#L296 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L1059

TODO comments

There are a few TODO comments, meaning there is incomplete work or expected changes. Either the TODO comments should be addressed or deleted.

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L492 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L579 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1027 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/Executor.sol#L7 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L303

for loops can be optimized

1- For loop index increments can be made unchecked for solidity versions > 0.8.0. There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. 2- Postfix increments can be changed as prefix as it is cheaper. 3- There is no need to set uint256 variable to 0 since default value is already 0.

Lines of code

There are 32 lines some of which are below: https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L254 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L268 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L289

Recommended Mitigation Steps

I suggest to change the original code from this

    for (uint256 i = 0; i < numTokens; i++) {
      s = s.add(xp[i]);
    }

to this

    for (uint256 i; i < numTokens; ) {
      s = s.add(xp[i]);
      unchecked { ++i; }
    }

Error string longer than 32 characters

Error reason strings 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.

Lines of code

There are many lines some of which are: https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L121 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L123 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L132

Using != 0 is cheaper than > 0 when used on a uint in a require() statement with the optimizer enabled

Lines of code

There are many lines some of which are: https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L121 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L226 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L247

Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

All the for loop index inits to zero and additioanlly the lines below. https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L68 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/VersionFacet.sol#L16

Prefix increment/decrements are cheaper than postfix

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L613 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L134 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L153