Platform: Code4rena
Start Date: 26/01/2023
Pot Size: $60,500 USDC
Total HM: 7
Participants: 31
Period: 6 days
Judge: berndartmueller
Total Solo HM: 3
Id: 207
League: ETH
Rank: 8/31
Findings: 2
Award: $1,548.21
š Selected for report: 0
š Solo Findings: 0
š Selected for report: CodingNameKiki
Also found by: 0xAgro, 0xSmartContract, IllIllI, Rolezn, SleepingBugs, btk, chrisdior4, matrix_0wl
997.1116 USDC - $997.11
| Issue | Instances | |
|---|---|---|
| [Lā01] | Loss of precision | 1 |
| [Lā02] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 1 |
Total: 2 instances over 2 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | 2**<n> - 1 should be re-written as type(uint<n>).max | 1 |
| [Nā02] | constants should be defined rather than using magic numbers | 27 |
| [Nā03] | Use a more recent version of solidity | 1 |
| [Nā04] | Use a more recent version of solidity | 2 |
| [Nā05] | Inconsistent method of specifying a floating pragma | 1 |
| [Nā06] | Non-library/interface files should use fixed compiler versions, not floating ones | 4 |
| [Nā07] | Using >/>= without specifying an upper bound is unsafe | 3 |
| [Nā08] | Typos | 1 |
| [Nā09] | File does not contain an SPDX Identifier | 1 |
| [Nā10] | Misplaced SPDX identifier | 1 |
| [Nā11] | File is missing NatSpec | 3 |
| [Nā12] | NatSpec is incomplete | 3 |
| [Nā13] | Not using the named return variables anywhere in the function is confusing | 2 |
| [Nā14] | Duplicated require()/revert() checks should be refactored to a modifier or function | 1 |
| [Nā15] | Contracts should have full test coverage | 1 |
| [Nā16] | Large or complicated code bases should implement fuzzing tests | 1 |
| [Nā17] | Function ordering does not follow the Solidity style guide | 3 |
| [Nā18] | Contract does not follow the Solidity style guide's suggested layout ordering | 8 |
Total: 64 instances over 18 issues
Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator
There is 1 instance of this issue:
File: src/core/JumpRate.sol 42: return (borrowedLiquidity * 1e18) / totalLiquidity;
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
There is 1 instance of this issue:
File: src/periphery/libraries/LendgineAddress.sol 24 keccak256( 25 abi.encodePacked( 26 hex"ff", 27 factory, 28 keccak256(abi.encode(token0, token1, token0Exp, token1Exp, upperBound)), 29 bytes32(INIT_CODE_HASH) 30 ) 31: )
2**<n> - 1 should be re-written as type(uint<n>).maxEarlier versions of solidity can use uint<n>(-1) instead. Expressions not including the - 1 can often be re-written to accomodate the change (e.g. by using a > rather than a >=, which will also save some gas)
There is 1 instance of this issue:
File: src/libraries/SafeCast.sol 16: require(y < 2 ** 255);
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 27 instances of this issue:
File: src/core/Factory.sol /// @audit 18 /// @audit 6 /// @audit 18 /// @audit 6 77: if (token0Exp > 18 || token0Exp < 6 || token1Exp > 18 || token1Exp < 6) revert ScaleError();
File: src/core/JumpRate.sol /// @audit 1e18 17: return (util * multiplier) / 1e18; /// @audit 1e18 19: uint256 normalRate = (kink * multiplier) / 1e18; /// @audit 1e18 21: return ((excessUtil * jumpMultiplier) / 1e18) + normalRate; /// @audit 1e18 37: return (borrowRate * util) / 1e18; /// @audit 1e18 42: return (borrowedLiquidity * 1e18) / totalLiquidity;
File: src/core/Lendgine.sol /// @audit 1e18 225: return FullMath.mulDiv(collateral * token1Scale, 1e18, 2 * upperBound); /// @audit 1e18 230: return FullMath.mulDiv(liquidity, 2 * upperBound, 1e18) / token1Scale; /// @audit 1e18 /// @audit 365 252: uint256 dilutionLPRequested = (FullMath.mulDiv(borrowRate, _totalLiquidityBorrowed, 1e18) * timeElapsed) / 365 days; /// @audit 1e18 257: rewardPerPositionStored += FullMath.mulDiv(dilutionSpeculative, 1e18, totalPositionSize);
File: src/core/Pair.sol /// @audit 1e18 56: uint256 scale0 = FullMath.mulDiv(amount0, 1e18, liquidity) * token0Scale; /// @audit 1e18 57: uint256 scale1 = FullMath.mulDiv(amount1, 1e18, liquidity) * token1Scale; /// @audit 1e18 61: uint256 a = scale0 * 1e18; /// @audit 4 63: uint256 c = (scale1 * scale1) / 4;
File: src/libraries/Balance.sol /// @audit 32 15: if (!success || data.length < 32) revert BalanceReturnError();
File: src/libraries/SafeCast.sol /// @audit 255 16: require(y < 2 ** 255);
File: src/periphery/LiquidityManager.sol /// @audit 1e18 178: position.tokensOwed += FullMath.mulDiv(position.size, rewardPerPositionPaid - position.rewardPerPositionPaid, 1e18); /// @audit 1e18 214: position.tokensOwed += FullMath.mulDiv(position.size, rewardPerPositionPaid - position.rewardPerPositionPaid, 1e18); /// @audit 1e18 238: position.tokensOwed += FullMath.mulDiv(position.size, rewardPerPositionPaid - position.rewardPerPositionPaid, 1e18);
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol /// @audit 997 62: uint256 amountInWithFee = amountIn * 997; /// @audit 1000 64: uint256 denominator = (reserveIn * 1000) + amountInWithFee; /// @audit 1000 80: uint256 numerator = reserveIn * amountOut * 1000; /// @audit 997 81: uint256 denominator = (reserveOut - amountOut) * 997;
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There is 1 instance of this issue:
File: src/core/Lendgine.sol 2: pragma solidity ^0.8.4;
Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
There are 2 instances of this issue:
File: src/periphery/libraries/LendgineAddress.sol 2: pragma solidity >=0.5.0;
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol 1: pragma solidity >=0.8.0;
Some files use >=, some use ^. The instances below are examples of the method that has the fewest instances for a specific version. Note that using >= without also specifying <= will lead to failures to compile, or external project incompatability, when the major version changes and there are breaking-changes, so ^ should be preferred regardless of the instance counts
There is 1 instance of this issue:
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol 1: pragma solidity >=0.8.0;
There are 4 instances of this issue:
File: src/core/Factory.sol 2: pragma solidity ^0.8.4;
File: src/core/Lendgine.sol 2: pragma solidity ^0.8.4;
File: src/periphery/LendgineRouter.sol 2: pragma solidity ^0.8.4;
File: src/periphery/LiquidityManager.sol 2: pragma solidity ^0.8.4;
>/>= without specifying an upper bound is unsafeThere will be breaking changes in future versions of solidity, and at that point your code will no longer be compatable. While you may have the specific version to use in a configuration file, others that include your source files may not.
There are 3 instances of this issue:
File: src/core/libraries/PositionMath.sol 2: pragma solidity >=0.5.0;
File: src/periphery/libraries/LendgineAddress.sol 2: pragma solidity >=0.5.0;
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol 1: pragma solidity >=0.8.0;
There is 1 instance of this issue:
File: src/periphery/LiquidityManager.sol /// @audit liqudity 229: /// @notice Collects interest owed to the callers liqudity position
There is 1 instance of this issue:
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol 0: pragma solidity >=0.8.0;
The SPDX identifier should be on the very first line of the file
There is 1 instance of this issue:
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol 1: pragma solidity >=0.8.0;
There are 3 instances of this issue:
File: src/core/Factory.sol
File: src/core/ImmutableState.sol
File: src/core/JumpRate.sol
There are 3 instances of this issue:
File: src/core/libraries/Position.sol /// @audit Missing: '@param position' /// @audit Missing: '@return' 67 /// @notice Helper function for determining the amount of tokens owed to a position 68 /// @param rewardPerPosition The global accrued interest 69: function newTokensOwed(Position.Info memory position, uint256 rewardPerPosition) internal pure returns (uint256) {
File: src/periphery/SwapHelper.sol /// @audit Missing: '@param params' 65 /// @notice Handles swaps on Uniswap V2 or V3 66 /// @param swapType A selector for UniswapV2 or V3 67 /// @param data Extra data that is not used by all types of swaps 68 /// @return amount The amount in or amount out depending on whether the call was exact in or exact out 69: function swap(SwapType swapType, SwapParams memory params, bytes memory data) internal returns (uint256 amount) {
Consider changing the variable to be an unnamed one
There are 2 instances of this issue:
File: src/core/JumpRate.sol /// @audit rate 25 function getSupplyRate( 26 uint256 borrowedLiquidity, 27 uint256 totalLiquidity 28 ) 29 external 30 pure 31 override 32: returns (uint256 rate) /// @audit rate 40: function utilizationRate(uint256 borrowedLiquidity, uint256 totalLiquidity) private pure returns (uint256 rate) {
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There is 1 instance of this issue:
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol 79: require(reserveIn > 0 && reserveOut > 0, "UniswapV2Library: INSUFFICIENT_LIQUIDITY");
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is 1 instance of this issue:
File: Various Files
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
There is 1 instance of this issue:
File: Various Files
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern
There are 3 instances of this issue:
File: src/core/JumpRate.sol /// @audit getBorrowRate() came earlier 25 function getSupplyRate( 26 uint256 borrowedLiquidity, 27 uint256 totalLiquidity 28 ) 29 external 30 pure 31 override 32: returns (uint256 rate)
File: src/core/Pair.sol /// @audit burn() came earlier 116: function swap(address to, uint256 amount0Out, uint256 amount1Out, bytes calldata data) external override nonReentrant {
File: src/periphery/Payment.sol /// @audit sweepToken() came earlier 44 function refundETH() external payable { 45: if (address(this).balance > 0) SafeTransferLib.safeTransferETH(msg.sender, address(this).balance);
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There are 8 instances of this issue:
File: src/core/Factory.sol /// @audit event LendgineCreated came earlier 39 mapping(address => mapping(address => mapping(uint256 => mapping(uint256 => mapping(uint256 => address))))) 40 public 41: override getLendgine;
File: src/core/Lendgine.sol /// @audit event Collect came earlier 56: mapping(address => Position.Info) public override positions;
File: src/core/Pair.sol /// @audit event Swap came earlier 40: uint120 public override reserve0;
File: src/periphery/LendgineRouter.sol /// @audit event Burn came earlier 43: address public immutable factory; /// @audit function constructor came earlier 65 modifier checkDeadline(uint256 deadline) { 66 if (deadline < block.timestamp) revert LivelinessError(); 67 _; 68: }
File: src/periphery/LiquidityManager.sol /// @audit event Collect came earlier 60: address public immutable factory; /// @audit function constructor came earlier 83 modifier checkDeadline(uint256 deadline) { 84 if (deadline < block.timestamp) revert LivelinessError(); 85 _; 86: }
File: src/periphery/SwapHelper.sol /// @audit function uniswapV3SwapCallback came earlier 49 enum SwapType { 50 UniswapV2, 51 UniswapV3 52: }
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | |
|---|---|---|
| [Lā01] | Missing checks for address(0x0) when assigning values to address state variables | 5 |
| [Lā02] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 1 |
Total: 6 instances over 2 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | require()/revert() statements should have descriptive reason strings | 3 |
| [Nā02] | constants should be defined rather than using magic numbers | 2 |
| [Nā03] | Event is missing indexed fields | 10 |
Total: 15 instances over 3 issues
address(0x0) when assigning values to address state variablesThere are 5 instances of this issue:
File: src/periphery/LendgineRouter.sol /// @audit (valid but excluded finding) 58: factory = _factory;
File: src/periphery/LiquidityManager.sol /// @audit (valid but excluded finding) 76: factory = _factory;
File: src/periphery/Payment.sol /// @audit (valid but excluded finding) 18: weth = _weth;
File: src/periphery/SwapHelper.sol /// @audit (valid but excluded finding) 30: uniswapV2Factory = _uniswapV2Factory; /// @audit (valid but excluded finding) 31: uniswapV3Factory = _uniswapV3Factory;
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
There is 1 instance of this issue:
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol /// @audit (valid but excluded finding) 22 keccak256( 23 abi.encodePacked( 24 hex"ff", 25 factory, 26 keccak256(abi.encodePacked(token0, token1)), 27 hex"e18a34eb0e04b04f7a0ac29a6e80748dca96319b42c54d679cb821dca90c6303" // init code hash 28 ) 29: )
require()/revert() statements should have descriptive reason stringsThere are 3 instances of this issue:
File: src/libraries/SafeCast.sol /// @audit (valid but excluded finding) 9: require((z = uint120(y)) == y); /// @audit (valid but excluded finding) 16: require(y < 2 ** 255);
File: src/periphery/SwapHelper.sol /// @audit (valid but excluded finding) 116: require(amountOutReceived == params.amount);
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 2 instances of this issue:
File: src/core/ImmutableState.sol /// @audit 18 - (valid but excluded finding) 35: token0Scale = 10 ** (18 - _token0Exp); /// @audit 18 - (valid but excluded finding) 36: token1Scale = 10 ** (18 - _token1Exp);
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 10 instances of this issue:
File: src/core/Lendgine.sol /// @audit (valid but excluded finding) 25: event Mint(address indexed sender, uint256 collateral, uint256 shares, uint256 liquidity, address indexed to); /// @audit (valid but excluded finding) 27: event Burn(address indexed sender, uint256 collateral, uint256 shares, uint256 liquidity, address indexed to); /// @audit (valid but excluded finding) 29: event Deposit(address indexed sender, uint256 size, uint256 liquidity, address indexed to); /// @audit (valid but excluded finding) 31: event Withdraw(address indexed sender, uint256 size, uint256 liquidity, address indexed to); /// @audit (valid but excluded finding) 33: event AccrueInterest(uint256 timeElapsed, uint256 collateral, uint256 liquidity); /// @audit (valid but excluded finding) 35: event AccruePositionInterest(address indexed owner, uint256 rewardPerPosition); /// @audit (valid but excluded finding) 37: event Collect(address indexed owner, address indexed to, uint256 amount);
File: src/core/Pair.sol /// @audit (valid but excluded finding) 21: event Mint(uint256 amount0In, uint256 amount1In, uint256 liquidity); /// @audit (valid but excluded finding) 23: event Burn(uint256 amount0Out, uint256 amount1Out, uint256 liquidity, address indexed to); /// @audit (valid but excluded finding) 25: event Swap(uint256 amount0Out, uint256 amount1Out, uint256 amount0In, uint256 amount1In, address indexed to);
#0 - c4-judge
2023-02-08T14:48:00Z
berndartmueller marked the issue as grade-a
#1 - c4-sponsor
2023-02-09T16:46:21Z
kyscott18 marked the issue as sponsor confirmed
#2 - kyscott18
2023-02-09T16:53:16Z
The issues here are very helpful but I am not going to change anything in the codebase unless explicitly necessary.
š Selected for report: NoamYakov
Also found by: 0xSmartContract, 0xackermann, Aymen0909, Deivitto, Diana, IllIllI, RaymondFam, ReyAdmirado, Rolezn, antonttc, arialblack14, c3phas, cryptostellar5, matrix_0wl, nadin, oyc_109
551.0959 USDC - $551.10
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Structs can be packed into fewer storage slots | 4 | - |
| [Gā02] | Using storage instead of memory for structs/arrays saves gas | 3 | 12600 |
| [Gā03] | Avoid contract existence checks by using low level calls | 20 | 2000 |
| [Gā04] | State variables should be cached in stack variables rather than re-reading them from storage | 2 | 194 |
| [Gā05] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 4 | 452 |
| [Gā06] | Optimize names to save gas | 3 | 66 |
| [Gā07] | Use a more recent version of solidity | 2 | - |
| [Gā08] | Use a more recent version of solidity | 4 | - |
| [Gā09] | >= costs less gas than > | 3 | 9 |
| [Gā10] | Splitting require() statements that use && saves gas | 2 | 6 |
| [Gā11] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 7 | - |
| [Gā12] | Stack variable used as a cheaper cache for a state variable is only used once | 1 | 3 |
| [Gā13] | Multiple if-statements with mutually-exclusive conditions should be changed to if-else statements | 1 | - |
| [Gā14] | Constructors can be marked payable | 5 | - |
Total: 61 instances over 14 issues with 15330 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings
There are 4 instances of this issue:
File: src/periphery/LendgineRouter.sol /// @audit Variable ordering with 8 slots instead of the current 9: /// uint256(32):token0Exp, uint256(32):token1Exp, uint256(32):upperBound, uint256(32):collateralMax, bytes(32):swapExtraData, address(20):token0, uint8(1):swapType, address(20):token1, address(20):payer 74 struct MintCallbackData { 75 address token0; 76 address token1; 77 uint256 token0Exp; 78 uint256 token1Exp; 79 uint256 upperBound; 80 uint256 collateralMax; 81 SwapType swapType; 82 bytes swapExtraData; 83 address payer; 84: } /// @audit Variable ordering with 11 slots instead of the current 12: /// uint256(32):token0Exp, uint256(32):token1Exp, uint256(32):upperBound, uint256(32):amountIn, uint256(32):amountBorrow, uint256(32):sharesMin, bytes(32):swapExtraData, uint256(32):deadline, address(20):token0, uint8(1):swapType, address(20):token1, address(20):recipient 126 struct MintParams { 127 address token0; 128 address token1; 129 uint256 token0Exp; 130 uint256 token1Exp; 131 uint256 upperBound; 132 uint256 amountIn; 133 uint256 amountBorrow; 134 uint256 sharesMin; 135 SwapType swapType; 136 bytes swapExtraData; 137 address recipient; 138 uint256 deadline; 139: } /// @audit Variable ordering with 10 slots instead of the current 11: /// uint256(32):token0Exp, uint256(32):token1Exp, uint256(32):upperBound, uint256(32):collateralMin, uint256(32):amount0Min, uint256(32):amount1Min, bytes(32):swapExtraData, address(20):token0, uint8(1):swapType, address(20):token1, address(20):recipient 175 struct PairMintCallbackData { 176 address token0; 177 address token1; 178 uint256 token0Exp; 179 uint256 token1Exp; 180 uint256 upperBound; 181 uint256 collateralMin; 182 uint256 amount0Min; 183 uint256 amount1Min; 184 SwapType swapType; 185 bytes swapExtraData; 186 address recipient; 187: } /// @audit Variable ordering with 12 slots instead of the current 13: /// uint256(32):token0Exp, uint256(32):token1Exp, uint256(32):upperBound, uint256(32):shares, uint256(32):collateralMin, uint256(32):amount0Min, uint256(32):amount1Min, bytes(32):swapExtraData, uint256(32):deadline, address(20):token0, uint8(1):swapType, address(20):token1, address(20):recipient 240 struct BurnParams { 241 address token0; 242 address token1; 243 uint256 token0Exp; 244 uint256 token1Exp; 245 uint256 upperBound; 246 uint256 shares; 247 uint256 collateralMin; 248 uint256 amount0Min; 249 uint256 amount1Min; 250 SwapType swapType; 251 bytes swapExtraData; 252 address recipient; 253 uint256 deadline; 254: }
storage instead of memory for structs/arrays saves gasWhen fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
There are 3 instances of this issue:
File: src/core/Lendgine.sol 167: Position.Info memory positionInfo = positions[msg.sender]; // SLOAD
File: src/core/libraries/Position.sol 47: Position.Info memory _positionInfo = positionInfo;
File: src/periphery/LiquidityManager.sol 211: Position memory position = positions[msg.sender][lendgine]; // SLOAD
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence
There are 20 instances of this issue:
File: src/core/ImmutableState.sol /// @audit parameters() 33: (token0, token1, _token0Exp, _token1Exp, upperBound) = Factory(msg.sender).parameters();
File: src/core/Pair.sol /// @audit swapCallback() 127: ISwapCallback(msg.sender).swapCallback(amount0Out, amount1Out, data);
File: src/periphery/LendgineRouter.sol /// @audit mint() 147: shares = ILendgine(lendgine).mint( /// @audit reserve0() 198: uint256 r0 = ILendgine(msg.sender).reserve0(); /// @audit reserve1() 199: uint256 r1 = ILendgine(msg.sender).reserve1(); /// @audit totalLiquidity() 200: uint256 totalLiquidity = ILendgine(msg.sender).totalLiquidity(); /// @audit convertLiquidityToCollateral() 231: uint256 collateralTotal = ILendgine(msg.sender).convertLiquidityToCollateral(liquidity); /// @audit burn() 266: amount = ILendgine(lendgine).burn(
File: src/periphery/LiquidityManager.sol /// @audit reserve0() 140: uint256 r0 = ILendgine(lendgine).reserve0(); /// @audit reserve1() 141: uint256 r1 = ILendgine(lendgine).reserve1(); /// @audit totalLiquidity() 142: uint256 totalLiquidity = ILendgine(lendgine).totalLiquidity(); /// @audit deposit() 157: uint256 size = ILendgine(lendgine).deposit( /// @audit positions() 177: (, uint256 rewardPerPositionPaid,) = ILendgine(lendgine).positions(address(this)); /// @audit withdraw() 208: (uint256 amount0, uint256 amount1, uint256 liquidity) = ILendgine(lendgine).withdraw(recipient, params.size); /// @audit positions() 213: (, uint256 rewardPerPositionPaid,) = ILendgine(lendgine).positions(address(this)); /// @audit positions() 237: (, uint256 rewardPerPositionPaid,) = ILendgine(params.lendgine).positions(address(this)); /// @audit collect() 246: uint256 collectAmount = ILendgine(params.lendgine).collect(recipient, amount);
File: src/periphery/Payment.sol /// @audit withdraw() 30: IWETH9(weth).withdraw(balanceWETH);
File: src/periphery/SwapHelper.sol /// @audit swap() 88: IUniswapV2Pair(pair).swap(amount0Out, amount1Out, params.recipient, bytes(""));
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol /// @audit getReserves() 46: (uint256 reserve0, uint256 reserve1,) = IUniswapV2Pair(pairFor(factory, tokenA, tokenB)).getReserves();
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 2 instances of this issue:
File: src/core/Lendgine.sol /// @audit totalPositionSize on line 135 142: if (totalLiquiditySupplied == 0 && totalPositionSize > 0) revert CompleteUtilizationError(); /// @audit totalLiquidityBorrowed on line 239 247: uint256 _totalLiquidityBorrowed = totalLiquidityBorrowed; // SLOAD
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There are 4 instances of this issue:
File: src/core/Lendgine.sol 91: totalLiquidityBorrowed += liquidity; 114: totalLiquidityBorrowed -= liquidity; 176: totalPositionSize -= size; 257: rewardPerPositionStored += FullMath.mulDiv(dilutionSpeculative, 1e18, totalPositionSize);
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 3 instances of this issue:
File: src/periphery/LendgineRouter.sol /// @audit mint(), burn() 20: contract LendgineRouter is Multicall, Payment, SelfPermit, SwapHelper, IMintCallback, IPairMintCallback {
File: src/periphery/LiquidityManager.sol /// @audit pairMintCallback(), addLiquidity(), removeLiquidity(), collect() 16: contract LiquidityManager is Multicall, Payment, SelfPermit, IPairMintCallback {
File: src/periphery/Payment.sol /// @audit unwrapWETH(), sweepToken(), refundETH() 12: abstract contract Payment {
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There are 2 instances of this issue:
File: src/core/libraries/PositionMath.sol 2: pragma solidity >=0.5.0;
File: src/periphery/libraries/LendgineAddress.sol 2: pragma solidity >=0.5.0;
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There are 4 instances of this issue:
File: src/core/ImmutableState.sol 2: pragma solidity ^0.8.0;
File: src/core/JumpRate.sol 2: pragma solidity ^0.8.0;
File: src/libraries/SafeCast.sol 2: pragma solidity ^0.8.0;
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol 1: pragma solidity >=0.8.0;
>= costs less gas than >The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas
There are 3 instances of this issue:
File: src/core/Lendgine.sol 198: collateral = collateralRequested > tokensOwed ? tokensOwed : collateralRequested; 253: uint256 dilutionLP = dilutionLPRequested > _totalLiquidityBorrowed ? _totalLiquidityBorrowed : dilutionLPRequested;
File: src/periphery/LiquidityManager.sol 241: amount = params.amountRequested > position.tokensOwed ? position.tokensOwed : params.amountRequested;
require() statements that use && saves gasSee this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas
There are 2 instances of this issue:
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol 61: require(reserveIn > 0 && reserveOut > 0, "UniswapV2Library: INSUFFICIENT_LIQUIDITY"); 79: require(reserveIn > 0 && reserveOut > 0, "UniswapV2Library: INSUFFICIENT_LIQUIDITY");
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contractĆ¢ā¬ā¢s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
There are 7 instances of this issue:
File: src/core/Pair.sol /// @audit uint120 reserve0 85: reserve0 = _reserve0 + SafeCast.toUint120(amount0In); // SSTORE /// @audit uint120 reserve1 86: reserve1 = _reserve1 + SafeCast.toUint120(amount1In); // SSTORE /// @audit uint120 reserve0 108: reserve0 = _reserve0 - SafeCast.toUint120(amount0); // SSTORE /// @audit uint120 reserve1 109: reserve1 = _reserve1 - SafeCast.toUint120(amount1); // SSTORE /// @audit uint120 reserve0 135: reserve0 = _reserve0 + SafeCast.toUint120(amount0In) - SafeCast.toUint120(amount0Out); // SSTORE /// @audit uint120 reserve1 136: reserve1 = _reserve1 + SafeCast.toUint120(amount1In) - SafeCast.toUint120(amount1Out); // SSTORE
File: src/libraries/SafeCast.sol /// @audit uint120 z 9: require((z = uint120(y)) == y);
If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend
There is 1 instance of this issue:
File: src/core/Lendgine.sol 163: uint256 _totalPositionSize = totalPositionSize; // SLOAD
if-statements with mutually-exclusive conditions should be changed to if-else statementsIf two conditions are the same, their blocks should be combined
There is 1 instance of this issue:
File: src/core/libraries/Position.sol 55 if (sizeDelta == 0) { 56 if (_positionInfo.size == 0) revert NoPositionError(); 57 sizeNext = _positionInfo.size; 58 } else { 59 sizeNext = PositionMath.addDelta(_positionInfo.size, sizeDelta); 60 } 61 62: if (sizeDelta != 0) positionInfo.size = sizeNext;
payablePayable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.
There are 5 instances of this issue:
File: src/core/ImmutableState.sol 27 constructor() { 28: factory = msg.sender;
File: src/periphery/LendgineRouter.sol 49 constructor( 50 address _factory, 51 address _uniswapV2Factory, 52 address _uniswapV3Factory, 53 address _weth 54 ) 55 SwapHelper(_uniswapV2Factory, _uniswapV3Factory) 56: Payment(_weth)
File: src/periphery/LiquidityManager.sol 75: constructor(address _factory, address _weth) Payment(_weth) {
File: src/periphery/Payment.sol 17: constructor(address _weth) {
File: src/periphery/SwapHelper.sol 29: constructor(address _uniswapV2Factory, address _uniswapV3Factory) {
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | require()/revert() strings longer than 32 bytes cost extra gas | 5 | - |
| [Gā02] | Using > 0 costs more gas than != 0 when used on a uint in a require() statement | 2 | 12 |
| [Gā03] | Division by two should use bit shifting | 1 | 20 |
| [Gā04] | Use custom errors rather than revert()/require() strings to save gas | 9 | - |
Total: 17 instances over 4 issues with 32 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
require()/revert() strings longer than 32 bytes cost extra gasEach extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
There are 5 instances of this issue:
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol /// @audit (valid but excluded finding) 11: require(tokenA != tokenB, "UniswapV2Library: IDENTICAL_ADDRESSES"); /// @audit (valid but excluded finding) 60: require(amountIn > 0, "UniswapV2Library: INSUFFICIENT_INPUT_AMOUNT"); /// @audit (valid but excluded finding) 61: require(reserveIn > 0 && reserveOut > 0, "UniswapV2Library: INSUFFICIENT_LIQUIDITY"); /// @audit (valid but excluded finding) 78: require(amountOut > 0, "UniswapV2Library: INSUFFICIENT_OUTPUT_AMOUNT"); /// @audit (valid but excluded finding) 79: require(reserveIn > 0 && reserveOut > 0, "UniswapV2Library: INSUFFICIENT_LIQUIDITY");
> 0 costs more gas than != 0 when used on a uint in a require() statementThis change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.
There are 2 instances of this issue:
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol /// @audit (valid but excluded finding) 60: require(amountIn > 0, "UniswapV2Library: INSUFFICIENT_INPUT_AMOUNT"); /// @audit (valid but excluded finding) 78: require(amountOut > 0, "UniswapV2Library: INSUFFICIENT_OUTPUT_AMOUNT");
<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two
There is 1 instance of this issue:
File: src/core/Pair.sol /// @audit (valid but excluded finding) 63: uint256 c = (scale1 * scale1) / 4;
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 9 instances of this issue:
File: src/core/libraries/PositionMath.sol /// @audit (valid but excluded finding) 14: require((z = x - uint256(-y)) < x, "LS"); /// @audit (valid but excluded finding) 16: require((z = x + uint256(y)) >= x, "LA");
File: src/periphery/Payment.sol /// @audit (valid but excluded finding) 22: require(msg.sender == weth, "Not WETH9");
File: src/periphery/UniswapV2/libraries/UniswapV2Library.sol /// @audit (valid but excluded finding) 11: require(tokenA != tokenB, "UniswapV2Library: IDENTICAL_ADDRESSES"); /// @audit (valid but excluded finding) 13: require(token0 != address(0), "UniswapV2Library: ZERO_ADDRESS"); /// @audit (valid but excluded finding) 60: require(amountIn > 0, "UniswapV2Library: INSUFFICIENT_INPUT_AMOUNT"); /// @audit (valid but excluded finding) 61: require(reserveIn > 0 && reserveOut > 0, "UniswapV2Library: INSUFFICIENT_LIQUIDITY"); /// @audit (valid but excluded finding) 78: require(amountOut > 0, "UniswapV2Library: INSUFFICIENT_OUTPUT_AMOUNT"); /// @audit (valid but excluded finding) 79: require(reserveIn > 0 && reserveOut > 0, "UniswapV2Library: INSUFFICIENT_LIQUIDITY");
#0 - c4-judge
2023-02-08T15:22:56Z
berndartmueller marked the issue as grade-a
#1 - c4-judge
2023-02-08T15:25:07Z
berndartmueller marked the issue as selected for report
#2 - noamyakov
2023-02-18T23:12:43Z
Some of the instances described in this gas report are false and the report saves less gas than claimed.
storage instead of memory for structs/arrays saves gasFile: src/core/libraries/Position.sol 47: Position.Info memory _positionInfo = positionInfo;
All of _positionInfo's fields are read, so no gas will be saved. size is read on line 50, rewardPerPositionPaid is read on line 70 (inside newTokensOwed()) and tokensOwed is read on line 64.
File: src/periphery/LiquidityManager.sol 211: Position memory position = positions[msg.sender][lendgine]; // SLOAD
All of position's fields are read, so no gas will be saved. size is read on line 214, rewardPerPositionPaid is read on line 214 and tokensOwed is read on line 214.
Total gas actually saved: 4200
File: src/core/Pair.sol /// @audit swapCallback() 127: ISwapCallback(msg.sender).swapCallback(amount0Out, amount1Out, data);
swapCallback() doesn't have a return value so the optimization doesn't apply here and no gas will be saved.
File: src/periphery/Payment.sol /// @audit withdraw() 30: IWETH9(weth).withdraw(balanceWETH);
withdraw() doesn't have a return value so the optimization doesn't apply here and no gas will be saved.
Total gas actually saved: 1800
Overall, this gas report actually saves a total of 6730 gas. Therefore, I think report #98 is better and should be selected for the report.
#3 - berndartmueller
2023-02-23T10:52:18Z
I invite @IllIllI000 to add feedback here.
#4 - IllIllI000
2023-02-23T13:58:20Z
noamyakov is right about the second G-02, and both of the G-03s (the G-03 bugs I've fixed). I haven't looked at his report so I can't vouch for correctness, but if his numbers are right, then yes, he should be selected
#5 - berndartmueller
2023-02-23T16:03:31Z
I will reconsider my decision to select this gas submission for the report. Thanks @noamyakov, for pointing out the inaccuracies, and big thanks to @IllIllI000 for being such a collegial fellow warden.
G-02: No possible gas savings in both mentioned cases by Noam
G-03: Due to safety precautions, the EXTCODESIZE check done by Solidity is valid in the case of swapCallback() and withdraw to ensure a contract exists at the target address. Hence, the gas optimization recommendations are invalid.
#6 - c4-judge
2023-02-23T16:03:40Z
berndartmueller marked the issue as not selected for report