Connext Amarok contest - joestakey's results

QA Report

Table of Contents

• summary
• Comment Missing function parameter
• Events indexing
• Event should be emitted in setters
• Events emitted early
• Function missing comments
• open TODOs
• Payable function not rejecting payments to ERC20 Tokens
• Public functions can be external
• Use scientific notation rather than exponentiation
• Immutable addresses lack zero-address check
• safeApprove is deprecated
• Setters should check the input value
• Timelock in fee setters

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the setters not checking the input value properly and the use of deprecated functions.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AssetFacet.sol

address _stableSwapPool
ConnextMessage.TokenId calldata _canonical, address _stableSwapPool

BaseConnextFacet.sol

address _potentialReplica

BridgeFacet.sol

XCallArgs calldata _args,address _asset,bytes32 _transferId,uint256 _amount
ExecuteArgs calldata _args
XCallArgs calldata _args, ConnextMessage.TokenId memory _canonical
ExecuteArgs calldata _args
CallParams calldata _params,uint256 _amount,uint256 _nonce,bytes32 _canonicalId,uint32 _canonicalDomain,address _originSender
bytes32 _transferId,bool _isFast,ExecuteArgs calldata _args
ExecuteArgs calldata _args,uint256 _amount,address _asset, // adopted (or local if specified)bytes32 _transferId,bool _reconciled
bytes32 _transferId,uint256 _fastTransferAmount,address _local,address _router bytes memory _message

PortalFacet.sol

bytes32 _transferId

StableSwapFacet.sol

uint256 minAmountOut,uint256 deadline
uint256 maxAmountIn,uint256 deadline

LPToken.sol

address from,address to,uint256 amount

ProposedOwnableUpgradeable.sol

address newlyProposed

AssetLogic.sol

uint256 _maxIn
uint256 _slippageTol
uint256 _maxIn

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Events indexing

PROBLEM

Events should use indexed fields when possible, up to three per event.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AssetFacet.sol

event WrapperUpdated(address oldWrapper, address newWrapper, address caller)
event TokenRegistryUpdated(address oldTokenRegistry, address newTokenRegistry, address caller)
event StableSwapAdded(bytes32 canonicalId, uint32 domain, address swapPool, address caller)
event AssetAdded(bytes32 canonicalId, uint32 domain, address adoptedAsset, address supportedAsset, address caller)
event AssetRemoved(bytes32 canonicalId, address caller)

BridgeFacet.sol

event XCalled
event Reconciled
event Executed
event TransferRelayerFeesUpdated
event ForcedReceiveLocal
event AavePortalMintUnbacked
event AavePortalRepayment
event AavePortalRepaymentDebt
event SponsorVaultUpdated
event PromiseRouterUpdated
event ExecutorUpdated

PortalFacet.sol

event AavePortalRouterRepayment

ProposedOwnableFacet.sol

event RouterOwnershipRenunciationProposed(uint256 timestamp)
event RouterOwnershipRenounced(bool renounced)
event AssetOwnershipRenunciationProposed(uint256 timestamp)
event AssetOwnershipRenounced(bool renounced)

RelayerFacet.sol

event RelayerFeeRouterUpdated
event RelayerAdded
event RelayerRemoved
event InitiatedClaim
event Claimed

RoutersFacet.sol

event RouterAdded
event RouterRemoved
event MaxRoutersPerTransferUpdated
event LiquidityFeeNumeratorUpdated
event RouterApprovedForPortal
event RouterUnapprovedForPortal
event RouterLiquidityAdded
event RouterLiquidityRemoved

ConnextPriceOracle.sol

event NewAdmin(address oldAdmin, address newAdmin)
event PriceRecordUpdated(address token, address baseToken, address lpToken, bool _active)
event DirectPriceUpdated(address token, uint256 oldPrice, uint256 newPrice)
event AggregatorUpdated(address tokenAddress, address source)
event V1PriceOracleUpdated(address oldAddress, address newAddress)

ProposedOwnableUpgradeable.sol

event RouterOwnershipRenunciationProposed(uint256 timestamp)
event RouterOwnershipRenounced(bool renounced)
event AssetOwnershipRenunciationProposed(uint256 timestamp)
event AssetOwnershipRenounced(bool renounced)
event OwnershipProposed(address indexed proposedOwner)
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner)

