Platform: Code4rena
Start Date: 24/03/2022
Pot Size: $75,000 USDC
Total HM: 15
Participants: 59
Period: 7 days
Judge: gzeon
Id: 103
League: ETH
Rank: 53/59
Findings: 1
Award: $94.33
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Dravee
Also found by: 0v3rf10w, 0xDjango, 0xNazgul, 0xkatana, ACai, CertoraInc, FSchmoede, Funen, Hawkeye, IllIllI, Jujic, Kenshin, PPrieditis, Picodes, SolidityScan, TerrierLover, Tomio, WatchPug, catchup, csanuragjain, defsec, dimitri, hake, hickuphh3, kenta, minhquanym, obront, peritoflores, rayn, rfa, robee, saian, samruna, tchkvsky, teryanarmen, ych18
94.3348 USDC - $94.33
For the type uint
it is cheaper to use the logical operator !=
instead of the >
when checking the value is not 0
. The value can never be below 0
since it is unsigned and hence would underflow instead.
The relevant code:
AnyswapFacet.sol line 92: _postSwapBalance > 0 AnyswapFacet.sol line 105: _postSwapBalance > 0 CBridgeFacet.sol line 105: _postSwapBalance > 0 CBridgeFacet.sol line 116: _postSwapBalance > 0 HopFacet.sol line 109: _postSwapBalance > 0 NXTPFacet.sol line 98: _postSwapBalance > 0 LibAsset.sol line 67: allowance > 0 LibDiamond.sol line 84: _functionSelectors.length > 0 LibDiamond.sol line 102: _functionSelectors.length > 0 LibDiamond.sol line 121: _functionSelectors.length > 0 LibDiamond.sol line 189: _calldata.length > 0 LibDiamond.sol line 196: error.length > 0 LibDiamond.sol line 212: contractSize > 0
Strings are stored in slots of 32 bytes, and hence the length of the revert string should be at max 32 bytes to fit inside 1 slot. If the string is just 33 bytes it will occupy 2 slots (64 bytes). Keeping the string size at 32 bytes or below will save gas on both deployment and when the revert condition is met.
Since the used version of Solidity is >=0.8.4
it would also be worth considering using Custom Errors which is both more gas efficient and allows error descriptions using NatSpec.
The relevant code:
AnyswapFacet.sol line 133: "Cannot bridge to the same network." CBridgeFacet.sol line 147: "Cannot bridge to the same network." HopFacet.sol line 146: "Cannot bridge to the same network." LibDiamond:sol line 56: "LibDiamond: Must be contract owner" LibDiamond:sol line 84: "LibDiamondCut: No selectors in facet to cut" LibDiamond:sol line 86: "LibDiamondCut: Add facet can't be address(0)" LibDiamond:sol line 95: "LibDiamondCut: Can't add function that already exists" LibDiamond:sol line 102: "LibDiamondCut: No selectors in facet to cut" LibDiamond:sol line 104: "LibDiamondCut: Add facet can't be address(0)" LibDiamond:sol line 113: "LibDiamondCut: Can't replace function with same function" LibDiamond:sol line 121: "LibDiamondCut: No selectors in facet to cut" LibDiamond:sol line 124: "LibDiamondCut: Remove facet address must be address(0)" LibDiamond:sol line 133: "LibDiamondCut: New facet has no code" LibDiamond:sol line 154: "LibDiamondCut: Can't remove function that doesn't exist" LibDiamond:sol line 156: "LibDiamondCut: Can't remove immutable function" LibDiamond:sol line 187: "LibDiamondCut: _init is address(0) but_calldata is not empty" LibDiamond:sol line 189: "LibDiamondCut: _calldata is empty but _init is not address(0)" LibDiamond:sol line 191: "LibDiamondCut: _init address has no code" LibDiamond:sol line 200: "LibDiamondCut: _init function reverted"
The constant variable MAX_INT
is never used in LibSwap.sol
and can hence be removed.
The relevant code:
LibSwap.sol line 8: uint256 private constant MAX_INT = 2**256 - 1;
It is cheaper to prefix increment (++i
) a loop counter rather than postfix increment it (i++
). Furthermore, there is no reason to use checked arithmetic on a uint
/uint256
loop counter, since there is little to no risk of it overflowing. In newer versions of Solidity there is a built-in overflow check for increments, which is not necessary in this case and is hence a waste of gas. Instead it is possible to skip the overflow check by using unchecked arithmetic like unchecked{++i}
This means that a loop looking like this
for (uint i; i < array.length; i++) { // ... }
Should be rewritten into this
for (uint i; i < array.length; ) { // ... unchecked { ++i; } }
These optimizations will save gas for each iteration and hence the total amount of saved gas depends on the size of the element that is looped over.
The relevant code:
DexManagerFacet.sol line 33-39 DexManagerFacet.sol line 52-57 DexManagerFacet.sol line 65-76 DexManagerFacet.sol line 70-75 DiamondLoupeFacet.sol line 24-28 LibDiamond.sol line 67-78 LibDiamond.sol line 92-98 LibDiamond.sol line 110-117 LibDiamond.sol line 125-129 // The below use uint8 and hence only the prefix increment is relevant here HopFacet.sol line 48-51 Swapper.sol line 14-21
The library LibDiamond
is imported twice in AnySwapFacet.sol
and the library is not used in the file and hence should be removed.
The relevant code:
AnySwapFacet.sol line 6 AnySwapFacet.sol line 9
In AnySwapFacet.sol
the _postSwapBalance
is cached in line 103, however, the exact same operation is used in the line above the caching (line 101). Therefore, it is possible to save a BALANCE
instruction by swapping the two lines such that:
_executeSwaps(_lifiData, _swapData); require(address(this).balance - _fromBalance >= _anyswapData.amount, "ERR_INVALID_AMOUNT"); uint256 _postSwapBalance = address(this).balance - _fromBalance; require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT"); _anyswapData.amount = _postSwapBalance;
Becomes this:
_executeSwaps(_lifiData, _swapData); uint256 _postSwapBalance = address(this).balance - _fromBalance; require(_postSwapBalance >= _anyswapData.amount, "ERR_INVALID_AMOUNT"); require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT"); _anyswapData.amount = _postSwapBalance;
#0 - H3xept
2022-04-06T14:50:24Z
Fixed in previous commit.
#1 - H3xept
2022-04-06T14:50:51Z
Fixed in lifinance/lifi-contracts@f35ed79a266a69b363d72332b7861d15d18b98cb
#2 - H3xept
2022-04-06T14:51:42Z
Removed in previous commit.
We internally decided to avoid prefix increments for now.
Fixed in lifinance/lifi-contracts@87a27cee2fbde337c4ab873971f37573d2240994
#3 - H3xept
2022-04-11T12:05:25Z
We internally decided to avoid previx increments for now.