Connext Amarok contest - _Adam's results

L01 Missing != address(0) checks for Immutable Addresses

I recommend adding a address(0) check in the constructor when setting immutable addresses. Executor.sol#L48

L02 SafeApprove is deprecated

As per Open Zepplin safeApprove should only be used when setting initial allowance or reseting to zero. safeIncreaseAllowance should be used instead. AssetLogic.sol#L347

N01 Open Todos

Recommend removing open todos before deploying contracts BridgeFacet.sol#L492 BridgeFacet.sol#L579 BridgeFacet.sol#L1027 Executor.sol#L7 LibConnextStorage.sol#L303

N02 Incomplete Natspec

AssetFacet.sol#L130 - missing @param _stableSwapPool BaseConnextFacet.sol#L113 - missing @return BridgeFacet.sol#L626 - missing @return BridgeFacet.sol#L744 - missing @return BridgeFacet.sol#L913 - missing @param _message PortalFacet.sol#L79 - missing @param _transferId RoutersFacet.sol#L130 - missing @param cananonicalId StableSwapFacet.sol#L251 - missing @param amountIn, @param minAmountOut, @ param deadline, @return StableSwapFacet.sol#L278 - missing @param amountIn, @param minAmountOut, @ param deadline, @return AssetLogic.sol#L223 - missing @param _maxIn AssetLogic.sol#L259 - missing @param _slippageTol AssetLogic.sol#L301 - missing @param _maxIn LibConnextStorage.sol#L72 - missing @param routerSignatures LibConnextStorage.sol#L90- missing @param approvedForPortalRouters SwapUtils.sol#L171 - missing @param totalSupply SwapUtils.sol#L480 - missing @param balances SwapUtils.sol#L511 - missing @param balances SwapUtils.sol#L541 - missing @param self

G01 Custom Errors

Custom errors are cheaper in deployment costs and runtime costs. I ran a test in remix comparing a revert string vs custom errors and found that replacing a single revert string with a custom error saved 12,404 gas in deployment cost and 86 gas on each function call.

contract Test {
	uint256 a;
	function check() external {
		require(a != 0, "check failed");
	}
}   (Deployment cost: 114,703, Cost on Function call: 23,392)
vs 
contract Test {
	uint256 a;
	error checkFailed();
	function check() external {
		if (a != 0) revert checkFailed();
	}
}   (Deployment cost: 102,299, Cost on Function call: 23,306)

I recommend replacing the revert strings in the following locations with custom errors: BaseConnextFacet.sol#L38 BaseConnextFacet.sol#L125 ConnextPriceOracle.sol#L72 ConnextPriceOracle.sol#L150 Executor.sol#L57 LPToken.sol#L35 LPToken.sol#L50 StableSwap.sol#L75-L77 StableSwap.sol#L84-L90 StableSwap.sol#L96-L98 StableSwap.sol#L102 StableSwap.sol#L125 StableSwap.sol#L155 StableSwap.sol#L167 StableSwap.sol#L177 AmplificationUtils.sol#L84-L86 AmplificationUtils.sol#L92-L94 AmplificationUtils.sol#L111 ConnextMessage.sol#L116 LibDiamond.sol#L66 LibDiamond.sol#L100-L103 LibDiamond.sol#L121-L123 LibDiamond.sol#L132 LibDiamond.sol#L139-L141 LibDiamond.sol#L150 LibDiamond.sol#L158-L161 LibDiamond.sol#L191-L193 LibDiamond.sol#L224-L226 SwapUtils.sol#L191 SwapUtils.sol#L198 SwapUtils.sol#L248 SwapUtils.sol#L342 SwapUtils.sol#L396-L397 SwapUtils.sol#L493 SwapUtils.sol#L524 SwapUtils.sol#L554 SwapUtils.sol#L615 SwapUtils.sol#L649 SwapUtils.sol#L662 SwapUtils.sol#L697 SwapUtils.sol#L703 SwapUtils.sol#L717 SwapUtils.sol#L723 SwapUtils.sol#L750 SwapUtils.sol#L756 SwapUtils.sol#L784 SwapUtils.sol#L790 SwapUtils.sol#L823 SwapUtils.sol#L845 SwapUtils.sol#L861 SwapUtils.sol#L890 SwapUtils.sol#L916-L917 SwapUtils.sol#L925 SwapUtils.sol#L954-L955 SwapUtils.sol#L961 SwapUtils.sol#L1005-L1007 SwapUtils.sol#L1032-L1035 SwapUtils.sol#L1071 SwapUtils.sol#L1084