SponsorVault.sol

event ConnextUpdated
event RateUpdated
event RelayerFeeCapUpdated
event GasTokenOracleUpdated
event TokenExchangeUpdated
event ReimburseLiquidityFees
event ReimburseRelayerFees
event Deposit
event Withdraw

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events, so that there is up to three indexed fields when possible.

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AssetFacet.sol

emit AssetAdded(_canonical.id, _canonical.domain, _adoptedAssetId, supported, msg.sender) emitted before call to _addStableSwapPool()

LibDiamond.sol

emit DiamondCut(_diamondCut, _init, _calldata) emitted before call to initializeDiamondCut()

TOOLS USED

Manual Analysis

MITIGATION

Place the event emission in the last position in the function.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PortalFacet.sol

setAavePool(address _aavePool)
setAavePortalFee(uint256 _aavePortalFeeNumerator)

StableSwapFacet.sol

setSwapAdminFee(bytes32 canonicalId, uint256 newAdminFee)
setSwapFee(bytes32 canonicalId, uint256 newSwapFee)

SponsorVault.sol

setConnext(address _connext)\

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AssetFacet.sol

All the getters

BridgeFacet.sol

All the getters and admin methods.

DiamondCutFacet.sol

proposeDiamondCut rescindDiamondCut

NomadFacet.sol

All the getters

PortalFacet.sol

All the getters

ProposedOwnableFacet.sol

All the internal functions

RelayerFacet.sol

All the getters

RoutersFacet.sol

Some of the getters

ConnextPriceOracle.sol

All the functions

ProposedOwnableUpgradeable.sol

All the internal functions

SponsorVault.sol

_setConnext

LibDiamond.sol

Almost all functions

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

open TODOs

PROBLEM

There are open TODOs in the code. Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BridgeFacet.sol

here
here
here

Executor.sol

here

TOOLS USED

Manual Analysis

MITIGATION

Remove the TODOs

Payable function not rejecting payments to ERC20 Tokens

PROBLEM

In the case of an ERC20 token, functions should check msg.value is zero.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Executor.sol

The execute() function does not check whether msg.value is zero when the token transferred is not ether. Any ETH sent in this situation would get locked as there is no withdrawal function. This is non-critical since the function has the onlyConnext modifier.

TOOLS USED

Manual Analysis

MITIGATION

Check hat msg.value == 0 when !isNative == true

Public functions can be external

PROBLEM

Public functions that are not called by the contract should be declared external.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

redeemMaturedPositions(ISetToken _setToken)

AssetFacet.sol

canonicalToAdopted()
adoptedToCanonical()
approvedAssets()
adoptedToLocalPools()
wrapper()
tokenRegistry()

BridgeFacet.sol

executor()
nonce()
sponsorVault()
promiseRouter()

NomadFacet.sol

remotes()

ProposedOwnableFacet.sol

routerOwnershipRenounced()
assetOwnershipRenounced()
proposed()
proposedTimestamp()
routerOwnershipTimestamp()
assetOwnershipTimestamp()
delay()
proposeRouterOwnershipRenunciation()
renounceRouterOwnership()
proposeAssetOwnershipRenunciation()
renounceAssetOwnership()
renounced()
proposeNewOwner(address newlyProposed)
renounceOwnership()
acceptProposedOwner()
pause()
unpause()

RelayerFacet.sol

transferRelayer(bytes32 _transferId)
approvedRelayers(address _relayer)

RoutersFacet.sol

getProposedRouterOwner
getProposedRouterOwnerTimestamp
maxRoutersPerTransfer
routerBalances
getRouterApprovalForPortal

VersionFacet.sol

VERSION

TOOLS USED

Manual Analysis

MITIGATION

Make these functions external

Use scientific notation rather than exponentiation

PROBLEM

Use scientific notation rather than exponentiation, e.g: 10e10 instead of 10**10

SEVERITY

Non-critical

PROOF OF CONCEPT

Instances include:

SwapUtils.sol

