Connext Amarok contest - apostle0x01's results

The interoperability protocol of L2 Ethereum.

General Information

Platform: Code4rena

Start Date: 08/06/2022

Pot Size: $115,000 USDC

Total HM: 26

Participants: 72

Period: 11 days

Judge: leastwood

Total Solo HM: 14

Id: 132

League: ETH

Connext

Findings Distribution

Researcher Performance

Rank: 68/72

Findings: 1

Award: $85.33

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Connext

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, 0xmint, BowTiedWardens, ElKu, Fitraldys, Funen, Kaiziron, Lambda, Metatron, MiloTruck, Randyyy, Ruhum, SmartSek, TomJ, Tomio, UnusualTurtle, Waze, _Adam, apostle0x01, asutorufos, c3phas, catchup, csanuragjain, defsec, fatherOfBlocks, hansfriese, hyh, ignacio, joestakey, k, kaden, nahnah, oyc_109, rfa, robee, sach1r0, simon135, slywaters

Awards

85.3344 USDC - $85.33

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

[G-01] Splitting require() statements that use && saves gas

IMPACT

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

POC

Vulnerable location:

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

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/AmplificationUtils.sol#L86 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L397 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L493 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L524 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L1007

MITIGATION

Breakdown each condition in a separate require , e.g.:

require(futureA_ > 0);
require(futureA_ < MAX_A);

[G-02] No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

POC

