Connext Amarok contest - 0xkatana'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: 24/72

Findings: 2

Award: $479.12

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Connext

Findings Information

🌟 Selected for report: BowTiedWardens

Also found by: 0x1f8b, 0x29A, 0x52, 0xNazgul, 0xNineDec, 0xf15ers, 0xkatana, 0xmint, Chom, ElKu, Funen, IllIllI, JMukesh, Jujic, Kaiziron, Lambda, MiloTruck, Ruhum, SmartSek, SooYa, TerrierLover, TomJ, WatchPug, Waze, _Adam, asutorufos, auditor0517, bardamu, c3phas, catchup, cccz, ch13fd357r0y3r, cloudjunky, cmichel, cryptphi, csanuragjain, defsec, fatherOfBlocks, hansfriese, hyh, jayjonah8, joestakey, k, kenta, obtarian, oyc_109, robee, sach1r0, shenwilly, simon135, slywaters, sorrynotsorry, tintin, unforgiven, xiaoming90, zzzitron

Awards

141.8552 USDC - $141.86

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

[L-01] LP spot price used as oracle

getPriceFromDex uses the spot price of a LP to determine token price. Using a single data point from the LP, instead of using a TWAP, makes the price vulnerable to manipulation. https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L99-L115

More advice from samczsun: https://shouldiusespotpriceasmyoracle.com/

Recommended Mitigation Steps

Use a TWAP or similar secure method for a backup price oracle.

#0 - jakekidd
2022-07-01T22:15:26Z

This is not a QA issue but a level 2, and a duplicate of others

Duplicate of https://github.com/code-423n4/2022-06-connext-findings/issues/13

#1 - 0xleastwood
2022-08-15T00:58:01Z

I'm keeping this as QA because ConnextPriceOracle.sol is not currently used at all within the codebase.

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

337.2601 USDC - $337.26

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

[G-01] Use prefix not postfix in loops

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

[G-02] Redundant zero initialization

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 i; instead of uint256 i = 0;

[G-03] Split up require statements instead of &&

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

One example is

require(_bathAssetAddress != address(0) && _bathQuoteAddress != address(0), "tokenToBathToken error");

This can be improved to

require(_bathAssetAddress != address(0));
require(_bathQuoteAddress != address(0));

Several places had require statements with many logical "and"s. Instead, split into two to save gas https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L397 https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L493 https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L524 https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L1007 https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/AmplificationUtils.sol#L86

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

[G-04] Cache array length before loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

Recommended Mitigation Steps

Cache the array length before the for loop

[G-05] Use != 0 instead of > 0

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

[G-06] Short require strings save gas

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Recommended Mitigation Steps

Shorten all require strings to less than 32 characters

[G-07] For loop incrementing can be unsafe

For loops that use i++ do not need to use safemath for this operation because the loop would run out of gas long before this point. Making this addition operation unsafe using unchecked saves gas.

Sample code to make the for loop increment unsafe

for (uint i = 0; i < length; i = unchecked_inc(i)) {
    // do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
    unchecked {
        return i + 1;
    }
}

Idea borrowed from https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#the-increment-in-for-loop-post-condition-can-be-made-unchecked

Recommended Mitigation Steps

Make the increment in for loops unsafe to save gas

[G-08] Use iszero assembly for zero checks

Comparing a value to zero can be done using the iszero EVM opcode. This can save gas

Source from t11s https://twitter.com/transmissions11/status/1474465495243898885

Recommended Mitigation Steps

Use the assembly iszero evm opcode to compare values to zero

[G-09] Unnecessary Import

SafeMath is imported in contracts that are using solidity 0.8.X. This version of solidity already applies safemath to math operations. The SafeMath import can be removed to save gas.

Places with SafeMath imports: https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L4 https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/helpers/OZERC20.sol#L10 https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L4 https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/AmplificationUtils.sol#L5

Recommended Mitigation Steps

Remove SafeMath import from solidity 0.8.X files.

[G-10] Add payable to functions that won't receive ETH

Identifying a function as payable saves gas. Functions that have a modifier like onlyOwner or auth cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas.

Recommended Mitigation Steps

Add payable to these functions for gas savings

[G-11] Use internal function in place of modifier

An internal function can save gas vs. a modifier. A modifier inlines the code of the original function but an internal function does not.

Source https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#dde7

Recommended Mitigation Steps

Use internal functions in place of modifiers to save gas.

[G-12] Use simple comparison in trinary logic