uint256 internal constant MAX_SWAP_FEE = 10**8
uint256 internal constant MAX_ADMIN_FEE = 10**10
uint256 internal constant MAX_LOOP_LIMIT = 256

TOOLS USED

Manual Analysis

MITIGATION

Replace ** with e

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Executor.sol

connext = _connext

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for the function argument.

safeApprove is deprecated

PROBLEM

This function is deprecated.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

AssetLogic.sol

SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn)

TOOLS USED

Manual Analysis

MITIGATION

safeIncreaseAllowance() and safeDecreaseAllowance() should be used instead.

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address or zero

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

NomadFacet.sol

setXAppConnectionManager(address _xAppConnectionManager)

PortalFacet.sol

setAavePool(address _aavePool)

ConnextPriceOracle.sol

setV1PriceOracle(address _v1PriceOracle)
setAdmin(address newAdmin)

SponsorVault.sol

setGasTokenOracle(address _gasTokenOracle)

TOOLS USED

Manual Analysis

MITIGATION

Add zero address checks to these setters

Timelock in fee setters

PROBLEM

Setters that change the fees should use a timelock to give time for users to react. Even if it does not directly impact users, the admin fee can be set as high as a 100%, impacting the earning of liquidity pools. Adding a timelock would give the protocol more legitimacy.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

SwapUtils.sol

setAdminFee
setSwapFee

TOOLS USED

Manual Analysis

MITIGATION

Add a timelock to these setters

report submitted via email on 2022/06/13 at 08:43 UTC

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparison operators
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Event emitting of local variable
• Group logic in modifier
• Immutable variables can save gas
• Prefix increments
• Redundant libraries
• Require instead of AND
• Tight variable packing
• unchecked arithmetic
• unnecessary computation

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high

PROOF OF CONCEPT

Instances include:

ConnextPriceOracle.sol

scope: getTokenPrice()

• v1PriceOracle is read twice:

line 93 line 94

ProposedOwnableUpgradeable.sol

scope: renounceRouterOwnership()

• _routerOwnershipTimestamp is read twice:

line 175 line 178

scope: renounceAssetOwnership()

• _routerOwnershipTimestamp is read twice:

line 217 line 220

scope: renounceOwnership()

• _routerOwnershipTimestamp is read twice:

line 255 line 258

• proposed is read twice:

line 262 line 265

scope: acceptProposedOwner()

• proposed is read twice:

line 274 line 286

SponsorVault.sol

scope: reimburseLiquidityFees()

• tokenExchanges[_token] is read twice:

line 203 line 205

scope: reimburseRelayerFees()

• gasTokenOracle is read twice:

line 243 line 244

• relayerFeeCap is read twice, but only if sponsoredFee is greater relayerFeeCap than:

line 256

ProposedOwnable.sol

scope: renounceOwnership()

• _proposedOwnershipTimestamp is read twice:

line 125 line 128

• _proposedO is read twice:

line 132 line 135

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

BridgeFacet.sol

scope: handle()

bytes memory _message

scope: _reconcile()

bytes memory _message

scope: _getTransferId()

ConnextMessage.TokenId memory _canonical

scope: _reconcileProcessMessage()

bytes memory _message

PromiseRouter.sol

scope: handle()

bytes memory _message

RelayerFeeRouter.sol

scope: handle()

bytes memory _message

AssetLogic.sol

scope: swapToLocalAssetIfNeeded()

ConnextMessage.TokenId memory _canonical

ConnextMessage.sol

scope: formatTokenId()

TokenId memory _tokenId

scope: formatDetailsHash()

string memory _name
string memory _symbol

LibCrossDomainProperty.sol

scope: parseDomainAndSenderBytes()

bytes memory _property

LibDiamond.sol

scope: proposeDiamondCut()

IDiamondCut.FacetCut[] memory _diamondCut
bytes memory _calldata

scope: rescindDiamondCut()

IDiamondCut.FacetCut[] memory _diamondCut
bytes memory _calldata

scope: diamondCut()

IDiamondCut.FacetCut[] memory _diamondCut
bytes memory _calldata

scope: addFunctions()

bytes4[] memory _functionSelectors

scope: replaceFunctions()

bytes4[] memory _functionSelectors