G02 Long Revert Strings

If opting not to update revert strings to custom errors, keeping revert strings <= 32 bytes in length will save gas. I ran a test in remix and found the savings for a single short revert string vs long string to be 9,377 gas in deployment cost and 18 gas on function call.

contract Test {
	uint256 a;
	function check() external {
		require(a != 0, "short error message"); 
		(Deployment cost: 114,799, Cost on function call: 23,392)	
		vs 
		require(a != 0, "A longer Error Message over 32 bytes in     
        length"); 
		(Deployment cost: 124,176, Cost on function call: 23,410)	
	}
}

I recommend shortenning the following revert strings to <= 32 bytes in length:

LibDiamond.sol#L66 LibDiamond.sol#L113 LibDiamond.sol#L121 LibDiamond.sol#L123 LibDiamond.sol#L132 LibDiamond.sol#L139 LibDiamond.sol#L141 LibDiamond.sol#L150 LibDiamond.sol#L158 LibDiamond.sol#L161 LibDiamond.sol#L191 LibDiamond.sol#L193 LibDiamond.sol#L224 LibDiamond.sol#L226 LibDiamond.sol#L236 SwapUtils.sol#L697 SwapUtils.sol#L784

G03 Uint > 0 Checks in Require Functions

If opting not to use custom errors when checking whether a uint is > 0 in a require function you can save a small amount of gas by replacing with != 0. This is only true if optimiser is turned on and in a require statement. I ran a test in remix with optimisation set to 10,000 and found the savings for a single occurance is 632 in deployment cost and 6 gas on each function call.

contract Test {
	uint256 a;
	function check() external {
		require(a > 0); 
		(Deployment cost: 79,763, Cost on function call: 23,305)	
		vs
		require(a != 0); 
		(Deployment cost: 79,331, Cost on function call: 23,299)	
	}
}

Instances where a uint is compared > 0: ConnextPriceOracle.sol#L150 AmplificationUtils.sol#L86 LibDiamond.sol#L121 LibDiamond.sol#L139 LibDiamond.sol#L158 LibDiamond.sol#L226 LibDiamond.sol#L247 SwapUtils.sol#L845

G04 && in Require Functions

If not opting to use custom errors and If optimising for running costs over deployment costs you can seperate && in require functions into 2 parts. I ran a basic test in remix and it cost an extra 234 gas to deploy but will save ~9 gas everytime the require function is called.

contract Test {
	uint256 a = 0;
	uint256 b = 1;

	function test() external {
		require(a == 0 && b > a) 
		(Deployment cost: 123,291, Cost on function call: 29,371)
		vs
		require(a == 0);
		require(b > a);
		(Deployment cost: 123,525, Cost on function call: 29,362)
	}
}

Instances where require statements can be split into seperate statements: StableSwap.sol#L84-L87 AmplificationUtils.sol#L86 SwapUtils.sol#L397 SwapUtils.sol#L493 SwapUtils.sol#L524

G05 For Loop Optimisations

When incrementing i in for loops there is no chance of overflow so unchecked can be used to save gas. I ran a simple test in remix and found deployment savings of 31,653 gas and on each function call saved ~141 gas per iteration.

contract Test {
	function loopTest() external {
		for (uint256 i; i < 1; ++i) {
		Deployment Cost: 125,637, Cost on function call: 24,601
		vs
		for (uint256 i; i < 1; ) {
		// for loop body
		unchecked { ++i }
		Deployment Cost: 93,984, Cost on function call: 24,460
		}
	}
}

In for loops pre increments can also be used to save a small amount of gas per iteraition. I ran a test in remix using a for loop and found the deployment savings of 497 gas and ~5 gas per iteration.

contract Test {
	function loopTest() external {
		for (uint256 i; i < 1; i++) {
		(Deployment cost: 118,408, Cost on function call: 24,532)
		vs
		for (uint256 i; i < 1; ++i) {
		(Deployment cost: 117,911, Cost on function call: 24,527)
		}
	}
}

Whenever looping over the length of a memory variable you can also cache the value and save ~3 gas per iteration.

Loops Where Pre/Unchecked Increments can be implemented: BridgeFacet.sol#L613 - pre increment only BridgeFacet.sol#L684 - pre increment only BridgeFacet.sol#L799 - pre increment only DiamondLoupeFacet.sol#L31 RelayerFacet.sol#L144 - pre increment only RelayerFacet.sol#L168 - pre increment only StableSwapFacet.sol#L415 - can also cache _ pooledTokens.length ConnextPriceOracle.sol#L176 StableSwap.sol#L81 - can also cache _ pooledTokens.length LibDiamond.sol#L104 - can also cache _ diamondCut.length LibDiamond.sol#L129- can also cache _ functionSelectors.length LibDiamond.sol#L147- can also cache _ functionSelectors.length LibDiamond.sol#L162- can also cache _ functionSelectors.length SwapUtils.sol#L205- can also cache xp.length SwapUtils.sol#L254 SwapUtils.sol#L268 SwapUtils.sol#L289 SwapUtils.sol#L300 SwapUtils.sol#L302 SwapUtils.sol#L344 SwapUtils.sol#L405 SwapUtils.sol#L425 SwapUtils.sol#L558 - can also cache balances.length SwapUtils.sol#L591 - can also cache balances.length SwapUtils.sol#L844 SwapUtils.sol#L869 SwapUtils.sol#L924 SwapUtils.sol#L1014 SwapUtils.sol#L1019 SwapUtils.sol#L1039 SwapUtils.sol#L1055 RelayerFeeMessage.sol#L85- pre increment only

G06 Emiting Storage Variables

You can save 1 SLOAD (~100 gas) by emiting local variables over storage variables when they have the same value.

ProposedOwnableUpgradeable.sol#L323 - can emit newlyProposed instead of _ proposed ProposedOwnable.sol#L172 - can emit newlyProposed instead of _ proposed

G07 Initialising to Default Values

When initialising a variable to its default variable, it is cheaper to leave blank. This can be shown by running a simple test on remix that initialises a single variable and shows a savings of 3,086 gas.

contract Test {
	uint16 variable = 0;   (80,212 gas)
	vs
	uint16 variable;      (77,126 gas)
}

Recommend removing the = 0 from the 2 following locations: BridgeFacet.sol#L68 VersionFacet.sol#L16

G08 Variables that can be Constant

The 2 following variables are initialised and then never modified and can be changed to constant to save gas. Executor.sol#L38 - save ~25,000 gas based on test on remix Executor.sol#L43 - save ~11,000 gas based on test on remix

G09 Unused Imports

The following are imported and never used in their contracts and can be removed to save gas. BridgeFacet.sol#L22 - IWrapped AmplificationUtils.sol#L4 - SafeERC20 AssetLogic.sol#L6 - IWrapped AssetLogic.sol#L8 - ITokenRegistry

G10 Minimize SLOADS

Whenever using a storage variable more than once in a function you can save ~97 gas per use. (normally 100 gas each use vs 103 gas to SLOAD/MSTORE for the first use and then only 3 gas for further uses)

ProposedOwnableFacet.sol#L148-L151 - can cache s._routerOwnershipTimestamp ProposedOwnableFacet.sol#L181-L184 - can cache s._assetOwnershipTimestamp ProposedOwnableFacet.sol#L219-L222 - can cache s._proposedOwnershipTimestamp ProposedOwnableFacet.sol#L226-L229 - can cache s._proposed ProposedOwnableFacet.sol#L238-L250 - can cache s._proposed ProposedOwnableUpgradeable.sol#L175-L178 - can cache _routerOwnershipTimestamp ProposedOwnableUpgradeable.sol#L217-L220 - can cache _assetOwnershipTimestamp ProposedOwnableUpgradeable.sol#L255-L258 - can cache _proposedOwnershipTimestamp ProposedOwnableUpgradeable.sol#L274-L286 - can cache _proposed ProposedOwnable.sol#L125-L128 - can cache _proposedOwnershipTimestamp ProposedOwnable.sol#L132-L135 - can cache _proposed

G11 Changing a mappings value to the Default Value

When setting the value of a mapping to the default value it is cheaper to use delete instead.

PromiseRouter.sol#L251 can change to: delete callbackFees[transferId]