You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is the top-ranked automated findings report, from IllIllI-bot bot. All findings in this report will be considered known issues for the purposes of your C4 audit.
Note: There is a section for disputed findings below the usual findings sections
[L-01] Addition/multiplication in unchecked block is unsafe
The additions/multiplications may silently overflow because they're in unchecked blocks with no preceding value checks, which may lead to unexpected results
[L-02] Code does not follow the best practice of check-effects-interaction
Code should follow the best-practice of check-effects-interaction, where state variables are updated before any external calls are made. Doing so prevents a large class of reentrancy bugs.
There are 4 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit tickSpacing() called prior to this assignment626: s_accountLiquidity[positionKey_to] = fromLiq;
/// @audit tickSpacing() called prior to this assignment627: s_accountLiquidity[positionKey_from] =0;
/// @audit tickSpacing() called prior to this assignment629: s_accountFeesBase[positionKey_to] = fromBase;
/// @audit tickSpacing() called prior to this assignment630: s_accountFeesBase[positionKey_from] =0;
[L-03] Consider implementing two-step procedure for updating protocol addresses
A copy-paste error or a typo may end up bricking protocol functionality, or sending tokens to an address with no known private key. Consider implementing a two-step procedure for updating protocol addresses, where the recipient is set as pending, and must 'accept' the assignment by making an affirmative call. A straight forward way of doing this would be to have the target contracts implement EIP-165, and to have the 'set' functions ensure that the recipient is of the right interface type.
[L-04] External calls in an un-bounded for-loop may result in a DOS
Consider limiting the number of iterations in for-loops that make external calls
There are 11 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit tickSpacing() : 583for (uint256 leg =0; leg < numLegs; ) {
584// for this leg index: extract the liquidity chunk: a 256bit word containing the liquidity amount and upper/lower tick585// @dev see `contracts/types/LiquidityChunk.sol`586uint256 liquidityChunk = PanopticMath.getLiquidityChunk(
587 id,
588 leg,
589uint128(amount),
590 univ3pool.tickSpacing()
591 );
592593//construct the positionKey for the from and to addresses594bytes32 positionKey_from =keccak256(
595abi.encodePacked(
596address(univ3pool),
597 from,
598 id.tokenType(leg),
599 liquidityChunk.tickLower(),
600 liquidityChunk.tickUpper()
601 )
602 );
603bytes32 positionKey_to =keccak256(
604abi.encodePacked(
605address(univ3pool),
606 to,
607 id.tokenType(leg),
608 liquidityChunk.tickLower(),
609 liquidityChunk.tickUpper()
610 )
611 );
612613// Revert if recipient already has that position614if (
615 (s_accountLiquidity[positionKey_to] !=0) ||616 (s_accountFeesBase[positionKey_to] !=0)
617 ) revert Errors.TransferFailed();
618619// Revert if not all balance is transferred620uint256 fromLiq = s_accountLiquidity[positionKey_from];
621if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();
622623int256 fromBase = s_accountFeesBase[positionKey_from];
624625//update+store liquidity and fee values between accounts626 s_accountLiquidity[positionKey_to] = fromLiq;
627 s_accountLiquidity[positionKey_from] =0;
628629 s_accountFeesBase[positionKey_to] = fromBase;
630 s_accountFeesBase[positionKey_from] =0;
631unchecked {
632++leg;
633 }
634: }
/// @audit positions() : _getFeesBase(), _createLegInAMM()/// @audit positions() : _getFeesBase(), _collectAndWritePositionData(), _createLegInAMM()/// @audit tickSpacing() : /// @audit mint() : _mintLiquidity(), _createLegInAMM()/// @audit fee() : _mintLiquidity(), _createLegInAMM()/// @audit token1() : _mintLiquidity(), _createLegInAMM()/// @audit token0() : _mintLiquidity(), _createLegInAMM()/// @audit burn() : _burnLiquidity(), _createLegInAMM()/// @audit collect() : _collectAndWritePositionData(), _createLegInAMM()860for (uint256 leg =0; leg < numLegs; ) {
861int256 _moved;
862int256 _itmAmounts;
863int256 _totalCollected;
864865 {
866// cache the univ3pool, tokenId, isBurn, and _positionSize variables to get rid of stack too deep error867 IUniswapV3Pool _univ3pool = univ3pool;
868uint256 _tokenId = tokenId;
869bool _isBurn = isBurn;
870uint128 _positionSize = positionSize;
871uint256 _leg;
872873unchecked {
874// Reverse the order of the legs if this call is burning a position (LIFO)875// We loop in reverse order if burning a position so that any dependent long liquidity is returned to the pool first,876// allowing the corresponding short liquidity to be removed877 _leg = _isBurn ? numLegs - leg -1 : leg;
878 }
879880// for this _leg index: extract the liquidity chunk: a 256bit word containing the liquidity amount and upper/lower tick881// @dev see `contracts/types/LiquidityChunk.sol`882uint256 liquidityChunk = PanopticMath.getLiquidityChunk(
883 _tokenId,
884 _leg,
885 _positionSize,
886 _univ3pool.tickSpacing()
887 );
888889 (_moved, _itmAmounts, _totalCollected) =_createLegInAMM(
890 _univ3pool,
891 _tokenId,
892 _leg,
893 liquidityChunk,
894 _isBurn
895 );
896897unchecked {
898// increment accumulators of the upper bound on tokens contained across all legs of the position at any given tick899 amount0 += Math.getAmount0ForLiquidity(liquidityChunk);
900901 amount1 += Math.getAmount1ForLiquidity(liquidityChunk);
902 }
903 }
904905 totalMoved = totalMoved.add(_moved);
906 itmAmounts = itmAmounts.add(_itmAmounts);
907 totalCollected = totalCollected.add(_totalCollected);
908909unchecked {
910++leg;
911 }
912: }
File: contracts/multicall/Multicall.sol
/// @audit delegatecall() : 14for (uint256 i =0; i < data.length; ) {
15 (boolsuccess, bytesmemoryresult) =address(this).delegatecall(data[i]);
1617if (!success) {
18// Bubble up the revert reason19// The bytes type is ABI encoded as a length-prefixed byte array20// So we simply need to add 32 to the pointer to get the start of the data21// And then revert with the size loaded from the first 32 bytes22// Other solutions will do work to differentiate the revert reasons and provide paranthetical information23// However, we have chosen to simply replicate the the normal behavior of the call24// NOTE: memory-safe because it reads from memory already allocated by solidity (the bytes memory result)25assembly ("memory-safe") {
26revert(add(result, 32), mload(result))
27 }
28 }
2930 results[i] = result;
3132unchecked {
33++i;
34 }
35: }
[L-05] File allows a version of solidity that is susceptible to an assembly optimizer bug
In solidity versions 0.8.13 and 0.8.14, there is an optimizer bug where, if the use of a variable is in a separate assembly block from the block in which it was stored, the mstore operation is optimized out, leading to uninitialized memory. The code currently does not have such a pattern of execution, but it does use mstores in assembly blocks, so it is a risk for future changes. The affected solidity versions should be avoided if at all possible.
There are 1 instance(s) of this issue:
File: contracts/libraries/SafeTransferLib.sol
19assembly ("memory-safe") {
20// Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here.21let p :=mload(0x40)
2223// Write the abi-encoded calldata into memory, beginning with the function selector.24mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
25mstore(add(4, p), from) // Append the "from" argument.26mstore(add(36, p), to) // Append the "to" argument.27mstore(add(68, p), amount) // Append the "amount" argument.2829 success :=and(
30// Set success to whether the call reverted, if not we check it either31// returned exactly 1 (can't just be non-zero data), or had no return data.32or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
33// We use 100 because that's the total length of our calldata (4 + 32 * 3)34// Counterintuitively, this call() must be positioned after the or() in the35// surrounding and() because and() evaluates its arguments from right to left.36call(gas(), token, 0, p, 100, 0, 32)
37 )
38: }
[L-06] Functions calling tokens with transfer hooks are missing reentrancy guards
Even if the function follows the best practice of check-effects-interaction, not using a reentrancy guard when there may be transfer hooks will open the users of this protocol up to read-only reentrancies with no way to protect against it, except by block-listing the whole protocol.
There are 2 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit safeTransferFrom() : uniswapV3MintCallback()407function uniswapV3MintCallback(
408uint256amount0Owed,
409uint256amount1Owed,
410bytescalldatadata411 ) external {
412// Decode the mint callback data413 CallbackLib.CallbackData memory decoded =abi.decode(data, (CallbackLib.CallbackData));
414// Validate caller to ensure we got called from the AMM pool415 CallbackLib.validateCallback(msg.sender, address(FACTORY), decoded.poolFeatures);
416// Sends the amount0Owed and amount1Owed quantities provided417if (amount0Owed >0)
418 SafeTransferLib.safeTransferFrom(
419 decoded.poolFeatures.token0,
420 decoded.payer,
421msg.sender,
422 amount0Owed
423 );
424if (amount1Owed >0)
425 SafeTransferLib.safeTransferFrom(
426 decoded.poolFeatures.token1,
427 decoded.payer,
428msg.sender,
429 amount1Owed
430 );
431: }
/// @audit safeTransferFrom() : uniswapV3SwapCallback()441function uniswapV3SwapCallback(
442int256amount0Delta,
443int256amount1Delta,
444bytescalldatadata445 ) external {
446// Decode the swap callback data, checks that the UniswapV3Pool has the correct address.447 CallbackLib.CallbackData memory decoded =abi.decode(data, (CallbackLib.CallbackData));
448// Validate caller to ensure we got called from the AMM pool449 CallbackLib.validateCallback(msg.sender, address(FACTORY), decoded.poolFeatures);
450451// Extract the address of the token to be sent (amount0 -> token0, amount1 -> token1)452address token = amount0Delta >0453?address(decoded.poolFeatures.token0)
454 : address(decoded.poolFeatures.token1);
455456// Transform the amount to pay to uint256 (take positive one from amount0 and amount1)457uint256 amountToPay = amount0Delta >0?uint256(amount0Delta) : uint256(amount1Delta);
458459// Pay the required token from the payer to the caller of this contract460 SafeTransferLib.safeTransferFrom(token, decoded.payer, msg.sender, amountToPay);
461: }
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
[L-08] Missing contract-existence checks before yul call()
Low-level calls return success if there is no code present at the specified address. In addition to the zero-address checks, add a check to verify that extcodesize() is non-zero.
There are 1 instance(s) of this issue:
File: contracts/libraries/SafeTransferLib.sol
19assembly ("memory-safe") {
20// Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here.21let p :=mload(0x40)
2223// Write the abi-encoded calldata into memory, beginning with the function selector.24mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
25mstore(add(4, p), from) // Append the "from" argument.26mstore(add(36, p), to) // Append the "to" argument.27mstore(add(68, p), amount) // Append the "amount" argument.2829 success :=and(
30// Set success to whether the call reverted, if not we check it either31// returned exactly 1 (can't just be non-zero data), or had no return data.32or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
33// We use 100 because that's the total length of our calldata (4 + 32 * 3)34// Counterintuitively, this call() must be positioned after the or() in the35// surrounding and() because and() evaluates its arguments from right to left.36call(gas(), token, 0, p, 100, 0, 32)
37 )
38: }
[L-09] Solidity version 0.8.20 may not work on other chains due to PUSH0
The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which includes the new PUSH0 op code. This op code may not yet be implemented on all L2s, so deployment on these chains will fail. To work around this issue, use an earlier EVMversion. While the project itself may or may not compile with 0.8.20, other projects with which it integrates, or which extend this project may, and those projects will have problems deploying these contracts/libraries.
When a type is downcast to a smaller type, the higher order bits are truncated, effectively applying a modulo to the original value. Without any other checks, this wrapping will lead to unexpected behavior and bugs
[G-01]do-while is cheaper than for-loops when the initial check can be skipped
for (uint256 i; i < len; ++i){ ... } -> do { ...; ++i } while (i < len);
There are 6 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
550: for (uint256 i =0; i < ids.length; ) {
583: for (uint256 leg =0; leg < numLegs; ) {
860: for (uint256 leg =0; leg < numLegs; ) {
[G-02] Assembly: Check msg.sender using xor and the scratch space
See this prior finding for details on the conversion
There are 2 instance(s) of this issue:
File: contracts/tokens/ERC1155Minimal.sol
97: if (!(msg.sender== from || isApprovedForAll[from][msg.sender])) revertNotAuthorized();
135: if (!(msg.sender== from || isApprovedForAll[from][msg.sender])) revertNotAuthorized();
[G-03] Use uint256(1)/uint256(2) instead of true/false to save gas for changes
Avoids a Gsset (20000 gas) when changing from false to true, after having been true in the past. Since most of the bools aren't changed twice in one transaction, I've counted the amount of gas as half of the full amount, for each variable. Note that public state variables can be re-written to be private and use uint256, but have public getters returning bools.
[G-04] Avoid updating storage when the value hasn't changed
If the old value is equal to the new value, not re-storing the value will avoid a Gsreset (2900 gas), potentially at the expense of a Gcoldsload (2100 gas) or a Gwarmaccess (100 gas)
[G-05] Assembly: Use scratch space for building calldata
If an external call's calldata can fit into two or fewer words, use the scratch space to build the calldata, rather than allowing Solidity to do a memory expansion.
[G-06] Avoid contract existence checks by using low-level calls
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. Note that it still saves gas, even if the return value is not directly used.
// Booleans are more expensive than uint256 or any type that takes up a full// word because each write operation emits an extra SLOAD to first read the// slot's contents, replace the bits taken up by the boolean, and then write// back. This is the compiler's defense against contract upgrades and// pointer aliasing, and it cannot be disabled.
[G-09] Combine mappings referenced in the same function by the same key
Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Even if the values can't be packed, if both fields are accessed in the same function (as is the case for these instances), combining them can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
There are 4 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit combine: s_accountFeesBase,s_accountLiquidity578function registerTokenTransfer(addressfrom, addressto, uint256id, uint256amount) internal {
579// Extract univ3pool from the poolId map to Uniswap Pool580 IUniswapV3Pool univ3pool = s_poolContext[id.validate()].pool;
581582uint256 numLegs = id.countLegs();
583for (uint256 leg =0; leg < numLegs; ) {
584// for this leg index: extract the liquidity chunk: a 256bit word containing the liquidity amount and upper/lower tick585// @dev see `contracts/types/LiquidityChunk.sol`586uint256 liquidityChunk = PanopticMath.getLiquidityChunk(
587 id,
588 leg,
589uint128(amount),
590 univ3pool.tickSpacing()
591 );
592593//construct the positionKey for the from and to addresses594bytes32 positionKey_from =keccak256(
595abi.encodePacked(
596address(univ3pool),
597 from,
598 id.tokenType(leg),
599 liquidityChunk.tickLower(),
600 liquidityChunk.tickUpper()
601 )
602 );
603bytes32 positionKey_to =keccak256(
604abi.encodePacked(
605address(univ3pool),
606 to,
607 id.tokenType(leg),
608 liquidityChunk.tickLower(),
609 liquidityChunk.tickUpper()
610 )
611 );
612613// Revert if recipient already has that position614if (
615 (s_accountLiquidity[positionKey_to] !=0) ||616 (s_accountFeesBase[positionKey_to] !=0)
617 ) revert Errors.TransferFailed();
618619// Revert if not all balance is transferred620uint256 fromLiq = s_accountLiquidity[positionKey_from];
621if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();
622623int256 fromBase = s_accountFeesBase[positionKey_from];
624625//update+store liquidity and fee values between accounts626 s_accountLiquidity[positionKey_to] = fromLiq;
627 s_accountLiquidity[positionKey_from] =0;
628629 s_accountFeesBase[positionKey_to] = fromBase;
630 s_accountFeesBase[positionKey_from] =0;
631unchecked {
632++leg;
633 }
634 }
635: }
/// @audit combine: s_accountFeesBase,s_accountLiquidity936function _createLegInAMM(
937 IUniswapV3Pool _univ3pool,
938uint256_tokenId,
939uint256_leg,
940uint256_liquidityChunk,
941bool_isBurn942 ) internalreturns (int256_moved, int256_itmAmounts, int256_totalCollected) {
943uint256 _tokenType = TokenId.tokenType(_tokenId, _leg);
944// unique key to identify the liquidity chunk in this uniswap pool945bytes32 positionKey =keccak256(
946abi.encodePacked(
947address(_univ3pool),
948msg.sender,
949 _tokenType,
950 _liquidityChunk.tickLower(),
951 _liquidityChunk.tickUpper()
952 )
953 );
954955// update our internal bookkeeping of how much liquidity we have deployed in the AMM956// for example: if this _leg is short, we add liquidity to the amm, make sure to add that to our tracking957uint128 updatedLiquidity;
958uint256 isLong = TokenId.isLong(_tokenId, _leg);
959uint256 currentLiquidity = s_accountLiquidity[positionKey]; //cache960961unchecked {
962// did we have liquidity already deployed in Uniswap for this chunk range from some past mint?963964// s_accountLiquidity is a LeftRight. The right slot represents the liquidity currently sold (added) in the AMM owned by the user965// the left slot represents the amount of liquidity currently bought (removed) that has been removed from the AMM - the user owes it to a seller966// the reason why it is called "removedLiquidity" is because long options are created by removing -ie.short selling LP positions967uint128 startingLiquidity = currentLiquidity.rightSlot();
968uint128 removedLiquidity = currentLiquidity.leftSlot();
969uint128 chunkLiquidity = _liquidityChunk.liquidity();
970971if (isLong ==0) {
972// selling/short: so move from msg.sender *to* uniswap973// we're minting more liquidity in uniswap: so add the incoming liquidity chunk to the existing liquidity chunk974 updatedLiquidity = startingLiquidity + chunkLiquidity;
975976/// @dev If the isLong flag is 0=short but the position was burnt, then this is closing a long position977/// @dev so the amount of short liquidity should decrease.978if (_isBurn) {
979 removedLiquidity -= chunkLiquidity;
980 }
981 } else {
982// the _leg is long (buying: moving *from* uniswap to msg.sender)983// so we seek to move the incoming liquidity chunk *out* of uniswap - but was there sufficient liquidity sitting in uniswap984// in the first place?985if (startingLiquidity < chunkLiquidity) {
986// the amount we want to move (liquidityChunk.legLiquidity()) out of uniswap is greater than987// what the account that owns the liquidity in uniswap has (startingLiquidity)988// we must ensure that an account can only move its own liquidity out of uniswap989// so we revert in this case990revert Errors.NotEnoughLiquidity();
991 } else {
992// we want to move less than what already sits in uniswap, no problem:993 updatedLiquidity = startingLiquidity - chunkLiquidity;
994 }
995996/// @dev If the isLong flag is 1=long and the position is minted, then this is opening a long position997/// @dev so the amount of short liquidity should increase.998if (!_isBurn) {
999 removedLiquidity += chunkLiquidity;
1000 }
1001 }
10021003// update the starting liquidity for this position for next time around1004 s_accountLiquidity[positionKey] =uint256(0).toLeftSlot(removedLiquidity).toRightSlot(
1005 updatedLiquidity
1006 );
1007 }
10081009// track how much liquidity we need to collect from uniswap1010// add the fees that accumulated in uniswap within the liquidityChunk:1011 {
1012/** if the position is NOT long (selling a put or a call), then _mintLiquidity to move liquidity1013 from the msg.sender to the uniswap v3 pool:1014 Selling(isLong=0): Mint chunk of liquidity in Uniswap (defined by upper tick, lower tick, and amount)1015 ┌─────────────────────────────────┐1016 ▲ ┌▼┐ liquidityChunk │1017 │ ┌──┴─┴──┐ ┌───┴──┐1018 │ │ │ │ │1019 └──┴───────┴──► └──────┘1020 Uniswap v3 msg.sender1021 1022 else: the position is long (buying a put or a call), then _burnLiquidity to remove liquidity from univ31023 Buying(isLong=1): Burn in Uniswap1024 ┌─────────────────┐1025 ▲ ┌┼┐ │1026 │ ┌──┴─┴──┐ ┌───▼──┐1027 │ │ │ │ │1028 └──┴───────┴──► └──────┘1029 Uniswap v3 msg.sender 1030 */1031 _moved = isLong ==01032?_mintLiquidity(_liquidityChunk, _univ3pool)
1033 : _burnLiquidity(_liquidityChunk, _univ3pool); // from msg.sender to Uniswap1034// add the moved liquidity chunk to amount we need to collect from uniswap:10351036// Is this _leg ITM?1037// if tokenType is 1, and we transacted some token0: then this leg is ITM!1038if (_tokenType ==1) {
1039// extract amount moved out of UniswapV3 pool1040 _itmAmounts = _itmAmounts.toRightSlot(_moved.rightSlot());
1041 }
1042// if tokenType is 0, and we transacted some token1: then this leg is ITM1043if (_tokenType ==0) {
1044// Add this in-the-money amount transacted.1045 _itmAmounts = _itmAmounts.toLeftSlot(_moved.leftSlot());
1046 }
1047 }
10481049// if there was liquidity at that tick before the transaction, collect any accumulated fees1050if (currentLiquidity.rightSlot() >0) {
1051 _totalCollected =_collectAndWritePositionData(
1052 _liquidityChunk,
1053 _univ3pool,
1054 currentLiquidity,
1055 positionKey,
1056 _moved,
1057 isLong
1058 );
1059 }
10601061// position has been touched, update s_accountFeesBase with the latest values from the pool.positions1062 s_accountFeesBase[positionKey] =_getFeesBase(
1063 _univ3pool,
1064 updatedLiquidity,
1065 _liquidityChunk
1066 );
1067: }
/// @audit combine: s_accountPremiumGross,s_accountPremiumOwed1073function _updateStoredPremia(
1074bytes32positionKey,
1075uint256currentLiquidity,
1076int256collectedAmounts1077 ) private {
1078 (uint256deltaPremiumOwed, uint256deltaPremiumGross) =_getPremiaDeltas(
1079 currentLiquidity,
1080 collectedAmounts
1081 );
10821083 s_accountPremiumOwed[positionKey] = s_accountPremiumOwed[positionKey].add(deltaPremiumOwed);
1084 s_accountPremiumGross[positionKey] = s_accountPremiumGross[positionKey].add(
1085 deltaPremiumGross
1086 );
1087: }
/// @audit combine: s_accountFeesBase,s_accountLiquidity,s_accountPremiumGross,s_accountPremiumOwed1371function getAccountPremium(
1372addressuniv3pool,
1373addressowner,
1374uint256tokenType,
1375int24tickLower,
1376int24tickUpper,
1377int24atTick,
1378uint256isLong1379 ) externalviewreturns (uint128premiumToken0, uint128premiumToken1) {
1380bytes32 positionKey =keccak256(
1381abi.encodePacked(address(univ3pool), owner, tokenType, tickLower, tickUpper)
1382 );
13831384// Extract the account liquidity for a given uniswap pool, owner, token type, and ticks1385uint256 acctPremia = isLong ==11386? s_accountPremiumOwed[positionKey]
1387 : s_accountPremiumGross[positionKey];
13881389// Compute the premium up to the current block (ie. after last touch until now). Do not proceed if atTick == type(int24).max = 83886081390if (atTick <type(int24).max) {
1391// unique key to identify the liquidity chunk in this uniswap pool1392uint256 accountLiquidities = s_accountLiquidity[positionKey];
1393uint128 netLiquidity = accountLiquidities.rightSlot();
1394if (netLiquidity !=0) {
1395int256 amountToCollect;
1396 {
1397 IUniswapV3Pool _univ3pool =IUniswapV3Pool(univ3pool);
1398uint256 tempChunk =uint256(0).createChunk(tickLower, tickUpper, 0);
1399// how much fees have been accumulated within the liquidity chunk since last time we updated this chunk?1400// Compute (currentFeesGrowth - oldFeesGrowth), the amount to collect1401// currentFeesGrowth (calculated from FeesCalc.calculateAMMSwapFeesLiquidityChunk) is (ammFeesCollectedPerLiquidity * liquidityChunk.liquidity())1402// oldFeesGrowth is the last stored update of fee growth within the position range in the past (feeGrowthRange*liquidityChunk.liquidity()) (s_accountFeesBase[positionKey])1403int256 feesBase = FeesCalc.calculateAMMSwapFeesLiquidityChunk(
1404 _univ3pool,
1405 atTick,
1406 netLiquidity,
1407 tempChunk
1408 );
1409 amountToCollect = feesBase.sub(s_accountFeesBase[positionKey]);
1410 }
14111412 (uint256deltaPremiumOwed, uint256deltaPremiumGross) =_getPremiaDeltas(
1413 accountLiquidities,
1414 amountToCollect
1415 );
1416// Extract the account liquidity for a given uniswap pool, owner, token type, and ticks1417 acctPremia = isLong ==11418? acctPremia.add(deltaPremiumOwed)
1419 : acctPremia.add(deltaPremiumGross);
1420 }
1421 }
14221423 premiumToken0 = acctPremia.rightSlot();
1424 premiumToken1 = acctPremia.leftSlot();
1425: }
[G-11] Multiple accesses of a mapping/array should use a local variable cache
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit s_poolContext[poolId] on line 323326: s_poolContext[poolId].locked =true;
[G-12] Assigning state variables directly with named struct constructors wastes gas
Using named arguments for struct means that the compiler needs to organize the fields in memory before doing the assignment, which wastes gas. Set each field directly in storage (use dot-notation), or use the unnamed version of the constructor.
[G-13] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead
When using elements that are smaller than 32 bytes, your contracts 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
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
Payable 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.
[G-16]internal/private functions only called once can be inlined to save gas
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls. The inliner can do it only for 'simple' cases:
Now to get back to the point why we require the routine to be simple: As soon as you do more complicated things like for example branching, calling external contracts, the Common Subexpression Eliminator cannot re-construct the code anymore or does not do full symbolic evaluation of the expressions.
[G-17] Division by powers of two should use bit shifting
<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.
[G-18] Assembly: Use scratch space when building emitted events with two data arguments
Using the scratch space for more than one, but at most two words worth of data (non-indexed arguments) will save gas over needing Solidity's abi memory expansion used for emitting normally.
There are 3 instance(s) of this issue:
File: contracts/tokens/ERC1155Minimal.sol
108: emitTransferSingle(msg.sender, from, to, id, amount);
220: emitTransferSingle(msg.sender, address(0), to, id, amount);
239: emitTransferSingle(msg.sender, from, address(0), id, amount);
[G-20] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct
Saves a storage slot for each of the removed mappings. The instances below refer to both mappings using the same key in the same function, so the mappings are related.
There are 3 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit combine: s_accountFeesBase,s_accountLiquidity/// @audit combine: s_accountFeesBase,s_accountLiquidity,s_accountPremiumGross,s_accountPremiumOwed/// @audit combine: s_accountPremiumGross,s_accountPremiumOwed72contractSemiFungiblePositionManagerisERC1155, Multicall {
73/*//////////////////////////////////////////////////////////////74 EVENTS75 //////////////////////////////////////////////////////////////*/7677/// @notice Emitted when a UniswapV3Pool is initialized.78: /// @param uniswapPool Address of the underlying Uniswap v3 pool
File: contracts/SemiFungiblePositionManager.sol
72contractSemiFungiblePositionManagerisERC1155, Multicall {
73/*//////////////////////////////////////////////////////////////74 EVENTS75 //////////////////////////////////////////////////////////////*/7677/// @notice Emitted when a UniswapV3Pool is initialized.78: /// @param uniswapPool Address of the underlying Uniswap v3 pool
If the variable is only accessed once, it's cheaper to use the assigned value directly that one time, and save the 3 gas the extra stack assignment would spend
The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas. If < is being used, the condition can be inverted.
There are 21 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
417if (amount0Owed >0)
418 SafeTransferLib.safeTransferFrom(
419 decoded.poolFeatures.token0,
420 decoded.payer,
421msg.sender,
422 amount0Owed
423: );
424if (amount1Owed >0)
425 SafeTransferLib.safeTransferFrom(
426 decoded.poolFeatures.token1,
427 decoded.payer,
428msg.sender,
429 amount1Owed
430: );
687if (tickLimitLow > tickLimitHigh) {
688 swapAtMint =true;
689 (tickLimitLow, tickLimitHigh) = (tickLimitHigh, tickLimitLow);
690: }
985if (startingLiquidity < chunkLiquidity) {
986// the amount we want to move (liquidityChunk.legLiquidity()) out of uniswap is greater than987// what the account that owns the liquidity in uniswap has (startingLiquidity)988// we must ensure that an account can only move its own liquidity out of uniswap989// so we revert in this case990revert Errors.NotEnoughLiquidity();
991 } else {
992// we want to move less than what already sits in uniswap, no problem:993 updatedLiquidity = startingLiquidity - chunkLiquidity;
994: }
1050if (currentLiquidity.rightSlot() >0) {
1051 _totalCollected =_collectAndWritePositionData(
1052 _liquidityChunk,
1053 _univ3pool,
1054 currentLiquidity,
1055 positionKey,
1056 _moved,
1057 isLong
1058 );
1059: }
1390if (atTick <type(int24).max) {
1391// unique key to identify the liquidity chunk in this uniswap pool1392uint256 accountLiquidities = s_accountLiquidity[positionKey];
1393uint128 netLiquidity = accountLiquidities.rightSlot();
1394if (netLiquidity !=0) {
1395int256 amountToCollect;
1396 {
1397 IUniswapV3Pool _univ3pool =IUniswapV3Pool(univ3pool);
1398uint256 tempChunk =uint256(0).createChunk(tickLower, tickUpper, 0);
1399// how much fees have been accumulated within the liquidity chunk since last time we updated this chunk?1400// Compute (currentFeesGrowth - oldFeesGrowth), the amount to collect1401// currentFeesGrowth (calculated from FeesCalc.calculateAMMSwapFeesLiquidityChunk) is (ammFeesCollectedPerLiquidity * liquidityChunk.liquidity())1402// oldFeesGrowth is the last stored update of fee growth within the position range in the past (feeGrowthRange*liquidityChunk.liquidity()) (s_accountFeesBase[positionKey])1403int256 feesBase = FeesCalc.calculateAMMSwapFeesLiquidityChunk(
1404 _univ3pool,
1405 atTick,
1406 netLiquidity,
1407 tempChunk
1408 );
1409 amountToCollect = feesBase.sub(s_accountFeesBase[positionKey]);
1410 }
14111412 (uint256deltaPremiumOwed, uint256deltaPremiumGross) =_getPremiaDeltas(
1413 accountLiquidities,
1414 amountToCollect
1415 );
1416// Extract the account liquidity for a given uniswap pool, owner, token type, and ticks1417 acctPremia = isLong ==11418? acctPremia.add(deltaPremiumOwed)
1419 : acctPremia.add(deltaPremiumGross);
1420 }
1421: }
[G-27] A memory array's length should not be looked up in every iteration of a for/while-loop
Caching the length of memory arrays changes each length lookup (i.e. on every iteration) from an MLOAD to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset, saving 3 gas.
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
550: for (uint256 i =0; i < ids.length; ) {
[G-29] Using private rather than public, saves gas
For constants, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There are 2 instance(s) of this issue:
File: contracts/tokens/ERC1155Minimal.sol
62: mapping(address account =>mapping(uint256 tokenId =>uint256balance)) public balanceOf;
67mapping(address owner =>mapping(address operator =>boolapprovedForAll))
68: public isApprovedForAll;
[N-01]2**<n> - 1 should be re-written as type(uint<n>).max
Earlier 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)
File: contracts/tokens/ERC1155Minimal.sol
202: interfaceId ==0x01ffc9a7||// ERC165 Interface ID for ERC165203: interfaceId ==0xd9b67a26; // ERC165 Interface ID for ERC1155
One level of nesting can be removed by not having an else block when the if-block returns, and if (foo) { return 1; } else { return 2; } becomes if (foo) { return 1; } return 2;. A following else if can become if
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
985if (startingLiquidity < chunkLiquidity) {
986// the amount we want to move (liquidityChunk.legLiquidity()) out of uniswap is greater than987// what the account that owns the liquidity in uniswap has (startingLiquidity)988// we must ensure that an account can only move its own liquidity out of uniswap989// so we revert in this case990revert Errors.NotEnoughLiquidity();
991 } else {
992// we want to move less than what already sits in uniswap, no problem:993 updatedLiquidity = startingLiquidity - chunkLiquidity;
994: }
The code can be made more compact while also increasing readability by converting the following if-statements to ternaries (e.g. foo += (x > y) ? a : b)
function foo(address x, address) -> function foo(address x, address /* y */)
There are 3 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
492// Call a function that contains other functions to mint/burn position, collect amounts, swap if necessary493: (totalCollected, totalSwapped, newTick) =_validateAndForwardToAMM(
692// initialize some variables returned by the _createPositionInAMM function693int256 itmAmounts;
694695 {
696// calls a function that loops through each leg of tokenId and mints/burns liquidity in Uni v3 pool697: (totalMoved, totalCollectedFromAMM, itmAmounts) =_createPositionInAMM(
File: contracts/libraries/SafeTransferLib.sol
23// Write the abi-encoded calldata into memory, beginning with the function selector.24: mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
[N-08] Adding a return statement when the function defines a named return variable, is redundant
Once the return variable has been assigned (or has its default value), there is no need to explicitly return it at the end of the function, since it's returned automatically.
Writing data to the free memory pointer without later updating the free memory pointer will cause there to be dirty bits at that memory location. Not updating the free memory pointer will make it harder for the optimizer to reason about whether the memory needs to be cleaned, which may lead to worse optimizations. Annotate the block with assembly ("memory-safe") { ... } if the memory's value can be discarded. If the memory needs to be saved, update the free memory pointer in addtion to using the annotation. See this link for other cases where the annotation can be used
There are 1 instance(s) of this issue:
File: contracts/libraries/SafeTransferLib.sol
1819assembly ("memory-safe") {
20// Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here.21let p :=mload(0x40)
2223// Write the abi-encoded calldata into memory, beginning with the function selector.24mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
25mstore(add(4, p), from) // Append the "from" argument.26mstore(add(36, p), to) // Append the "to" argument.27mstore(add(68, p), amount) // Append the "amount" argument.2829 success :=and(
30// Set success to whether the call reverted, if not we check it either31// returned exactly 1 (can't just be non-zero data), or had no return data.32or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
33// We use 100 because that's the total length of our calldata (4 + 32 * 3)34// Counterintuitively, this call() must be positioned after the or() in the35// surrounding and() because and() evaluates its arguments from right to left.36call(gas(), token, 0, p, 100, 0, 32)
37 )
38: }
[N-10] Assembly blocks should have extensive comments
Assembly blocks take a lot more time to audit than normal Solidity code, and often have gotchas and side-effects that the Solidity versions of the same code do not. Consider adding more comments explaining what is being done in every step of the assembly code, and describe why assembly is being used instead of Solidity.
Consider whether the number of casts is really necessary, or whether using a different type would be more appropriate. Alternatively, add comments to explain in detail why the casts are necessary, and any implicit reasons why the cast does not introduce an overflow.
There are 25 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
1119 feesBase =int256(0)
1120: .toRightSlot(int128(int256(Math.mulDiv128(feeGrowthInside0LastX128, liquidity))))
1120 .toRightSlot(int128(int256(Math.mulDiv128(feeGrowthInside0LastX128, liquidity))))
1121: .toLeftSlot(int128(int256(Math.mulDiv128(feeGrowthInside1LastX128, liquidity))));
1158// from msg.sender to the uniswap pool, stored as negative value to represent amount debited1159: movedAmounts =int256(0).toRightSlot(int128(int256(amount0))).toLeftSlot(
1159 movedAmounts =int256(0).toRightSlot(int128(int256(amount0))).toLeftSlot(
1160: int128(int256(amount1))
1185unchecked {
1186: movedAmounts =int256(0).toRightSlot(-int128(int256(amount0))).toLeftSlot(
1186 movedAmounts =int256(0).toRightSlot(-int128(int256(amount0))).toLeftSlot(
1187: -int128(int256(amount1))
Doing so will significantly increase centralization, but will help to prevent hackers from using stolen tokens
There are 2 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit ERC1155 handles tokens72contractSemiFungiblePositionManagerisERC1155, Multicall {
73/*//////////////////////////////////////////////////////////////74 EVENTS75 //////////////////////////////////////////////////////////////*/7677/// @notice Emitted when a UniswapV3Pool is initialized.78: /// @param uniswapPool Address of the underlying Uniswap v3 pool
File: contracts/tokens/ERC1155Minimal.sol
/// @audit ERC1155 handles tokens10abstractcontractERC1155 {
11/*//////////////////////////////////////////////////////////////12 EVENTS13 //////////////////////////////////////////////////////////////*/1415/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20: /// @param amount the amount of tokens transferred
Consider using formal verification to mathematically prove that your code does what is intended, and does not have any edge cases with unexpected behavior. The solidity compiler itself has this functionality built in
The functions below take in an unbounded array, and make function calls for entries in the array. While the function will revert if it eventually runs out of gas, it may be a nicer user experience to require() that the length of the array is below some reasonable maximum, so that the user doesn't have to use up a full transaction's gas only to see that the transaction reverts.
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
550for (uint256 i =0; i < ids.length; ) {
551registerTokenTransfer(from, to, ids[i], amounts[i]);
552unchecked {
553++i;
554 }
555: }
This allows for bugs to be fixed in production, at the expense of significantly increasing centralization.
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
72contractSemiFungiblePositionManagerisERC1155, Multicall {
73/*//////////////////////////////////////////////////////////////74 EVENTS75 //////////////////////////////////////////////////////////////*/7677/// @notice Emitted when a UniswapV3Pool is initialized.78: /// @param uniswapPool Address of the underlying Uniswap v3 pool
The longer a string of operations is, the harder it is to understand it. Consider splitting the full calculation into more steps, with more descriptive temporary variable names, and add extensive comments.
[N-20] Consider using delete rather than assigning zero/false to clear values
The delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic
[N-22] Consider using descriptive constants when passing zero as a function argument
Passing zero as a function argument can sometimes result in a security issue (e.g. passing zero as the slippage parameter). Consider using a constant variable with a descriptive name, so it's clear that the argument is intentionally being used, and for the right reasons.
File: contracts/tokens/ERC1155Minimal.sol
110: if (to.code.length!=0) {
163: if (to.code.length!=0) {
202: interfaceId ==0x01ffc9a7||// ERC165 Interface ID for ERC165203: interfaceId ==0xd9b67a26; // ERC165 Interface ID for ERC1155222: if (to.code.length!=0) {
File: contracts/types/TokenId.sol
397: legLowerTick % tickSpacing !=0||398: legUpperTick % tickSpacing !=0||443: if (i ==0)
445: if (i ==1)
447: if (i ==2)
449: if (i ==3)
464: if (self.optionRatio(0) ==0) revert Errors.InvalidTokenIdParameter(1);
469: if (self.optionRatio(i) ==0) {
473: if ((self >> (64+48* i)) !=0) revert Errors.InvalidTokenIdParameter(1);
480: if ((self.width(i) ==0)) revert Errors.InvalidTokenIdParameter(5);
The contracts should expose an interface so that other projects can more easily integrate with it, without having to develop their own non-standard variants.
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.
[N-27] Critical system parameter changes should be behind a timelock
From the point of view of a user, the changing of the owner of a contract is a high risk operation that may have outcomes ranging from an attacker gaining control over the protocol, to the function no longer functioning due to a typo in the destination address. To give users plenty of warning so that they can validate any ownership or other critical parameter changes, these changes should be behind a timelock.
[N-29] Custom errors should be used rather than revert()/require()
Custom errors are available from solidity version 0.8.4. Custom errors are more easily processed in try-catch blocks, and are easier to re-use and maintain.
When an action is triggered based on a user's action, not being able to filter based on who triggered the action makes event processing a lot more cumbersome. Including the msg.sender the events of these types of action will make events much more useful to end users, especially when msg.sender is not tx.origin.
[N-33] Expressions for constant values should use immutable rather than constant
While it does not save gas for some simple binary expressions because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
Consider breaking down these blocks into more manageable units, by splitting things into utility functions, by reducing nesting, and by using early returns
There are 2 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
936function _createLegInAMM(
937 IUniswapV3Pool _univ3pool,
938uint256_tokenId,
939uint256_leg,
940uint256_liquidityChunk,
941bool_isBurn942 ) internalreturns (int256_moved, int256_itmAmounts, int256_totalCollected) {
943uint256 _tokenType = TokenId.tokenType(_tokenId, _leg);
944// unique key to identify the liquidity chunk in this uniswap pool945bytes32 positionKey =keccak256(
946abi.encodePacked(
947address(_univ3pool),
948msg.sender,
949 _tokenType,
950 _liquidityChunk.tickLower(),
951 _liquidityChunk.tickUpper()
952 )
953 );
954955// update our internal bookkeeping of how much liquidity we have deployed in the AMM956// for example: if this _leg is short, we add liquidity to the amm, make sure to add that to our tracking957uint128 updatedLiquidity;
958uint256 isLong = TokenId.isLong(_tokenId, _leg);
959uint256 currentLiquidity = s_accountLiquidity[positionKey]; //cache960961unchecked {
962// did we have liquidity already deployed in Uniswap for this chunk range from some past mint?963964// s_accountLiquidity is a LeftRight. The right slot represents the liquidity currently sold (added) in the AMM owned by the user965// the left slot represents the amount of liquidity currently bought (removed) that has been removed from the AMM - the user owes it to a seller966// the reason why it is called "removedLiquidity" is because long options are created by removing -ie.short selling LP positions967uint128 startingLiquidity = currentLiquidity.rightSlot();
968uint128 removedLiquidity = currentLiquidity.leftSlot();
969uint128 chunkLiquidity = _liquidityChunk.liquidity();
970971if (isLong ==0) {
972// selling/short: so move from msg.sender *to* uniswap973// we're minting more liquidity in uniswap: so add the incoming liquidity chunk to the existing liquidity chunk974 updatedLiquidity = startingLiquidity + chunkLiquidity;
975976/// @dev If the isLong flag is 0=short but the position was burnt, then this is closing a long position977/// @dev so the amount of short liquidity should decrease.978if (_isBurn) {
979 removedLiquidity -= chunkLiquidity;
980 }
981 } else {
982// the _leg is long (buying: moving *from* uniswap to msg.sender)983// so we seek to move the incoming liquidity chunk *out* of uniswap - but was there sufficient liquidity sitting in uniswap984// in the first place?985if (startingLiquidity < chunkLiquidity) {
986// the amount we want to move (liquidityChunk.legLiquidity()) out of uniswap is greater than987// what the account that owns the liquidity in uniswap has (startingLiquidity)988// we must ensure that an account can only move its own liquidity out of uniswap989// so we revert in this case990revert Errors.NotEnoughLiquidity();
991 } else {
992// we want to move less than what already sits in uniswap, no problem:993 updatedLiquidity = startingLiquidity - chunkLiquidity;
994 }
995996/// @dev If the isLong flag is 1=long and the position is minted, then this is opening a long position997/// @dev so the amount of short liquidity should increase.998if (!_isBurn) {
999 removedLiquidity += chunkLiquidity;
1000 }
1001 }
10021003// update the starting liquidity for this position for next time around1004 s_accountLiquidity[positionKey] =uint256(0).toLeftSlot(removedLiquidity).toRightSlot(
1005 updatedLiquidity
1006 );
1007 }
10081009// track how much liquidity we need to collect from uniswap1010// add the fees that accumulated in uniswap within the liquidityChunk:1011 {
1012/** if the position is NOT long (selling a put or a call), then _mintLiquidity to move liquidity1013 from the msg.sender to the uniswap v3 pool:1014 Selling(isLong=0): Mint chunk of liquidity in Uniswap (defined by upper tick, lower tick, and amount)1015 ┌─────────────────────────────────┐1016 ▲ ┌▼┐ liquidityChunk │1017 │ ┌──┴─┴──┐ ┌───┴──┐1018 │ │ │ │ │1019 └──┴───────┴──► └──────┘1020 Uniswap v3 msg.sender1021 1022 else: the position is long (buying a put or a call), then _burnLiquidity to remove liquidity from univ31023 Buying(isLong=1): Burn in Uniswap1024 ┌─────────────────┐1025 ▲ ┌┼┐ │1026 │ ┌──┴─┴──┐ ┌───▼──┐1027 │ │ │ │ │1028 └──┴───────┴──► └──────┘1029 Uniswap v3 msg.sender 1030 */1031 _moved = isLong ==01032?_mintLiquidity(_liquidityChunk, _univ3pool)
1033 : _burnLiquidity(_liquidityChunk, _univ3pool); // from msg.sender to Uniswap1034// add the moved liquidity chunk to amount we need to collect from uniswap:10351036// Is this _leg ITM?1037// if tokenType is 1, and we transacted some token0: then this leg is ITM!1038if (_tokenType ==1) {
1039// extract amount moved out of UniswapV3 pool1040 _itmAmounts = _itmAmounts.toRightSlot(_moved.rightSlot());
1041 }
1042// if tokenType is 0, and we transacted some token1: then this leg is ITM1043if (_tokenType ==0) {
1044// Add this in-the-money amount transacted.1045 _itmAmounts = _itmAmounts.toLeftSlot(_moved.leftSlot());
1046 }
1047 }
10481049// if there was liquidity at that tick before the transaction, collect any accumulated fees1050if (currentLiquidity.rightSlot() >0) {
1051 _totalCollected =_collectAndWritePositionData(
1052 _liquidityChunk,
1053 _univ3pool,
1054 currentLiquidity,
1055 positionKey,
1056 _moved,
1057 isLong
1058 );
1059 }
10601061// position has been touched, update s_accountFeesBase with the latest values from the pool.positions1062 s_accountFeesBase[positionKey] =_getFeesBase(
1063 _univ3pool,
1064 updatedLiquidity,
1065 _liquidityChunk
1066 );
1067: }
File: contracts/types/TokenId.sol
463function validate(uint256self) internalpurereturns (uint64) {
464if (self.optionRatio(0) ==0) revert Errors.InvalidTokenIdParameter(1);
465466// loop through the 4 (possible) legs in the tokenId `self`467unchecked {
468for (uint256 i =0; i <4; ++i) {
469if (self.optionRatio(i) ==0) {
470// final leg in this position identified;471// make sure any leg above this are zero as well472// (we don't allow gaps eg having legs 1 and 4 active without 2 and 3 is not allowed)473if ((self >> (64+48* i)) !=0) revert Errors.InvalidTokenIdParameter(1);
474475break; // we are done iterating over potential legs476 }
477// now validate this ith leg in the position:478479// The width cannot be 0; the minimum is 1480if ((self.width(i) ==0)) revert Errors.InvalidTokenIdParameter(5);
481// Strike cannot be MIN_TICK or MAX_TICK482if (
483 (self.strike(i) == Constants.MIN_V3POOL_TICK) ||484 (self.strike(i) == Constants.MAX_V3POOL_TICK)
485 ) revert Errors.InvalidTokenIdParameter(4);
486487// In the following, we check whether the risk partner of this leg is itself488// or another leg in this position.489// Handles case where riskPartner(i) != i ==> leg i has a risk partner that is another leg490uint256 riskPartnerIndex = self.riskPartner(i);
491if (riskPartnerIndex != i) {
492// Ensures that risk partners are mutual493if (self.riskPartner(riskPartnerIndex) != i)
494revert Errors.InvalidTokenIdParameter(3);
495496// Ensures that risk partners have 1) the same asset, and 2) the same ratio497if (
498 (self.asset(riskPartnerIndex) != self.asset(i)) ||499 (self.optionRatio(riskPartnerIndex) != self.optionRatio(i))
500 ) revert Errors.InvalidTokenIdParameter(3);
501502// long/short status of associated legs503uint256 isLong = self.isLong(i);
504uint256 isLongP = self.isLong(riskPartnerIndex);
505506// token type status of associated legs (call/put)507uint256 tokenType = self.tokenType(i);
508uint256 tokenTypeP = self.tokenType(riskPartnerIndex);
509510// if the position is the same i.e both long calls, short put's etc.511// then this is a regular position, not a defined risk position512if ((isLong == isLongP) && (tokenType == tokenTypeP))
513revert Errors.InvalidTokenIdParameter(4);
514515// if the two token long-types and the tokenTypes are both different (one is a short call, the other a long put, e.g.), this is a synthetic position516// A synthetic long or short is more capital efficient than each leg separated because the long+short premia accumulate proportionally517if ((isLong != isLongP) && (tokenType != tokenTypeP))
518revert Errors.InvalidTokenIdParameter(5);
519 }
520 } // end for loop over legs521 }
522523return self.univ3pool();
524: }
[N-35] Inconsistent method of specifying a floating pragma
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
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
There are 4 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
593: //construct the positionKey for the from and to addresses625: //update+store liquidity and fee values between accounts808: //compute the swap amount, set as positive (exact input)959: uint256 currentLiquidity = s_accountLiquidity[positionKey]; //cache
NatSpec must begin with ///, or use the /** ... */ syntax. The compiler doesn't count // or /* ... */ as NatSpec
There are 7 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
359: // @dev pools can be initialized from a Panoptic pool or by calling initializeAMMPool directly, reverting361: // @dev some pools may not be deployable if the poolId has a collision (since we take only 8 bytes)366: // @dev in the unlikely case that there is a collision between the first 8 bytes of two different Uni v3 pools367: // @dev increase the poolId by a pseudo-random number585: // @dev see `contracts/types/LiquidityChunk.sol`823: // @dev note this triggers our swap callback function881: // @dev see `contracts/types/LiquidityChunk.sol`
[N-39] Large or complicated code bases should implement invariant tests
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement invariant fuzzing tests. Invariant 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 invariant fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
[N-40] Libraries should be defined in separate files from their usage
The libraries below should be defined in separate files, so that it's easier for future projects to import them, and to avoid duplication later on if they need to be used elsewhere in the project
[N-43] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, for readability
Well-organized data structures make code reviews easier, which may lead to fewer bugs. Consider combining related mappings into mappings to structs, so it's clear what data is related. The instances below refer to both mappings using the same key in the same function, so the mappings are related.
There are 3 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit combine: s_accountFeesBase,s_accountLiquidity/// @audit combine: s_accountFeesBase,s_accountLiquidity,s_accountPremiumGross,s_accountPremiumOwed/// @audit combine: s_accountPremiumGross,s_accountPremiumOwed72contractSemiFungiblePositionManagerisERC1155, Multicall {
73/*//////////////////////////////////////////////////////////////74 EVENTS75 //////////////////////////////////////////////////////////////*/7677/// @notice Emitted when a UniswapV3Pool is initialized.78: /// @param uniswapPool Address of the underlying Uniswap v3 pool
File: contracts/tokens/ERC1155Minimal.sol
10abstractcontractERC1155 {
11/*//////////////////////////////////////////////////////////////12 EVENTS13 //////////////////////////////////////////////////////////////*/1415/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20: /// @param amount the amount of tokens transferred
File: contracts/SemiFungiblePositionManager.sol
/// @audit Missing '@param uniswapPool'69/// @title Semi-Fungible Position Manager (ERC1155) - a gas-efficient Uniswap V3 position manager.70/// @notice Wraps Uniswap V3 positions with up to 4 legs behind an ERC1155 token.71/// @dev Replaces the NonfungiblePositionManager.sol (ERC721) from Uniswap Labs72contractSemiFungiblePositionManagerisERC1155, Multicall {
73/*//////////////////////////////////////////////////////////////74 EVENTS75 //////////////////////////////////////////////////////////////*/7677/// @notice Emitted when a UniswapV3Pool is initialized.78/// @param uniswapPool Address of the underlying Uniswap v3 pool79: event PoolInitialized(addressindexeduniswapPool);
/// @audit Missing '@param recipient'77/// @notice Emitted when a UniswapV3Pool is initialized.78/// @param uniswapPool Address of the underlying Uniswap v3 pool79event PoolInitialized(addressindexeduniswapPool);
8081/// @notice Emitted when a position is destroyed/burned82/// @dev Recipient is used to track whether it was burned directly by the user or through an option contract83/// @param recipient The address of the user who burned the position84/// @param tokenId The tokenId of the burned position85/// @param positionSize The number of contracts burnt, expressed in terms of the asset86event TokenizedPositionBurnt(
87: addressindexedrecipient,
/// @audit Missing '@param tokenId'78/// @param uniswapPool Address of the underlying Uniswap v3 pool79event PoolInitialized(addressindexeduniswapPool);
8081/// @notice Emitted when a position is destroyed/burned82/// @dev Recipient is used to track whether it was burned directly by the user or through an option contract83/// @param recipient The address of the user who burned the position84/// @param tokenId The tokenId of the burned position85/// @param positionSize The number of contracts burnt, expressed in terms of the asset86event TokenizedPositionBurnt(
87addressindexedrecipient,
88: uint256indexedtokenId,
/// @audit Missing '@param positionSize'79event PoolInitialized(addressindexeduniswapPool);
8081/// @notice Emitted when a position is destroyed/burned82/// @dev Recipient is used to track whether it was burned directly by the user or through an option contract83/// @param recipient The address of the user who burned the position84/// @param tokenId The tokenId of the burned position85/// @param positionSize The number of contracts burnt, expressed in terms of the asset86event TokenizedPositionBurnt(
87addressindexedrecipient,
88uint256indexedtokenId,
89: uint128positionSize/// @audit Missing '@param caller'88uint256indexedtokenId,
89uint128positionSize90 );
9192/// @notice Emitted when a position is created/minted93/// @dev Recipient is used to track whether it was minted directly by the user or through an option contract94/// @param caller the caller who created the position. In 99% of cases `caller` == `recipient`.95/// @param tokenId The tokenId of the minted position96/// @param positionSize The number of contracts minted, expressed in terms of the asset97event TokenizedPositionMinted(
98: addressindexedcaller,
/// @audit Missing '@param tokenId'89uint128positionSize90 );
9192/// @notice Emitted when a position is created/minted93/// @dev Recipient is used to track whether it was minted directly by the user or through an option contract94/// @param caller the caller who created the position. In 99% of cases `caller` == `recipient`.95/// @param tokenId The tokenId of the minted position96/// @param positionSize The number of contracts minted, expressed in terms of the asset97event TokenizedPositionMinted(
98addressindexedcaller,
99: uint256indexedtokenId,
/// @audit Missing '@param positionSize'90 );
9192/// @notice Emitted when a position is created/minted93/// @dev Recipient is used to track whether it was minted directly by the user or through an option contract94/// @param caller the caller who created the position. In 99% of cases `caller` == `recipient`.95/// @param tokenId The tokenId of the minted position96/// @param positionSize The number of contracts minted, expressed in terms of the asset97event TokenizedPositionMinted(
98addressindexedcaller,
99uint256indexedtokenId,
100: uint128positionSize
File: contracts/tokens/ERC1155Minimal.sol
/// @audit Missing '@param operator'12 EVENTS
13//////////////////////////////////////////////////////////////*/1415/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20/// @param amount the amount of tokens transferred21event TransferSingle(
22: addressindexedoperator,
/// @audit Missing '@param from'13//////////////////////////////////////////////////////////////*/1415/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20/// @param amount the amount of tokens transferred21event TransferSingle(
22addressindexedoperator,
23: addressindexedfrom,
/// @audit Missing '@param to'1415/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20/// @param amount the amount of tokens transferred21event TransferSingle(
22addressindexedoperator,
23addressindexedfrom,
24: addressindexedto,
/// @audit Missing '@param id'15/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20/// @param amount the amount of tokens transferred21event TransferSingle(
22addressindexedoperator,
23addressindexedfrom,
24addressindexedto,
25: uint256id,
/// @audit Missing '@param amount'16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20/// @param amount the amount of tokens transferred21event TransferSingle(
22addressindexedoperator,
23addressindexedfrom,
24addressindexedto,
25uint256id,
26: uint256amount/// @audit Missing '@param operator'26uint256amount27 );
2829/// @notice Emitted when multiple tokens are transferred from one user to another30/// @param operator the user who initiated the transfer31/// @param from the user who sent the tokens32/// @param to the user who received the tokens33/// @param ids the ERC1155 token ids34/// @param amounts the amounts of tokens transferred35event TransferBatch(
36: addressindexedoperator,
/// @audit Missing '@param from'27 );
2829/// @notice Emitted when multiple tokens are transferred from one user to another30/// @param operator the user who initiated the transfer31/// @param from the user who sent the tokens32/// @param to the user who received the tokens33/// @param ids the ERC1155 token ids34/// @param amounts the amounts of tokens transferred35event TransferBatch(
36addressindexedoperator,
37: addressindexedfrom,
/// @audit Missing '@param to'2829/// @notice Emitted when multiple tokens are transferred from one user to another30/// @param operator the user who initiated the transfer31/// @param from the user who sent the tokens32/// @param to the user who received the tokens33/// @param ids the ERC1155 token ids34/// @param amounts the amounts of tokens transferred35event TransferBatch(
36addressindexedoperator,
37addressindexedfrom,
38: addressindexedto,
/// @audit Missing '@param ids'29/// @notice Emitted when multiple tokens are transferred from one user to another30/// @param operator the user who initiated the transfer31/// @param from the user who sent the tokens32/// @param to the user who received the tokens33/// @param ids the ERC1155 token ids34/// @param amounts the amounts of tokens transferred35event TransferBatch(
36addressindexedoperator,
37addressindexedfrom,
38addressindexedto,
39: uint256[] ids,
/// @audit Missing '@param amounts'30/// @param operator the user who initiated the transfer31/// @param from the user who sent the tokens32/// @param to the user who received the tokens33/// @param ids the ERC1155 token ids34/// @param amounts the amounts of tokens transferred35event TransferBatch(
36addressindexedoperator,
37addressindexedfrom,
38addressindexedto,
39uint256[] ids,
40: uint256[] amounts/// @audit Missing '@param owner'/// @audit Missing '@param operator'/// @audit Missing '@param approved'34/// @param amounts the amounts of tokens transferred35event TransferBatch(
36addressindexedoperator,
37addressindexedfrom,
38addressindexedto,
39uint256[] ids,
40uint256[] amounts41 );
4243/// @notice Emitted when an operator is approved to transfer all tokens on behalf of a user44: event ApprovalForAll(addressindexedowner, addressindexedoperator, boolapproved);
File: contracts/types/TokenId.sol
/// @audit Missing '@param _poolId'170/// @notice Add the Uniswap v3 Pool pointed to by this option position.171/// @param self the option position Id.172/// @return the tokenId with the Uniswap V3 pool added to it.173: function addUniv3pool(uint256self, uint64_poolId) internalpurereturns (uint256) {
/// @audit Missing '@param _asset'183/// @notice Add the asset basis for this position.184/// @param self the option position Id.185/// @param legIndex the leg index of this position (in {0,1,2,3})186/// @dev occupies the leftmost bit of the optionRatio 4 bits slot187/// @dev The final mod: "% 2" = takes the rightmost bit of the pattern188/// @return the tokenId with numerarire added to the incoming leg index189function addAsset(
190uint256self,
191: uint256_asset,
/// @audit Missing '@param _optionRatio'199/// @notice Add the number of contracts to leg index `legIndex`.200/// @param self the option position Id201/// @param legIndex the leg index of the position (in {0,1,2,3})202/// @dev The final mod: "% 128" = takes the rightmost (2 ** 7 = 128) 7 bits of the pattern.203/// @return the tokenId with optionRatio added to the incoming leg index204function addOptionRatio(
205uint256self,
206: uint256_optionRatio,
/// @audit Missing '@param _tokenType'230/// @notice Add the type of token moved for a given leg (implies a call or put). Either Token0 or Token1.231/// @param self the tokenId in the SFPM representing an option position232/// @param legIndex the leg index of this position (in {0,1,2,3})233/// @return the tokenId with tokenType added to its relevant leg.234function addTokenType(
235uint256self,
236: uint256_tokenType,
/// @audit Missing '@param _riskPartner'244/// @notice Add the associated risk partner of the leg index (generally another leg in the overall position).245/// @param self the tokenId in the SFPM representing an option position246/// @param legIndex the leg index of this position (in {0,1,2,3})247/// @return the tokenId with riskPartner added to its relevant leg.248function addRiskPartner(
249uint256self,
250: uint256_riskPartner,
/// @audit Missing '@param _strike'258/// @notice Add the strike price tick of the nth leg (index `legIndex`).259/// @param self the tokenId in the SFPM representing an option position.260/// @param legIndex the leg index of this position (in {0,1,2,3})261/// @return the tokenId with strike price tick added to its relevant leg262function addStrike(
263uint256self,
264: int24_strike,
/// @audit Missing '@param _width'272/// @notice Add the width of the nth leg (index `legIndex`). This is half the tick-range covered by the leg (tickUpper - tickLower)/2.273/// @param self the tokenId in the SFPM representing an option position.274/// @param legIndex the leg index of this position (in {0,1,2,3})275/// @return the tokenId with width added to its relevant leg276function addWidth(
277uint256self,
278: int24_width,
File: contracts/SemiFungiblePositionManager.sol
/// @audit Missing '@return feesBase'1089/// @notice Compute the feesGrowth * liquidity / 2**128 by reading feeGrowthInside0LastX128 and feeGrowthInside1LastX128 from univ3pool.positions.1090/// @param univ3pool the Uniswap pool.1091/// @param liquidity the total amount of liquidity in the AMM for the specific position1092/// @param liquidityChunk has lower tick, upper tick, and liquidity amount to mint1093function _getFeesBase(
1094 IUniswapV3Pool univ3pool,
1095uint128liquidity,
1096uint256liquidityChunk1097: ) privateviewreturns (int256feesBase) {
File: contracts/types/TokenId.sol
/// @audit Missing '@return '99/// @notice Get the number of contracts per leg.100/// @param self the option position Id.101/// @param legIndex the leg index of this position (in {0,1,2,3})102/// @dev The final mod: "% 2**7" = takes the rightmost (2 ** 7 = 128) 7 bits of the pattern.103: function optionRatio(uint256self, uint256legIndex) internalpurereturns (uint256) {
/// @audit Missing '@return '322/// @notice Flip all the `isLong` positions in the legs in the `tokenId` option position.323/// @dev uses XOR on existing isLong bits.324/// @dev useful during burning an option position. So we need to take an existing tokenId but now burn it.325/// The way to do this is to simply flip it to a short instead.326/// @param self the tokenId in the SFPM representing an option position.327: function flipToBurnToken(uint256self) internalpurereturns (uint256) {
[N-53] Not using the named return variables anywhere in the function is confusing
Consider changing the variable to be an unnamed one, since the variable is never assigned, nor is it returned by name. If the optimizer is not turned on, leaving the code as it is will also waste gas for the stack variable.
[N-54] Polymorphic functions make security audits more time-consuming and error-prone
The instances below point to one of two functions with the same name. Consider naming each function differently, in order to make code navigation and analysis easier.
[N-56] Style guide: Contract does not follow the Solidity style guide's suggested layout ordering
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 2 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit event TokenizedPositionMinted came earlier127: boolinternal constant MINT =false;
[N-58] Style guide: Function ordering does not follow the Solidity style guide
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
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. The solidity style guide recommends a maximumum line length of 120 characters, so the lines below should be split when they reach that length.
There are 162 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
24: // ,. .,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,. ,,25: // ,,,,,,, ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, ,,,,,,26: // .,,,,,,,,,,. ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, ,,,,,,,,,,,27: // .,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,28: // ,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,29: // ,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,30: // ,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,,,, .,,,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,31: // ,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,,, .,,,,,,,,,,,,,32: // ,,,,,,,,,,,,,. .,,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,.33: // ,,,,,,,,,,,,, ,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,, ,,,,,,,,,,,,,34: // ,,,,,,,,,,,,, ,,,,,,,,,,,,,,. ,,,,,,,,,,,,,, ,,,,,,,,,,,,,35: // ,,,,,,,,,,,,, ,,,,,,,,,,,,,, ,,,,,,,,,,,,,, ,,,,,,,,,,,,,36: // ,,,,,,,,,,,,, ,,,,,,,,,,,,, ,,,,,,,,,,,,,, ,,,,,,,,,,,,.37: // .,,,,,,,,,,,, .,,,,,,,,,,,,, ,,,,,,,,,,,,, ,,,,,,,,,,,,38: // ,,,,,,,,,,,, ,,,,,,,,,,,, ,,,,,,,,,,,,, .,,,,,,,,,,,,39: // ,,,,,,,,,,,, ,,,,,,,,,,,, ,,,,,,,,,,,,. ,,,,,,,,,,,,40: // ,,,,,,,,,,,, ,,,,,,,,,,,,. █████████ ███████████ ███████████ ██████ ██████ ,,,,,,,,,,,, ,,,,,,,,,,,,41: // .,,,,,,,,,,,, ,,,,,,,,,,,, ███░░░░░███░░███░░░░░░█░░███░░░░░███░░██████ ██████ .,,,,,,,,,,,, ,,,,,,,,,,,,42: // ,,,,,,,,,,,, ,,,,,,,,,,,, ░███ ░░░ ░███ █ ░ ░███ ░███ ░███░█████░███ ,,,,,,,,,,,, ,,,,,,,,,,,,.43: // ,,,,,,,,,,,, ,,,,,,,,,,,, ░░█████████ ░███████ ░██████████ ░███░░███ ░███ .,,,,,,,,,,, ,,,,,,,,,,,.44: // ,,,,,,,,,,,, ,,,,,,,,,,,, ░░░░░░░░███ ░███░░░█ ░███░░░░░░ ░███ ░░░ ░███ ,,,,,,,,,,,. ,,,,,,,,,,,,45: // ,,,,,,,,,,,, ,,,,,,,,,,,, ███ ░███ ░███ ░ ░███ ░███ ░███ ,,,,,,,,,,,, ,,,,,,,,,,,,46: // ,,,,,,,,,,,, ,,,,,,,,,,,, ░░█████████ █████ █████ █████ █████ ,,,,,,,,,,, ,,,,,,,,,,,,47: // ,,,,,,,,,,,, ,,,,,,,,,,,, ░░░░░░░░░ ░░░░░ ░░░░░ ░░░░░ ░░░░░ ,,,,,,,,,,,, ,,,,,,,,,,,.48: // ,,,,,,,,,,,, .,,,,,,,,,,,. ,,,,,,,,,,,, ,,,,,,,,,,,,49: // .,,,,,,,,,,,, ,,,,,,,,,,,, .,,,,,,,,,,,, ,,,,,,,,,,,,50: // ,,,,,,,,,,,, ,,,,,,,,,,,,, ,,,,,,,,,,,, ,,,,,,,,,,,,51: // ,,,,,,,,,,,,. ,,,,,,,,,,,,. ,,,,,,,,,,,,. ,,,,,,,,,,,,52: // ,,,,,,,,,,,, ,,,,,,,,,,,,, ,,,,,,,,,,,,, .,,,,,,,,,,,,53: // ,,,,,,,,,,,, ,,,,,,,,,,,,, ,,,,,,,,,,,,, .,,,,,,,,,,,,54: // .,,,,,,,,,,,, ,,,,,,,,,,,,, ,,,,,,,,,,,,,. ,,,,,,,,,,,,55: // ,,,,,,,,,,,,, ,,,,,,,,,,,,,, .,,,,,,,,,,,,,. ,,,,,,,,,,,,56: // ,,,,,,,,,,,,, .,,,,,,,,,,,,,, .,,,,,,,,,,,,,, .,,,,,,,,,,,,57: // ,,,,,,,,,,,,, ,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,. ,,,,,,,,,,,,,.58: // ,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,, .,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,59: // .,,,,,,,,,,,,, ,,,,,,,,,,,,,,,,, .,,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,60: // ,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,61: // ,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,,,,,,,, .,,,,,,,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,62: // ,,,,,,,,,,,,,,, .,,,,,,,,,,,,,,,,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, .,,,,,,,,,,,,,,.63: // ,,,,,,,,,,,,,,. ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, ,,,,,,,,,,,,,,,64: // ,,,,,,,,,, ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, .,,,,,,,,,,65: // ,,,,,. ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, ,,,,,,131: // Similar to vega in options because the liquidity utilization is somewhat reflective of the implied volatility (IV),134: // The effect of vegoid on the long premium multiplier can be explored here: https://www.desmos.com/calculator/mdeqob2m04175: /// @dev mapping that stores the liquidity data of keccak256(abi.encodePacked(address poolAddress, address owner, int24 tickLower, int24 tickUpper))176: // liquidityData is a LeftRight. The right slot represents the liquidity currently sold (added) in the AMM owned by the user177: // the left slot represents the amount of liquidity currently bought (removed) that has been removed from the AMM - the user owes it to a seller178: // the reason why it is called "removedLiquidity" is because long options are created by removed liquidity -ie. short selling LP positions292: /// @dev mapping that stores a LeftRight packing of feesBase of keccak256(abi.encodePacked(address poolAddress, address owner, int24 tickLower, int24 tickUpper))293: /// @dev Base fees is stored as int128((feeGrowthInside0LastX128 * liquidity) / 2**128), which allows us to store the accumulated fees as int128 instead of uint256295: /// feesBase represents the baseline fees collected by the position last time it was updated - this is recalculated every time the position is collected from with the new value392: // we need this because enabling `memoryguard` and therefore StackLimitEvader increases the size of the contract significantly beyond the size limit471: /// @param slippageTickLimitLow The lower price slippage limit when minting an ITM position (set to larger than slippageTickLimitHigh for swapping when minting)472: /// @param slippageTickLimitHigh The higher slippage limit when minting an ITM position (set to lower than slippageTickLimitLow for swapping when minting)473: /// @return totalCollected A LeftRight encoded word containing the total amount of token0 and token1 collected as fees474: /// @return totalSwapped A LeftRight encoded word containing the total amount of token0 and token1 swapped if minting ITM505: /// @param slippageTickLimitLow The lower price slippage limit when minting an ITM position (set to larger than slippageTickLimitHigh for swapping when minting)506: /// @param slippageTickLimitHigh The higher slippage limit when minting an ITM position (set to lower than slippageTickLimitLow for swapping when minting)507: /// @return totalCollected A LeftRight encoded word containing the total amount of token0 and token1 collected as fees508: /// @return totalSwapped A LeftRight encoded word containing the total amount of token0 and token1 swapped if minting ITM573: /// @dev token transfers are only allowed if you transfer your entire balance of a given tokenId and the recipient has none584: // for this leg index: extract the liquidity chunk: a 256bit word containing the liquidity amount and upper/lower tick641: /// @notice Helper that checks the proposed option position and size and forwards the minting and potential swapping tasks.646: /// @notice and then forwards the minting/burning/swapping to another internal helper functions which perform the AMM pool actions.705: // if the in-the-money amount is not zero (i.e. positions were minted ITM) and the user did provide tick limits LOW > HIGH, then swap necessary amounts718: /// @notice When a position is minted or burnt in-the-money (ITM) we are *not* 100% token0 or 100% token1: we have a mix of both tokens.719: /// @notice The swapping for ITM options is needed because only one of the tokens are "borrowed" by a user to create the position.737: /// If we take token0 as an example, we deploy it to the AMM pool and *then* swap to get the right mix of token0 and token1739: /// It that position is burnt, then we remove a mix of the two tokens and swap one of them so that the user receives only one.771: // note: upstream users of this function such as the Panoptic Pool should ensure users always compensate for the ITM amount delta772: // the netting swap is not perfectly accurate, and it is possible for swaps to run out of liquidity, so we do not want to rely on it778: // note: negative ITM amounts denote a surplus of tokens (burning liquidity), while positive amounts denote a shortage of tokens (minting liquidity)780: // we do this by flipping the signs on the token1 ITM amount converting+deducting it against the token0 ITM amount875: // We loop in reverse order if burning a position so that any dependent long liquidity is returned to the pool first,880: // for this _leg index: extract the liquidity chunk: a 256bit word containing the liquidity amount and upper/lower tick898: // increment accumulators of the upper bound on tokens contained across all legs of the position at any given tick914: // Ensure upper bound on amount of tokens contained across all legs of the position on any given tick does not exceed a maximum of (2**127-1).915: // This is the maximum value of the `int128` type we frequently use to hold token amounts, so a given position's size should be guaranteed to964: // s_accountLiquidity is a LeftRight. The right slot represents the liquidity currently sold (added) in the AMM owned by the user965: // the left slot represents the amount of liquidity currently bought (removed) that has been removed from the AMM - the user owes it to a seller966: // the reason why it is called "removedLiquidity" is because long options are created by removing -ie.short selling LP positions973: // we're minting more liquidity in uniswap: so add the incoming liquidity chunk to the existing liquidity chunk983: // so we seek to move the incoming liquidity chunk *out* of uniswap - but was there sufficient liquidity sitting in uniswap1089: /// @notice Compute the feesGrowth * liquidity / 2**128 by reading feeGrowthInside0LastX128 and feeGrowthInside1LastX128 from univ3pool.positions.1114: /// @dev here we're converting the value to an int128 even though all values (feeGrowth, liquidity, Q128) are strictly positive.1115: /// That's because of the way feeGrowthInside works in Uniswap v3, where it can underflow when stored for the first time.1116: /// This is not a problem in Uniswap v3 because the fees are always calculated by taking the difference of the feeGrowths,1118: /// So by using int128 instead of uint128, we remove the need to handle extremely large underflows and simply allow it to be negative1164: /// @notice Burn a chunk of liquidity (`liquidityChunk`) in the Uniswap v3 pool and send to msg.sender; return the amount moved.1192: /// @notice Helper to collect amounts between msg.sender and Uniswap and also to update the Uniswap fees collected to date from the AMM.1195: /// @param currentLiquidity the existing liquidity msg.sender owns in the AMM for this chunk before the SFPM was called1199: /// @return collectedOut the incoming totalCollected with potentially whatever is collected in this function added to it1219: // Collect only if there was existing startingLiquidity=liquidities.rightSlot() at that position: collect all fees1253: /// @return deltaPremiumOwed The extra premium (per liquidity X64) to be added to the owed accumulator for token0 (right) and token1 (right)1254: /// @return deltaPremiumGross The extra premium (per liquidity X64) to be added to the gross accumulator for token0 (right) and token1 (right)1264: // explains how we get from the premium per liquidity (calculated here) to the total premia collected and the multiplier1342: /// @return accountLiquidities The amount of liquidity that has been shorted/added to the Uniswap contract (removedLiquidity:netLiquidity -> rightSlot:leftSlot)1351: /// @dev tokenType input here is the asset of the positions minted, this avoids put liquidity to be used for call, and vice-versa1357: /// @notice Return the premium associated with a given position, where Premium is an accumulator of feeGrowth for the touched position.1358: /// @dev Computes s_accountPremium{isLong ? Owed : Gross}[keccak256(abi.encodePacked(univ3pool, owner, tokenType, tickLower, tickUpper))]1359: /// @dev if an atTick parameter is provided that is different from type(int24).max, then it will update the premium up to the current1360: /// @dev block at the provided atTick value. We do this because this may be called immediately after the Uni v3 pool has been touched1367: /// @param atTick The current tick. Set atTick < type(int24).max = 8388608 to get latest premium up to the current block1369: /// @return premiumToken0 The amount of premium (per liquidity X64) for token0 = sum (feeGrowthLast0X128) over every block where the position has been touched1370: /// @return premiumToken1 The amount of premium (per liquidity X64) for token1 = sum (feeGrowthLast0X128) over every block where the position has been touched1389: // Compute the premium up to the current block (ie. after last touch until now). Do not proceed if atTick == type(int24).max = 83886081399: // how much fees have been accumulated within the liquidity chunk since last time we updated this chunk?1401: // currentFeesGrowth (calculated from FeesCalc.calculateAMMSwapFeesLiquidityChunk) is (ammFeesCollectedPerLiquidity * liquidityChunk.liquidity())1402: // oldFeesGrowth is the last stored update of fee growth within the position range in the past (feeGrowthRange*liquidityChunk.liquidity()) (s_accountFeesBase[positionKey])
File: contracts/libraries/Errors.sol
16: /// @notice A mint or swap callback was attempted from an address that did not match the canonical Uniswap V3 pool with the claimed features
File: contracts/libraries/FeesCalc.sol
14: /// @notice Some options positions involve moving liquidity chunks to the AMM/Uniswap. Those chunks can then earn AMM swap fees.53: /// @return feesEachToken the fees collected from the AMM for each token (LeftRight-packed) with token0 in the right slot and token1 in the left slot96: // lowerOut0: For token0: fee growth per unit of liquidity on the _other_ side of tickLower (relative to currentTick)
File: contracts/libraries/Math.sol
34: /// @dev Implemented using Uniswap's "incorrect" constants. Supplying commented-out real values for an accurate calculation180: /// @notice Calculates floor(a×b÷denominator) with full precision. Throws if result overflows a uint256 or denominator == 0.282: /// @notice Calculates floor(a×b÷2^64) with full precision. Throws if result overflows a uint256 or denominator == 0.329: // Divide [prod1 prod0] by the factors of two (note that this is just 2**96 since the denominator is a power of 2 itself)344: /// @notice Calculates floor(a×b÷2^96) with full precision. Throws if result overflows a uint256 or denominator == 0.391: // Divide [prod1 prod0] by the factors of two (note that this is just 2**96 since the denominator is a power of 2 itself)406: /// @notice Calculates floor(a×b÷2^128) with full precision. Throws if result overflows a uint256 or denominator == 0.453: // Divide [prod1 prod0] by the factors of two (note that this is just 2**128 since the denominator is a power of 2 itself)468: /// @notice Calculates floor(a×b÷2^192) with full precision. Throws if result overflows a uint256 or denominator == 0.515: // Divide [prod1 prod0] by the factors of two (note that this is just 2**96 since the denominator is a power of 2 itself)
File: contracts/libraries/PanopticMath.sol
28: /// the 64 bits are the 64 *last* (most significant) bits - and thus corresponds to the *first* 16 hex characters (reading left to right)47: /// @return finalPoolId the final 64-bit pool id as encoded in the `TokenId` type - composed of the last 64 bits of the address and a hash of the parameters65: /// @notice For a given option position (`tokenId`), leg index within that position (`legIndex`), and `positionSize` get the tick range spanned and its81: /// @return liquidityChunk a uint256 bit-packed (see `LiquidityChunk.sol`) with `tickLower`, `tickUpper`, and `liquidity`104: // Because Uni v3 chooses token0 and token1 from the alphanumeric order, there is no consistency as to whether token0 is105: // stablecoin, ETH, or an ERC20. Some pools may want ETH to be the asset (e.g. ETH-DAI) and some may wish the stablecoin to109: // To solve this, we encode the asset value in tokenId. This parameter specifies which of token0 or token1 is the140: /// @notice Convert an amount of token0 into an amount of token1 given the sqrtPriceX96 in a Uniswap pool defined as sqrt(1/0)*2^96.163: /// @notice Convert an amount of token0 into an amount of token1 given the sqrtPriceX96 in a Uniswap pool defined as sqrt(1/0)*2^96.
File: contracts/libraries/SafeTransferLib.sol
20: // Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here.
File: contracts/multicall/Multicall.sol
22: // Other solutions will do work to differentiate the revert reasons and provide paranthetical information24: // NOTE: memory-safe because it reads from memory already allocated by solidity (the bytes memory result)
File: contracts/types/LeftRight.sol
12: /// @notice higher-order bits are cut off. For example: uint32 a = 0x12345678; uint16 b = uint16(a); // b will be 0x5678 now37: /// @dev Typically, the slot is already clear when writing to it, but if it is not, the bits will be added to the existing bits101: /// @dev Typically, the slot is already clear when writing to it, but if it is not, the bits will be added to the existing bits
File: contracts/types/LiquidityChunk.sol
7: /// @title A Panoptic Liquidity Chunk. Tracks Tick Range and Liquidity Information for a "chunk." Used to track movement of chunks.11: /// A liquidity chunk is an amount of `liquidity` (an amount of WETH, e.g.) deployed between two ticks: `tickLower` and `tickUpper`
File: contracts/types/TokenId.sol
13: /// @notice our terminology: "leg n" or "nth leg" (in {1,2,3,4}) corresponds to "leg index n-1" or `legIndex` (in {0,1,2,3})21: /// (1) univ3pool 64bits 0bits : first 8 bytes of the Uniswap v3 pool address (first 64 bits; little-endian), plus a pseudorandom number in the event of a collision47: /// - if a leg is active (e.g. leg 1) there can be no gaps in other legs meaning: if leg 1 is active then leg 3 cannot be active if leg 2 is inactive.51: /// We also refer to the legs via their index, so leg number 2 has leg index 1 (legIndex) (counting from zero), and in general leg number N has leg index N-1.52: /// - the underlying strike price of the 2nd leg (leg index = 1) in this option position starts at bit index (64 + 12 + 48 * (leg index=1))=12362: // This mask contains zero bits where the poolId is. It is used via & to strip the poolId section from a number, leaving the rest.130: /// @notice that returning the riskPartner for any leg is 0 by default, this does not necessarily imply that token 1 (index 0)131: /// @notice is the risk partner of that leg. We are assuming here that the position has been validated before this and that132: /// @notice the risk partner of any leg always makes sense in this way. A leg btw. does not need to have a risk partner.133: /// @notice the point here is that this function is very low level and must be used with utmost care because it comes down155: /// @notice Get the width of the nth leg (index `legIndex`). This is half the tick-range covered by the leg (tickUpper - tickLower)/2.272: /// @notice Add the width of the nth leg (index `legIndex`). This is half the tick-range covered by the leg (tickUpper - tickLower)/2.330: // We copy the logic from the countLegs function, using it here adds 5K to the contract size with IR for some reason336: // Since only the option ratios remain, we can be sure that no bits above the start of the inactive legs will be 1351: // i.e the whole mask is used to flip all legs with 4 legs, but only the first leg is flipped with 1 leg so we shift by 3 legs352: // We also clear the poolId area of the mask to ensure the bits that are shifted right into the area don't flip and cause issues383: // The max/min ticks that can be initialized are the closest multiple of tickSpacing to the actual max/min tick abs()=887272408: /// @dev ASSUMPTION: For any leg, the option ratio is always > 0 (the leg always has a number of contracts associated with it).416: // Since only the option ratios remain, we can be sure that no bits above the start of the inactive legs will be 1515: // if the two token long-types and the tokenTypes are both different (one is a short call, the other a long put, e.g.), this is a synthetic position516: // A synthetic long or short is more capital efficient than each leg separated because the long+short premia accumulate proportionally
[N-60] Style guide: Modifier names should use lowerCamelCase
According to the Solidity style guide modifier names should be in mixedCase (lowerCamelCase)
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
306modifier ReentrancyLock(uint64poolId) {
307// check if the pool is already locked308// init lock if not309beginReentrancyLock(poolId);
310311// execute function312_;313314// remove lock315endReentrancyLock(poolId);
316: }
File: contracts/SemiFungiblePositionManager.sol
/// @audit postion1069: /// @notice caches/stores the accumulated premia values for the specified postion./// @audit seperated1267: // premia, and is only seperated to simplify the calculation
File: contracts/multicall/Multicall.sol
/// @audit paranthetical22: // Other solutions will do work to differentiate the revert reasons and provide paranthetical information
File: contracts/types/LeftRight.sol
/// @audit explictily144: // adding leftRight packed uint128's is same as just adding the values explictily/// @audit occured150: // then an overflow has occured
[N-65] Unsafe conversion from unsigned to signed values
Solidity follows two's complement rules for its integers, meaning that the most significant bit for signed integers is used to denote the sign, and converting between the two requires inverting all of the bits and adding one. Because of this, casting an unsigned integer to a signed one may result in a change of the sign and or magnitude of the value. For example, int8(type(uint8).max) is not equal to type(int8).max, but is equal to -1. type(uint8).max in binary is 11111111, which if cast to a signed value, means the first binary 1 indicates a negative value, and the binary 1s, invert to all zeroes, and when one is added, it becomes one, but negative, and therefore the decimal value of binary 11111111 is -1.
Note that there may be cases where an error superficially appears to be used, but this is only because there are multiple definitions of the error in different files. In such cases, the error definition should be moved into a separate file. The instances below are the unused definitions.
Note that there may be cases where a variable appears to be used, but this is only because there are multiple definitions of the varible in different files. In such cases, the variable definition should be moved into a separate file. The instances below are the unused variables.
Starting with Solidity version 0.8.8, using the override keyword when the function solely overrides an interface function, and the function doesn't exist in multiple base contracts, is unnecessary.
The default value for variables is zero, so initializing them to zero is superfluous.
There are 7 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
550: for (uint256 i =0; i < ids.length; ) {
583: for (uint256 leg =0; leg < numLegs; ) {
860: for (uint256 leg =0; leg < numLegs; ) {
The general rule is valid, but the instances below are invalid
There are 1 instance(s) of this issue:
File: contracts/multicall/Multicall.sol
12function multicall(bytes[] calldatadata) publicpayablereturns (bytes[] memoryresults) {
13 results =newbytes[](data.length);
14for (uint256 i =0; i < data.length; ) {
15 (boolsuccess, bytesmemoryresult) =address(this).delegatecall(data[i]);
1617if (!success) {
18// Bubble up the revert reason19// The bytes type is ABI encoded as a length-prefixed byte array20// So we simply need to add 32 to the pointer to get the start of the data21// And then revert with the size loaded from the first 32 bytes22// Other solutions will do work to differentiate the revert reasons and provide paranthetical information23// However, we have chosen to simply replicate the the normal behavior of the call24// NOTE: memory-safe because it reads from memory already allocated by solidity (the bytes memory result)25assembly ("memory-safe") {
26revert(add(result, 32), mload(result))
27 }
28 }
2930 results[i] = result;
3132unchecked {
33++i;
34 }
35 }
36: }
[D-11]safeTransfer function does not check for contract existence
The examples below are either not token transfers, or are making high-level transfer()/transferFrom() calls (which check for contract existence), or are from a library that checks for contract existence.
File: contracts/multicall/Multicall.sol
8abstractcontractMulticall {
9/// @notice Performs multiple calls on the inheritor in a single transaction, and returns the data from each call.10/// @param data The calldata for each call11: /// @return results The data returned by each call
File: contracts/tokens/ERC1155Minimal.sol
10abstractcontractERC1155 {
11/*//////////////////////////////////////////////////////////////12 EVENTS13 //////////////////////////////////////////////////////////////*/1415/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20: /// @param amount the amount of tokens transferred
File: contracts/libraries/Math.sol
303assembly ("memory-safe") {
304// Right shift by n is equivalent and 2 gas cheaper than division by 2^n305 result :=shr(64, prod0)
306: }
330assembly ("memory-safe") {
331// Right shift by n is equivalent and 2 gas cheaper than division by 2^n332 prod0 :=shr(64, prod0)
333: }
365assembly ("memory-safe") {
366// Right shift by n is equivalent and 2 gas cheaper than division by 2^n367 result :=shr(96, prod0)
368: }
392assembly ("memory-safe") {
393// Right shift by n is equivalent and 2 gas cheaper than division by 2^n394 prod0 :=shr(96, prod0)
395: }
427assembly ("memory-safe") {
428// Right shift by n is equivalent and 2 gas cheaper than division by 2^n429 result :=shr(128, prod0)
430: }
454assembly ("memory-safe") {
455// Right shift by n is equivalent and 2 gas cheaper than division by 2^n456 prod0 :=shr(128, prod0)
457: }
489assembly ("memory-safe") {
490// Right shift by n is equivalent and 2 gas cheaper than division by 2^n491 result :=shr(192, prod0)
492: }
516assembly ("memory-safe") {
517// Right shift by n is equivalent and 2 gas cheaper than division by 2^n518 prod0 :=shr(192, prod0)
519: }
File: contracts/libraries/SafeTransferLib.sol
19assembly ("memory-safe") {
20// Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here.21let p :=mload(0x40)
2223// Write the abi-encoded calldata into memory, beginning with the function selector.24mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
25mstore(add(4, p), from) // Append the "from" argument.26mstore(add(36, p), to) // Append the "to" argument.27mstore(add(68, p), amount) // Append the "amount" argument.2829 success :=and(
30// Set success to whether the call reverted, if not we check it either31// returned exactly 1 (can't just be non-zero data), or had no return data.32or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
33// We use 100 because that's the total length of our calldata (4 + 32 * 3)34// Counterintuitively, this call() must be positioned after the or() in the35// surrounding and() because and() evaluates its arguments from right to left.36call(gas(), token, 0, p, 100, 0, 32)
37 )
38: }
[D-18]Assembly: Check msg.sender using xor and the scratch space
These are not msg.sender checks where assembly can be used. If calling a function, the check should be flagged in the function, since assembly cannot involve solidity function calls, especially external ones
There are 4 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
417: if (amount0Owed >0)
424: if (amount1Owed >0)
File: contracts/tokens/ERC1155Minimal.sol
97: if (!(msg.sender== from || isApprovedForAll[from][msg.sender])) revertNotAuthorized();
135: if (!(msg.sender== from || isApprovedForAll[from][msg.sender])) revertNotAuthorized();
[D-19]Assembly: Checks for address(0x0) are more efficient in assembly
The gas savings only applies when the variable being checked is smaller than a full word, and for anything besides an address, including the higher-order bits will introduce bugs.
The titles below correspond to issues submitted by various bots, where the submitting bot solely submitted invalid findings (i.e. the submitter didn't filter the results of the rule), so they should be given extra scrutiny:
Max allowance is not compatible with all tokens - internal approval for the contract's own balance, so the rule is pointing to the support for max allowance
increase/decrease allowance should be used instead of approve - this is an internal approval function
Must approve or increase allowance first - the rule is flagging all transferFrom() calls, without approval logic
Contract existence is not checked before low level call - reading calldata, not making an external call
Empty function blocks - the bot's removed the extensive comment documentation in the 'code blocks' it shows for these virtual functions used to allow child contracts to implement functionality, or are constructors
Utility contracts can be made into libraries - all provided examples are invalid
Address values should be used through variables rather than used as literals - none of the examples are of addresses
Employ Explicit Casting to Bytes or Bytes32 for Enhanced Code Clarity and Meaning - the large majority of the examples are of multiple arguments, not just one
Some if-statement can be converted to a ternary - you can't use a ternary when only one of the branches is a return
Addresses shouldn't be hard-coded - none of these are addresses
State variables used within a function more than once should be cached to save gas - none of these are state variables
Use storage instead of memory for structs/arrays - these all are array call arguments, not arrays copied from storage
Use bitmap to save gas - none of these are examples where bitmaps can be used
Consider merging sequential for loops - the examples cannot be merged
Emitting storage values instead of the memory one. - this is a gas finding, not a Low one
selfbalance() is cheaper than address(this).balance - some bots submit the issue twice (under the heading Use assembly when getting a contract's balance of ETH)
Imports could be organized more systematically - a lot of bots are blindly checking for interfaces not coming first. That is not the only way of organizing imports, and most projects are doing it in a systematic, valid, way
Unused * definition - some bots are reporting false positives for these rules. Check that it isn't used, or that if it's used, that there are two definitions, with one being unused
internal functions not called by the contract should be removed - some bots are reporting false positives when the function is called by a child contract, rather than the defining contract
Change public to external for functions that are not called internally - some bots are reporting false positives when the function is called by a child contract, rather than the defining contract
Avoid contract existence checks by using low level calls - at least one bot isn't checking that the version is prior to 0.8.10
For Operations that will not overflow, you could use unchecked - at least one bot is flagging every single line, which has nothing to do with using unchecked
Some of these have been raised as invalid in multiple contests, and the bot owners have not fixed them. Without penalties, they're unlikely to make any changes
File: contracts/multicall/Multicall.sol
8abstractcontractMulticall {
9/// @notice Performs multiple calls on the inheritor in a single transaction, and returns the data from each call.10/// @param data The calldata for each call11: /// @return results The data returned by each call
[D-25]Contract implements a payable multicall and has payable functions
The general rule is valid, but the instances below are invalid
There are 1 instance(s) of this issue:
File: contracts/multicall/Multicall.sol
/// @audit multicall8abstractcontractMulticall {
9/// @notice Performs multiple calls on the inheritor in a single transaction, and returns the data from each call.10/// @param data The calldata for each call11: /// @return results The data returned by each call
These are library functions, so they cannot use an interface
There are 1 instance(s) of this issue:
File: contracts/libraries/FeesCalc.sol
54function calculateAMMSwapFeesLiquidityChunk(
55 IUniswapV3Pool univ3pool,
56int24currentTick,
57uint128startingLiquidity,
58uint256liquidityChunk59 ) publicviewreturns (int256feesEachToken) {
60// extract the amount of AMM fees collected within the liquidity chunk`61: // note: the fee variables are *per unit of liquidity*; so more "rate" variables
[D-28]Control structures do not follow the Solidity Style Guide
The instances below properly drop down to the next line when the arguments are too long
There are 53 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
406/// @param data Contains the payer address and the pool features required to validate the callback407function uniswapV3MintCallback(
408: uint256amount0Owed,
440/// @param data Contains the payer address and the pool features required to validate the callback441function uniswapV3SwapCallback(
442: int256amount0Delta,
475/// @return newTick the current tick in the pool after all the mints and swaps476function burnTokenizedPosition(
477: uint256tokenId,
509/// @return newTick the current tick in the pool after all the mints and swaps510function mintTokenizedPosition(
511: uint256tokenId,
543/// @param amounts The amounts of each token being transferred544function afterTokenTransfer(
545: addressfrom,
562/// @param amount The amount of the token being transferred563function afterTokenTransfer(
564: addressfrom,
613// Revert if recipient already has that position614if (
615: (s_accountLiquidity[positionKey_to] !=0) ||662/// @return newTick the tick *after* the mint+swap663function _validateAndForwardToAMM(
664: uint256tokenId,
742/// @return totalSwapped the amount swapped in the AMM743function swapInAMM(
744: IUniswapV3Pool univ3pool,
847/// @return itmAmounts the amount of tokens swapped due to legs being in-the-money848function _createPositionInAMM(
849: IUniswapV3Pool univ3pool,
935/// @return _totalCollected the total amount of liquidity collected from Uniswap to msg.sender936function _createLegInAMM(
937: IUniswapV3Pool _univ3pool,
1072/// @param collectedAmounts amount of tokens (token0 and token1) collected from Uniswap1073function _updateStoredPremia(
1074: bytes32positionKey,
1092/// @param liquidityChunk has lower tick, upper tick, and liquidity amount to mint1093function _getFeesBase(
1094: IUniswapV3Pool univ3pool,
1128/// @return movedAmounts how many tokens were moved from msg.sender to Uniswap1129function _mintLiquidity(
1130: uint256liquidityChunk,
1168/// @return movedAmounts how many tokens were moved from Uniswap to msg.sender1169function _burnLiquidity(
1170: uint256liquidityChunk,
1199/// @return collectedOut the incoming totalCollected with potentially whatever is collected in this function added to it1200function _collectAndWritePositionData(
1201: uint256liquidityChunk,
1254/// @return deltaPremiumGross The extra premium (per liquidity X64) to be added to the gross accumulator for token0 (right) and token1 (right)1255function _getPremiaDeltas(
1256: uint256currentLiquidity,
1342/// @return accountLiquidities The amount of liquidity that has been shorted/added to the Uniswap contract (removedLiquidity:netLiquidity -> rightSlot:leftSlot)1343function getAccountLiquidity(
1344: addressuniv3pool,
1370/// @return premiumToken1 The amount of premium (per liquidity X64) for token1 = sum (feeGrowthLast0X128) over every block where the position has been touched1371function getAccountPremium(
1372: addressuniv3pool,
1436/// @return feesBase1 The feesBase of the position for token11437function getAccountFeesBase(
1438: addressuniv3pool,
1456/// @return UniswapV3Pool The unique poolId for that Uni v3 pool1457function getUniswapV3PoolFromId(
1458: uint64poolId
File: contracts/libraries/CallbackLib.sol
27/// @param features The features `sender` claims to contain28function validateCallback(
29: addresssender,
34// then, check against the actual address and verify that the callback came from the real, correct pool35if (
36: address(
File: contracts/libraries/FeesCalc.sol
53/// @return feesEachToken the fees collected from the AMM for each token (LeftRight-packed) with token0 in the right slot and token1 in the left slot54function calculateAMMSwapFeesLiquidityChunk(
55: IUniswapV3Pool univ3pool,
88/// @return feeGrowthInside1X128 the fee growth in the AMM of token189function _getAMMSwapFeesPerLiquidityCollected(
90: IUniswapV3Pool univ3pool,
File: contracts/libraries/Math.sol
100/// @return amount0 The amount of token0101function getAmount0ForLiquidity(
102: uint256liquidityChunk118/// @return amount1 The amount of token1119function getAmount1ForLiquidity(
120: uint256liquidityChunk134/// @return liquidity The calculated amount of liquidity135function getLiquidityForAmount0(
136: uint256liquidityChunk,
153/// @return liquidity The calculated amount of liquidity154function getLiquidityForAmount1(
155: uint256liquidityChunk,
185/// @dev Credit to Remco Bloemen under MIT license https://xn--2-umb.com/21/muldiv186function mulDiv(
187: uint256a,
File: contracts/libraries/PanopticMath.sol
47/// @return finalPoolId the final 64-bit pool id as encoded in the `TokenId` type - composed of the last 64 bits of the address and a hash of the parameters48function getFinalPoolId(
49: uint64basePoolId,
81/// @return liquidityChunk a uint256 bit-packed (see `LiquidityChunk.sol`) with `tickLower`, `tickUpper`, and `liquidity`82function getLiquidityChunk(
83: uint256tokenId,
File: contracts/tokens/ERC1155Minimal.sol
89/// @param data optional data to include in the receive hook90function safeTransferFrom(
91: addressfrom,
110if (to.code.length!=0) {
111if (
112: ERC1155Holder(to).onERC1155Received(msg.sender, from, id, amount, data) !=127/// @param data optional data to include in the receive hook128function safeBatchTransferFrom(
129: addressfrom,
163if (to.code.length!=0) {
164if (
165: ERC1155Holder(to).onERC1155BatchReceived(msg.sender, from, ids, amounts, data) !=177/// @return balances the balances for each user-token pair in the same order as the input178function balanceOfBatch(
179: address[] calldataowners,
222if (to.code.length!=0) {
223if (
224: ERC1155Holder(to).onERC1155Received(msg.sender, address(0), id, amount, "") !=251/// @param amounts the amounts of tokens to transfer252function afterTokenTransfer(
253: addressfrom,
264/// @param amount the amount of tokens to transfer265function afterTokenTransfer(
266: addressfrom,
File: contracts/types/TokenId.sol
188/// @return the tokenId with numerarire added to the incoming leg index189function addAsset(
190: uint256self,
203/// @return the tokenId with optionRatio added to the incoming leg index204function addOptionRatio(
205: uint256self,
219/// @return the tokenId with isLong added to its relevant leg220function addIsLong(
221: uint256self,
233/// @return the tokenId with tokenType added to its relevant leg.234function addTokenType(
235: uint256self,
247/// @return the tokenId with riskPartner added to its relevant leg.248function addRiskPartner(
249: uint256self,
261/// @return the tokenId with strike price tick added to its relevant leg262function addStrike(
263: uint256self,
275/// @return the tokenId with width added to its relevant leg276function addWidth(
277: uint256self,
297/// @return tokenId the tokenId with the leg added298function addLeg(
299: uint256self,
373/// @return legUpperTick the upper tick of the leg/liquidity chunk.374function asTicks(
375: uint256self,
395// These are invalid states, and would revert silently later in `univ3Pool.mint`396if (
397: legLowerTick % tickSpacing !=0||481// Strike cannot be MIN_TICK or MAX_TICK482if (
483: (self.strike(i) == Constants.MIN_V3POOL_TICK) ||496// Ensures that risk partners have 1) the same asset, and 2) the same ratio497if (
498: (self.asset(riskPartnerIndex) != self.asset(i)) ||
Using delete instead of assigning zero/false to state variables does not save any extra gas with the optimizer on (saves 5-8 gas with optimizer completely off), so this finding is invalid, especially since if they were interested in gas savings, they'd have the optimizer enabled. Some bots are also flagging true rather than just false
The general rule is valid, but the instances below are invalid
There are 2 instance(s) of this issue:
File: contracts/types/TokenId.sol
382383// The max/min ticks that can be initialized are the closest multiple of tickSpacing to the actual max/min tick abs()=887272384// Dividing and multiplying by tickSpacing rounds down and forces the tick to be a multiple of tickSpacing385: int24 minTick = (Constants.MIN_V3POOL_TICK / tickSpacing) * tickSpacing;
383// The max/min ticks that can be initialized are the closest multiple of tickSpacing to the actual max/min tick abs()=887272384// Dividing and multiplying by tickSpacing rounds down and forces the tick to be a multiple of tickSpacing385int24 minTick = (Constants.MIN_V3POOL_TICK / tickSpacing) * tickSpacing;
386: int24 maxTick = (Constants.MAX_V3POOL_TICK / tickSpacing) * tickSpacing;
[D-34]Duplicated require()/revert() checks should be refactored to a modifier Or function to save gas
If the compiler inlines the function, there will be no gas savings. If it doesn't, there's extra runtime overhead due to the JUMP instructions. Either way, this suggestion is not helpful.
There are 2 instance(s) of this issue:
File: contracts/tokens/ERC1155Minimal.sol
135: if (!(msg.sender== from || isApprovedForAll[from][msg.sender])) revertNotAuthorized();
File: contracts/SemiFungiblePositionManager.sol
134: // The effect of vegoid on the long premium multiplier can be explored here: https://www.desmos.com/calculator/mdeqob2m041263: // premia spread equations are graphed and documented here: https://www.desmos.com/calculator/mdeqob2m04
[D-41]Inline modifiers that are only used once, to save gas
These modifiers are used more than once
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
306modifier ReentrancyLock(uint64poolId) {
307// check if the pool is already locked308// init lock if not309beginReentrancyLock(poolId);
310311// execute function312_;313314// remove lock315endReentrancyLock(poolId);
316: }
If the caller makes a copy-paste error, the lengths may be mismatched and an operation believed to have been completed may not in fact have been completed (e.g. if the array being iterated over is shorter than the one being indexed into).
[D-43]It is standard for all external and public functions to be override from an interface
According to the Solidity docs, "Starting from Solidity 0.8.8, the override keyword is not required when overriding an interface function, except for the case where the function is defined in multiple bases", so while it may have been a requirement in the past, they're trying to change that. Paired with the advice of making all public and external functions a part of an interface, this finding would end up having all sponsors mark all public/external functions with override, making the keyword meaningless. It's better to use override only when something is actually being overridden.
[D-48]Low level calls with Solidity before 0.8.14 result in an optimiser bug
This assembly block does not call mstore(), so it's not possible to hit the bug here even if there are small future changes, so this doesn't seem low severity.
There are 28 instance(s) of this issue:
File: contracts/libraries/Math.sol
199assembly ("memory-safe") {
200let mm :=mulmod(a, b, not(0))
201 prod0 :=mul(a, b)
202 prod1 :=sub(sub(mm, prod0), lt(mm, prod0))
203: }
208assembly ("memory-safe") {
209 result :=div(prod0, denominator)
210: }
225assembly ("memory-safe") {
226 remainder :=mulmod(a, b, denominator)
227: }
229assembly ("memory-safe") {
230 prod1 :=sub(prod1, gt(remainder, prod0))
231 prod0 :=sub(prod0, remainder)
232: }
239assembly ("memory-safe") {
240 denominator :=div(denominator, twos)
241: }
244assembly ("memory-safe") {
245 prod0 :=div(prod0, twos)
246: }
250assembly ("memory-safe") {
251 twos :=add(div(sub(0, twos), twos), 1)
252: }
295assembly ("memory-safe") {
296let mm :=mulmod(a, b, not(0))
297 prod0 :=mul(a, b)
298 prod1 :=sub(sub(mm, prod0), lt(mm, prod0))
299: }
303assembly ("memory-safe") {
304// Right shift by n is equivalent and 2 gas cheaper than division by 2^n305 result :=shr(64, prod0)
306: }
320assembly ("memory-safe") {
321 remainder :=mulmod(a, b, 0x10000000000000000)
322: }
324assembly ("memory-safe") {
325 prod1 :=sub(prod1, gt(remainder, prod0))
326 prod0 :=sub(prod0, remainder)
327: }
330assembly ("memory-safe") {
331// Right shift by n is equivalent and 2 gas cheaper than division by 2^n332 prod0 :=shr(64, prod0)
333: }
357assembly ("memory-safe") {
358let mm :=mulmod(a, b, not(0))
359 prod0 :=mul(a, b)
360 prod1 :=sub(sub(mm, prod0), lt(mm, prod0))
361: }
365assembly ("memory-safe") {
366// Right shift by n is equivalent and 2 gas cheaper than division by 2^n367 result :=shr(96, prod0)
368: }
382assembly ("memory-safe") {
383 remainder :=mulmod(a, b, 0x1000000000000000000000000)
384: }
386assembly ("memory-safe") {
387 prod1 :=sub(prod1, gt(remainder, prod0))
388 prod0 :=sub(prod0, remainder)
389: }
392assembly ("memory-safe") {
393// Right shift by n is equivalent and 2 gas cheaper than division by 2^n394 prod0 :=shr(96, prod0)
395: }
419assembly ("memory-safe") {
420let mm :=mulmod(a, b, not(0))
421 prod0 :=mul(a, b)
422 prod1 :=sub(sub(mm, prod0), lt(mm, prod0))
423: }
427assembly ("memory-safe") {
428// Right shift by n is equivalent and 2 gas cheaper than division by 2^n429 result :=shr(128, prod0)
430: }
444assembly ("memory-safe") {
445 remainder :=mulmod(a, b, 0x100000000000000000000000000000000)
446: }
448assembly ("memory-safe") {
449 prod1 :=sub(prod1, gt(remainder, prod0))
450 prod0 :=sub(prod0, remainder)
451: }
454assembly ("memory-safe") {
455// Right shift by n is equivalent and 2 gas cheaper than division by 2^n456 prod0 :=shr(128, prod0)
457: }
481assembly ("memory-safe") {
482let mm :=mulmod(a, b, not(0))
483 prod0 :=mul(a, b)
484 prod1 :=sub(sub(mm, prod0), lt(mm, prod0))
485: }
489assembly ("memory-safe") {
490// Right shift by n is equivalent and 2 gas cheaper than division by 2^n491 result :=shr(192, prod0)
492: }
506assembly ("memory-safe") {
507 remainder :=mulmod(a, b, 0x1000000000000000000000000000000000000000000000000)
508: }
510assembly ("memory-safe") {
511 prod1 :=sub(prod1, gt(remainder, prod0))
512 prod0 :=sub(prod0, remainder)
513: }
516assembly ("memory-safe") {
517// Right shift by n is equivalent and 2 gas cheaper than division by 2^n518 prod0 :=shr(192, prod0)
519: }
[D-50]Missing contract-existence checks before low-level calls
The contract or caller exists, or it's a transfer of funds
There are 1 instance(s) of this issue:
File: contracts/multicall/Multicall.sol
9/// @notice Performs multiple calls on the inheritor in a single transaction, and returns the data from each call.10/// @param data The calldata for each call11/// @return results The data returned by each call12function multicall(bytes[] calldatadata) publicpayablereturns (bytes[] memoryresults) {
13 results =newbytes[](data.length);
14for (uint256 i =0; i < data.length; ) {
15 (boolsuccess, bytesmemoryresult) =address(this).delegatecall(data[i]);
1617if (!success) {
18: // Bubble up the revert reason
The bot is just flagging transferFrom() calls without a prior approval. Many projects require you to approve their contract before using it, so this suggestion is not helpful, and certainly is not 'Low' severity, since that's the design and no funds are lost. There is no way for the project to address this issue other than by requiring that the caller send the tokens themselves, which has its own risks.
[D-53]NatSpec: Contract declarations should have @notice tags
The compiler interprets /// or /** comments as this tag if one wasn't explicitly provided
There are 13 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
72contractSemiFungiblePositionManagerisERC1155, Multicall {
73/*//////////////////////////////////////////////////////////////74 EVENTS75 //////////////////////////////////////////////////////////////*/7677/// @notice Emitted when a UniswapV3Pool is initialized.78: /// @param uniswapPool Address of the underlying Uniswap v3 pool
File: contracts/multicall/Multicall.sol
8abstractcontractMulticall {
9/// @notice Performs multiple calls on the inheritor in a single transaction, and returns the data from each call.10/// @param data The calldata for each call11: /// @return results The data returned by each call
File: contracts/tokens/ERC1155Minimal.sol
10abstractcontractERC1155 {
11/*//////////////////////////////////////////////////////////////12 EVENTS13 //////////////////////////////////////////////////////////////*/1415/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20: /// @param amount the amount of tokens transferred
File: contracts/libraries/FeesCalc.sol
54function calculateAMMSwapFeesLiquidityChunk(
55 IUniswapV3Pool univ3pool,
56int24currentTick,
57uint128startingLiquidity,
58uint256liquidityChunk59 ) publicviewreturns (int256feesEachToken) {
60// extract the amount of AMM fees collected within the liquidity chunk`61: // note: the fee variables are *per unit of liquidity*; so more "rate" variables
[D-55]NatSpec: Modifier declarations should have @notice tags
The compiler interprets /// or /** comments as this tag if one wasn't explicitly provided
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
306modifier ReentrancyLock(uint64poolId) {
307// check if the pool is already locked308// init lock if not309beginReentrancyLock(poolId);
310311// execute function312_;313314// remove lock315endReentrancyLock(poolId);
316: }
Low-level assembly calls are usually used to avoid externalities. If these are using assembly to do so, the suggestion to use non-assembly low-level calls is invalid.
There are 1 instance(s) of this issue:
File: contracts/libraries/SafeTransferLib.sol
/// @audit call(): safeTransferFrom()19assembly ("memory-safe") {
20// Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here.21let p :=mload(0x40)
2223// Write the abi-encoded calldata into memory, beginning with the function selector.24mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
25mstore(add(4, p), from) // Append the "from" argument.26mstore(add(36, p), to) // Append the "to" argument.27mstore(add(68, p), amount) // Append the "amount" argument.2829 success :=and(
30// Set success to whether the call reverted, if not we check it either31// returned exactly 1 (can't just be non-zero data), or had no return data.32or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
33// We use 100 because that's the total length of our calldata (4 + 32 * 3)34// Counterintuitively, this call() must be positioned after the or() in the35// surrounding and() because and() evaluates its arguments from right to left.36call(gas(), token, 0, p, 100, 0, 32)
37 )
38: }
[D-60]Not initializing local variables to zero saves gas
This is only true for state variables, and does not save gas for local variables. The examples below are for local variables and therefore do not save gas, and are invalid.
There are 7 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
550: for (uint256 i =0; i < ids.length; ) {
583: for (uint256 leg =0; leg < numLegs; ) {
860: for (uint256 leg =0; leg < numLegs; ) {
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There are 2 instance(s) of this issue:
File: contracts/libraries/Math.sol
170: /// @param toDowncast the uint256 to be downcasted172: function toUint128(uint256toDowncast) internalpurereturns (uint128downcastedInt) {
[D-64]Optimize Gas Usage by Combining Mappings into a Struct
Saves a storage slot for each of the removed mappings, but this is only a deployment gas savings, so if a bot is counting this as anything but zero, it's invalid. In order to save gas, the mappings have to actually be used in the same function/transaction, and that needs to be shown specifically, rather than just flagging all mappings.
There are 8 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit mapping s_poolContext/// @audit mapping s_accountLiquidity/// @audit mapping s_accountPremiumOwed/// @audit mapping s_accountPremiumGross/// @audit mapping s_accountFeesBase/// @audit mapping s_AddrToPoolIdData72contractSemiFungiblePositionManagerisERC1155, Multicall {
73/*//////////////////////////////////////////////////////////////74 EVENTS75 //////////////////////////////////////////////////////////////*/7677/// @notice Emitted when a UniswapV3Pool is initialized.78: /// @param uniswapPool Address of the underlying Uniswap v3 pool
File: contracts/tokens/ERC1155Minimal.sol
/// @audit mapping balanceOf/// @audit mapping isApprovedForAll10abstractcontractERC1155 {
11/*//////////////////////////////////////////////////////////////12 EVENTS13 //////////////////////////////////////////////////////////////*/1415/// @notice Emitted when only a single token is transferred16/// @param operator the user who initiated the transfer17/// @param from the user who sent the tokens18/// @param to the user who received the tokens19/// @param id the ERC1155 token id20: /// @param amount the amount of tokens transferred
[D-67]Public functions not used internally can be marked as external to save gas
After Solidity version 0.6.9 both public and external functions save the same amount of gas, and since these files are >0.6.9, these findings are invalid
[D-75]Storage Write Removal Bug On Conditional Early Termination
In solidity versions 0.8.13 through 0.8.16, there is a bug involving the use of the Yul functions return() and stop(). If those functions aren't called, or if the Solidity version doesn't match, the finding is not low severity.
File: contracts/libraries/SafeTransferLib.sol
19assembly ("memory-safe") {
20// Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here.21let p :=mload(0x40)
2223// Write the abi-encoded calldata into memory, beginning with the function selector.24mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
25mstore(add(4, p), from) // Append the "from" argument.26mstore(add(36, p), to) // Append the "to" argument.27mstore(add(68, p), amount) // Append the "amount" argument.2829 success :=and(
30// Set success to whether the call reverted, if not we check it either31// returned exactly 1 (can't just be non-zero data), or had no return data.32or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
33// We use 100 because that's the total length of our calldata (4 + 32 * 3)34// Counterintuitively, this call() must be positioned after the or() in the35// surrounding and() because and() evaluates its arguments from right to left.36call(gas(), token, 0, p, 100, 0, 32)
37 )
38: }
[D-82]The result of function calls should be cached rather than re-calling the function
The instances below point to the second+ call of the function within a single function
There are 1 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
/// @audit Errors.TransferFailed() on line 617621: if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();
File: contracts/tokens/ERC1155Minimal.sol
97: if (!(msg.sender== from || isApprovedForAll[from][msg.sender])) revertNotAuthorized();
135: if (!(msg.sender== from || isApprovedForAll[from][msg.sender])) revertNotAuthorized();
202 interfaceId ==0x01ffc9a7||// ERC165 Interface ID for ERC165203: interfaceId ==0xd9b67a26; // ERC165 Interface ID for ERC1155
When a type is downcast to a smaller type, the higher order bits are truncated, effectively applying a modulo to the original value. Without any other checks, this wrapping will lead to unexpected behavior and bugs
[D-95]Unused named return variables without optimizer waste gas
Suggestions that only apply when the optimizer is off are not useful to sponsors. Why would they pay for gas optimizations if they don't have the optimizer on, and don't plan to turn it on? Only a small minority have the optimizer off; the majority have it set to more than 200 runs
[D-100]Use @inheritdoc rather than using a non-standard annotation
There are 2 instance(s) of this issue:
File: contracts/SemiFungiblePositionManager.sol
539/// @notice called after batch transfers (NOT mints or burns)540/// @param from The address of the sender541/// @param to The address of the recipient542/// @param ids The tokenIds being transferred543/// @param amounts The amounts of each token being transferred544function afterTokenTransfer(
545addressfrom,
546addressto,
547uint256[] memoryids,
548uint256[] memoryamounts549: ) internaloverride {
558/// @notice called after single transfers (NOT mints or burns)559/// @param from The address of the sender560/// @param to The address of the recipient561/// @param id The tokenId being transferred562/// @param amount The amount of the token being transferred563function afterTokenTransfer(
564addressfrom,
565addressto,
566uint256id,
567uint256amount568: ) internaloverride {
[D-102]Use a single file for all system-wide constants
It's perfectly reasonable to split constants across multiple files, as long as they're appropriately scoped and not duplicated. This is especially true if the constants are members of libraries, which is the case for these constants.
[D-104]Use assembly to emit events, in order to save gas
For these events, there doesn't appear to be more than one word's worth of unindexed args, or the arguments are too large to fit in the scratch space, so findings related to these events are likey invalid and definitely invalid, respectively.
[D-106]Use delete instead of setting mapping/state variable to zero, to save gas
Using delete instead of assigning zero to state variables does not save any extra gas with the optimizer on (saves 5-8 gas with optimizer completely off), so this finding is invalid, especially since if they were interested in gas savings, they'd have the optimizer enabled.
[D-107]Use multiple require() and if statements instead of &&
The suggestion in this rule is not logically equivalent for if-statements unless they're nested, and cannot be done if there's an else-block without spending more gas. It doesn't seem more readable for require()s either
Report was inadvertently truncated. Remainder of report added below:
GitHub : 76,76,77,77
GitHub : 41,89,173
GitHub : 26,33,90,97,151,151,161,162,164,165,179,180,182,183,199,206
GitHub : 114,114,123,123,132
[D-89]
Unusederror
definitionThe error is used in a file that is different from the one in which it is defined
There are 14 instance(s) of this issue:
GitHub : 11,14,17,21,24,30,33,36,39,42,45,49,52,55
[D-90]
Unusedstruct
definitionThese structs are used outside of the defining contract
There are 2 instance(s) of this issue:
GitHub : 12,19
[D-91]
Unused contract variablesThe variable is used outside of the contract in which it is defined
There are 21 instance(s) of this issue:
GitHub : 127,128,135,139,147,152,179,288,290,296
GitHub : 8,11,14,17,20,25
GitHub : 16
GitHub : 60,63,66,68
[D-92]
Unused function parameterThe variable is in fact used, so the instances below are invalid
There are 14 instance(s) of this issue:
GitHub : 187,188,286,286,348,348,410,410,472,472
GitHub : 16,16,16,16
[D-93]
Unused importThese instances are used
There are 33 instance(s) of this issue:
GitHub : 5,6,8,9,11,12,13,14,15,16,17,19,20,21
GitHub : 5,6
GitHub : 5,7,9,10,11
GitHub : 5,6,8
GitHub : 5,7,8,9
GitHub : 5
GitHub : 5
GitHub : 5
GitHub : 5,6
[D-94]
Unused local variableThere are 5 instance(s) of this issue:
GitHub : 224,319,381,443,505
[D-95]
Unused named return variables without optimizer waste gasSuggestions that only apply when the optimizer is off are not useful to sponsors. Why would they pay for gas optimizations if they don't have the optimizer on, and don't plan to turn it on? Only a small minority have the optimizer off; the majority have it set to more than 200 runs
There are 5 instance(s) of this issue:
GitHub : 1457
GitHub : 101,119,135,154
[D-96]
Unusual loop variableThe general rule is valid, but the instances below are invalid
There are 2 instance(s) of this issue:
GitHub : 583,860
[D-97]
Unusual loop variableThese instances all properly use 'i' as the outer for-loop loop variable
There are 4 instance(s) of this issue:
GitHub : 550
GitHub : 14
GitHub : 141,187
[D-98]
Use != 0 instead of > 0 for unsigned integer comparisonOnly valid prior to Solidity version 0.8.13, and only for
require()
statements, and at least one of those is not true for the examples belowThere are 18 instance(s) of this issue:
GitHub : 417,424,452,457,814,811,806,1050,1234,1237
GitHub : 25,40,86
GitHub : 158,153,185,176
GitHub : 55
[D-99]
Use_safeMint
instead of_mint
for ERC721The contract here isn't an ERC721 - it's some other token. Note that ERC1155 defines
_mint()
, not_safeMint()
There are 4 instance(s) of this issue:
GitHub : 407,510,1129
GitHub : 214
[D-100]
Use@inheritdoc
rather than using a non-standard annotationThere are 2 instance(s) of this issue:
GitHub : 539,558
[D-101]
Usebytes.concat()
on bytes instead ofabi.encodePacked()
for clearer semantic meaningThese instances don't use only bytes/strings, so they're invalid
There are 9 instance(s) of this issue:
GitHub : 595,604,946,1105,1353,1381,1446