scope: removeFunctions()

bytes4[] memory _functionSelectors

scope: initializeDiamondCut()

bytes memory _calldata

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

ProposedOwnableFacet.sol

(block.timestamp - s._routerOwnershipTimestamp) <= _delay
(block.timestamp - s._assetOwnershipTimestamp) <= _delay
(block.timestamp - s._proposedOwnershipTimestamp) <= _delay
(block.timestamp - s._proposedOwnershipTimestamp) <= _delay

RoutersFacet.sol

block.timestamp - s.routerPermissionInfo.proposedRouterTimestamp[router] <= _delay

getSwapToken.sol

index >= s.swapStorages[canonicalId].pooledTokens.length
index >= s.swapStorages[canonicalId].balances.length
_pooledTokens.length <= 1
_a >= AmplificationUtils.MAX_A
_fee >= SwapUtils.MAX_SWAP_FEE
_adminFee >= SwapUtils.MAX_ADMIN_FEE

ProposedOwnableUpgradeable.sol

(block.timestamp - _routerOwnershipTimestamp) <= _delay
(block.timestamp - _assetOwnershipTimestamp) <= _delay
(block.timestamp - _proposedOwnershipTimestamp) <= _delay
(block.timestamp - _proposedOwnershipTimestamp) <= _delay

SponsorVault.sol

currentBalance >= amountIn

StableSwap.sol

_pooledTokens.length <= 32
decimals[i] <= SwapUtils.POOL_PRECISION_DECIMALS
block.timestamp <= deadline

ProposedOwnable.sol

(block.timestamp - _proposedOwnershipTimestamp) <= _delay
(block.timestamp - _proposedOwnershipTimestamp) <= _delay

AmplificationUtils.sol

block.timestamp >= self.initialATime.add(1 days)
futureTime_ >= block.timestamp.add(MIN_RAMP_TIME)
futureAPrecise.mul(MAX_A_CHANGE) >= initialAPrecise
futureAPrecise <= initialAPrecise.mul(MAX_A_CHANGE)

AssetLogic.sol

_maxIn >= ipool.calculateSwapInv(tokenIndexIn, tokenIndexOut, _amountOut)
_amountIn <= _maxIn

MathUtils.sol

difference(a, b) <= 1

SwapUtils.sol

tokenAmount <= xp[tokenIndex]
amount <= totalSupply
dx <= tokenFrom.balanceOf(msg.sender)
dy >= minDy
dy <= self.balances[tokenIndexTo]
dx <= maxDx
dx <= tokenFrom.balanceOf(msg.sender)
dx <= tokenFrom.balanceOf(msg.sender)
dy >= minDy
dy <= self.balances[tokenIndexTo]
dx <= maxDx
toMint >= minToMint
amount <= lpToken.balanceOf(msg.sender)
amounts[i] >= minAmounts[i]
tokenAmount <= lpToken.balanceOf(msg.sender)
dy >= minAmount
maxBurnAmount <= v.lpToken.balanceOf(msg.sender)https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L1007\ tokenAmount <= maxBurnAmount
newAdminFee <= MAX_ADMIN_FEE
newSwapFee <= MAX_SWAP_FEE

PromiseMessage.sol

_len <= LENGTH_RETURNDATA_START
_len >= MIN_CLAIM_LEN

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-(block.timestamp - s._proposedOwnershipTimestamp) <= _delay
+(block.timestamp - s._proposedOwnershipTimestamp) < _delay - 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

example:

-(block.timestamp - s._proposedOwnershipTimestamp) <= _delay;
+(block.timestamp - s._proposedOwnershipTimestamp) <= _delay;

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters: it costs approximately 400 less gas in deployment per variable hardcoded.

PROOF OF CONCEPT

Instances include:

ConnextPriceOracle.sol

wrapped = _wrapped;

Executor.sol

connext = _connext

TOOLS USED

Manual Analysis

MITIGATION

Hardcode variables with their initial value instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

BaseConnextFacet.sol

require(s._status != _ENTERED, "ReentrancyGuard: reentrant call")
require(_remote != bytes32(0), "!remote")

ConnextPriceOracle.sol

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