contracts/core/connext/libraries/SwapUtils.sol:205:    for (uint256 i = 0; i < xp.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:254:    for (uint256 i = 0; i < numTokens; i++) {
contracts/core/connext/libraries/SwapUtils.sol:268:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
contracts/core/connext/libraries/SwapUtils.sol:289:    for (uint256 i = 0; i < numTokens; i++) {
contracts/core/connext/libraries/SwapUtils.sol:300:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
contracts/core/connext/libraries/SwapUtils.sol:344:    for (uint256 i = 0; i < numTokens; i++) {
contracts/core/connext/libraries/SwapUtils.sol:405:    for (uint256 i = 0; i < numTokens; i++) {
contracts/core/connext/libraries/SwapUtils.sol:425:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
contracts/core/connext/libraries/SwapUtils.sol:558:    for (uint256 i = 0; i < balances.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:591:    for (uint256 i = 0; i < balances.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:844:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:869:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:924:    for (uint256 i = 0; i < amounts.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1014:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1019:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1039:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1055:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/relayer-fee/libraries/RelayerFeeMessage.sol:81:    for (uint256 i = 0; i < length; )

Mitigation

As an example: for (uint256 i = 0; i < xp.length;) { should be replaced with for (uint256 i; i < xp.length;) {

[G-03] Shorten require messages to less than 32 characters to save gas

Impact

Strings that are more than 32 characters will require more than 1 storage slot, costing more gas. Consider reducing the message length to less than 32 characters or use error codes:

POC

contracts/nomad-core/contracts/governance/GovernanceRouter.sol:240:    require(_domains.length == _remoteCalls.length, "!domains length matches calls length");
contracts/core/connext/libraries/LibDiamond.sol:66:    require(msg.sender == diamondStorage().contractOwner, "LibDiamond: Must be contract owner");
contracts/core/connext/libraries/LibDiamond.sol:132:      require(oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists");
contracts/core/connext/libraries/LibDiamond.sol:150:      require(oldFacetAddress != _facetAddress, "LibDiamondCut: Can't replace function with same function");
contracts/core/connext/libraries/LibDiamond.sol:121:    require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
contracts/core/connext/libraries/LibDiamond.sol:139:    require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
contracts/core/connext/libraries/LibDiamond.sol:158:    require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");
contracts/core/connext/helpers/OZERC20.sol:253:    require(_owner != address(0), "ERC20: approve from the zero address");
contracts/core/connext/libraries/LibDiamond.sol:132:      require(oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists");

[G-04] ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integers. This is because the pre-increment operation is cheaper (about 5 GAS per iteration).

POC

contracts/core/connext/facets/DiamondLoupeFacet.sol:31:    for (uint256 i; i < numFacets; i++) {
contracts/core/connext/facets/StableSwapFacet.sol:415:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
contracts/core/connext/helpers/ConnextPriceOracle.sol:176:    for (uint256 i = 0; i < tokenAddresses.length; i++) {
contracts/core/connext/helpers/Multicall.sol:16:    for (uint256 i = 0; i < calls.length; i++) {
contracts/core/connext/helpers/StableSwap.sol:81:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:205:    for (uint256 i = 0; i < xp.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:254:    for (uint256 i = 0; i < numTokens; i++) {
contracts/core/connext/libraries/SwapUtils.sol:268:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
contracts/core/connext/libraries/SwapUtils.sol:289:    for (uint256 i = 0; i < numTokens; i++) {
contracts/core/connext/libraries/SwapUtils.sol:300:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
contracts/core/connext/libraries/SwapUtils.sol:344:    for (uint256 i = 0; i < numTokens; i++) {
contracts/core/connext/libraries/SwapUtils.sol:405:    for (uint256 i = 0; i < numTokens; i++) {
contracts/core/connext/libraries/SwapUtils.sol:425:    for (uint256 i = 0; i < MAX_LOOP_LIMIT; i++) {
contracts/core/connext/libraries/SwapUtils.sol:558:    for (uint256 i = 0; i < balances.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:591:    for (uint256 i = 0; i < balances.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:844:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:869:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:924:    for (uint256 i = 0; i < amounts.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1014:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1019:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1039:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1055:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceMessage.sol:97:    for (uint256 i = 0; i < _calls.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:244:    for (uint256 i = 0; i < _localCalls.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:248:    for (uint256 i = 0; i < _remoteCalls.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:326:    for (uint256 i = 0; i < domains.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:419:    for (uint256 i = 0; i < _calls.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:443:    for (uint256 i = 0; i < domains.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:531:    for (uint256 i = 0; i < domains.length; i++) {
contracts/test/TestStableSwap.sol:30:    for (uint8 i; i < n; i++) {
contracts/test/TestStableSwap.sol:134:    for (uint8 i = 0; i < n; i++) {
contracts/test/TestStableSwap.sol:140:    for (uint8 i = 0; i < n; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:215:    for (uint256 i; i < pathLen; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:249:    for (uint256 i; i < num; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:682:      for (uint256 i; i < pathLen; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:736:        for (uint256 i; i < pathLen; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:787:    for (uint256 i; i < savedRouters.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:984:    for (uint256 i = 0; i < s.routedTransfers[transferId].length; i++) {

[G-05] Cache storage values in memory to save gas

Impact

Anytime you are reading from storage more than once, it is cheaper to cache variables in memory. An SLOAD cost 100 GAS while MLOAD and MSTORE only cost 3 GAS.

This is especially true 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 when the array length is high.

// Before
for(uint 1; i < approvals.length; i++); // approvals is a storage variable

// After
uint memory numApprovals = approvals.length;
for(uint 1; i < numApprovals; i++);

POC

contracts/core/connext/facets/RelayerFacet.sol:140:    for (uint256 i; i < _transferIds.length; ) {
contracts/core/connext/facets/RelayerFacet.sol:164:    for (uint256 i; i < _transferIds.length; ) {
contracts/core/connext/facets/StableSwapFacet.sol:415:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
contracts/core/connext/helpers/ConnextPriceOracle.sol:176:    for (uint256 i = 0; i < tokenAddresses.length; i++) {
contracts/core/connext/helpers/Multicall.sol:16:    for (uint256 i = 0; i < calls.length; i++) {
contracts/core/connext/helpers/StableSwap.sol:81:    for (uint8 i = 0; i < _pooledTokens.length; i++) {
contracts/core/connext/libraries/LibDiamond.sol:104:    for (uint256 facetIndex; facetIndex < _diamondCut.length; facetIndex++) {
contracts/core/connext/libraries/LibDiamond.sol:129:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
contracts/core/connext/libraries/LibDiamond.sol:147:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
contracts/core/connext/libraries/LibDiamond.sol:162:    for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {
contracts/core/connext/libraries/SwapUtils.sol:205:    for (uint256 i = 0; i < xp.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:558:    for (uint256 i = 0; i < balances.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:591:    for (uint256 i = 0; i < balances.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:844:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:869:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:924:    for (uint256 i = 0; i < amounts.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1014:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1019:      for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1039:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/core/connext/libraries/SwapUtils.sol:1055:    for (uint256 i = 0; i < pooledTokens.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceMessage.sol:97:    for (uint256 i = 0; i < _calls.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:244:    for (uint256 i = 0; i < _localCalls.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:248:    for (uint256 i = 0; i < _remoteCalls.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:326:    for (uint256 i = 0; i < domains.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:419:    for (uint256 i = 0; i < _calls.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:443:    for (uint256 i = 0; i < domains.length; i++) {
contracts/nomad-core/contracts/governance/GovernanceRouter.sol:531:    for (uint256 i = 0; i < domains.length; i++) {
contracts/nomad-core/libs/Queue.sol:71:    for (uint256 i = 0; i < _items.length; i += 1) {
contracts/nomad-core/libs/TypedMemView.sol:813:    for (uint256 i = 0; i < memViews.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:787:    for (uint256 i; i < savedRouters.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:984:    for (uint256 i = 0; i < s.routedTransfers[transferId].length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:1048:        for (uint256 i = 0; i < routers.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:1863:    for (uint256 i; i < args.routers.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:1934:    for (uint256 i; i < args.routers.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:1981:    for (uint256 i = 1; i < args.routers.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:1994:    for (uint256 i = 0; i < args.routers.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:2291:    for (uint256 i; i < _args.routers.length; i++) {
contracts_forge/core/connext/facets/BridgeFacet.t.sol:2390:    for (uint256 i = 1; i < args.routers.length; i++) {
contracts_forge/core/relayer-fee/libraries/RelayerFeeMessage.t.sol:55:    for (uint256 i = 0; i < transferIds.length; i++) {

[G-06] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

Impact

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 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: https://twitter.com/gzeon/status/1485428085885640706

POC

contracts/core/connext/helpers/ConnextPriceOracle.sol:150:    require(baseTokenPrice > 0, "invalid base token");
contracts/core/connext/libraries/AmplificationUtils.sol:86:    require(futureA_ > 0 && futureA_ < MAX_A, "futureA_ must be > 0 and < MAX_A");
contracts/core/connext/libraries/LibDiamond.sol:247:    require(contractSize > 0, _errorMessage);

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L150 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/AmplificationUtils.sol#L86 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L247

MITIGATION

Change > 0 with != 0 when used on a uint in require statement to save gas.

[G-07] Expressions for constant values such as a call to keccak256(), should use immutable

Impact

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant See https://github.com/ethereum/solidity/issues/9232 issue for a detail description of the issue Similar bug: https://github.com/code-423n4/2021-11-unlock-findings/issues/238

POC

contracts/core/connext/libraries/LibDiamond.sol:14:  bytes32 constant DIAMOND_STORAGE_POSITION = keccak256("diamond.standard.diamond.storage");

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L14

Connext Amarok contest - apostle0x01's results

The interoperability protocol of L2 Ethereum.

General Information

Findings Distribution

Researcher Performance

External Links

[G-09] Gas Report - $85.33

Findings Information

Awards

Labels

External Links

[G-01] Splitting require() statements that use && saves gas

IMPACT

POC

MITIGATION

[G-02] No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

POC

Mitigation

[G-03] Shorten require messages to less than 32 characters to save gas

Impact

POC

[G-04] ++i costs less gas compared to i++ or i += 1

Impact

POC

[G-05] Cache storage values in memory to save gas

Impact

POC

[G-06] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

Impact

POC

MITIGATION

[G-07] Expressions for constant values such as a call to keccak256(), should use immutable

Impact

POC