The comparison operators >= and <= use more gas than >, <, or ==. Replacing the >= and ≤ operators with a comparison operator that has an opcode in the EVM saves gas

The existing code is https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/helpers/SponsorVault.sol#L208

amountIn = currentBalance >= amountIn ? amountIn : currentBalance;

A simple comparison can be used for gas savings by reversing the logic

amountIn = currentBalance < amountIn ? currentBalance : amountIn;

Recommended Mitigation Steps

Replace the comparison operator and reverse the logic to save gas using the suggestions above

[G-13] Unnecessary require statements

There is a require statement that is not necessary because the transaction will still revert without it. This can be removed for gas savings because they don't provide any additional protection.

The unnecessary line https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L649

Recommended Mitigation Steps

Remove unnecessary require statements to save gas

[G-14] Use type(uint256).max

The value ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff is used for some variables but using type(uint232).max uses less gas than this constant value. Source https://forum.openzeppelin.com/t/using-the-maximum-integer-in-solidity/3000/13

There are two places where this change can save gas https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/LibCrossDomainProperty.sol#L37 https://github.com/code-423n4/2022-06-connext/tree/main/contracts/contracts/core/connext/libraries/LibCrossDomainProperty.sol#L38

Recommended Mitigation Steps

Use type(uint232).max instead

[G-15] Non-public variables save gas

Many constant variables are public, but changing the visibility of these variables to private or internal can save gas.

Recommended Mitigation Steps

Declare some public variables as private or internal to save gas

[G-16] Use calldata instead of memory for function arguments

Using calldata instead of memory for function arguments saves gas sometimes. This can happen when a function is called externally and the memory array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). If the array is used in a for loop, arr[i] accesses the value in memory using a mload. If calldata is used instead, then instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Source https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#use-calldata-instead-of-memory-for-function-parameters

Recommended Mitigation Steps

Change function arguments from memory to calldata

[G-17] Write contracts in vyper

The contracts are all written entirely in solidity. Writing contracts with vyper instead of solidity can save gas.

Source https://twitter.com/eiber_david/status/1515737811881807876 doggo demonstrates https://twitter.com/fubuloubu/status/1528179581974417414?t=-hcq_26JFDaHdAQZ-wYxCA&s=19

Recommended Mitigation Steps

Write some or all of the contracts in vyper to save gas

#0 - 0xleastwood
2022-08-16T19:40:31Z

Good series of optimisations. However, I'd like to note that G-10 is not a safe practice to follow. Best to avoid making arbitrary functions payable in order to save gas.

Connext Amarok contest - 0xkatana's results

The interoperability protocol of L2 Ethereum.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-06] QA Report - $141.86

Findings Information

Awards

Labels

External Links

[L-01] LP spot price used as oracle

Recommended Mitigation Steps

#0 - jakekidd2022-07-01T22:15:26Z

#1 - 0xleastwood2022-08-15T00:58:01Z

[G-05] Gas Report - $337.26

Findings Information

Awards

Labels

External Links

[G-01] Use prefix not postfix in loops

Recommended Mitigation Steps

[G-02] Redundant zero initialization

Recommended Mitigation Steps

[G-03] Split up require statements instead of &&

Recommended Mitigation Steps

[G-04] Cache array length before loop

Recommended Mitigation Steps

[G-05] Use != 0 instead of > 0

Recommended Mitigation Steps

[G-06] Short require strings save gas

Recommended Mitigation Steps

[G-07] For loop incrementing can be unsafe

Recommended Mitigation Steps

[G-08] Use iszero assembly for zero checks

Recommended Mitigation Steps

[G-09] Unnecessary Import

Recommended Mitigation Steps

[G-10] Add payable to functions that won't receive ETH

Recommended Mitigation Steps

[G-11] Use internal function in place of modifier

Recommended Mitigation Steps

[G-12] Use simple comparison in trinary logic

Recommended Mitigation Steps

[G-13] Unnecessary require statements

Recommended Mitigation Steps

[G-14] Use type(uint256).max

Recommended Mitigation Steps

[G-15] Non-public variables save gas

Recommended Mitigation Steps

[G-16] Use calldata instead of memory for function arguments

Recommended Mitigation Steps

[G-17] Write contracts in vyper

Recommended Mitigation Steps

#0 - 0xleastwood2022-08-16T19:40:31Z

#0 - jakekidd
2022-07-01T22:15:26Z

#1 - 0xleastwood
2022-08-15T00:58:01Z

#0 - 0xleastwood
2022-08-16T19:40:31Z