LPToken.sol

require(amount != 0, "LPToken: cannot mint 0")
require(to != address(this), "LPToken: cannot send to itself")

StableSwap.sol

require(_pooledTokens.length > 1, "_pooledTokens.length <= 1")
require(_pooledTokens.length <= 32, "_pooledTokens.length > 32")
require(_pooledTokens.length == decimals.length, "_pooledTokens decimals mismatch")
require(tokenIndexes[address(_pooledTokens[i])] == 0 && _pooledTokens[0] != _pooledTokens[i],"Duplicate tokens")
require(address(_pooledTokens[i]) != address(0), "The 0 address isn't an ERC-20")
require(decimals[i] <= SwapUtils.POOL_PRECISION_DECIMALS, "Token decimals exceeds max")
require(_a < AmplificationUtils.MAX_A, "_a exceeds maximum")
require(_fee < SwapUtils.MAX_SWAP_FEE, "_fee exceeds maximum")
require(_adminFee < SwapUtils.MAX_ADMIN_FEE, "_adminFee exceeds maximum")
require(lpToken.initialize(lpTokenName, lpTokenSymbol), "could not init lpToken clone")
require(index < swapStorage.pooledTokens.length, "Out of range")
require(address(getToken(index)) == tokenAddress, "Token does not exist")
require(index < swapStorage.pooledTokens.length, "Index out of range")

AmplificationUtils.sol

require(block.timestamp >= self.initialATime.add(1 days), "Wait 1 day before starting ramp")
require(futureTime_ >= block.timestamp.add(MIN_RAMP_TIME), "Insufficient ramp time")
require(futureA_ > 0 && futureA_ < MAX_A, "futureA_ must be > 0 and < MAX_A")
require(futureAPrecise.mul(MAX_A_CHANGE) >= initialAPrecise, "futureA_ is too small")
require(futureAPrecise <= initialAPrecise.mul(MAX_A_CHANGE), "futureA_ is too large")
require(self.futureATime > block.timestamp, "Ramp is already stopped")

ConnextMessage.sol

require(isValidAction(_action), "!action")

LibDiamond.sol

