Platform: Code4rena
Start Date: 01/04/2024
Pot Size: $120,000 USDC
Total HM: 11
Participants: 55
Period: 21 days
Judge: Picodes
Total Solo HM: 6
Id: 354
League: ETH
Rank: 42/55
Findings: 1
Award: $32.96
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: DadeKuma
Also found by: 0xStalin, 0xhacksmithh, 99Crits, Aymen0909, Bauchibred, CodeWasp, Dup1337, IllIllI, John_Femi, K42, KupiaSec, Naresh, Rhaydden, Rolezn, Sathish9098, Topmark, ZanyBonzy, albahaca, bareli, blockchainbuttonmasher, cheatc0d3, codeslide, crc32, d3e4, favelanky, grearlake, hihen, jasonxiale, jesjupyter, lanrebayode77, lirezArAzAvi, lsaudit, mining_mario, oualidpro, pfapostol, radin100, rbserver, sammy, satoshispeedrunner, slvDev, twcctop, zabihullahazadzoi
32.9585 USDC - $32.96
The content section shows only 10 examples, subsequent cases are listed as links.
111 instances over 10 issues
790 instances over 37 issues
<a name="L-01"></a> To the top
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.
- contracts/CollateralTracker.sol
// @audit Some statements does not follow CEI 417 function deposit(uint256 assets, address receiver) external returns (uint256 shares) { ... // @audit Statement out of CEI order 436 s_poolAssets += uint128(assets); ... // @audit Some statements does not follow CEI 477 function mint(uint256 shares, address receiver) external returns (uint256 assets) { ... // @audit Statement out of CEI order 496 s_poolAssets += uint128(assets); ... // @audit Some statements does not follow CEI 995 function takeCommissionAddData( 996 address optionOwner, 997 int128 longAmount, 998 int128 shortAmount, 999 int128 swappedAmount 1000 ) external onlyPanopticPool returns (int256 utilization) { ... // @audit Statement out of CEI order 1031 utilization = _poolUtilization(); ... // @audit Some statements does not follow CEI 1043 function exercise( 1044 address optionOwner, 1045 int128 longAmount, 1046 int128 shortAmount, 1047 int128 swappedAmount, 1048 int128 realizedPremium 1049 ) external onlyPanopticPool returns (int128) { ... // @audit Statement out of CEI order 1085 s_inAMM = uint128(uint256(int256(uint256(s_inAMM)) - (shortAmount - longAmount)));
- contracts/PanopticPool.sol
// @audit Some statements does not follow CEI 338 function assertPriceWithinBounds(uint160 sqrtLowerBound, uint160 sqrtUpperBound) external view { ... // @audit Statement out of CEI order 342 revert Errors.PriceBoundFail();
342 664 693 945 1165..1168 1394 1493
- contracts/SemiFungiblePositionManager.sol
- contracts/libraries/InteractionHelper.sol
- contracts/libraries/Math.sol
- contracts/libraries/PanopticMath.sol
- contracts/libraries/SafeTransferLib.sol
- contracts/tokens/ERC1155Minimal.sol
- contracts/types/LeftRight.sol
</details>- contracts/types/TokenId.sol
<a name="L-02"></a> To the top
The following expressions may underflow, as the subtrahend could be greater than the minuend in case the former is multiplied by a large number.
- contracts/PanopticPool.sol
1952 (int256( 1953 grossPremiumLast.leftSlot() * 1954 totalLiquidityBefore 1955 ) - 1956 int256( 1957 _premiumAccumulatorsByLeg[_leg][1] * 1958 positionLiquidity 1959 )) + int256(legPremia.leftSlot()) * 2 ** 64, ... 1952 (int256( 1953 grossPremiumLast.leftSlot() * 1954 totalLiquidityBefore 1955 ) - 1956 int256( 1957 _premiumAccumulatorsByLeg[_leg][1] * 1958 positionLiquidity 1959 )) + int256(legPremia.leftSlot()) * 2 ** 64, ... 1935 (int256( 1936 grossPremiumLast.rightSlot() * 1937 totalLiquidityBefore 1938 ) - 1939 int256( 1940 _premiumAccumulatorsByLeg[_leg][0] * 1941 positionLiquidity 1942 )) + int256(legPremia.rightSlot() * 2 ** 64), ... 1935 (int256( 1936 grossPremiumLast.rightSlot() * 1937 totalLiquidityBefore 1938 ) - 1939 int256( 1940 _premiumAccumulatorsByLeg[_leg][0] * 1941 positionLiquidity 1942 )) + int256(legPremia.rightSlot() * 2 ** 64),
1952..1959 1952..1959 1935..1942 1935..1942
- contracts/SemiFungiblePositionManager.sol
1388 uint256 numerator = totalLiquidity ** 2 - 1389 totalLiquidity * 1390 removedLiquidity +
- contracts/libraries/Math.sol
418 inv *= 2 - denominator * inv; // inverse mod 2**8 ... 419 inv *= 2 - denominator * inv; // inverse mod 2**16 ... 420 inv *= 2 - denominator * inv; // inverse mod 2**32 ... 421 inv *= 2 - denominator * inv; // inverse mod 2**64 ... 422 inv *= 2 - denominator * inv; // inverse mod 2**128
</details>- contracts/libraries/PanopticMath.sol
<a name="L-03"></a> To the top
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.`
- contracts/CollateralTracker.sol
323 function transfer( 324 address recipient, 325 uint256 amount 326 ) public override(ERC20Minimal) returns (bool) { ... // @audit Function `transfer` doesn't have the `nonReentrant` modifier 333 return ERC20Minimal.transfer(recipient, amount); ... 341 function transferFrom( 342 address from, 343 address to, 344 uint256 amount 345 ) public override(ERC20Minimal) returns (bool) { ... // @audit Function `transferFrom` doesn't have the `nonReentrant` modifier 352 return ERC20Minimal.transferFrom(from, to, amount); ... 417 function deposit(uint256 assets, address receiver) external returns (uint256 shares) { ... // @audit Function `deposit` doesn't have the `nonReentrant` modifier 424 SafeTransferLib.safeTransferFrom( ... 477 function mint(uint256 shares, address receiver) external returns (uint256 assets) { ... // @audit Function `mint` doesn't have the `nonReentrant` modifier 484 SafeTransferLib.safeTransferFrom( ... 531 function withdraw( 532 uint256 assets, 533 address receiver, 534 address owner 535 ) external returns (uint256 shares) { ... // @audit Function `withdraw` doesn't have the `nonReentrant` modifier 556 SafeTransferLib.safeTransferFrom(
- contracts/PanopticFactory.sol
</details>- contracts/SemiFungiblePositionManager.sol
abi.encodePacked with dynamic types inside keccak256<a name="L-04"></a> To the top
abi.encodePacked should not be used with dynamic types when passing the result to a hash function such as keccak256. Use abi.encode instead, which will pad items to 32 bytes, to prevent any hash collisions.
- contracts/PanopticPool.sol
461 bytes32 chunkKey = keccak256( 462 abi.encodePacked( 463 tokenId.strike(leg), 464 tokenId.width(leg), 465 tokenId.tokenType(leg) 466 ) 467 ); ... 1642 bytes32 chunkKey = keccak256( 1643 abi.encodePacked( 1644 tokenId.strike(legIndex), 1645 tokenId.width(legIndex), 1646 tokenId.tokenType(legIndex) 1647 ) 1648 ); ... 1673 bytes32 chunkKey = keccak256( 1674 abi.encodePacked(tokenId.strike(leg), tokenId.width(leg), tokenId.tokenType(leg)) 1675 ); ... 1855 bytes32 chunkKey = keccak256( 1856 abi.encodePacked(tokenId.strike(leg), tokenId.width(leg), tokenId.tokenType(leg)) 1857 );
461..467 1642..1648 1673..1675 1855..1857
- contracts/SemiFungiblePositionManager.sol
611 bytes32 positionKey_from = keccak256( 612 abi.encodePacked( 613 address(univ3pool), 614 from, 615 id.tokenType(leg), 616 liquidityChunk.tickLower(), 617 liquidityChunk.tickUpper() 618 ) 619 ); ... 620 bytes32 positionKey_to = keccak256( 621 abi.encodePacked( 622 address(univ3pool), 623 to, 624 id.tokenType(leg), 625 liquidityChunk.tickLower(), 626 liquidityChunk.tickUpper() 627 ) 628 ); ... 974 bytes32 positionKey = keccak256( 975 abi.encodePacked( 976 address(univ3pool), 977 msg.sender, 978 tokenType, 979 liquidityChunk.tickLower(), 980 liquidityChunk.tickUpper() 981 ) 982 ); ... 1150 keccak256( 1151 abi.encodePacked( 1152 address(this), 1153 liquidityChunk.tickLower(), 1154 liquidityChunk.tickUpper() 1155 ) 1156 ) ... 1431 keccak256(abi.encodePacked(univ3pool, owner, tokenType, tickLower, tickUpper)) ... 1458 bytes32 positionKey = keccak256( 1459 abi.encodePacked(univ3pool, owner, tokenType, tickLower, tickUpper) 1460 );
611..619 620..628 974..982 1150..1156 1431 1458..1460 1544
</details>- contracts/libraries/PanopticMath.sol
<a name="L-05"></a> To the top
Ensure such accounts are protected and consider implementing multi sig to prevent a single point of failure
- contracts/CollateralTracker.sol
// @audit Modifiers that checks `msg.sender`: `onlyOwner`, `onlyPanopticPool`, `onlyRole` 864 function delegate( 865 address delegator, 866 address delegatee, 867 uint256 assets 868 ) external onlyPanopticPool { ... // @audit Modifiers that checks `msg.sender`: `onlyOwner`, `onlyPanopticPool`, `onlyRole` 894 function delegate(address delegatee, uint256 assets) external onlyPanopticPool { ... // @audit Modifiers that checks `msg.sender`: `onlyOwner`, `onlyPanopticPool`, `onlyRole` 903 function refund(address delegatee, uint256 assets) external onlyPanopticPool { ... // @audit Modifiers that checks `msg.sender`: `onlyOwner`, `onlyPanopticPool`, `onlyRole` 911 function revoke( 912 address delegator, 913 address delegatee, 914 uint256 assets 915 ) external onlyPanopticPool { ... // @audit Modifiers that checks `msg.sender`: `onlyOwner`, `onlyPanopticPool`, `onlyRole` 975 function refund(address refunder, address refundee, int256 assets) external onlyPanopticPool { ... // @audit Modifiers that checks `msg.sender`: `onlyOwner`, `onlyPanopticPool`, `onlyRole` 995 function takeCommissionAddData( 996 address optionOwner, 997 int128 longAmount, 998 int128 shortAmount, 999 int128 swappedAmount 1000 ) external onlyPanopticPool returns (int256 utilization) { ... // @audit Modifiers that checks `msg.sender`: `onlyOwner`, `onlyPanopticPool`, `onlyRole` 1043 function exercise( 1044 address optionOwner, 1045 int128 longAmount, 1046 int128 shortAmount, 1047 int128 swappedAmount, 1048 int128 realizedPremium 1049 ) external onlyPanopticPool returns (int128) { ... // @audit `msg.sender` is checked in function body 169 modifier onlyPanopticPool() { ... // @note `msg.sender` is checked here 170 if (msg.sender != address(s_panopticPool)) revert Errors.NotPanopticPool();
864..868 894 903 911..915 975 995..1000 1043..1049 169
- contracts/PanopticFactory.sol
</details>// @audit `msg.sender` is checked in function body 147 function transferOwnership(address newOwner) external {
<a name="L-06"></a> To the top
In the context of ERC20 tokens, a token holder needs to "approve" another account to transfer a certain amount of their tokens on their behalf. This is accomplished by calling the approve() function, which takes the address of the spender and the amount they're allowed to spend as parameters.
This approval step is a crucial part of the ERC20 standard because it allows for secure delegated transfers. A common use case for this feature is in decentralized exchanges or DeFi protocols, where a user can approve a smart contract to transfer tokens on their behalf.
After the approval step, the approved account (or contract) can then transfer tokens up to the approved amount from the token holder's account by calling the transferFrom() function. This function takes three parameters: the address of the token holder, the recipient's address, and the amount to transfer.
If an account tries to call transferFrom() before the token holder has called approve(), the transaction will fail because the ERC20 contract checks whether the transferFrom() caller has an adequate allowance.
In summary, if a contract or user needs to move ERC20 tokens on behalf of another account, it's necessary to ensure that the token holder has first called approve() to set a sufficient allowance. This is a key aspect of ERC20 token security and functionality.
- contracts/CollateralTracker.sol
// @audit function call transfer from, but not approve 417 function deposit(uint256 assets, address receiver) external returns (uint256 shares) { ... 424 SafeTransferLib.safeTransferFrom( ... // @audit function call transfer from, but not approve 477 function mint(uint256 shares, address receiver) external returns (uint256 assets) { ... 484 SafeTransferLib.safeTransferFrom( ... // @audit function call transfer from, but not approve 531 function withdraw( 532 uint256 assets, 533 address receiver, 534 address owner 535 ) external returns (uint256 shares) { ... 556 SafeTransferLib.safeTransferFrom( ... // @audit function call transfer from, but not approve 591 function redeem( 592 uint256 shares, 593 address receiver, 594 address owner 595 ) external returns (uint256 assets) { ... 616 SafeTransferLib.safeTransferFrom(
- contracts/PanopticFactory.sol
// @audit function call transfer from, but not approve 172 function uniswapV3MintCallback( 173 uint256 amount0Owed, 174 uint256 amount1Owed, 175 bytes calldata data 176 ) external { ... 182 SafeTransferLib.safeTransferFrom(
</details>- contracts/SemiFungiblePositionManager.sol
<a name="L-07"></a> To the top
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.
- contracts/PanopticPool.sol
// @audit array parameter `positionIdList` is used as loop condition and length is not cheched in function 799 TokenId[] calldata positionIdList ... // @audit loop uses unbounded parameter 802 for (uint256 i = 0; i < positionIdList.length; ) {
- contracts/SemiFungiblePositionManager.sol
// @audit array parameter `ids` is used as loop condition and length is not cheched in function 569 uint256[] calldata ids, ... // @audit loop uses unbounded parameter 575 for (uint256 i = 0; i < ids.length; ) {
- contracts/libraries/FeesCalc.sol
// @audit array parameter `positionIdList` is used as loop condition and length is not cheched in function 49 TokenId[] calldata positionIdList ... // @audit loop uses unbounded parameter 51 for (uint256 k = 0; k < positionIdList.length; ) {
- contracts/libraries/PanopticMath.sol
// @audit array parameter `positionIdList` is used as loop condition and length is not cheched in function 770 TokenId[] memory positionIdList, ... // @audit loop uses unbounded parameter 781 for (uint256 i = 0; i < positionIdList.length; ++i) { ... // @audit array parameter `positionIdList` is used as loop condition and length is not cheched in function 770 TokenId[] memory positionIdList, ... // @audit loop uses unbounded parameter 860 for (uint256 i = 0; i < positionIdList.length; i++) {
- contracts/multicall/Multicall.sol
- contracts/tokens/ERC1155Minimal.sol
</details>- contracts/tokens/ERC1155Minimal.sol
<a name="L-08"></a> To the top
If the length of the arrays are not required to be of the same length, user operations may not be fully executed due to a mismatch in the number of items iterated over, versus the number of items provided in the second array
- contracts/PanopticPool.sol
586 function burnOptions( 587 TokenId[] calldata positionIdList, 588 TokenId[] calldata newPositionIdList, 589 int24 tickLimitLow, 590 int24 tickLimitHigh 591 ) external { ... 1017 function liquidate( 1018 TokenId[] calldata positionIdListLiquidator, 1019 address liquidatee, 1020 LeftRightUnsigned delegations, 1021 TokenId[] calldata positionIdList 1022 ) external { ... 1179 function forceExercise( 1180 address account, 1181 TokenId[] calldata touchedId, 1182 TokenId[] calldata positionIdListExercisee, 1183 TokenId[] calldata positionIdListExercisor 1184 ) external {
586..591 1017..1022 1179..1184
- contracts/SemiFungiblePositionManager.sol
566 function safeBatchTransferFrom( 567 address from, 568 address to, 569 uint256[] calldata ids, 570 uint256[] calldata amounts, 571 bytes calldata data 572 ) public override {
- contracts/libraries/PanopticMath.sol
768 function haircutPremia( 769 address liquidatee, 770 TokenId[] memory positionIdList, 771 LeftRightSigned[4][] memory premiasByLeg, 772 LeftRightSigned collateralRemaining, 773 CollateralTracker collateral0, 774 CollateralTracker collateral1, 775 uint160 sqrtPriceX96Final, 776 mapping(bytes32 chunkKey => LeftRightUnsigned settledTokens) storage settledTokens 777 ) external returns (int256, int256) {
- contracts/tokens/ERC1155Minimal.sol
</details>130 function safeBatchTransferFrom( 131 address from, 132 address to, 133 uint256[] calldata ids, 134 uint256[] calldata amounts, 135 bytes calldata data 136 ) public virtual { ... 178 function balanceOfBatch( 179 address[] calldata owners, 180 uint256[] calldata ids 181 ) public view returns (uint256[] memory balances) {
<a name="L-09"></a> To the top
Incorporating multiple delegate calls or ordinary calls in a single payable function can pose significant security risks, especially concerning the potential drainage of funds. Each call in a function can potentially make use of msg.value (the amount of Ether sent with the function call), and if there's no explicit control over the division of msg.value across these calls, a user could intentionally or inadvertently trigger a condition where funds are drained from the contract in unexpected ways.
To mitigate these risks, it's crucial to implement rigorous checks and explicit control mechanisms over how msg.value is used. This may involve ensuring that msg.value is divided as intended across multiple calls or including checks to prevent recursive or reentrant calls. Additionally, using a 'pull over push' payment design can also provide added security, wherein instead of the contract sending out payments, recipients request withdrawals. This method can limit the amount of Ether that leaves the contract during a single function call and provide better control over funds.
- contracts/multicall/Multicall.sol
</details>12 function multicall(bytes[] calldata data) public payable returns (bytes[] memory results) { 13 results = new bytes[](data.length); 14 for (uint256 i = 0; i < data.length; ) { 15 (bool success, bytes memory result) = address(this).delegatecall(data[i]); 16 17 if (!success) { 18 // Bubble up the revert reason 19 // The bytes type is ABI encoded as a length-prefixed byte array 20 // So we simply need to add 32 to the pointer to get the start of the data 21 // And then revert with the size loaded from the first 32 bytes 22 // Other solutions will do work to differentiate the revert reasons and provide paranthetical information 23 // However, we have chosen to simply replicate the the normal behavior of the call 24 // NOTE: memory-safe because it reads from memory already allocated by solidity (the bytes memory result) 25 assembly ("memory-safe") { 26 revert(add(result, 32), mload(result)) 27 } 28 } 29 30 results[i] = result; 31 32 unchecked { 33 ++i; 34 } 35 } 36 }
<a name="L-10"></a> To the top
There is no limit specified on the amount of gas used, so the recipient can use up all of the transaction's gas, causing it to revert. Use addr.call{gas: <amount>}("") or this library instead
- contracts/multicall/Multicall.sol
</details>15 (bool success, bytes memory result) = address(this).delegatecall(data[i]);
<a name="N-01"></a> To the top
Consider splitting long arithmetic calculations into multiple steps to improve the code readability.
- contracts/CollateralTracker.sol
202 2230 + 203 (12500 * ratioTick) / 204 10_000 + 205 (7812 * ratioTick ** 2) / 206 10_000 ** 2 + 207 (6510 * ratioTick ** 3) / 208 10_000 ** 3 ... 202 2230 + 203 (12500 * ratioTick) / 204 10_000 + 205 (7812 * ratioTick ** 2) / 206 10_000 ** 2 + ... 202 2230 + 203 (12500 * ratioTick) / 204 10_000 + ... 205 (7812 * ratioTick ** 2) / 206 10_000 ** 2 + ... 207 (6510 * ratioTick ** 3) / 208 10_000 ** 3 ... 796 min_sell_ratio + 797 ((DECIMALS - min_sell_ratio) * (uint256(utilization) - TARGET_POOL_UTIL)) / 798 (SATURATED_POOL_UTIL - TARGET_POOL_UTIL); ... 797 ((DECIMALS - min_sell_ratio) * (uint256(utilization) - TARGET_POOL_UTIL)) / 798 (SATURATED_POOL_UTIL - TARGET_POOL_UTIL); ... 849 (BUYER_COLLATERAL_RATIO + 850 (BUYER_COLLATERAL_RATIO * (SATURATED_POOL_UTIL - utilization)) / 851 (SATURATED_POOL_UTIL - TARGET_POOL_UTIL)) / 2; // do the division by 2 at the end after all addition and multiplication; b/c y1 = buyCollateralRatio / 2 ... 849 (BUYER_COLLATERAL_RATIO + 850 (BUYER_COLLATERAL_RATIO * (SATURATED_POOL_UTIL - utilization)) / 851 (SATURATED_POOL_UTIL - TARGET_POOL_UTIL)) / 2; // do the division by 2 at the end after all addition and multiplication; b/c y1 = buyCollateralRatio / 2 ... 849 (BUYER_COLLATERAL_RATIO + 850 (BUYER_COLLATERAL_RATIO * (SATURATED_POOL_UTIL - utilization)) / 851 (SATURATED_POOL_UTIL - TARGET_POOL_UTIL)) / 2; // do the division by 2 at the end after all addition and multiplication; b/c y1 = buyCollateralRatio / 2
202..208 202..206 202..204 205..206 207..208 796..798 797..798 849..851 849..851 849..851 850..851
- contracts/PanopticPool.sol
309..314 309..313 1487 1559..1561 1550..1552 1737..1740 1729..1732 1768..1769 1770..1771 1952..1959
- contracts/SemiFungiblePositionManager.sol
- contracts/libraries/Math.sol
- contracts/libraries/PanopticMath.sol
108 108 108 107 107 107 178 178 179 179 209 223 223 223 223 223 227..230 227..229 249
- contracts/types/LiquidityChunk.sol
- contracts/types/TokenId.sol
110 110 110 120 120 120 120 120 130 130 130 130 130 140 140 140 140 140 150 150 150 150 150 160 160 160 171 171 171 171 171 212 212 212 229 229 229 229 229 246 246 246 246 246 263 263 263 263 263 281 281 281 281 281 300 300 300 319..320 320 320 320 320 394..395 395 395 395 395 406 512 512
</details><a name="N-02"></a> To the top
Use mixedCase for local and state variables that are not constants, and add a trailing underscore for internal variables. Documentation
- contracts/CollateralTracker.sol
89 address internal s_underlyingToken; ... 93 bool internal s_initialized; ... 96 address internal s_univ3token0; ... 99 address internal s_univ3token1; ... 102 bool internal s_underlyingIsToken0; ... 109 PanopticPool internal s_panopticPool; ... 112 uint128 internal s_poolAssets; ... 115 uint128 internal s_inAMM; ... 121 uint128 internal s_ITMSpreadFee; ... 124 uint24 internal s_poolFee;
89 93 96 99 102 109 112 115 121 124 131 136 141 145 149 154 158 162 247 773 1219
- contracts/PanopticFactory.sol
63 66 69 72 75 78 96 99 102 223
- contracts/PanopticPool.sol
179 186 225 231 233 238..239 245 251 258..259 272 439 895 1117 1118 1119 1139 1923 1924
- contracts/SemiFungiblePositionManager.sol
137 145 150 177..178 287 289 295 611 620 765 883 884 885 889 890 891 892 893 1342 1343 1363 1364 1384 1385 1472 1473 1474
- contracts/libraries/PanopticMath.sol
186 186 192 192 855 862 865..866
- contracts/types/LeftRight.sol
</details>- contracts/types/TokenId.sol
require/if statements should be refactored<a name="N-03"></a> To the top
These statements should be refactored to a separate function, as there are multiple parts of the codebase that use the same logic, to improve the code readability and reduce code duplication.
- contracts/CollateralTracker.sol
331 if (s_panopticPool.numberOfPositions(msg.sender) != 0) revert Errors.PositionCountNotZero(); ... 350 if (s_panopticPool.numberOfPositions(from) != 0) revert Errors.PositionCountNotZero(); ... 418 if (assets > type(uint104).max) revert Errors.DepositTooLarge(); ... 480 if (assets > type(uint104).max) revert Errors.DepositTooLarge(); ... 541 if (msg.sender != owner) { ... 544 if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares; ... 599 if (msg.sender != owner) { ... 602 if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares; ... 708 (tokenType == 0 && currentValue1 < oracleValue1) || 709 (tokenType == 1 && currentValue0 < oracleValue0) ... 1010 if (tokenToPay > 0) {
331 350 418 480 541 544 599 602 708..709 1010 1018 1067 1075 1339 1346..1347 1374..1375 1451 1479 1488
- contracts/PanopticPool.sol
533 664 768 865 944 1156..1162 1566 1679
- contracts/SemiFungiblePositionManager.sol
- contracts/libraries/Math.sol
297 303 360 447 448 474 537 587 588 614 664 665 691
- contracts/libraries/PanopticMath.sol
325 494 511 528 551 584 619 627 706 724 785 864 931 949
- contracts/libraries/SafeTransferLib.sol
- contracts/tokens/ERC1155Minimal.sol
101 112 114..115 137 163 222 224..225
- contracts/types/LeftRight.sol
- contracts/types/TokenId.sol
376 378 380 382 439 441 443 445 501 508 531..532 546..547 560 566 589
</details><a name="N-04"></a> To the top
While integers with a large number of bits are unlikely to overflow on human time scales, it is not strictly correct to use an unchecked block around them, because eventually they will overflow, and unchecked blocks are meant for cases where it's mathematically impossible for an operation to trigger an overflow (e.g. a prior require() statement prevents the overflow case)
- contracts/CollateralTracker.sol
197 unchecked { 198 // taylor expand log(1-sellCollateralRatio)/log(1.0001) around sellCollateralRatio=2000 up to 3rd order 199 /// since we're dropping the higher order terms, which are all negative, this will underestimate the number of ticks for a 20% move 200 int256 ratioTick = (int256(_sellerCollateralRatio) - 2000); 201 TICK_DEVIATION = uint256( 202 2230 + 203 (12500 * ratioTick) / 204 10_000 + 205 (7812 * ratioTick ** 2) / 206 10_000 ** 2 + 207 (6510 * ratioTick ** 3) / 208 10_000 ** 3 209 ); 210 } ... 261 unchecked { 262 s_ITMSpreadFee = uint128((ITM_SPREAD_MULTIPLIER * _poolFee) / DECIMALS); 263 } ... 401 unchecked { 402 shares = Math.mulDiv( 403 assets * (DECIMALS - COMMISSION_FEE), 404 totalSupply, 405 totalAssets() * DECIMALS 406 ); 407 } ... 445 unchecked { 446 return (convertToShares(type(uint104).max) * DECIMALS) / (DECIMALS + COMMISSION_FEE); 447 } ... 461 unchecked { 462 assets = Math.mulDivRoundingUp( 463 shares * DECIMALS, 464 totalAssets(), 465 totalSupply * (DECIMALS - COMMISSION_FEE) 466 ); 467 } ... 661 unchecked { 662 for (uint256 leg = 0; leg < positionId.countLegs(); ++leg) { 663 // short legs are not counted - exercise is intended to be based on long legs 664 if (positionId.isLong(leg) == 0) continue; 665 666 { 667 int24 range = int24( 668 int256( 669 Math.unsafeDivRoundingUp( 670 uint24(positionId.width(leg) * positionId.tickSpacing()), 671 2 672 ) 673 ) 674 ); 675 maxNumRangesFromStrike = Math.max( 676 maxNumRangesFromStrike, 677 uint256(Math.abs(currentTick - positionId.strike(leg)) / range) 678 ); 679 } 680 681 uint256 currentValue0; 682 uint256 currentValue1; 683 uint256 oracleValue0; 684 uint256 oracleValue1; 685 686 { 687 LiquidityChunk liquidityChunk = PanopticMath.getLiquidityChunk( 688 positionId, 689 leg, 690 positionBalance 691 ); 692 693 (currentValue0, currentValue1) = Math.getAmountsForLiquidity( 694 currentTick, 695 liquidityChunk 696 ); 697 698 (oracleValue0, oracleValue1) = Math.getAmountsForLiquidity( 699 oracleTick, 700 liquidityChunk 701 ); 702 } 703 704 uint256 tokenType = positionId.tokenType(leg); 705 // compensate user for loss in value if chunk has lost money between current and median tick 706 // note: the delta for one token will be positive and the other will be negative. This cancels out any moves in their positions 707 if ( 708 (tokenType == 0 && currentValue1 < oracleValue1) || 709 (tokenType == 1 && currentValue0 < oracleValue0) 710 ) 711 exerciseFees = exerciseFees.sub( 712 LeftRightSigned 713 .wrap(0) 714 .toRightSlot( 715 int128(uint128(oracleValue0)) - int128(uint128(currentValue0)) 716 ) 717 .toLeftSlot( 718 int128(uint128(oracleValue1)) - int128(uint128(currentValue1)) 719 ) 720 ); 721 } 722 723 // note: we HAVE to start with a negative number as the base exercise cost because when shifting a negative number right by n bits, 724 // the result is rounded DOWN and NOT toward zero 725 // this divergence is observed when n (the number of half ranges) is > 10 (ensuring the floor is not zero, but -1 = 1bps at that point) 726 // subtract 1 from max half ranges from strike so fee starts at FORCE_EXERCISE_COST when moving OTM 727 int256 fee = (FORCE_EXERCISE_COST >> (maxNumRangesFromStrike - 1)); // exponential decay of fee based on number of half ranges away from the price 728 729 // store the exercise fees in the exerciseFees variable 730 exerciseFees = exerciseFees 731 .toRightSlot(int128((longAmounts.rightSlot() * fee) / DECIMALS_128)) 732 .toLeftSlot(int128((longAmounts.leftSlot() * fee) / DECIMALS_128)); 733 } ... 742 unchecked { 743 return int256((s_inAMM * DECIMALS) / totalAssets()); 744 } ... 794 unchecked { 795 return 796 min_sell_ratio + 797 ((DECIMALS - min_sell_ratio) * (uint256(utilization) - TARGET_POOL_UTIL)) / 798 (SATURATED_POOL_UTIL - TARGET_POOL_UTIL); 799 } ... 847 unchecked { 848 return 849 (BUYER_COLLATERAL_RATIO + 850 (BUYER_COLLATERAL_RATIO * (SATURATED_POOL_UTIL - utilization)) / 851 (SATURATED_POOL_UTIL - TARGET_POOL_UTIL)) / 2; // do the division by 2 at the end after all addition and multiplication; b/c y1 = buyCollateralRatio / 2 852 } ... 1103 unchecked { 1104 // intrinsic value is the amount that need to be exchanged due to minting in-the-money 1105 int256 intrinsicValue = swappedAmount - (shortAmount - longAmount); 1106 1107 if (intrinsicValue != 0) { 1108 // the swap commission is paid on the intrinsic value, and it is always positive 1109 uint256 swapCommission = Math.unsafeDivRoundingUp( 1110 s_ITMSpreadFee * uint256(Math.abs(intrinsicValue)), 1111 DECIMALS 1112 ); 1113 1114 // set the exchanged amount to the sum of the intrinsic value and swapCommission 1115 exchangedAmount = intrinsicValue + int256(swapCommission); 1116 } 1117 1118 //compute total commission amount = commission rate + spread fee 1119 exchangedAmount += int256( 1120 Math.unsafeDivRoundingUp( 1121 uint256(uint128(shortAmount + longAmount)) * COMMISSION_FEE, 1122 DECIMALS 1123 ) 1124 ); 1125 }
197..210 261..263 401..407 445..447 461..467 661..733 742..744 794..799 847..852 1103..1125 1230..1232 1254..1267 1338..1421 1495..1497 1485..1487 1555..1573
- contracts/PanopticFactory.sol
- contracts/PanopticPool.sol
482..484 487..489 778..780 812..814 871..873 1328..1330 1344..1355 1388..1390 1486..1488 1539..1569 1571..1573 1631..1655 1717..1743 1764..1795 1922..1969 1976..1978
- contracts/SemiFungiblePositionManager.sol
387..389 578..580 649..651 931..933 1339..1406
- contracts/libraries/FeesCalc.sol
- contracts/libraries/Math.sol
129..180 345..432 445..451 459..514 522..577 585..591 599..654 662..668 676..731 754..770
- contracts/libraries/PanopticMath.sol
132..156 175..232 246..267 343..362 406..408 491..499 508..516 525..539 548..566 659..753 778..907
- contracts/multicall/Multicall.sol
- contracts/tokens/ERC1155Minimal.sol
- contracts/types/TokenId.sol
97..99 109..111 119..121 129..131 139..141 149..151 159..161 170..172 210..213 226..231 245..247 260..265 278..283 296..302 316..322 367..397 504..570 579..595
</details>uint to int conversion<a name="N-05"></a> To the top
The int type in Solidity uses the two's complement system, so it is possible to accidentally overflow a very large uint to an int, even if they share the same number of bytes (e.g. a uint256 number > type(uint128).max will overflow a int256 cast).
Consider using the SafeCast library to prevent any overflows.
- contracts/CollateralTracker.sol
200 int256 ratioTick = (int256(_sellerCollateralRatio) - 2000); ... 668 int256( 669 Math.unsafeDivRoundingUp( 670 uint24(positionId.width(leg) * positionId.tickSpacing()), 671 2 672 ) 673 ) ... 718 int128(uint128(oracleValue1)) - int128(uint128(currentValue1)) ... 718 int128(uint128(oracleValue1)) - int128(uint128(currentValue1)) ... 715 int128(uint128(oracleValue0)) - int128(uint128(currentValue0)) ... 715 int128(uint128(oracleValue0)) - int128(uint128(currentValue0)) ... 743 return int256((s_inAMM * DECIMALS) / totalAssets()); ... 959 uint256(Math.max(1, int256(totalAssets()) - int256(assets))) ... 959 uint256(Math.max(1, int256(totalAssets()) - int256(assets))) ... 1003 int256 updatedAssets = int256(uint256(s_poolAssets)) - swappedAmount;
200 668..673 718 718 715 715 743 959 959 1003 1029 1052 1085 1115 1119..1124 1330 1329 1586 1585 1637 1638
- contracts/PanopticPool.sol
477 1144 1149 1558..1562 1549..1553 1636 1635 1901 1952..1955 1956..1959 1935..1938 1939..1942 1870
- contracts/SemiFungiblePositionManager.sol
1177 1176 1172 1169 1215 1214 1242 1241
- contracts/libraries/FeesCalc.sol
- contracts/libraries/Math.sol
- contracts/libraries/PanopticMath.sol
140 140 141 151 178 179 188 188 196 217 258 377 681 683..688 693 695 929 938..940 947 956..958
- contracts/types/LeftRight.sol
- contracts/types/LiquidityChunk.sol
</details>- contracts/types/TokenId.sol
<a name="N-06"></a> To the top
Consider grouping all the system constants under a single file.
- contracts/CollateralTracker.sol
70 string internal constant TICKER_PREFIX = "po"; ... 73 string internal constant NAME_PREFIX = "POPT-V1"; ... 77 uint256 internal constant DECIMALS = 10_000; ... 81 int128 internal constant DECIMALS_128 = 10_000;
- contracts/PanopticFactory.sol
82 uint256 internal constant FULL_RANGE_LIQUIDITY_AMOUNT_WETH = 0.1 ether; ... 86 uint256 internal constant FULL_RANGE_LIQUIDITY_AMOUNT_TOKEN = 1e6; ... 89 uint16 internal constant CARDINALITY_INCREASE = 100;
- contracts/PanopticPool.sol
103 int24 internal constant MIN_SWAP_TICK = Constants.MIN_V3POOL_TICK + 1; ... 105 int24 internal constant MAX_SWAP_TICK = Constants.MAX_V3POOL_TICK - 1; ... 109 bool internal constant COMPUTE_ALL_PREMIA = true;
103 105 109 111 114 119 120 123 128 133 136 139 145 148 152 156 160 165 168 171 174
- contracts/SemiFungiblePositionManager.sol
- contracts/libraries/Constants.sol
- contracts/libraries/Math.sol
- contracts/libraries/PanopticMath.sol
- contracts/types/LeftRight.sol
- contracts/types/LiquidityChunk.sol
- contracts/types/TokenId.sol
62..63 66..67 70..71 74..75 78
</details><a name="N-07"></a> To the top
Add a preceding underscore to the state variable name, take care to refactor where there variables are read/wrote
- contracts/CollateralTracker.sol
89 address internal s_underlyingToken; ... 93 bool internal s_initialized; ... 96 address internal s_univ3token0; ... 99 address internal s_univ3token1; ... 102 bool internal s_underlyingIsToken0; ... 109 PanopticPool internal s_panopticPool; ... 112 uint128 internal s_poolAssets; ... 115 uint128 internal s_inAMM; ... 121 uint128 internal s_ITMSpreadFee; ... 124 uint24 internal s_poolFee;
89 93 96 99 102 109 112 115 121 124 131 136 141 145 149 154 158 162
- contracts/PanopticFactory.sol
- contracts/PanopticPool.sol
179 186 225 231 233 238..239 245 251 258..259 272
- contracts/SemiFungiblePositionManager.sol
137 145 150 177..178 287 289 295
</details><a name="N-08"></a> To the top
In Solidity, while function overriding allows for functions with the same name to coexist, it is advisable to avoid this practice to enhance code readability and maintainability. Having multiple functions with the same name, even with different parameters or in inherited contracts, can cause confusion and increase the likelihood of errors during development, testing, and debugging. Using distinct and descriptive function names not only clarifies the purpose and behavior of each function, but also helps prevent unintended function calls or incorrect overriding. By adopting a clear and consistent naming convention, developers can create more comprehensible and maintainable smart contracts.
- contracts/CollateralTracker.sol
864 function delegate( 865 address delegator, 866 address delegatee, 867 uint256 assets 868 ) external onlyPanopticPool { ... 894 function delegate(address delegatee, uint256 assets) external onlyPanopticPool { ... 903 function refund(address delegatee, uint256 assets) external onlyPanopticPool { ... 975 function refund(address refunder, address refundee, int256 assets) external onlyPanopticPool {
- contracts/PanopticPool.sol
569 function burnOptions( 570 TokenId tokenId, 571 TokenId[] calldata newPositionIdList, 572 int24 tickLimitLow, 573 int24 tickLimitHigh 574 ) external { ... 586 function burnOptions( 587 TokenId[] calldata positionIdList, 588 TokenId[] calldata newPositionIdList, 589 int24 tickLimitLow, 590 int24 tickLimitHigh 591 ) external {
- contracts/libraries/Math.sol
57 function max(uint256 a, uint256 b) internal pure returns (uint256) { ... 65 function max(int256 a, int256 b) internal pure returns (int256) { ... 41 function min(uint256 a, uint256 b) internal pure returns (uint256) { ... 49 function min(int256 a, int256 b) internal pure returns (int256) {
- contracts/libraries/PanopticMath.sol
490 524 507 547 419..424 445..450
- contracts/types/LeftRight.sol
148..151 194 214 101 108 39 46 171..174 232 121..124 134 59..62 78..81
</details><a name="N-09"></a> To the top
- contracts/CollateralTracker.sol
2 pragma solidity ^0.8.18;
- contracts/PanopticFactory.sol
2 pragma solidity ^0.8.18;
- contracts/PanopticPool.sol
2 pragma solidity ^0.8.18;
- contracts/SemiFungiblePositionManager.sol
2 pragma solidity ^0.8.18;
- contracts/libraries/CallbackLib.sol
2 pragma solidity ^0.8.0;
- contracts/libraries/Constants.sol
2 pragma solidity ^0.8.0;
- contracts/libraries/Errors.sol
2 pragma solidity ^0.8.0;
- contracts/libraries/FeesCalc.sol
2 pragma solidity ^0.8.0;
- contracts/libraries/InteractionHelper.sol
2 pragma solidity ^0.8.0;
- contracts/libraries/Math.sol
2 pragma solidity ^0.8.0;
- contracts/libraries/PanopticMath.sol
- contracts/libraries/SafeTransferLib.sol
- contracts/multicall/Multicall.sol
- contracts/tokens/ERC1155Minimal.sol
- contracts/tokens/ERC20Minimal.sol
- contracts/tokens/interfaces/IDonorNFT.sol
- contracts/tokens/interfaces/IERC20Partial.sol
- contracts/types/LeftRight.sol
- contracts/types/LiquidityChunk.sol
</details>- contracts/types/TokenId.sol
<a name="N-10"></a> To the top
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.
- contracts/CollateralTracker.sol
361 function asset() external view returns (address assetTokenAddress) { ... 370 function totalAssets() public view returns (uint256 totalManagedAssets) { ... 379 function convertToShares(uint256 assets) public view returns (uint256 shares) { ... 386 function convertToAssets(uint256 shares) public view returns (uint256 assets) { ... 392 function maxDeposit(address) external pure returns (uint256 maxAssets) { ... 444 function maxMint(address) external view returns (uint256 maxShares) { ... 507 function maxWithdraw(address owner) public view returns (uint256 maxAssets) { ... 518 function previewWithdraw(uint256 assets) public view returns (uint256 shares) { ... 572 function maxRedeem(address owner) public view returns (uint256 maxShares) { ... 581 function previewRedeem(uint256 shares) public view returns (uint256 assets) {
361 370 379 386 392 444 507 518 572 581 741 753 808 1284
- contracts/PanopticPool.sol
</details>- contracts/SemiFungiblePositionManager.sol
require/error<a name="N-11"></a> To the top
Some parts of the codebase use require statements, while others use custom errors. Consider refactoring the code to use the same approach: the following findings represent the minority of require vs error, and they show the first occurance in each file, for brevity.
- contracts/libraries/Math.sol
131 if (absTick > uint256(int256(Constants.MAX_V3POOL_TICK))) revert Errors.InvalidTick(); ... 297 if ((downcastedInt = uint128(toDowncast)) != toDowncast) revert Errors.CastingError(); ... 312 if ((downcastedInt = int128(toCast)) < 0) revert Errors.CastingError(); ... 319 if (!((downcastedInt = int128(toCast)) == toCast)) revert Errors.CastingError(); ... 326 if (toCast > uint256(type(int256).max)) revert Errors.CastingError(); ... 361 require(denominator > 0); ... 370 require(denominator > prod1); ... 448 require(result < type(uint256).max); ... 484 require(2 ** 64 > prod1); ... 547 require(2 ** 96 > prod1);
131 297 312 319 326 361 370 448 484 547 588 624 665 701
</details><a name="N-12"></a> To the top
Ensure that events follow the best practice of check-effects-interaction, and are emitted before external calls
- contracts/CollateralTracker.sol
// @audit functions: `DepositTooLarge`, `safeTransferFrom` called before this event 439 emit Deposit(msg.sender, receiver, assets, shares); ... // @audit functions: `DepositTooLarge`, `safeTransferFrom` called before this event 499 emit Deposit(msg.sender, receiver, assets, shares); ... // @audit functions: `ExceedsMaximumRedemption`, `safeTransferFrom` called before this event 563 emit Withdraw(msg.sender, receiver, owner, assets, shares); ... // @audit functions: `ExceedsMaximumRedemption`, `safeTransferFrom` called before this event 623 emit Withdraw(msg.sender, receiver, owner, assets, shares);
- contracts/PanopticFactory.sol
// @audit functions: `NotOwner` called before this event 154 emit OwnershipTransferred(currentOwner, newOwner); ... // @audit functions: `InvalidSalt`, `NotOwner`, `getPool`, `UniswapPoolNotInitialized`, `PoolAlreadyInitialized`, `initializeAMMPool`, `cloneDeterministic`, `clone`, `clone`, `startToken`, `startToken`, `startPool`, `increaseObservationCardinalityNext`, `issueNFT` called before this event 268 emit PoolDeployed( 269 newPoolContract, 270 v3Pool, 271 collateralTracker0, 272 collateralTracker1, 273 amount0, 274 amount1 275 );
- contracts/PanopticPool.sol
// @audit functions: `poolId`, `getPoolId`, `InvalidTokenIdParameter`, `unwrap`, `PositionAlreadyMinted`, `toRightSlot`, `toLeftSlot`, `wrap` called before this event 666 emit OptionMinted(msg.sender, positionSize, tokenId, poolUtilizations); ... // @audit functions: `rightSlot` called before this event 853 emit OptionBurnt(owner, positionSize, tokenId, premiaOwed); ... // @audit functions: `slot0`, `abs`, `StaleTWAP`, `getAccountMarginDetails`, `rightSlot`, `getAccountMarginDetails`, `leftSlot`, `getSqrtRatioAtTick`, `NotMarginCalled`, `delegate`, `rightSlot`, `delegate`, `leftSlot`, `slot0`, `getLiquidationBonus`, `getSqrtRatioAtTick`, `getSqrtRatioAtTick`, `haircutPremia`, `getSqrtRatioAtTick`, `revoke`, `rightSlot`, `revoke`, `leftSlot`, `NotEnoughCollateral`, `toLeftSlot`, `toRightSlot`, `wrap` called before this event 1170 emit AccountLiquidated(msg.sender, liquidatee, bonusAmounts); ... // @audit functions: `InputListFail`, `rightSlot`, `computeExercisedAmounts`, `slot0`, `sub`, `validateIsExercisable`, `delegate`, `rightSlot`, `delegate`, `leftSlot`, `exerciseCost`, `add`, `getRefundAmounts`, `refund`, `rightSlot`, `rightSlot`, `refund`, `leftSlot`, `leftSlot`, `refund`, `rightSlot`, `refund`, `leftSlot` called before this event 1277 emit ForcedExercised(msg.sender, account, touchedId[0], exerciseFees);
</details>- contracts/SemiFungiblePositionManager.sol
require() and if statements instead of &&<a name="N-13"></a> To the top
Using multiple require() and if improves code readability and makes it easier to debug.
- contracts/CollateralTracker.sol
707 if ( 708 (tokenType == 0 && currentValue1 < oracleValue1) || 709 (tokenType == 1 && currentValue0 < oracleValue0) 710 ) 711 exerciseFees = exerciseFees.sub( 712 LeftRightSigned 713 .wrap(0) 714 .toRightSlot( 715 int128(uint128(oracleValue0)) - int128(uint128(currentValue0)) 716 ) 717 .toLeftSlot( 718 int128(uint128(oracleValue1)) - int128(uint128(currentValue1)) 719 ) 720 ); ... 1060 if ((intrinsicValue != 0) && ((shortAmount != 0) || (longAmount != 0))) { 1061 // intrinsic value is the amount that need to be exchanged due to burning in-the-money 1062 1063 // add the intrinsic value to the tokenToPay 1064 tokenToPay += intrinsicValue; 1065 } ... 1345 if ( 1346 ((atTick >= tickUpper) && (tokenType == 1)) || // strike OTM when price >= upperTick for tokenType=1 1347 ((atTick < tickLower) && (tokenType == 0)) // strike OTM when price < lowerTick for tokenType=0 1348 ) { 1349 // position is out-the-money, collateral requirement = SCR * amountMoved 1350 required; 1351 } else { 1352 int24 strike = tokenId.strike(index); 1353 // if position is ITM or ATM, then the collateral requirement depends on price: 1354 1355 // compute the ratio of strike to price for calls (or price to strike for puts) 1356 // (- and * 2 in tick space are / and ^ 2 in price space so sqrtRatioAtTick(2 *(a - b)) = a/b (*2^96) 1357 // both of these ratios decrease as the position becomes deeper ITM, and it is possible 1358 // for the ratio of the prices to go under the minimum price 1359 // (which is the limit of what getSqrtRatioAtTick supports) 1360 // so instead we cap it at the minimum price, which is acceptable because 1361 // a higher ratio will result in an increased slope for the collateral requirement 1362 uint160 ratio = tokenType == 1 // tokenType 1363 ? Math.getSqrtRatioAtTick( 1364 Math.max24(2 * (atTick - strike), Constants.MIN_V3POOL_TICK) 1365 ) // puts -> price/strike 1366 : Math.getSqrtRatioAtTick( 1367 Math.max24(2 * (strike - atTick), Constants.MIN_V3POOL_TICK) 1368 ); // calls -> strike/price 1369 1370 // compute the collateral requirement depending on whether the position is ITM & out-of-range or ITM and in-range: 1371 1372 /// ITM and out-of-range 1373 if ( 1374 ((atTick < tickLower) && (tokenType == 1)) || // strike ITM but out of range price < lowerTick for tokenType=1 1375 ((atTick >= tickUpper) && (tokenType == 0)) // strike ITM but out of range when price >= upperTick for tokenType=0 1376 ) { 1377 /** 1378 Short put BPR = 100% - (price/strike) + SCR 1379 1380 BUYING 1381 POWER 1382 REQUIREMENT 1383 1384 ^ . . 1385 | <- ITM . <-ATM-> . OTM -> 1386 100% + SCR% - |--__ . . . 1387 100% - | . .¯¯--__ . . . 1388 | . ¯¯--__ . . 1389 SCR - | . .¯¯--__________ 1390 | . . . . 1391 +----+----------+----+----+---> current 1392 0 Liqui- Pa strike Pb price 1393 dation 1394 price = SCR*strike 1395 */ 1396 1397 uint256 c2 = Constants.FP96 - ratio; 1398 1399 // compute the tokens required 1400 // position is in-the-money, collateral requirement = amountMoved*(1-ratio) + SCR*amountMoved 1401 required += Math.mulDiv96RoundingUp(amountMoved, c2); 1402 } else { 1403 // position is in-range (ie. current tick is between upper+lower tick): we draw a line between the 1404 // collateral requirement at the lowerTick and the one at the upperTick. We use that interpolation as 1405 // the collateral requirement when in-range, which always over-estimates the amount of token required 1406 // Specifically: 1407 // required = amountMoved * (scaleFactor - ratio) / (scaleFactor + 1) + sellCollateralRatio*amountMoved 1408 uint160 scaleFactor = Math.getSqrtRatioAtTick( 1409 (tickUpper - strike) + (strike - tickLower) 1410 ); 1411 uint256 c3 = Math.mulDivRoundingUp( 1412 amountMoved, 1413 scaleFactor - ratio, 1414 scaleFactor + Constants.FP96 1415 ); 1416 // position is in-the-money, collateral requirement = amountMoved*(1-SRC)*(scaleFactor-ratio)/(scaleFactor+1) + SCR*amountMoved 1417 required += c3; 1418 } 1419 } ... 1373 if ( 1374 ((atTick < tickLower) && (tokenType == 1)) || // strike ITM but out of range price < lowerTick for tokenType=1 1375 ((atTick >= tickUpper) && (tokenType == 0)) // strike ITM but out of range when price >= upperTick for tokenType=0 1376 ) { 1377 /** 1378 Short put BPR = 100% - (price/strike) + SCR 1379 1380 BUYING 1381 POWER 1382 REQUIREMENT 1383 1384 ^ . . 1385 | <- ITM . <-ATM-> . OTM -> 1386 100% + SCR% - |--__ . . . 1387 100% - | . .¯¯--__ . . . 1388 | . ¯¯--__ . . 1389 SCR - | . .¯¯--__________ 1390 | . . . . 1391 +----+----------+----+----+---> current 1392 0 Liqui- Pa strike Pb price 1393 dation 1394 price = SCR*strike 1395 */ 1396 1397 uint256 c2 = Constants.FP96 - ratio; 1398 1399 // compute the tokens required 1400 // position is in-the-money, collateral requirement = amountMoved*(1-ratio) + SCR*amountMoved 1401 required += Math.mulDiv96RoundingUp(amountMoved, c2); 1402 } else { 1403 // position is in-range (ie. current tick is between upper+lower tick): we draw a line between the 1404 // collateral requirement at the lowerTick and the one at the upperTick. We use that interpolation as 1405 // the collateral requirement when in-range, which always over-estimates the amount of token required 1406 // Specifically: 1407 // required = amountMoved * (scaleFactor - ratio) / (scaleFactor + 1) + sellCollateralRatio*amountMoved 1408 uint160 scaleFactor = Math.getSqrtRatioAtTick( 1409 (tickUpper - strike) + (strike - tickLower) 1410 ); 1411 uint256 c3 = Math.mulDivRoundingUp( 1412 amountMoved, 1413 scaleFactor - ratio, 1414 scaleFactor + Constants.FP96 1415 ); 1416 // position is in-the-money, collateral requirement = amountMoved*(1-SRC)*(scaleFactor-ratio)/(scaleFactor+1) + SCR*amountMoved 1417 required += c3; 1418 }
707..720 1060..1065 1345..1419 1373..1418
- contracts/PanopticFactory.sol
224 if (_owner != address(0) && _owner != msg.sender) revert Errors.NotOwner();
- contracts/PanopticPool.sol
460 if (tokenId.isLong(leg) == 0 && !includePendingPremium) { 461 bytes32 chunkKey = keccak256( 462 abi.encodePacked( 463 tokenId.strike(leg), 464 tokenId.width(leg), 465 tokenId.tokenType(leg) 466 ) 467 ); 468 469 LeftRightUnsigned availablePremium = _getAvailablePremium( 470 _getTotalLiquidity(tokenId, leg), 471 s_settledTokens[chunkKey], 472 s_grossPremiumLast[chunkKey], 473 LeftRightUnsigned.wrap(uint256(LeftRightSigned.unwrap(premiaByLeg[leg]))), 474 premiumAccumulatorsByLeg[leg] 475 ); 476 portfolioPremium = portfolioPremium.add( 477 LeftRightSigned.wrap(int256(LeftRightUnsigned.unwrap(availablePremium))) 478 ); 479 } else { 480 portfolioPremium = portfolioPremium.add(premiaByLeg[leg]); 481 }
- contracts/SemiFungiblePositionManager.sol
787 if ((itm0 != 0) && (itm1 != 0)) { 788 (uint160 sqrtPriceX96, , , , , , ) = _univ3pool.slot0(); 789 790 // implement a single "netting" swap. Thank you @danrobinson for this puzzle/idea 791 // note: negative ITM amounts denote a surplus of tokens (burning liquidity), while positive amounts denote a shortage of tokens (minting liquidity) 792 // compute the approximate delta of token0 that should be resolved in the swap at the current tick 793 // we do this by flipping the signs on the token1 ITM amount converting+deducting it against the token0 ITM amount 794 // couple examples (price = 2 1/0): 795 // - 100 surplus 0, 100 surplus 1 (itm0 = -100, itm1 = -100) 796 // normal swap 0: 100 0 => 200 1 797 // normal swap 1: 100 1 => 50 0 798 // final swap amounts: 50 0 => 100 1 799 // netting swap: net0 = -100 - (-100/2) = -50, ZF1 = true, 50 0 => 100 1 800 // - 100 surplus 0, 100 shortage 1 (itm0 = -100, itm1 = 100) 801 // normal swap 0: 100 0 => 200 1 802 // normal swap 1: 50 0 => 100 1 803 // final swap amounts: 150 0 => 300 1 804 // netting swap: net0 = -100 - (100/2) = -150, ZF1 = true, 150 0 => 300 1 805 // - 100 shortage 0, 100 surplus 1 (itm0 = 100, itm1 = -100) 806 // normal swap 0: 200 1 => 100 0 807 // normal swap 1: 100 1 => 50 0 808 // final swap amounts: 300 1 => 150 0 809 // netting swap: net0 = 100 - (-100/2) = 150, ZF1 = false, 300 1 => 150 0 810 // - 100 shortage 0, 100 shortage 1 (itm0 = 100, itm1 = 100) 811 // normal swap 0: 200 1 => 100 0 812 // normal swap 1: 50 0 => 100 1 813 // final swap amounts: 100 1 => 50 0 814 // netting swap: net0 = 100 - (100/2) = 50, ZF1 = false, 100 1 => 50 0 815 // - = Net surplus of token0 816 // + = Net shortage of token0 817 int256 net0 = itm0 - PanopticMath.convert1to0(itm1, sqrtPriceX96); 818 819 zeroForOne = net0 < 0; 820 821 //compute the swap amount, set as positive (exact input) 822 swapAmount = -net0; 823 } else if (itm0 != 0) { 824 zeroForOne = itm0 < 0; 825 swapAmount = -itm0; 826 } else { 827 zeroForOne = itm1 > 0; 828 swapAmount = -itm1; 829 }
- contracts/libraries/PanopticMath.sol
218 if ((below) && (lastObservedTick > entry)) { 219 shift += 1; 220 below = false; 221 } ... 704 if (!(paid0 > balance0 && paid1 > balance1)) { 705 // liquidatee cannot pay back the liquidator fully in either token, so no protocol loss can be avoided 706 if ((paid0 > balance0)) { 707 // liquidatee has insufficient token0 but some token1 left over, so we use what they have left to mitigate token0 losses 708 // we do this by substituting an equivalent value of token1 in our refund to the liquidator, plus a bonus, for the token0 we convert 709 // we want to convert the minimum amount of tokens required to achieve the lowest possible protocol loss (to avoid overpaying on the conversion bonus) 710 // the maximum level of protocol loss mitigation that can be achieved is the liquidatee's excess token1 balance: balance1 - paid1 711 // and paid0 - balance0 is the amount of token0 that the liquidatee is missing, i.e the protocol loss 712 // if the protocol loss is lower than the excess token1 balance, then we can fully mitigate the loss and we should only convert the loss amount 713 // if the protocol loss is higher than the excess token1 balance, we can only mitigate part of the loss, so we should convert only the excess token1 balance 714 // thus, the value converted should be min(balance1 - paid1, paid0 - balance0) 715 bonus1 += Math.min( 716 balance1 - paid1, 717 PanopticMath.convert0to1(paid0 - balance0, sqrtPriceX96Final) 718 ); 719 bonus0 -= Math.min( 720 PanopticMath.convert1to0(balance1 - paid1, sqrtPriceX96Final), 721 paid0 - balance0 722 ); 723 } 724 if ((paid1 > balance1)) { 725 // liquidatee has insufficient token1 but some token0 left over, so we use what they have left to mitigate token1 losses 726 // we do this by substituting an equivalent value of token0 in our refund to the liquidator, plus a bonus, for the token1 we convert 727 // we want to convert the minimum amount of tokens required to achieve the lowest possible protocol loss (to avoid overpaying on the conversion bonus) 728 // the maximum level of protocol loss mitigation that can be achieved is the liquidatee's excess token0 balance: balance0 - paid0 729 // and paid1 - balance1 is the amount of token1 that the liquidatee is missing, i.e the protocol loss 730 // if the protocol loss is lower than the excess token0 balance, then we can fully mitigate the loss and we should only convert the loss amount 731 // if the protocol loss is higher than the excess token0 balance, we can only mitigate part of the loss, so we should convert only the excess token0 balance 732 // thus, the value converted should be min(balance0 - paid0, paid1 - balance1) 733 bonus0 += Math.min( 734 balance0 - paid0, 735 PanopticMath.convert1to0(paid1 - balance1, sqrtPriceX96Final) 736 ); 737 bonus1 -= Math.min( 738 PanopticMath.convert0to1(balance0 - paid0, sqrtPriceX96Final), 739 paid1 - balance1 740 ); 741 } 742 } ... 797 if ( 798 longPremium.rightSlot() < collateralDelta0 && 799 longPremium.leftSlot() > collateralDelta1 800 ) { 801 int256 protocolLoss1 = collateralDelta1; 802 (collateralDelta0, collateralDelta1) = ( 803 -Math.min( 804 collateralDelta0 - longPremium.rightSlot(), 805 PanopticMath.convert1to0( 806 longPremium.leftSlot() - collateralDelta1, 807 sqrtPriceX96Final 808 ) 809 ), 810 Math.min( 811 longPremium.leftSlot() - collateralDelta1, 812 PanopticMath.convert0to1( 813 collateralDelta0 - longPremium.rightSlot(), 814 sqrtPriceX96Final 815 ) 816 ) 817 ); 818 819 haircut0 = longPremium.rightSlot(); 820 haircut1 = protocolLoss1 + collateralDelta1; 821 } else if ( 822 longPremium.leftSlot() < collateralDelta1 && 823 longPremium.rightSlot() > collateralDelta0 824 ) { 825 int256 protocolLoss0 = collateralDelta0; 826 (collateralDelta0, collateralDelta1) = ( 827 Math.min( 828 longPremium.rightSlot() - collateralDelta0, 829 PanopticMath.convert1to0( 830 collateralDelta1 - longPremium.leftSlot(), 831 sqrtPriceX96Final 832 ) 833 ), 834 -Math.min( 835 collateralDelta1 - longPremium.leftSlot(), 836 PanopticMath.convert0to1( 837 longPremium.rightSlot() - collateralDelta0, 838 sqrtPriceX96Final 839 ) 840 ) 841 ); 842 843 haircut0 = collateralDelta0 + protocolLoss0; 844 haircut1 = longPremium.leftSlot(); 845 } else { 846 // for each token, haircut until the protocol loss is mitigated or the premium paid is exhausted 847 haircut0 = Math.min(collateralDelta0, longPremium.rightSlot()); 848 haircut1 = Math.min(collateralDelta1, longPremium.leftSlot()); 849 850 collateralDelta0 = 0; 851 collateralDelta1 = 0; 852 }
218..221 704..742 797..852 821..852
</details>- contracts/types/TokenId.sol
<a name="N-14"></a> To the top
Consider adding minimum/maximum value checks to ensure that the state variables below can never be used to excessively harm users, including via griefing
- contracts/CollateralTracker.sol
// @audit: parameter `assets` is not limited in function body 914 uint256 assets
- contracts/PanopticFactory.sol
// @audit: parameter `amount0Owed` is not limited in function body 173 uint256 amount0Owed, ... // @audit: parameter `amount1Owed` is not limited in function body 174 uint256 amount1Owed, ... // @audit: parameter `fee` is not limited in function body 213 uint24 fee, ... // @audit: parameter `fee` is not limited in function body 339 uint24 fee
- contracts/SemiFungiblePositionManager.sol
// @audit: parameter `amount0Owed` is not limited in function body 403 uint256 amount0Owed, ... // @audit: parameter `amount1Delta` is not limited in function body 437 int256 amount1Delta, ... // @audit: parameter `amount1Owed` is not limited in function body 404 uint256 amount1Owed, ... // @audit: parameter `amount` is not limited in function body 593 function registerTokenTransfer(address from, address to, TokenId id, uint256 amount) internal {
- contracts/libraries/PanopticMath.sol
</details>// @audit: parameter `amount` is not limited in function body 490 function convert0to1(uint256 amount, uint160 sqrtPriceX96) internal pure returns (uint256) {
<a name="N-15"></a> To the top
According to the Solidity style guide function names should be in mixedCase (lowerCamelCase)
- contracts/PanopticPool.sol
677 function _mintInSFPMAndUpdateCollateral( 678 TokenId tokenId, 679 uint128 positionSize, 680 int24 tickLimitLow, 681 int24 tickLimitHigh 682 ) internal returns (uint128) { ... 1450 function getUniV3TWAP() internal view returns (int24 twapTick) {
- contracts/SemiFungiblePositionManager.sol
350 function initializeAMMPool(address token0, address token1, uint24 fee) external { ... 680 function _validateAndForwardToAMM( 681 TokenId tokenId, 682 uint128 positionSize, 683 int24 tickLimitLow, 684 int24 tickLimitHigh, 685 bool isBurn 686 ) internal returns (LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalMoved) { ... 756 function swapInAMM( 757 IUniswapV3Pool univ3pool, 758 LeftRightSigned itmAmounts 759 ) internal returns (LeftRightSigned totalSwapped) { ... 863 function _createPositionInAMM( 864 IUniswapV3Pool univ3pool, 865 TokenId tokenId, 866 uint128 positionSize, 867 bool isBurn 868 ) 869 internal 870 returns ( 871 LeftRightSigned totalMoved, 872 LeftRightUnsigned[4] memory collectedByLeg, 873 LeftRightSigned itmAmounts 874 ) 875 { ... 958 function _createLegInAMM( 959 IUniswapV3Pool univ3pool, 960 TokenId tokenId, 961 uint256 leg, 962 LiquidityChunk liquidityChunk, 963 bool isBurn 964 ) 965 internal 966 returns ( 967 LeftRightSigned moved, 968 LeftRightSigned itmAmounts, 969 LeftRightUnsigned collectedSingleLeg 970 ) 971 {
350 680..686 756..759 863..875 958..971
- contracts/libraries/FeesCalc.sol
97 function calculateAMMSwapFees( 98 IUniswapV3Pool univ3pool, 99 int24 currentTick, 100 int24 tickLower, 101 int24 tickUpper, 102 uint128 liquidity 103 ) public view returns (LeftRightSigned) { ... 130 function _getAMMSwapFeesPerLiquidityCollected( 131 IUniswapV3Pool univ3pool, 132 int24 currentTick, 133 int24 tickLower, 134 int24 tickUpper 135 ) internal view returns (uint256 feeGrowthInside0X128, uint256 feeGrowthInside1X128) {
- contracts/libraries/PanopticMath.sol
607 function _calculateIOAmounts( 608 TokenId tokenId, 609 uint128 positionSize, 610 uint256 legIndex 611 ) internal pure returns (LeftRightSigned longs, LeftRightSigned shorts) {
</details>- contracts/tokens/interfaces/IDonorNFT.sol
else-block not required<a name="N-16"></a> To the top
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;
- contracts/libraries/PanopticMath.sol
325 if (tokenId.asset(legIndex) == 0) { 326 return Math.getLiquidityForAmount0(tickLower, tickUpper, amount); 327 } else { 328 return Math.getLiquidityForAmount1(tickLower, tickUpper, amount); 329 } ... 425 if (tokenType == 0) { 426 return ( 427 tokenData0.rightSlot() + convert1to0(tokenData1.rightSlot(), sqrtPriceX96), 428 tokenData0.leftSlot() + convert1to0(tokenData1.leftSlot(), sqrtPriceX96) 429 ); 430 } else { 431 return ( 432 tokenData1.rightSlot() + convert0to1(tokenData0.rightSlot(), sqrtPriceX96), 433 tokenData1.leftSlot() + convert0to1(tokenData0.leftSlot(), sqrtPriceX96) 434 ); 435 } ... 494 if (sqrtPriceX96 < type(uint128).max) { 495 return Math.mulDiv192(amount, uint256(sqrtPriceX96) ** 2); 496 } else { 497 return Math.mulDiv128(amount, Math.mulDiv64(sqrtPriceX96, sqrtPriceX96)); 498 } ... 511 if (sqrtPriceX96 < type(uint128).max) { 512 return Math.mulDiv(amount, 2 ** 192, uint256(sqrtPriceX96) ** 2); 513 } else { 514 return Math.mulDiv(amount, 2 ** 128, Math.mulDiv64(sqrtPriceX96, sqrtPriceX96)); 515 } ... 528 if (sqrtPriceX96 < type(uint128).max) { 529 int256 absResult = Math 530 .mulDiv192(Math.absUint(amount), uint256(sqrtPriceX96) ** 2) 531 .toInt256(); 532 return amount < 0 ? -absResult : absResult; 533 } else { 534 int256 absResult = Math 535 .mulDiv128(Math.absUint(amount), Math.mulDiv64(sqrtPriceX96, sqrtPriceX96)) 536 .toInt256(); 537 return amount < 0 ? -absResult : absResult; 538 } ... 551 if (sqrtPriceX96 < type(uint128).max) { 552 int256 absResult = Math 553 .mulDiv(Math.absUint(amount), 2 ** 192, uint256(sqrtPriceX96) ** 2) 554 .toInt256(); 555 return amount < 0 ? -absResult : absResult; 556 } else { 557 int256 absResult = Math 558 .mulDiv( 559 Math.absUint(amount), 560 2 ** 128, 561 Math.mulDiv64(sqrtPriceX96, sqrtPriceX96) 562 ) 563 .toInt256(); 564 return amount < 0 ? -absResult : absResult; 565 }
325..329 425..435 494..498 511..515 528..538 551..565
- contracts/types/TokenId.sol
</details>439 if (optionRatios < 2 ** 64) { 440 return 0; 441 } else if (optionRatios < 2 ** 112) { 442 return 1; 443 } else if (optionRatios < 2 ** 160) { 444 return 2; 445 } else if (optionRatios < 2 ** 208) { 446 return 3; 447 } ... 441 } else if (optionRatios < 2 ** 112) { 442 return 1; 443 } else if (optionRatios < 2 ** 160) { 444 return 2; 445 } else if (optionRatios < 2 ** 208) { 446 return 3; 447 } ... 443 } else if (optionRatios < 2 ** 160) { 444 return 2; 445 } else if (optionRatios < 2 ** 208) { 446 return 3; 447 }
<a name="N-17"></a> To the top
Consider adding some comments on critical state variables to explain what they are supposed to do: this will help for future code reviews.
- contracts/PanopticPool.sol
120 bool internal constant DONOT_COMMIT_LONG_SETTLED = false; ... 133 bool internal constant SLOW_ORACLE_UNISWAP_MODE = false; ... 136 uint256 internal constant MEDIAN_PERIOD = 60; ... 156 int256 internal constant MAX_TWAP_DELTA_LIQUIDATION = 513; ... 171 uint256 internal constant BP_DECREASE_BUFFER = 13_333; ... 174 uint256 internal constant NO_BUFFER = 10_000;
- contracts/SemiFungiblePositionManager.sol
</details>126 bool internal constant BURN = true; ... 133 uint128 private constant VEGOID = 2; ... 289 mapping(bytes32 positionKey => LeftRightUnsigned accountPremium) private s_accountPremiumGross;
error should be used rather than require/assert<a name="N-18"></a> To the top
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.
- contracts/libraries/Math.sol
361 require(denominator > 0); ... 370 require(denominator > prod1); ... 448 require(result < type(uint256).max); ... 484 require(2 ** 64 > prod1); ... 547 require(2 ** 96 > prod1); ... 588 require(result < type(uint256).max); ... 624 require(2 ** 128 > prod1); ... 665 require(result < type(uint256).max); ... 701 require(2 ** 192 > prod1);
361 370 448 484 547 588 624 665 701
</details><a name="N-19"></a> To the top
There are units for seconds, minutes, hours, days, and weeks, and since they're defined, they should be used
- contracts/PanopticPool.sol
136 uint256 internal constant MEDIAN_PERIOD = 60; ... 148 uint256 internal constant SLOW_ORACLE_CARDINALITY = 7; ... 313 (uint256(uint24(currentTick)) << 24) + // add to slot 4
- contracts/libraries/PanopticMath.sol
</details>178 (int24(uint24(medianData >> ((uint24(medianData >> (192 + 3 * 3)) % 8) * 24))) + ... 179 int24(uint24(medianData >> ((uint24(medianData >> (192 + 3 * 4)) % 8) * 24)))) / ... 211 if (rank == 7) { ... 217 entry = int24(uint24(medianData >> (rank * 24))); ... 229 uint256(uint192(medianData << 24)) +
<a name="N-20"></a> To the top
Comment out the variable name to suppress compiler warnings
- contracts/libraries/Math.sol
340 function mulDiv( 341 uint256 a, 342 uint256 b, 343 uint256 denominator 344 ) internal pure returns (uint256 result) { ... 458 function mulDiv64(uint256 a, uint256 b) internal pure returns (uint256) { ... 521 function mulDiv96(uint256 a, uint256 b) internal pure returns (uint256) { ... 598 function mulDiv128(uint256 a, uint256 b) internal pure returns (uint256) { ... 675 function mulDiv192(uint256 a, uint256 b) internal pure returns (uint256) { ... 738 function unsafeDivRoundingUp(uint256 a, uint256 b) internal pure returns (uint256 result) {
- contracts/libraries/SafeTransferLib.sol
</details>21 function safeTransferFrom(address token, address from, address to, uint256 amount) internal { ... 52 function safeTransfer(address token, address to, uint256 amount) internal {
<a name="N-21"></a> To the top
Ensure that events follow the best practice of check-effects-interaction, and are emitted before external calls
- contracts/CollateralTracker.sol
417 function deposit(uint256 assets, address receiver) external returns (uint256 shares) { ... 477 function mint(uint256 shares, address receiver) external returns (uint256 assets) { ... 531 function withdraw( 532 uint256 assets, 533 address receiver, 534 address owner 535 ) external returns (uint256 shares) { ... 591 function redeem( 592 uint256 shares, 593 address receiver, 594 address owner 595 ) external returns (uint256 assets) {
- contracts/PanopticFactory.sol
210 function deployNewPool( 211 address token0, 212 address token1, 213 uint24 fee, 214 bytes32 salt 215 ) external returns (PanopticPool newPoolContract) {
- contracts/PanopticPool.sol
</details>1017 function liquidate( 1018 TokenId[] calldata positionIdListLiquidator, 1019 address liquidatee, 1020 LeftRightUnsigned delegations, 1021 TokenId[] calldata positionIdList 1022 ) external { ... 1179 function forceExercise( 1180 address account, 1181 TokenId[] calldata touchedId, 1182 TokenId[] calldata positionIdListExercisee, 1183 TokenId[] calldata positionIdListExercisor 1184 ) external {
immutable rather than constant<a name="N-22"></a> To the top
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.
- contracts/PanopticPool.sol
103 int24 internal constant MIN_SWAP_TICK = Constants.MIN_V3POOL_TICK + 1; ... 105 int24 internal constant MAX_SWAP_TICK = Constants.MAX_V3POOL_TICK - 1; ... 165 uint64 internal constant MAX_SPREAD = 9 * (2 ** 32);
- contracts/libraries/Constants.sol
12 int24 internal constant MIN_V3POOL_TICK = -887272;
- contracts/libraries/Math.sol
15 uint256 internal constant MAX_UINT256 = 2 ** 256 - 1;
- contracts/libraries/PanopticMath.sol
23 uint256 internal constant MAX_UINT256 = 2 ** 256 - 1;
- contracts/types/LeftRight.sol
</details>26 int256 internal constant LEFT_HALF_BIT_MASK_INT = 27 int256(uint256(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00000000000000000000000000000000));
<a name="N-23"></a> To the top
Events help non-contract tools to track changes, and timelocks prevent users from being surprised by changes
- contracts/PanopticPool.sol
859 function _updatePositionDataBurn(address owner, TokenId tokenId) internal { ... 1405 function _updatePositionsHash(address account, TokenId tokenId, bool addFlag) internal { ... 1587 function settleLongPremium( 1588 TokenId[] calldata positionIdList, 1589 address owner, 1590 uint256 legIndex 1591 ) external { ... 1666 function _updateSettlementPostMint( 1667 TokenId tokenId, 1668 LeftRightUnsigned[4] memory collectedByLeg, 1669 uint128 positionSize 1670 ) internal { ... 1833 function _updateSettlementPostBurn( 1834 address owner, 1835 TokenId tokenId, 1836 LeftRightUnsigned[4] memory collectedByLeg, 1837 uint128 positionSize, 1838 bool commitLongSettled 1839 ) internal returns (LeftRightSigned realizedPremia, LeftRightSigned[4] memory premiaByLeg) {
859 1405 1587..1591 1666..1670 1833..1839
- contracts/tokens/ERC1155Minimal.sol
</details>81 function setApprovalForAll(address operator, bool approved) public {
<a name="N-24"></a> To the top
This especially problematic when the setter also emits the same value, which may be confusing to offline parsers
- contracts/CollateralTracker.sol
221 function startToken( 222 bool underlyingIsToken0, 223 address token0, 224 address token1, 225 uint24 fee, 226 PanopticPool panopticPool 227 ) external {
- contracts/PanopticFactory.sol
134 function initialize(address _owner) public { ... 147 function transferOwnership(address newOwner) external {
- contracts/PanopticPool.sol
291 function startPool( 292 IUniswapV3Pool _univ3pool, 293 address token0, 294 address token1, 295 CollateralTracker collateralTracker0, 296 CollateralTracker collateralTracker1 297 ) external {
- contracts/tokens/ERC1155Minimal.sol
81 function setApprovalForAll(address operator, bool approved) public {
- contracts/tokens/ERC20Minimal.sol
</details>49 function approve(address spender, uint256 amount) public returns (bool) {
<a name="N-25"></a> To the top
error: `initalized` should be `initialized` --> project\2024-04-panoptic\contracts\CollateralTracker.sol:92:92 | 92 | /// @dev As each instance is deployed via proxy clone, initial parameters must only be initalized once via startToken(). | ^^^^^^^^^^ |
error: `caluculation` should be `calculation` --> project\2024-04-panoptic\contracts\PanopticPool.sol:107:42 | 107 | // Flags used as arguments to premia caluculation functions | ^^^^^^^^^^^^ | error: `wether` should be `weather`, `whether` --> project\2024-04-panoptic\contracts\PanopticPool.sol:122:40 | 122 | /// @dev Boolean flag to determine wether a position is added (true) or not (!ADD = false) | ^^^^^^ | error: `Mananger` should be `Manager` --> project\2024-04-panoptic\contracts\PanopticPool.sol:278:122 | 278 | /// @notice During construction: sets the address of the panoptic factory smart contract and the SemiFungiblePositionMananger (SFPM). | ^^^^^^^^ |
error: `postion` should be `position` --> project\2024-04-panoptic\contracts\SemiFungiblePositionManager.sol:1106:79 | 1106 | /// @notice caches/stores the accumulated premia values for the specified postion. | ^^^^^^^ | error: `seperated` should be `separated` --> project\2024-04-panoptic\contracts\SemiFungiblePositionManager.sol:1337:32 | 1337 | // premia, and is only seperated to simplify the calculation | ^^^^^^^^^ |
error: `avaiable` should be `available` --> project\2024-04-panoptic\contracts\libraries\Errors.sol:62:51 | 62 | /// @notice PanopticPool: there is not enough avaiable liquidity to buy an option | ^^^^^^^^ |
error: `accomodate` should be `accommodate` --> project\2024-04-panoptic\contracts\libraries\PanopticMath.sol:486:67 | 486 | /// @dev Uses reduced precision after tick 443636 in order to accomodate the full range of tick.s | ^^^^^^^^^^ | error: `accomodate` should be `accommodate` --> project\2024-04-panoptic\contracts\libraries\PanopticMath.sol:503:67 | 503 | /// @dev Uses reduced precision after tick 443636 in order to accomodate the full range of ticks. | ^^^^^^^^^^ | error: `accomodate` should be `accommodate` --> project\2024-04-panoptic\contracts\libraries\PanopticMath.sol:520:67 | 520 | /// @dev Uses reduced precision after tick 443636 in order to accomodate the full range of ticks. | ^^^^^^^^^^ | error: `accomodate` should be `accommodate` --> project\2024-04-panoptic\contracts\libraries\PanopticMath.sol:543:67 | 543 | /// @dev Uses reduced precision after tick 443636 in order to accomodate the full range of ticks. | ^^^^^^^^^^ | error: `consistentcy` should be `consistently` --> project\2024-04-panoptic\contracts\libraries\PanopticMath.sol:663:51 | 663 | // evaluate at TWAP price to keep consistentcy with solvency calculations | ^^^^^^^^^^^^ | error: `commited` should be `committed` --> project\2024-04-panoptic\contracts\libraries\PanopticMath.sol:886:52 | 886 | // The long premium is not commited to storage during the liquidation, so we add the entire adjusted amount | ^^^^^^^^ |
</details>error: `explictily` should be `explicitly` --> project\2024-04-panoptic\contracts\types\LeftRight.sol:153:84 | 153 | // adding leftRight packed uint128's is same as just adding the values explictily | ^^^^^^^^^^ | error: `occured` should be `occurred` --> project\2024-04-panoptic\contracts\types\LeftRight.sol:159:37 | 159 | // then an overflow has occured | ^^^^^^^ | error: `explictily` should be `explicitly` --> project\2024-04-panoptic\contracts\types\LeftRight.sol:176:94 | 176 | // subtracting leftRight packed uint128's is same as just subtracting the values explictily | ^^^^^^^^^^ | error: `occured` should be `occurred` --> project\2024-04-panoptic\contracts\types\LeftRight.sol:182:38 | 182 | // then an underflow has occured | ^^^^^^^ |
delete rather than assigning zero/false to clear values<a name="N-26"></a> To the top
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
- contracts/SemiFungiblePositionManager.sol
332 s_poolContext[poolId].locked = false;
- contracts/libraries/PanopticMath.sol
220 below = false; ... 850 collateralDelta0 = 0; ... 851 collateralDelta1 = 0;
- contracts/types/TokenId.sol
</details>377 optionRatios = 0;
override is unnecessary<a name="N-27"></a> To the top
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.
- contracts/CollateralTracker.sol
323 function transfer( 324 address recipient, 325 uint256 amount 326 ) public override(ERC20Minimal) returns (bool) { ... 341 function transferFrom( 342 address from, 343 address to, 344 uint256 amount 345 ) public override(ERC20Minimal) returns (bool) {
- contracts/SemiFungiblePositionManager.sol
</details>540 function safeTransferFrom( 541 address from, 542 address to, 543 uint256 id, 544 uint256 amount, 545 bytes calldata data 546 ) public override { ... 566 function safeBatchTransferFrom( 567 address from, 568 address to, 569 uint256[] calldata ids, 570 uint256[] calldata amounts, 571 bytes calldata data 572 ) public override {
address(this)<a name="N-28"></a> To the top
- contracts/CollateralTracker.sol
323 function transfer( 324 address recipient, 325 uint256 amount 326 ) public override(ERC20Minimal) returns (bool) {
- contracts/libraries/SafeTransferLib.sol
52 function safeTransfer(address token, address to, uint256 amount) internal {
- contracts/tokens/ERC20Minimal.sol
61 function transfer(address to, uint256 amount) public virtual returns (bool) {
- contracts/tokens/interfaces/IERC20Partial.sol
</details>27 function transfer(address to, uint256 amount) external;
@inheritdoc for overridden functions<a name="N-29"></a> To the top
It is recommended to use @inheritdoc for overridden functions.
- contracts/CollateralTracker.sol
319 /// @dev See {IERC20-transfer}. 320 /// Requirements: 321 /// - the caller must have a balance of at least 'amount'. 322 /// - the msg.sender must not have any position on the panoptic pool 323 function transfer( 324 address recipient, 325 uint256 amount 326 ) public override(ERC20Minimal) returns (bool) { ... 336 /// @dev See {IERC20-transferFrom}. 337 /// Requirements: 338 /// - the 'from' must have a balance of at least 'amount'. 339 /// - the caller must have allowance for 'from' of at least 'amount' tokens. 340 /// - 'from' must not have any open positions on the panoptic pool. 341 function transferFrom( 342 address from, 343 address to, 344 uint256 amount 345 ) public override(ERC20Minimal) returns (bool) {
- contracts/SemiFungiblePositionManager.sol
</details>533 /// @notice Transfer a single token from one user to another 534 /// @dev supports token approvals 535 /// @param from the user to transfer tokens from 536 /// @param to the user to transfer tokens to 537 /// @param id the ERC1155 token id to transfer 538 /// @param amount the amount of tokens to transfer 539 /// @param data optional data to include in the receive hook 540 function safeTransferFrom( 541 address from, 542 address to, 543 uint256 id, 544 uint256 amount, 545 bytes calldata data 546 ) public override { ... 558 /// @notice Transfer multiple tokens from one user to another 559 /// @dev supports token approvals 560 /// @dev ids and amounts must be of equal length 561 /// @param from the user to transfer tokens from 562 /// @param to the user to transfer tokens to 563 /// @param ids the ERC1155 token ids to transfer 564 /// @param amounts the amounts of tokens to transfer 565 /// @param data optional data to include in the receive hook 566 function safeBatchTransferFrom( 567 address from, 568 address to, 569 uint256[] calldata ids, 570 uint256[] calldata amounts, 571 bytes calldata data 572 ) public override {
msg.sender parameter<a name="N-30"></a> To the top
If msg.sender play a part in the functionality of a function, any emits of this function should include msg.sender to ensure transparency with users
- contracts/PanopticFactory.sol
// @audit Current function use `msg.sender`, but do not emit it 154 emit OwnershipTransferred(currentOwner, newOwner); ... // @audit Current function use `msg.sender`, but do not emit it 268 emit PoolDeployed( 269 newPoolContract, 270 v3Pool, 271 collateralTracker0, 272 collateralTracker1, 273 amount0, 274 amount1 275 );
- contracts/tokens/ERC20Minimal.sol
</details>// @audit Current function use `msg.sender`, but do not emit it 94 emit Transfer(from, to, amount);
constructor missing zero address check<a name="N-31"></a> To the top
It is important to ensure that the constructor does not allow zero address to be set. This is a common mistake that can lead to loss of funds or redeployment of the contract.
- contracts/PanopticFactory.sol
115 constructor( 116 address _WETH9, 117 SemiFungiblePositionManager _SFPM, 118 IUniswapV3Factory _univ3Factory, 119 IDonorNFT _donorNFT, 120 address _poolReference, 121 address _collateralReference 122 ) {
- contracts/PanopticPool.sol
280 constructor(SemiFungiblePositionManager _sfpm) {
- contracts/SemiFungiblePositionManager.sol
</details>341 constructor(IUniswapV3Factory _factory) {
<a name="N-32"></a> To the top
Doing so will significantly increase centralization, but will help to prevent hackers from using stolen tokens
- contracts/CollateralTracker.sol
36 contract CollateralTracker is ERC20Minimal, Multicall { 37 // Used for safecasting 38 using Math for uint256;
- contracts/PanopticFactory.sol
26 contract PanopticFactory is Multicall { 27 /*////////////////////////////////////////////////////////////// 28 EVENTS 29 //////////////////////////////////////////////////////////////*/ 30 31 /// @notice Emitted when the ownership of the factory is transferred. 32 /// @param oldOwner The previous owner of the factory 33 /// @param newOwner The new owner of the factory 34 event OwnershipTransferred(address indexed oldOwner, address indexed newOwner);
- contracts/SemiFungiblePositionManager.sol
</details>72 contract SemiFungiblePositionManager is ERC1155, Multicall { 73 /*////////////////////////////////////////////////////////////// 74 EVENTS 75 //////////////////////////////////////////////////////////////*/ 76 77 /// @notice Emitted when a UniswapV3Pool is initialized. 78 /// @param uniswapPool Address of the underlying Uniswap v3 pool 79 /// @param poolId The SFPM's pool identifier for the pool, including the 16-bit tick spacing and 48-bit pool pattern 80 event PoolInitialized(address indexed uniswapPool, uint64 poolId);
constant/immutable variables<a name="N-33"></a> To the top
If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
- contracts/PanopticFactory.sol
</details>116 address _WETH9, ... 117 SemiFungiblePositionManager _SFPM,
<a name="N-34"></a> To the top
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 before use, which will lead to worse optimizations. Update the free memory pointer and annotate the block (assembly (\"memory-safe\") { ... }) to avoid this issue.
- contracts/libraries/SafeTransferLib.sol
</details>24 assembly ("memory-safe") { 25 // Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here. 26 let p := mload(0x40) 27 28 // Write the abi-encoded calldata into memory, beginning with the function selector. 29 mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000) 30 mstore(add(4, p), from) // Append the "from" argument. 31 mstore(add(36, p), to) // Append the "to" argument. 32 mstore(add(68, p), amount) // Append the "amount" argument. 33 34 success := and( 35 // Set success to whether the call reverted, if not we check it either 36 // returned exactly 1 (can't just be non-zero data), or had no return data. 37 or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())), 38 // We use 100 because that's the total length of our calldata (4 + 32 * 3) 39 // Counterintuitively, this call() must be positioned after the or() in the 40 // surrounding and() because and() evaluates its arguments from right to left. 41 call(gas(), token, 0, p, 100, 0, 32) 42 ) 43 } ... 55 assembly ("memory-safe") { 56 // Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here. 57 let p := mload(0x40) 58 59 // Write the abi-encoded calldata into memory, beginning with the function selector. 60 mstore(p, 0xa9059cbb00000000000000000000000000000000000000000000000000000000) 61 mstore(add(4, p), to) // Append the "to" argument. 62 mstore(add(36, p), amount) // Append the "amount" argument. 63 64 success := and( 65 // Set success to whether the call reverted, if not we check it either 66 // returned exactly 1 (can't just be non-zero data), or had no return data. 67 or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())), 68 // We use 68 because that's the total length of our calldata (4 + 32 * 2) 69 // Counterintuitively, this call() must be positioned after the or() in the 70 // surrounding and() because and() evaluates its arguments from right to left. 71 call(gas(), token, 0, p, 68, 0, 32) 72 ) 73 }
<a name="N-35"></a> To the top
According to the Solidity Style Guide, contracts and libraries should be named using the CapWords style.
- contracts/tokens/ERC1155Minimal.sol
11 abstract contract ERC1155 { 12 /*////////////////////////////////////////////////////////////// 13 EVENTS 14 //////////////////////////////////////////////////////////////*/ 15 16 /// @notice Emitted when only a single token is transferred. 17 /// @param operator The user who initiated the transfer 18 /// @param from The user who sent the tokens 19 /// @param to The user who received the tokens 20 /// @param id The ERC1155 token id 21 /// @param amount The amount of tokens transferred 22 event TransferSingle(
- contracts/tokens/ERC20Minimal.sol
</details>9 abstract contract ERC20Minimal { 10 /*////////////////////////////////////////////////////////////// 11 EVENTS 12 //////////////////////////////////////////////////////////////*/ 13 14 /// @notice Emitted when tokens are transferred 15 /// @param from The sender of the tokens 16 /// @param to The recipient of the tokens 17 /// @param amount The amount of tokens transferred 18 event Transfer(address indexed from, address indexed to, uint256 amount);
<a name="N-36"></a> To the top
When a contract function accepts Ethereum and executes a .call() or similar function that also forwards Ethereum value, it's important to check for and refund any remaining balance. This is because some of the supplied value may not be used during the call execution due to gas constraints, a revert in the called contract, or simply because not all the value was needed.
If you do not account for this remaining balance, it can become "locked" in the contract. It's crucial to either return the remaining balance to the sender or handle it in a way that ensures it is not permanently stuck. Neglecting to do so can lead to loss of funds and degradation of the contract's reliability. Furthermore, it's good practice to ensure fairness and trust with your users by returning unused funds.
- contracts/multicall/Multicall.sol
</details>12 function multicall(bytes[] calldata data) public payable returns (bytes[] memory results) {
<a name="N-37"></a> To the top
- contracts/PanopticFactory.sol
</details>134 function initialize(address _owner) public {
#0 - c4-judge
2024-04-26T17:18:01Z
Picodes marked the issue as grade-b