require(msg.sender == diamondStorage().contractOwner, "LibDiamond: Must be contract owner")
require(diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] < block.timestamp,"LibDiamond: delay not elapsed")
require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut")
require(_facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)")
require(oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists")
require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut")
require(_facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)")
require(oldFacetAddress != _facetAddress, "LibDiamondCut: Can't replace function with same function")
require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut")
require(_facetAddress == address(0), "LibDiamondCut: Remove facet address must be address(0)")
require(_facetAddress != address(0), "LibDiamondCut: Can't remove function that doesn't exist")
require(_facetAddress != address(this), "LibDiamondCut: Can't remove immutable function")
require(_calldata.length == 0, "LibDiamondCut: _init is address(0) but_calldata is not empty")
require(_calldata.length > 0, "LibDiamondCut: _calldata is empty but _init is not address(0)")
require(contractSize > 0, _errorMessage)

SwapUtils.sol

require(tokenIndex < xp.length, "Token index out of range")
require(tokenAmount <= xp[tokenIndex], "Withdraw exceeds available")
require(tokenIndex < numTokens, "Token not found"
require(numTokens == precisionMultipliers.length, "Balances must match multipliers")
require(tokenIndexFrom != tokenIndexTo, "Can't compare token to itself")
require(tokenIndexFrom < numTokens && tokenIndexTo < numTokens, "Tokens must be in pool")
require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range")
require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range")
require(amount <= totalSupply, "Cannot exceed total supply")
require(index < self.pooledTokens.length, "Token index out of range")
require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own")
require(dy >= minDy, "Swap didn't result in min tokens")
require(dy <= self.balances[tokenIndexTo], "Cannot get more than pool balance")
require(dx <= maxDx, "Swap needs more than max tokens")
require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own")
require(dx == tokenFrom.balanceOf(address(this)).sub(beforeBalance), "not support fee token")
require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own")
require(dy >= minDy, "Swap didn't result in min tokens")
require(dy <= self.balances[tokenIndexTo], "Cannot get more than pool balance")
require(dx <= maxDx, "Swap didn't result in min tokens")
require(amounts.length == pooledTokens.length, "Amounts must match pooled tokens")
require(v.totalSupply != 0 || amounts[i] > 0, "Must supply all tokens in pool")
require(v.d1 > v.d0, "D should increase")
require(toMint >= minToMint, "Couldn't mint min requested")
require(amount <= lpToken.balanceOf(msg.sender), ">LP.balanceOf")
require(minAmounts.length == pooledTokens.length, "minAmounts must match poolTokens")
require(amounts[i] >= minAmounts[i], "amounts[i] < minAmounts[i]")
require(tokenAmount <= lpToken.balanceOf(msg.sender), ">LP.balanceOf")
require(tokenIndex < pooledTokens.length, "Token not found")
require(dy >= minAmount, "dy < minAmount")
require(amounts.length == pooledTokens.length, "Amounts should match pool tokens")
require(maxBurnAmount <= v.lpToken.balanceOf(msg.sender) && maxBurnAmount != 0, ">LP.balanceOf")
require(tokenAmount != 0, "Burnt amount cannot be zero")
require(tokenAmount <= maxBurnAmount, "tokenAmount > maxBurnAmount")
require(newAdminFee <= MAX_ADMIN_FEE, "Fee is too high")
require(newSwapFee <= MAX_SWAP_FEE, "Fee is too high")

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in StableSwap.sol:

Replace

require(_pooledTokens.length > 1, "_pooledTokens.length <= 1")

with

if (_pooledTokens.length < 2) {
		revert IncorrectPoolTokenLength();
}

and define the custom error in the contract

error IncorrectPoolTokenLength();

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

StableSwapFaucet.sol

uint8 i = 0

ConnextPriceOracle.sol

uint256 i = 0

StableSwap.sol

uint8 i = 0

Encoding.sol

uint8 i = 0

SwapUtils.sol

uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 j = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0

RelayerFeeMessage.sol

uint256 i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Event emitting of local variable

PROBLEM

When emitting an event, using a local variable instead of a storage variable saves gas.

PROOF OF CONCEPT

Instances include:

ProposedOwnableFacet.sol

emit RouterOwnershipRenunciationProposed(s._routerOwnershipTimestamp)
emit AssetOwnershipRenunciationProposed(s._assetOwnershipTimestamp)
emit OwnershipProposed(s._proposed)

ProposedOwnableUpgradeable.sol

emit RouterOwnershipRenunciationProposed(_routerOwnershipTimestamp)
emit AssetOwnershipRenunciationProposed(_assetOwnershipTimestamp)

ProposedOwnable.sol

emit OwnershipProposed(_proposed)

TOOLS USED

Manual Analysis

MITIGATION

In all instances, the storage variable can be replaced by the function argument that is written to that storage variable.

Group logic in modifier

PROBLEM

You can group repetitive require statements into a modifier to save on deployment costs. There is however a tradeoff to doing this: declaring the logic inline is cheaper than having it in a modifier when you call the function that uses that modifier.

PROOF OF CONCEPT

Instances include:

StableSwap.sol

require(index < swapStorage.pooledTokens.length, "Out of range")
require(index < swapStorage.pooledTokens.length, "Index out of range")

LibDiamonds.sol

require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut")
require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut")
require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut")

SwapUtils.sol

require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own")
require(dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own")

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

TOOLS USED

Manual Analysis

MITIGATION

You can create modifiers for repetitive require statements and apply them to the functions where the require statement mentioned are called. Again, this one is more of a suggestion because there is a trade-off.

Immutable variables can save gas

PROBLEM

If a storage variable is never modified, marking it as immutable can save gas, as it is saved as a constant instead of a storage variable, saving a SLOAD operation (100 gas) every time the variable is read.

PROOF OF CONCEPT

Instances include:

ConnextPriceOracle.sol

wrapped is initialized once and can not be modified afterwards. It is read once in every call to getTokenPrice.

TOOLS USED

Manual Analysis

MITIGATION

add the immutable keyword to address public wrapped

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments: it returns the incremented variable instead of returning a temporary variable storing the initial value of the variable. It saves 5 gas per iteration

PROOF OF CONCEPT

Instances include:

BridgeFacet.sol

i++
i++
i++

DiamondLoupeFacet.sol

i++

RelayerFacet.sol

i++
i++

StableSwapFacet.sol

i++

ConnextPriceOracle.sol

i++

StableSwap.sol

i++

LibDiamond.sol

facetIndex++
selectorIndex++
selectorPosition++
selectorIndex++
selectorPosition++
selectorIndex++

SwapUtils.sol

i++
i++
i++
i++
i++
j++
i++
i++
i++
i++
i++
i++
i++
i++
i++
i++
i++
i++

RelayerFeeMessage.sol

i++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Redundant libraries

IMPACT

As of Solidity 0.8.0, underflow and overflow checks are performed by default on additions and subtractions. Using an external library for these computations should be avoided as it will waste gas.

PROOF OF CONCEPT

Several contracts in scope use the SafeMath library of OpenZeppelin and its functions sub and add

ConnextPriceOracle.sol
AmplificationUtils.sol
SwapUtils.sol

TOOLS USED

Manual Analysis

MITIGATION

Use the native additions and subtractions.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

StableSwap.sol

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

AmplificationUtils.sol

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

SwapUtils.sol

require(tokenIndexFrom < numTokens && tokenIndexTo < numTokens, "Tokens must be in pool")
require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range")
require(tokenIndexFrom < xp.length && tokenIndexTo < xp.length, "Token index out of range")
require(maxBurnAmount <= v.lpToken.balanceOf(msg.sender) && maxBurnAmount != 0, ">LP.balanceOf")

TOOLS USED

Manual Analysis

MITIGATION

Split the require statement in several statements. For instance:

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

To improve the savings, you can also use custom errors instead of require statements

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As bool type variables are of size 1 byte, there's a slot here that can get saved by moving them closer to an address

PROOF OF CONCEPT

Instances include:

ProposedOwnableUpgradeable.sol

address private _owner;

address private _proposed;
uint256 private _proposedOwnershipTimestamp;

bool private _routerOwnershipRenounced;
uint256 private _routerOwnershipTimestamp;

bool private _assetOwnershipRenounced;

TOOLS USED

Manual Analysis

MITIGATION

Place _routerOwnershipRenounced after _owner and _assetOwnershipRenounced _proposed to save two storage slots

address private _owner;
+bool private _routerOwnershipRenounced;
address private _proposed;
+bool private _assetOwnershipRenounced;
uint256 private _proposedOwnershipTimestamp;

uint256 private _routerOwnershipTimestamp;

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas. This is particularly true in for loops, as it saves some gas at each iteration.

PROOF OF CONCEPT

Instances include:

BridgeFacet.sol

availableAmount - backUnbackedAmount, cannot overflow because of the condition line 1099

DiamondLoupeFacet.sol

i++

StableSwapFacet.sol

i++

ConnextPriceOracle.sol

i++

StableSwap.sol

i++

LibDiamond.sol

facetIndex++
selectorIndex++
selectorIndex++
selectorIndex++

SwapUtils.sol

i++
i++
i++
i++
i++
j++
i++
i++
i++
i++
i++
i++
i++
i++
i++
i++
i++
i++

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

When emitting an event that includes a new and an old value, it is cheaper in gas to avoid caching the old value in memory. Instead, emit the event, then save the new value in storage.

PROOF OF CONCEPT

Instances include:

ConnextPriceOracle.sol

line 169

address oldAdmin = admin;
admin = newAdmin;

emit NewAdmin(oldAdmin, newAdmin)

LibDiamond.sol

line 55

address previousOwner = ds.contractOwner;
ds.contractOwner = _newOwner;
emit OwnershipTransferred(previousOwner, _newOwner)

ProposedOwnable.sol

line 162

address oldOwner = _owner;
_owner = newOwner;
_proposedOwnershipTimestamp = 0;
_proposed = address(0);
emit OwnershipTransferred(oldOwner, newOwner)

TOOLS USED

Manual Analysis

MITIGATION

Emit the old storage variable before writing the new value.

e.g:

emit NewAdmin(admin, newAdmin)
admin = newAdmin