Add Multiple Security Rule Checks for DeFi Projects #2388
Conversation
BradMoonUESTC
requested review from
montyly,
0xalpharush and
smonicas
as code owners
|
xueyue seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis update introduces a broad range of new detectors in Slither, focusing on DeFi actions, price manipulation, centralized risk, and transaction order dependencies. It enhances Slither's ability to identify potential vulnerabilities and risks within smart contracts, particularly in the context of DeFi projects and centralized control mechanisms. Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 36
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (44)
- slither/detectors/all_detectors.py (1 hunks)
- slither/detectors/defi/defi_action_nested.py (1 hunks)
- slither/detectors/defi/k_value_error.py (1 hunks)
- slither/detectors/defi/price_manipulation_high.py (1 hunks)
- slither/detectors/defi/price_manipulation_info.py (1 hunks)
- slither/detectors/defi/price_manipulation_low.py (1 hunks)
- slither/detectors/defi/price_manipulation_medium.py (1 hunks)
- slither/detectors/defi/price_manipulation_tools.py (1 hunks)
- slither/detectors/functions/centralized_info.py (1 hunks)
- slither/detectors/functions/centralized_init_supply.py (1 hunks)
- slither/detectors/functions/centralized_low.py (1 hunks)
- slither/detectors/functions/centralized_medium.py (1 hunks)
- slither/detectors/functions/centralized_other.py (1 hunks)
- slither/detectors/functions/centralized_utils.py (1 hunks)
- slither/detectors/functions/modifier_utils.py (1 hunks)
- slither/detectors/functions/transaction_order_dependency_high.py (1 hunks)
- slither/detectors/functions/transaction_order_dependency_low.py (1 hunks)
- slither/detectors/functions/transaction_order_dependency_medium.py (1 hunks)
- slither/utils/function_dependency_tree.py (1 hunks)
- slither/utils/function_permission_check.py (1 hunks)
- tests/e2e/detectors/test_data/centralized-init-supply/0.8.6/test.sol (1 hunks)
- tests/e2e/detectors/test_data/centralized-risk-medium/0.4.24/Hundred.sol (1 hunks)
- tests/e2e/detectors/test_data/centralized-risk-medium/0.5.10/smartypay.sol (1 hunks)
- tests/e2e/detectors/test_data/centralized-risk-medium/0.6.11/PancakeBunnyPriceCalculatorBSC-pancakeBunny-45m.sol (1 hunks)
- tests/e2e/detectors/test_data/centralized-risk-medium/0.6.12/VOLT.sol (1 hunks)
- tests/e2e/detectors/test_data/centralized-risk-medium/0.7.4/ghost.sol (1 hunks)
- tests/e2e/detectors/test_data/centralized-risk-medium/0.7.6/vvisor.sol (1 hunks)
- tests/e2e/detectors/test_data/centralized-risk-medium/0.8.19/test.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.6.11/CommunityFund.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.6.11/StakeContract.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.6.11/StrategyEllipsisImpl.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.8.0/ArrayFinance.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.8.0/WUSDMaster.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.8.2/JAY.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.8.2/Keep3rV2Oracle.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.8.2/PancakeOracle.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.8.2/SmartChefFactory.sol (1 hunks)
- tests/e2e/detectors/test_data/defi-action-nested/0.8.2/defi_price_manipulation.sol (1 hunks)
- tests/e2e/detectors/test_data/price-manipulation-high/0.5.10/bnbcorp.sol (1 hunks)
- tests/e2e/detectors/test_data/price-manipulation-high/0.5.12/yDAI-EVENT-Value DeFi-21m.sol (1 hunks)
- tests/e2e/detectors/test_data/price-manipulation-high/0.6.11/PancakeBunnyPriceCalculatorBSC-pancakeBunny-45m.sol (1 hunks)
- tests/e2e/detectors/test_data/price-manipulation-high/0.6.11/StrategyEllipsisImpl-BeltFinance-6m.sol (1 hunks)
- tests/e2e/detectors/test_data/price-manipulation-high/0.8.0/ArrayFinance-unknown.sol (1 hunks)
- tests/e2e/detectors/test_data/price-manipulation-high/0.8.0/Context.sol (1 hunks)
Files not summarized due to errors (3)
- tests/e2e/detectors/test_data/defi-action-nested/0.6.11/CommunityFund.sol: Error: Message exceeds token limit
- tests/e2e/detectors/test_data/defi-action-nested/0.6.11/StrategyEllipsisImpl.sol: Error: Message exceeds token limit
- tests/e2e/detectors/test_data/price-manipulation-high/0.6.11/StrategyEllipsisImpl-BeltFinance-6m.sol: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- tests/e2e/detectors/test_data/price-manipulation-high/0.8.0/Context.sol
Additional Context Used
Additional comments not posted (125)
tests/e2e/detectors/test_data/centralized-init-supply/0.8.6/test.sol (1)
17-19: ThegetStateVarfunction implementation looks good and adheres to best practices.slither/detectors/defi/k_value_error.py (1)
57-60: The comparison of regex matches might not work as intended because it compares lists. Consider revising the logic to ensure it accurately identifies mismatches in the patterns found.Please verify the comparison logic for regex matches to ensure it functions as intended.
tests/e2e/detectors/test_data/centralized-risk-medium/0.8.19/test.sol (1)
37-50: The overriddentransferfunction includes logic for fee calculation and enforcing daily transfer limits. Ensure that theERC20base contract supports transferring fees to the owner in this manner, as this might not be standard behavior for allERC20implementations.Please verify the
ERC20implementation to ensure it supports the intended fee transfer behavior.slither/utils/function_permission_check.py (1)
32-64: Thefunction_has_caller_checkfunction's implementation is comprehensive and covers various scenarios for caller validation checks. Good job on ensuring thoroughness in security checks.slither/detectors/functions/modifier_utils.py (3)
8-22: The methodis_reentrancy_lockchecks for reentrancy locks by analyzing state variable reads and writes, and the presence of a placeholder. However, the logic might be too simplistic and could miss more complex reentrancy patterns. Consider enhancing the detection logic to cover more sophisticated reentrancy mechanisms.
24-35: The recursive approach in_get_function_variables_read_recursivelyis a good strategy for collecting variables read across function calls. However, ensure that there's a mechanism to prevent infinite recursion in cases of circular call dependencies.
37-46: The method_has_msg_sender_check_neweffectively checks formsg.senderusage in function modifiers. This is crucial for identifying functions with access control. However, consider adding comments to explain the rationale behind checking both the function and its modifiers formsg.sender.slither/detectors/functions/centralized_low.py (3)
66-77: The detector skips functions namedtransferandtransferFromto avoid false positives, which is a reasonable approach. However, ensure that this does not inadvertently exclude legitimate cases of centralized risk. Consider adding a comment explaining the rationale behind excluding these specific function names.
69-77: The methodcheck_if_state_vars_read_from_critical_riskis used to identify functions that read state variables associated with critical risks. Ensure that this method accurately identifies all relevant state variables and does not produce false negatives.
71-76: The logic for appendingcentralized_info_functionstocontract_infoand generating results seems sound. However, consider optimizing the data structure used forcontract_infoto avoid potential performance issues with large contracts.slither/detectors/functions/centralized_info.py (1)
66-77: Similar to the previous file, this detector skipstransferandtransferFromfunctions. As mentioned earlier, ensure that this does not miss legitimate cases of centralized risk. Additionally, consider abstracting common logic between this file andcentralized_low.pyto reduce code duplication.slither/detectors/functions/transaction_order_dependency_low.py (2)
57-63: The methodanalyzeTODanalyzes pairs of functions for transaction order dependencies. Ensure that the logic accurately identifies dependencies and does not produce false positives or negatives. Consider adding more comprehensive tests to cover various scenarios.
1-82: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-83]
The filtering logic for
permissionless_functionsis crucial for identifying functions that could contribute to transaction order dependencies. Ensure that the conditions for excluding functions are comprehensive and accurately reflect the intended detection scope.slither/detectors/functions/centralized_init_supply.py (1)
64-83: The logic for detecting centralized risks related to initial supply distribution is sound. However, the hardcoded checks for variable names like"balances(address)"might not cover all possible naming conventions. Consider enhancing the detection logic to be more flexible regarding variable naming.slither/detectors/functions/transaction_order_dependency_high.py (2)
28-43: The inclusion of anERC20_FUNCTIONlist is a good approach to focusing the analysis on relevant functions. However, ensure that this list is comprehensive and up-to-date with common ERC20 and related token standards. Consider externalizing this list for easier maintenance and updates.
75-99: The filtering logic forpermissionless_functionsand the subsequent analysis for transaction order dependencies are crucial. Ensure that the conditions for including functions in the analysis accurately reflect the intended detection scope, especially regarding token and money flow.slither/detectors/functions/transaction_order_dependency_medium.py (2)
26-66: The methodanalyzeTODand its helperanalyzePairFunctionare designed to identify medium-risk transaction order dependencies. Ensure that the distinction between medium, low, and high-risk dependencies is clear and justified. Consider adding comments or documentation to explain the criteria for classifying the risk levels.
75-103: The filtering logic forpermissionless_functionsand the construction of dependency trees are key components of this detector. As with other similar detectors, ensure that the logic accurately reflects the intended scope and does not exclude relevant functions or include irrelevant ones.slither/detectors/all_detectors.py (1)
101-114: The addition of new DeFi-related detector classes enhances Slither's capability to identify potential security risks in DeFi projects. Ensure that each of these new detectors is thoroughly tested to validate their effectiveness in detecting the intended vulnerabilities.tests/e2e/detectors/test_data/centralized-risk-medium/0.5.10/smartypay.sol (1)
1-221: TheSmartyPaycontract serves as a test case for the centralized risk medium detector. It's crucial to ensure that the contract covers a wide range of scenarios that could lead to centralized risks, including but not limited to ownership control, token distribution methods, and access control mechanisms.slither/utils/function_dependency_tree.py (2)
89-110: The implementation ofweak_dependandstrong_dependfunctions provides a foundational mechanism for analyzing function dependencies within contracts. It's important to ensure that these functions accurately capture the intended dependencies, especially in the context of DeFi contracts where such dependencies can lead to vulnerabilities.
189-218: Thebuild_dependency_tree_token_flow_or_money_flowfunction extends the dependency analysis to specifically address token flow or money flow scenarios. Given the critical nature of these flows in DeFi contracts, it's essential to validate the accuracy and comprehensiveness of this analysis through extensive testing.slither/detectors/defi/price_manipulation_info.py (1)
26-209: ThePriceManipulationInfodetector is a valuable addition to Slither's suite of security analysis tools for DeFi projects. It's crucial to ensure that the logic for identifying potential price manipulation risks is both accurate and comprehensive, covering various scenarios where price manipulation could occur. Additionally, consider enhancing the documentation within the code to provide more context on how each part of the detector contributes to identifying price manipulation risks.tests/e2e/detectors/test_data/price-manipulation-high/0.5.10/bnbcorp.sol (1)
1-364: TheBNBCropcontract serves as a test case for the price manipulation high detector. It's important to ensure that the contract scenarios are representative of real-world DeFi projects where price manipulation could be a concern. This includes testing various investment, withdrawal, and reinvestment patterns under different market conditions.slither/detectors/defi/price_manipulation_low.py (5)
27-30: The class documentation provides a clear overview of the detector's purpose. However, it might be beneficial to expand on how the detector identifies low-severity price manipulation risks specifically, as the current description is somewhat generic.
47-54: TheERC20_FUNCTIONlist is commented out for some functions likebalanceOfandtotalSupply. If these functions are not relevant for detecting low-severity price manipulation, it's good practice to remove them entirely rather than leaving them commented out, to avoid confusion.
92-113: The recursive method_get_calls_recursivelyis a critical part of the detection logic. Ensure that themaxdepthparameter is carefully chosen to balance thoroughness with performance, especially for complex contracts.
115-120: The method_check_if_can_output_call_infouses complex conditions to determine if a call's information can be output. Consider simplifying these conditions or breaking them into smaller, named predicates to enhance readability.
222-257: The_detectmethod effectively aggregates detection results. However, the code for generating theinfomessages is somewhat repetitive and could be refactored into a separate method to reduce duplication and improve clarity.slither/detectors/defi/price_manipulation_high.py (4)
26-29: The class documentation succinctly explains the detector's purpose. Given the high severity of the risks it identifies, including examples or more detailed explanations of the types of vulnerabilities detected could provide additional clarity.
46-53: TheERC20_FUNCTIONlist is focused on functions relevant to high-severity price manipulation. Ensure that all necessary functions are included and consider removing or adding comments to explain the selection criteria for these functions.
86-108: The recursive method_get_calls_recursivelyis crucial for the detection logic. As with the low-severity detector, ensure themaxdepthparameter is appropriately chosen to avoid performance issues.
216-254: The_detectmethod effectively compiles detection results. Consider refactoring the message generation code to reduce duplication and enhance code clarity, similar to the suggestion for the low-severity detector.slither/detectors/defi/price_manipulation_medium.py (4)
27-30: The class documentation clearly outlines the detector's purpose. Including specific examples or more detailed explanations of medium-severity vulnerabilities could enhance understanding.
47-55: TheERC20_FUNCTIONlist includes functions relevant to medium-severity price manipulation. Ensure that the list is comprehensive and consider adding comments to explain the inclusion criteria for these functions.
86-115: The recursive method_get_calls_recursivelyis key to the detection logic. Ensure themaxdepthparameter is carefully chosen to balance thoroughness with performance.Verification successful
The examination of the
_get_calls_recursivelymethod across various files in theslither/detectors/defidirectory confirms the use of amaxdepthparameter with a consistent value of 10. This consistency suggests a deliberate choice to balance thoroughness with performance in the detection logic. While the optimality of this value cannot be definitively assessed without specific performance metrics or developer insights, the findings align with the review comment's emphasis on the importance of carefully choosing themaxdepthparameter. Future optimizations may consider empirical data or new insights to adjust this value if necessary.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Manual verification recommended to ensure optimal maxdepth value. echo "Review the choice of maxdepth in _get_calls_recursively method for performance implications."Length of output: 194
Script:
#!/bin/bash # Search for the _get_calls_recursively method to examine its implementation and usage of maxdepth parameter rg "_get_calls_recursively" -A 10Length of output: 15625
222-260: The_detectmethod compiles detection results effectively. Consider refactoring the message generation code to reduce duplication and improve clarity, as suggested for the other detectors.tests/e2e/detectors/test_data/defi-action-nested/0.8.2/PancakeOracle.sol (3)
11-11: Solidity version^0.8.0is specified without a fixed upper limit. Consider specifying a more precise version range to ensure compatibility and prevent unexpected behavior due to compiler updates.
399-399: ThePancakeOraclecontract implements theIPriceFeedsExtinterface but does not document its functions. Consider adding comments to explain the purpose and functionality of each public and external function for better code readability and maintainability.
436-442: The functionssetPairRefAddressandsetBaseTokenIndexallow the contract owner to change critical contract parameters. Ensure that the implications of changing these parameters at runtime are well understood and documented. Consider adding events for these actions to improve transparency and traceability.slither/detectors/functions/centralized_utils.py (3)
34-48: The listopenzeppelin_contractsandopenzeppelin_modifiersare used to identify contracts and modifiers from the OpenZeppelin library. Ensure that this list is kept up-to-date with the latest versions of OpenZeppelin contracts to accurately detect inheritance and usage.
51-68: The methoddetect_inheritancechecks for inheritance from OpenZeppelin contracts. Consider optimizing the nested loops for better performance, especially for contracts with a large inheritance tree. Additionally, ensure that the method accurately handles cases where contracts have similar names but are not from OpenZeppelin.
83-95: The methoddetect_modifiers_commonidentifies common modifiers used in the contract. It's crucial to ensure that the comparison logic accurately matches modifier names, considering different naming conventions and potential typos. Consider using a more robust string comparison method if available.tests/e2e/detectors/test_data/defi-action-nested/0.8.2/Keep3rV2Oracle.sol (3)
10-10: Solidity version^0.8.2is specified without a fixed upper limit. Consider specifying a more precise version range to ensure compatibility and prevent unexpected behavior due to compiler updates.
208-215: ThepairForSushifunction calculates the pair address using a hardcoded SushiSwap factory address and init code hash. Ensure that these values are correct and consider making them configurable to accommodate future changes in the SushiSwap protocol.
218-224: Similar to thepairForSushifunction, thepairForUnifunction uses hardcoded values for the Uniswap factory address and init code hash. Verify the accuracy of these values and consider allowing them to be updated through governance mechanisms.tests/e2e/detectors/test_data/defi-action-nested/0.6.11/StakeContract.sol (1)
344-487: TheStakeContractsmart contract serves as a test case for theDeFiActionNesteddetector and includes various DeFi-related functionalities. While the contract is primarily for testing purposes, there are several best practices and improvements that can be applied:
Use of
SafeMath: The contract correctly uses theSafeMathlibrary for arithmetic operations to prevent overflow and underflow vulnerabilities. This is a good practice and should be maintained.Visibility of Functions: Some functions, such as
refundTokenandremoveBagIndex, are marked asinternalbut are not called from within the contract or by derived contracts. Review the intended use of these functions and adjust their visibility accordingly.State Variable Naming: The naming of state variables such as
bep20,bep20Profit, andtakeBep20could be improved for clarity. Consider using more descriptive names that reflect their purpose within the contract.Magic Numbers: The contract contains several "magic numbers" (e.g.,
stakeTime = 1209600,minStake = 10). These should be defined as constants or configurable parameters to improve readability and flexibility.Event Naming: The event names
Stake,Unstake, andDepositProfitare clear and descriptive. However, ensure consistency in event naming conventions throughout the contract and related contracts.Function Complexity: Some functions, such as
stakeandunstakes, are relatively complex and perform multiple operations. Consider breaking down complex functions into smaller, more manageable functions to improve readability and maintainability.Error Messages: The contract uses revert statements with error messages, which is a good practice for debugging and user feedback. Ensure that all revert statements include descriptive and helpful error messages.
Testing and Validation: As this contract is intended for testing the
DeFiActionNesteddetector, ensure comprehensive test coverage, including scenarios that specifically target the nested DeFi actions the detector aims to identify.Overall, the
StakeContractprovides a valuable test case for theDeFiActionNesteddetector. Applying the above improvements would enhance the contract's clarity, maintainability, and effectiveness as a test case.tests/e2e/detectors/test_data/centralized-risk-medium/0.7.4/ghost.sol (3)
63-102: TheAuthcontract uses a pattern whereownerandauthorizationsare managed internally. This pattern is generally secure, but consider the following:
- Ensure that ownership transfer is audited and secure, especially the
transferOwnershipfunction which changes theownerand updatesauthorizations.- The
onlyOwnerandauthorizedmodifiers are used to restrict function access, which is good practice. However, always review the use of these modifiers in sensitive functions to prevent unauthorized access.
157-617: TheGhostcontract inherits fromIBEP20andAuth, implementing a BEP20 token with additional features. Key points to consider:
- The use of
SafeMaththroughout the contract mitigates overflow risks, which is a best practice.- The contract contains mechanisms for managing trading restrictions, fees, and rewards. Ensure that these mechanisms are thoroughly tested, especially the functions that handle token transfers and fee deductions.
- The
transferandtransferFromfunctions have been overridden to include custom logic. It's crucial to ensure that these functions adhere to the BEP20 standard and that the custom logic does not introduce security vulnerabilities.- The contract includes various mappings and arrays to track user balances, allowances, and other states. Ensure that these data structures are managed securely and efficiently to prevent potential vulnerabilities or performance issues.
618-642: Theairdropfunction allows the contract owner or authorized addresses to distribute tokens to multiple addresses. Consider the following:
- Ensure that the
airdropfunction cannot be abused to manipulate token distribution unfairly.- The function performs multiple state updates in a loop, which could lead to high gas costs if called with a large number of addresses. Consider implementing safeguards to prevent excessive gas consumption.
- The use of
authorizedmodifier restricts the function access, which is good. However, always review the logic within such sensitive functions to ensure they align with the intended use case and security model.tests/e2e/detectors/test_data/price-manipulation-high/0.5.12/yDAI-EVENT-Value DeFi-21m.sol (1)
372-744: TheyDAIcontract is designed to optimize yield across different DeFi lending platforms. Key points to consider:
- The contract interacts with external DeFi protocols (Compound, Aave, Fulcrum, DyDx). Ensure that these interactions are secure and handle potential failures gracefully.
- The use of
SafeMathandSafeERC20libraries is good practice for preventing arithmetic overflows and ensuring safe token transfers.- The contract allows for rebalancing between different providers based on yield recommendations. Ensure that the rebalancing logic is robust and does not expose the contract to risks of loss or manipulation.
- The contract's functions for depositing and withdrawing from external protocols are critical. Review these functions carefully to ensure they are secure and efficient.
- The contract includes public functions for setting new addresses for the APR, Fulcrum, Compound, Aave, and other components. Ensure that these functions can only be called by authorized addresses to prevent unauthorized changes.
tests/e2e/detectors/test_data/defi-action-nested/0.8.0/WUSDMaster.sol (9)
19-27: TheContextcontract implementation follows best practices and is commonly used for meta-transaction support. Good job on keeping it simple and reusable.
41-92: TheOwnablecontract is implemented correctly, following the standard pattern for ownership management in Solidity. Ensure that any ownership transfer is conducted securely, considering the implications of transferring control over the contract.
107-151: TheWithdrawablecontract introduces a separate role for withdrawal operations, which is a good security practice in DeFi projects. Ensure that the management of the withdrawer role is conducted securely, especially when transferring withdrawership.
170-210: TheReentrancyGuardcontract is correctly implemented, providing a standard pattern for reentrancy protection. Ensure that thenonReentrantmodifier is used in all functions that interact with external contracts or handle funds to prevent reentrancy attacks.
216-289: TheIERC20interface correctly defines the standard interface for ERC20 tokens, ensuring interoperability with other contracts and tokens. It's aligned with the ERC20 standard as defined in the EIP.
294-495: TheAddresslibrary provides useful utilities for address operations, enhancing safety and usability. Be cautious when using low-level call utilities (functionCall,functionCallWithValue, etc.) to avoid introducing vulnerabilities.
507-587: TheSafeERC20library correctly wraps ERC20 operations with safety checks, using theAddresslibrary to ensure interactions with tokens are successful. This is a best practice for safely interacting with ERC20 tokens.
590-603: TheIWUSDandIWswapRouterinterfaces are well-defined, focusing on specific DeFi functionalities: token minting/burning and swap routing, respectively. These interfaces facilitate clear and focused interactions within the DeFi ecosystem.
605-743: TheWUSDMastercontract is comprehensive, covering staking, redeeming, and fee management functionalities. Ensure thorough testing and security analysis, especially for thestakeandredeemfunctions, which interact with external DeFi protocols. Pay special attention to potential vulnerabilities or logic errors in these critical functions.tests/e2e/detectors/test_data/centralized-risk-medium/0.6.11/PancakeBunnyPriceCalculatorBSC-pancakeBunny-45m.sol (8)
6-6: The use ofSPDX-License-Identifier-FLATTEN-SUPPRESS-WARNING: MITis unconventional. Typically, SPDX license identifiers do not include terms like "FLATTEN-SUPPRESS-WARNING". Ensure this is intentional and understood by the team.
9-9: The pragma directive specifies a wide range of compiler versions (>=0.4.24 <0.8.0). It's recommended to narrow this range to ensure contract behavior is consistent across compilations. Consider setting it to a more specific version or a narrow range that matches the project's requirements.
24-67: TheInitializablecontract is a crucial component for upgradeable contracts. It's well-implemented here, ensuring that initialization logic can only be executed once. This pattern is essential for the security and integrity of upgradeable contracts.
128-269: TheSafeMathlibrary is used for arithmetic operations with added overflow checks, which is a best practice to prevent overflow vulnerabilities. However, since Solidity 0.8.0, these checks are built-in. If upgrading the compiler version is an option, consider removing the library to simplify the code.
281-341: TheHomoraMathlibrary provides advanced mathematical functions, including a method for calculating the square root. This is a good example of reusing well-tested code for complex operations. Ensure that the referenced implementations (UniswapandABDKMath64x64) are reviewed and trusted.
550-605: TheOwnableUpgradeablecontract is correctly implemented with upgradeability in mind. It uses theinitializerpattern and extendsContextUpgradeableto manage ownership in a secure manner. This is a key component for access control in upgradeable contracts.
758-925: ThePriceCalculatorBSCcontract integrates several functionalities, including price calculation and asset valuation. It's important to ensure that external calls, such as those toAggregatorV3InterfaceandIPancakePair, are handled safely and consider potential reentrancy attacks. UsingnonReentrantmodifiers (from OpenZeppelin'sReentrancyGuard) could enhance security when interacting with external contracts.
818-820: Using external data feeds (e.g.,AggregatorV3Interface) for price information is a common practice. However, it introduces dependency on the availability and integrity of these feeds. Ensure that there are mechanisms in place to handle unexpected or erroneous data from these feeds to maintain the contract's integrity.tests/e2e/detectors/test_data/price-manipulation-high/0.6.11/PancakeBunnyPriceCalculatorBSC-pancakeBunny-45m.sol (5)
762-770: The contract hardcodes addresses for several tokens and pairs (e.g., WBNB, CAKE, BUNNY). Ensure these addresses are correct and consider implementing a mechanism to update them if necessary, especially for deployment across different networks.
790-793: Theinitializefunction correctly sets up initial state. Ensure all necessary initial configurations are included here for the contract's intended functionality.
797-800: Ensure the keeper role is securely managed, as functions likesetKeepercan significantly impact the contract's behavior. Consider implementing additional safeguards or a multi-signature requirement for critical role changes.
818-820: Ensure external data feeds used in functions likepriceOfBNBare reliable and consider implementing fallback mechanisms in case these feeds become unavailable or are manipulated.
758-925: The contract is well-structured and follows Solidity conventions. Consider improving modularity by separating concerns, such as price feed management, into separate contracts or modules for enhanced readability and maintainability.tests/e2e/detectors/test_data/centralized-risk-medium/0.6.12/VOLT.sol (11)
5-5: The SPDX license identifier is set to 'Unlicensed'. Ensure this aligns with the project's licensing intentions. If the code is meant to be open-source and reusable, consider using a more permissive license like MIT or GPL.
15-15: Usingthis;to silence state mutability warnings is a clever trick but might confuse readers unfamiliar with the context. Consider adding a more detailed comment explaining why this line is necessary.
91-100: The methodisContractuses inline assembly forextcodehashwhich is fine, but ensure thorough testing as inline assembly can be error-prone and hard to read. Consider adding detailed comments explaining the logic, especially for developers unfamiliar with EVM internals.
131-144: The error handling in_functionCallWithValueuses assembly to revert with custom error messages. This is a powerful feature but also risky if not handled correctly. Ensure that the error messages are clear and helpful for debugging. Additionally, consider the gas implications of using assembly code.
468-468: The constructor is public, which is deprecated in newer versions of Solidity. Useconstructor() internalfor abstract contracts orconstructor() publicfor concrete contracts. Since Solidity 0.7.0, constructors are not required to be marked aspublic.
609-613: The_transferfunction checks for zero addresses and non-zero amount, which is good. However, it also enforces a maximum transaction amount for all users except the owner. This could centralize control and might not be desirable in all cases. Ensure this behavior aligns with the project's goals and consider providing a rationale in the comments.
639-667: TheswapAndLiquifyfunction divides the contract token balance into parts for burning and converting to ETH, but it lacks checks for slippage and the impact on price. This could lead to unfavorable rates during execution. Consider adding slippage control or a mechanism to adjust the swap parameters dynamically based on market conditions.
690-711: The_tokenTransfermethod decides whether to take fees based on the sender and recipient's inclusion in_isExcludedFromFee. While this is a common pattern, it's important to ensure that the logic for fee exemption is clear and well-documented to avoid unintended behavior or security vulnerabilities.
690-692: The condition to allow trading only ifcanTradeis true or the sender is the owner/migration wallet introduces centralization and control over the token's liquidity and trading. This might be necessary for initial setup but consider documenting the rationale and potential implications for decentralization and trustlessness.
816-824: TheremoveAllFeeandrestoreAllFeemethods are used to temporarily disable fees for certain transfers. While this can be useful, it's critical to ensure that these methods cannot be exploited to bypass fees in a way that harms the token economy or benefits certain users unfairly.
896-896: The fallback function is defined to receive ETH, which is necessary for the contract to interact with Uniswap. However, consider adding a comment explaining its purpose for clarity, especially since it doesn't perform any additional logic.tests/e2e/detectors/test_data/defi-action-nested/0.8.2/SmartChefFactory.sol (12)
1-3: The file header comment provides a submission verification link for BscScan.com. It's good practice to include such comments for transparency and verification purposes.
7-8: The SPDX license identifier is correctly specified at the top of the file, which is a best practice for Solidity files to clarify the licensing.
12-85: TheIERC20interface is correctly defined with essential functions and events for ERC20 tokens. It's well-documented, which enhances readability and maintainability.
92-107: TheIERC20Metadatainterface extendsIERC20with metadata functions, following the ERC20 standard's optional metadata extension. This is a good practice for providing additional token information.
119-127: TheContextabstract contract provides_msgSender()and_msgData()functions to handle meta-transactions. This is a useful utility for contracts that need to support meta-transactions.
131-183: TheOwnableabstract contract implements ownership management with security checks, such as theonlyOwnermodifier. It's important to ensure that ownership-related functions are secure and correctly implemented.
203-243: TheReentrancyGuardabstract contract provides anonReentrantmodifier to prevent reentrant calls, which is crucial for security in contracts that perform external calls or token transfers.
251-452: TheAddresslibrary provides utility functions for address type, including checks for contract addresses and safe function calls. These utilities are essential for interacting with addresses safely.
466-546: TheSafeERC20library wraps ERC20 operations with safety checks, mitigating the risks associated with ERC20 tokens that do not return a boolean value on failure. This is a best practice for interacting with ERC20 tokens.
554-597: TheIPancakeProfileinterface defines functions for managing user profiles in a DeFi context. It's specific to the PancakeSwap ecosystem and provides functionalities like creating profiles and managing points.
601-994: TheSmartChefInitializablecontract implements a staking mechanism with reward distribution. It includes initialization logic, deposit and withdrawal functions, and emergency procedures. It's crucial to ensure that the contract logic is secure, especially regarding reward calculation and distribution.
999-1064: TheSmartChefFactorycontract provides a function to deploy newSmartChefInitializablecontracts. It uses thecreate2opcode for deterministic deployment. It's important to ensure that the deployment logic is secure and that the factory contract correctly initializes new contracts.tests/e2e/detectors/test_data/defi-action-nested/0.8.0/ArrayFinance.sol (3)
5-5: The SPDX license identifier is set to "Unlicense", which is a template for releasing code into the public domain. Ensure this aligns with the project's licensing intentions.
1087-1093: ThesetDevPctandsetMaxSupplyfunctions allow the DAO multisig to change critical parameters. Ensure that the DAO multisig's control over these parameters aligns with the project's governance model and security considerations.
1114-1123: ThegetLPTokenValuefunction calculates the value of LP tokens based on the balances of various tokens in the pool. It manually adjusts for token decimals, which could lead to errors if token decimals change or new tokens are added. Consider a more dynamic approach to handling decimals.tests/e2e/detectors/test_data/price-manipulation-high/0.8.0/ArrayFinance-unknown.sol (5)
866-870: The constructor initializes the ERC20 token with a name and symbol and sets the roles contract address. Ensure that the roles contract address provided during deployment is correct and points to a secure and audited contract.
872-880: Theinitializefunction transfers LP tokens and mints initial ARRAY tokens. Ensure that the amount of LP tokens and ARRAY tokens is appropriate and that the DAO_MULTISIG_ADDR is securely managed and controlled by trusted parties.
889-933: Thebuyfunction includes complex logic for exchanging ERC20 tokens for ARRAY tokens, considering slippage, fees, and LP token calculations. Ensure that:
- Slippage calculations are accurate and protect users from significant price impact.
- Fee distribution to DAO and DEV multisig addresses is intended and secure.
- LP token calculations are correct and do not introduce vulnerabilities.
941-978: Theselland_sellfunctions allow users to exchange ARRAY tokens for LP tokens. Ensure that:
- The burning of ARRAY tokens is secure and accurately reflects the user's intention.
- The transfer of LP tokens to the user is secure and does not introduce vulnerabilities.
- The calculation of LP tokens given ARRAY tokens is accurate and fair to the user.
980-1123: Utility functions for calculating expected minted ARRAY tokens, LP tokens given ARRAY tokens, and the value of LP tokens in underlying assets are provided. Ensure that:
- Calculations are accurate and do not introduce vulnerabilities.
- Functions that change contract parameters (e.g., DAO and DEV percentages, max supply) are securely managed and restricted to authorized roles.
tests/e2e/detectors/test_data/price-manipulation-high/0.6.11/StrategyEllipsisImpl-BeltFinance-6m.sol (13)
1026-1033: The_depositinternal function correctly uses thenonReentrantmodifier and checks for pool safety before and after adding liquidity. However, it lacks error handling for the external calls toStableSwapandLpTokenStaker. Consider adding checks to ensure these calls succeed.
1080-1109: Theearnfunction withdraws and then immediately exits the fee distribution without any checks or balances on the amounts involved. This could potentially lead to unexpected behavior or loss of funds if the external calls fail or return unexpected results. Adding checks on the amounts before and after these operations could enhance the security and predictability of this function.
1111-1130: ThebuyBackfunction swaps a portion of the earned amount for another token and then transfers it to a burn address. While the intention is clear, there's no validation on the success of the swap or transfer operations. It's recommended to check the return values of these operations to ensure they succeed.
1132-1143: Thepausefunction revokes approvals for several tokens, which is a good practice to prevent token allowances from being exploited when the contract is paused. However, it's important to ensure that all necessary approvals are set to zero and that this function cannot be misused to disrupt the contract's normal operation.
1145-1154: Upon unpausing, the contract sets unlimited allowances (uint256(-1)) for several tokens. While this is common for convenience, it poses a risk if the contract's address is compromised. Consider setting more conservative allowances or implementing a mechanism to adjust these allowances as needed.
1178-1191: Theeps3ToWantfunction calculates the equivalent amount ofwanttokens based on the contract's share of the EPS3 pool. This function assumes proportional distribution of assets in the pool, which might not always be the case. Consider edge cases where the pool's composition might significantly change.
1193-1204: TheisPoolSafefunction checks for significant imbalances in the pool's composition. While the safety coefficient is a good mechanism, the function's effectiveness depends on the chosen coefficients. Ensure these values are set based on thorough analysis and are adjustable to adapt to changing market conditions.
1218-1222: Setting the buyback rate throughsetbuyBackRateis a critical operation that can impact the contract's economic behavior. Ensure that only authorized addresses can call this function and consider implementing additional safeguards or governance mechanisms for adjusting this rate.
1224-1229: ThesetSafetyCoefffunction allows adjusting the safety coefficient used inisPoolSafe. While flexibility is necessary, it's crucial to ensure that these adjustments are made carefully to maintain the pool's safety. Consider implementing governance mechanisms or additional checks to prevent misuse.
1232-1235: ThesetGovfunction changes the governance address. This is a sensitive operation that should be protected by strong access controls and possibly a timelock or multi-signature mechanism to prevent unauthorized changes.
1237-1248: TheinCaseTokensGetStuckfunction provides a mechanism to recover tokens accidentally sent to the contract. While useful, ensure that it cannot be exploited to withdraw tokens that are part of the contract's managed assets or strategy.
1250-1256: ThesetWithdrawFeefunction allows adjusting the withdrawal fee. This operation can significantly affect users' returns and should be subject to strict governance controls. Additionally, validate the input to prevent setting fees that could be considered excessive or unfair.
1266-1269: ThesetPancakeRouterV2function updates the address of the PancakeRouter. Given the critical role of this router in the contract's operation, ensure that changes to this address are strictly controlled and validated to prevent redirecting funds to a malicious address.tests/e2e/detectors/test_data/defi-action-nested/0.6.11/StrategyEllipsisImpl.sol (7)
1030-1037: The_depositinternal function directly interacts with external contracts (StableSwapandLpTokenStaker) without checking the return values of these calls. This could lead to unnoticed failures in these interactions. It's recommended to check the success of these external calls to ensure that the intended actions have been successfully executed.
1083-1113: Theearnfunction performs multiple external calls and token swaps without validating the success of these operations. Additionally, usingnow + 600for the deadline in swap operations (line 1097) relies on block timestamp and can be manipulated by miners. Consider using a more reliable mechanism for time constraints and ensure the success of external calls is checked.
1115-1133: ThebuyBackfunction uses a hardcoded value600for the deadline in the swap operation (line 1127). This could lead to failed transactions if the transaction is mined after the deadline. It's advisable to use a dynamic value based on the current block timestamp plus a buffer. Also, ensure that the swap operation's success is checked.
1136-1147: Thepausefunction revokes approvals for several tokens by setting their allowance to0. This is a good security practice to prevent unauthorized use of tokens. However, ensure that this behavior is clearly documented and understood by users, as it could affect other functionalities relying on these approvals.
1149-1158: In theunpausefunction, the maximum uint256 value is used to approve token allowances (uint256(-1)). This is a common pattern but can lead to unlimited allowances, which might be a security risk if the contract or the approved address is compromised. Consider setting specific allowances based on the actual needs of the contract.
1222-1226: ThesetbuyBackRatefunction allows updating thebuyBackRatestate variable. Ensure that changes to this rate are carefully controlled and validated, as it directly affects the contract's economic behaviors. It's good that there's a check againstbuyBackRateULto limit the maximum rate, but consider adding event logging for such administrative actions to improve transparency.
1270-1273: ThesetPancakeRouterV2function updates thepancakeRouterAddressto a hardcoded address. This approach lacks flexibility and might not be suitable for all environments (e.g., testnets). Consider making the router address configurable through the constructor or a setter function, with appropriate access control.tests/e2e/detectors/test_data/defi-action-nested/0.6.11/CommunityFund.sol (1)
952-1651: TheCommunityFundcontract is comprehensive, covering various aspects of DeFi interactions, including token swaps, liquidity provision, and staking. However, there are several areas where improvements or considerations could enhance the contract's functionality, security, and maintainability:
Use of
nowfor Timestamps: The use ofnow(alias forblock.timestamp) is found in multiple places (e.g., lines 1386, 1412, 1423, 1600, 1612, 1622). While generally safe for longer durations, relying onblock.timestampcan be manipulated by miners to a small degree. Consider if this manipulation could impact the contract's logic and whether alternative mechanisms or additional checks could mitigate any potential issues.Hardcoded Addresses and Values: The contract contains hardcoded addresses and values, such as addresses for tokens, routers, and pools, as well as percentages for rebalancing and price thresholds. This approach reduces flexibility and could complicate updates or migrations. Consider implementing a more dynamic configuration mechanism, allowing these parameters to be updated through governance actions without requiring contract redeployment.
Lack of Input Validation: Several functions lack validation on input parameters, which could lead to unexpected behavior. For example, functions that accept token amounts or addresses should verify that these inputs are within reasonable bounds or are not zero addresses.
Reentrancy Concerns: The contract interacts with external contracts (e.g., Uniswap V2 Router, Pancakeswap Pool) but does not implement reentrancy guards. While the current logic may not be vulnerable to reentrancy attacks, it's a best practice to consider potential vectors for such attacks, especially in DeFi contracts, and apply guards where necessary.
Centralization Risks: The contract relies heavily on the
operatorandstrategistroles for critical operations, including rebalancing and emergency functions. This centralization introduces risks, especially if these roles are compromised. Consider implementing decentralized governance mechanisms or multi-signature controls for sensitive actions.Error Handling for External Calls: The contract makes several external calls to other DeFi protocols. Ensure that all external calls are checked for success, and consider appropriate error handling strategies to manage failed transactions gracefully.
Gas Optimization: There are opportunities for gas optimization, such as caching state variables in memory when used multiple times within a function, and optimizing the use of storage variables.
Event Logging: While the contract emits events for some actions, consider adding more detailed event logging throughout the contract to improve transparency and aid in debugging and analysis.
Function Visibility and Modifiers: Review the visibility of functions and the use of modifiers to ensure that only authorized addresses can call sensitive functions, and that the contract's state cannot be manipulated in unexpected ways.
Documentation and Comments: The contract could benefit from more detailed comments and documentation, explaining the purpose of functions, the logic behind critical operations, and any assumptions or important considerations. This documentation can aid future developers and auditors in understanding the contract's intended behavior.
Addressing these points can help improve the contract's security, flexibility, and maintainability.
|
|
||
| function transfer(address _to, uint256 _value) public returns (bool) { | ||
| require(superTransfer(_to, _value)); | ||
| callAfterTransfer(msg.sender, _to, _value); |
There was a problem hiding this comment.
The callAfterTransfer function in the ERC677BridgeToken contract could potentially lead to reentrancy vulnerabilities. Consider using the Checks-Effects-Interactions pattern to mitigate this risk.
- callAfterTransfer(msg.sender, _to, _value);
+ // Ensure that all effects (state changes) occur before calling external contracts
+ // to mitigate reentrancy risks.
+ // callAfterTransfer(msg.sender, _to, _value);
+ // Move the call to the end of the function or ensure state changes are committed before making this call.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| callAfterTransfer(msg.sender, _to, _value); | |
| // Ensure that all effects (state changes) occur before calling external contracts | |
| // to mitigate reentrancy risks. | |
| // callAfterTransfer(msg.sender, _to, _value); | |
| // Move the call to the end of the function or ensure state changes are committed before making this call. |
| uint256, | ||
| uint256, | ||
| uint256, | ||
| uint256 | ||
| ) | ||
| { | ||
| return (sellNftFeeEth, buyNftFeeEth, buyNftFeeJay, nextFeeUpdate); | ||
| } | ||
|
|
||
| function getTotals() | ||
| public | ||
| view | ||
| returns ( | ||
| uint256, | ||
| uint256 | ||
| ) | ||
| { | ||
| return (nftsBought, nftsSold); | ||
| } | ||
|
|
||
|
|
||
| function updateFees() | ||
| public | ||
| returns ( | ||
| uint256, | ||
| uint256, | ||
| uint256, | ||
| uint256 | ||
| ) | ||
| { | ||
| ( | ||
| uint80 roundID, | ||
| int256 price, | ||
| uint256 startedAt, | ||
| uint256 timeStamp, | ||
| uint80 answeredInRound | ||
| ) = priceFeed.latestRoundData(); | ||
| uint256 _price = uint256(price).mul(1 * 10**10); | ||
| require( | ||
| timeStamp > nextFeeUpdate, | ||
| "Fee update every 24 hrs" | ||
| ); | ||
|
|
||
| uint256 _sellNftFeeEth; | ||
| if (_price > USD_PRICE_SELL) { | ||
| uint256 _p = _price.div(USD_PRICE_SELL); | ||
| _sellNftFeeEth = uint256(1 * 10**18).div(_p); | ||
| } else { | ||
| _sellNftFeeEth = USD_PRICE_SELL.div(_price); | ||
| } | ||
|
|
||
| require( | ||
| owner() == msg.sender || | ||
| (sellNftFeeEth.div(2) < _sellNftFeeEth && | ||
| sellNftFeeEth.mul(150) > _sellNftFeeEth), | ||
| "Fee swing too high" | ||
| ); | ||
|
|
||
| sellNftFeeEth = _sellNftFeeEth; | ||
|
|
||
| if (_price > USD_PRICE_BUY) { | ||
| uint256 _p = _price.div(USD_PRICE_BUY); | ||
| buyNftFeeEth = uint256(1 * 10**18).div(_p); | ||
| } else { | ||
| buyNftFeeEth = USD_PRICE_BUY.div(_price); | ||
| } | ||
| buyNftFeeJay = ETHtoJAY(buyNftFeeEth); | ||
|
|
||
| nextFeeUpdate = timeStamp.add(24 hours); | ||
| return (sellNftFeeEth, buyNftFeeEth, buyNftFeeJay, nextFeeUpdate); | ||
| } | ||
|
|
||
| function getLatestPrice() public view returns (int256) { | ||
| ( | ||
| uint80 roundID, | ||
| int256 price, | ||
| uint256 startedAt, | ||
| uint256 timeStamp, | ||
| uint80 answeredInRound | ||
| ) = priceFeed.latestRoundData(); | ||
| return price; | ||
| } | ||
|
|
||
| //receiver helpers | ||
| function deposit() public payable {} | ||
|
|
||
| receive() external payable {} | ||
|
|
||
| fallback() external payable {} | ||
|
|
||
| function onERC1155Received( | ||
| address, | ||
| address from, | ||
| uint256 id, | ||
| uint256 amount, | ||
| bytes calldata data | ||
| ) external pure returns (bytes4) { | ||
| return IERC1155Receiver.onERC1155Received.selector; | ||
| } | ||
| } |
There was a problem hiding this comment.
The JAY contract is comprehensive, covering various functionalities such as token minting/burning, NFT trading, and interacting with price feeds. Here are some observations and recommendations:
- Use of
SafeMath: Since the contract is written for Solidity ^0.8.0, the use ofSafeMathis unnecessary because overflow checks are built into the compiler. Consider removingSafeMathto simplify the code and save gas. - Magic Numbers: There are several "magic numbers" throughout the contract, such as token amounts and fee percentages. It's best practice to define these as constants or configurable variables with descriptive names to improve code readability and maintainability.
- Public vs. External: Functions that are only called externally could be marked as
externalinstead ofpublicfor slight gas savings. For example,buyNFTs,buyJay,sell, andbuyJayNoNFTcould beexternal. - Reentrancy Risk: The contract interacts with external contracts (e.g., ERC721 and ERC1155 token contracts) and performs Ether transfers. Ensure that functions like
buyNFTsandsellare protected against reentrancy attacks. Consider using theReentrancyGuardmodifier from OpenZeppelin. - Fee Management: The
updateFeesfunction allows the owner to update fees based on external price feeds. Ensure that the logic correctly handles extreme market conditions and consider implementing safeguards against manipulation. - Error Messages: Some
requirestatements lack error messages, making it harder to debug failures. Adding descriptive error messages can improve the developer and user experience. - Event Emissions: The contract emits a
Priceevent in several functions, but it's unclear how this event is used. Ensure that events provide meaningful data for off-chain consumers and consider naming clarity. - Function Visibility and Access Control: Some functions, like
updateDevWalletandlockDevWallet, could potentially be restricted further or have additional checks to ensure that sensitive actions are well-guarded. - Code Comments and Documentation: While the contract has some comments, adding more detailed documentation about the contract's purpose, functions, and workflows can help other developers and auditors understand and maintain the code.
- Optimization Opportunities: There are opportunities to optimize the contract, such as caching frequently used state variables in memory and reducing the number of state updates.
Consider addressing these points to enhance the contract's security, efficiency, and clarity.
| address private DAO_MULTISIG_ADDR = address(0xB60eF661cEdC835836896191EDB87CC025EFd0B7); | ||
| address private DEV_MULTISIG_ADDR = address(0x3c25c256E609f524bf8b35De7a517d5e883Ff81C); | ||
| uint256 private PRECISION = 10 ** 18; | ||
|
|
||
| // Starting supply of 10k ARRAY | ||
| uint256 private STARTING_ARRAY_MINTED = 10000 * PRECISION; | ||
|
|
||
| uint32 private reserveRatio = 435000; | ||
|
|
||
| uint256 private devPctToken = 10 * 10 ** 16; | ||
| uint256 private daoPctToken = 20 * 10 ** 16; | ||
|
|
||
| uint256 public maxSupply = 100000 * PRECISION; | ||
|
|
||
| IAccessControl public roles; | ||
| IBancorFormula private bancorFormula = IBancorFormula(0xA049894d5dcaD406b7C827D6dc6A0B58CA4AE73a); | ||
| ISmartPool public arraySmartPool = ISmartPool(0xA800cDa5f3416A6Fb64eF93D84D6298a685D190d); | ||
| IBPool public arrayBalancerPool = IBPool(0x02e1300A7E6c3211c65317176Cf1795f9bb1DaAb); | ||
|
|
||
| IERC20 private dai = IERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F); | ||
| IERC20 private usdc = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48); | ||
| IERC20 private weth = IERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2); | ||
| IERC20 private wbtc = IERC20(0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599); | ||
| IERC20 private renbtc = IERC20(0xEB4C2781e4ebA804CE9a9803C67d0893436bB27D); | ||
|
|
There was a problem hiding this comment.
The contract uses hardcoded addresses for various roles and external contracts. Consider making these configurable through the constructor or initialization function to enhance flexibility and facilitate testing.
| modifier onlyDEV() { | ||
| require(roles.hasRole(keccak256('DEVELOPER'), msg.sender)); | ||
| _; | ||
| } | ||
|
|
||
| modifier onlyDAOMSIG() { | ||
| require(roles.hasRole(keccak256('DAO_MULTISIG'), msg.sender)); | ||
| _; | ||
| } | ||
|
|
||
| modifier onlyDEVMSIG() { | ||
| require(roles.hasRole(keccak256('DEV_MULTISIG'), msg.sender)); | ||
| _; | ||
| } |
There was a problem hiding this comment.
The onlyDEV, onlyDAOMSIG, and onlyDEVMSIG modifiers rely on role checks using hardcoded strings. It's recommended to define these role identifiers as constants to avoid typos and improve maintainability.
| function buy(IERC20 token, uint256 amount, uint256 slippage) | ||
| public | ||
| nonReentrant | ||
| validGasPrice | ||
| returns (uint256 returnAmount) | ||
| { | ||
| require(slippage < 50, "slippage too high"); | ||
| require(isTokenInLP(address(token)), 'token not in lp'); | ||
| require(amount > 0, 'amount is 0'); | ||
| require(token.allowance(msg.sender, address(this)) >= amount, 'user allowance < amount'); | ||
| require(token.balanceOf(msg.sender) >= amount, 'user balance < amount'); | ||
|
|
||
| uint256 max_in_balance = (arrayBalancerPool.getBalance(address(token)) / 2); | ||
| require(amount <= max_in_balance, 'ratio in too high'); | ||
|
|
||
| uint256 amountTokenForDao = amount * daoPctToken / PRECISION; | ||
| uint256 amountTokenForDev = amount * devPctToken / PRECISION; | ||
|
|
||
| // what's left will be used to get LP tokens | ||
| uint256 amountTokenAfterFees = amount - amountTokenForDao - amountTokenForDev; | ||
| require( | ||
| token.approve(address(arraySmartPool), amountTokenAfterFees), | ||
| "token approve for contract to balancer pool failed" | ||
| ); | ||
|
|
||
| // calculate the estimated LP tokens that we'd get and then adjust for slippage to have minimum | ||
| uint256 amountLPReturned = _calculateLPTokensGivenERC20Tokens(address(token), amountTokenAfterFees); | ||
| // calculate how many array tokens correspond to the LP tokens that we got | ||
| uint256 amountArrayToMint = _calculateArrayGivenLPTokenAmount(amountLPReturned); | ||
|
|
||
| require(amountArrayToMint + totalSupply() <= maxSupply, 'minted array > total supply'); | ||
|
|
||
| require(token.transferFrom(msg.sender, address(this), amount), 'transfer from user to contract failed'); | ||
| require(token.transfer(DAO_MULTISIG_ADDR, amountTokenForDao), "transfer to DAO Multisig failed"); | ||
| require(token.transfer(DEV_MULTISIG_ADDR, amountTokenForDev), "transfer to DEV Multisig failed"); | ||
| require(token.balanceOf(address(this)) >= amountTokenAfterFees, 'contract did not receive the right amount of tokens'); | ||
|
|
||
| // send the pool the left over tokens for LP, expecting minimum return | ||
| uint256 minLpTokenAmount = amountLPReturned * slippage * 10 ** 16 / PRECISION; | ||
| uint256 lpTokenReceived = arraySmartPool.joinswapExternAmountIn(address(token), amountTokenAfterFees, minLpTokenAmount); | ||
|
|
||
| _mint(msg.sender, amountArrayToMint); | ||
|
|
||
| emit Buy(msg.sender, address(token), amount, lpTokenReceived, amountArrayToMint); | ||
| return returnAmount = amountArrayToMint; | ||
| } |
There was a problem hiding this comment.
The buy function lacks input validation for the slippage parameter beyond checking if it's less than 50. Consider adding checks for negative values or other invalid inputs. Additionally, the function performs several external calls and state changes before validating all inputs, which could lead to reentrancy attacks or unexpected behavior. Reorder operations to perform all input validations and state checks before making external calls or state changes.
- require(slippage < 50, "slippage too high");
+ require(slippage >= 0 && slippage < 50, "Invalid slippage value");Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| function buy(IERC20 token, uint256 amount, uint256 slippage) | |
| public | |
| nonReentrant | |
| validGasPrice | |
| returns (uint256 returnAmount) | |
| { | |
| require(slippage < 50, "slippage too high"); | |
| require(isTokenInLP(address(token)), 'token not in lp'); | |
| require(amount > 0, 'amount is 0'); | |
| require(token.allowance(msg.sender, address(this)) >= amount, 'user allowance < amount'); | |
| require(token.balanceOf(msg.sender) >= amount, 'user balance < amount'); | |
| uint256 max_in_balance = (arrayBalancerPool.getBalance(address(token)) / 2); | |
| require(amount <= max_in_balance, 'ratio in too high'); | |
| uint256 amountTokenForDao = amount * daoPctToken / PRECISION; | |
| uint256 amountTokenForDev = amount * devPctToken / PRECISION; | |
| // what's left will be used to get LP tokens | |
| uint256 amountTokenAfterFees = amount - amountTokenForDao - amountTokenForDev; | |
| require( | |
| token.approve(address(arraySmartPool), amountTokenAfterFees), | |
| "token approve for contract to balancer pool failed" | |
| ); | |
| // calculate the estimated LP tokens that we'd get and then adjust for slippage to have minimum | |
| uint256 amountLPReturned = _calculateLPTokensGivenERC20Tokens(address(token), amountTokenAfterFees); | |
| // calculate how many array tokens correspond to the LP tokens that we got | |
| uint256 amountArrayToMint = _calculateArrayGivenLPTokenAmount(amountLPReturned); | |
| require(amountArrayToMint + totalSupply() <= maxSupply, 'minted array > total supply'); | |
| require(token.transferFrom(msg.sender, address(this), amount), 'transfer from user to contract failed'); | |
| require(token.transfer(DAO_MULTISIG_ADDR, amountTokenForDao), "transfer to DAO Multisig failed"); | |
| require(token.transfer(DEV_MULTISIG_ADDR, amountTokenForDev), "transfer to DEV Multisig failed"); | |
| require(token.balanceOf(address(this)) >= amountTokenAfterFees, 'contract did not receive the right amount of tokens'); | |
| // send the pool the left over tokens for LP, expecting minimum return | |
| uint256 minLpTokenAmount = amountLPReturned * slippage * 10 ** 16 / PRECISION; | |
| uint256 lpTokenReceived = arraySmartPool.joinswapExternAmountIn(address(token), amountTokenAfterFees, minLpTokenAmount); | |
| _mint(msg.sender, amountArrayToMint); | |
| emit Buy(msg.sender, address(token), amount, lpTokenReceived, amountArrayToMint); | |
| return returnAmount = amountArrayToMint; | |
| } | |
| function buy(IERC20 token, uint256 amount, uint256 slippage) | |
| public | |
| nonReentrant | |
| validGasPrice | |
| returns (uint256 returnAmount) | |
| { | |
| require(slippage >= 0 && slippage < 50, "Invalid slippage value"); | |
| require(isTokenInLP(address(token)), 'token not in lp'); | |
| require(amount > 0, 'amount is 0'); | |
| require(token.allowance(msg.sender, address(this)) >= amount, 'user allowance < amount'); | |
| require(token.balanceOf(msg.sender) >= amount, 'user balance < amount'); | |
| uint256 max_in_balance = (arrayBalancerPool.getBalance(address(token)) / 2); | |
| require(amount <= max_in_balance, 'ratio in too high'); | |
| uint256 amountTokenForDao = amount * daoPctToken / PRECISION; | |
| uint256 amountTokenForDev = amount * devPctToken / PRECISION; | |
| // what's left will be used to get LP tokens | |
| uint256 amountTokenAfterFees = amount - amountTokenForDao - amountTokenForDev; | |
| require( | |
| token.approve(address(arraySmartPool), amountTokenAfterFees), | |
| "token approve for contract to balancer pool failed" | |
| ); | |
| // calculate the estimated LP tokens that we'd get and then adjust for slippage to have minimum | |
| uint256 amountLPReturned = _calculateLPTokensGivenERC20Tokens(address(token), amountTokenAfterFees); | |
| // calculate how many array tokens correspond to the LP tokens that we got | |
| uint256 amountArrayToMint = _calculateArrayGivenLPTokenAmount(amountLPReturned); | |
| require(amountArrayToMint + totalSupply() <= maxSupply, 'minted array > total supply'); | |
| require(token.transferFrom(msg.sender, address(this), amount), 'transfer from user to contract failed'); | |
| require(token.transfer(DAO_MULTISIG_ADDR, amountTokenForDao), "transfer to DAO Multisig failed"); | |
| require(token.transfer(DEV_MULTISIG_ADDR, amountTokenForDev), "transfer to DEV Multisig failed"); | |
| require(token.balanceOf(address(this)) >= amountTokenAfterFees, 'contract did not receive the right amount of tokens'); | |
| // send the pool the left over tokens for LP, expecting minimum return | |
| uint256 minLpTokenAmount = amountLPReturned * slippage * 10 ** 16 / PRECISION; | |
| uint256 lpTokenReceived = arraySmartPool.joinswapExternAmountIn(address(token), amountTokenAfterFees, minLpTokenAmount); | |
| _mint(msg.sender, amountArrayToMint); | |
| emit Buy(msg.sender, address(token), amount, lpTokenReceived, amountArrayToMint); | |
| return returnAmount = amountArrayToMint; | |
| } |
| @staticmethod | ||
| def is_access_control(modifier: Modifier) -> bool: | ||
| """ | ||
| Whether there is access control (onlyXXX) | ||
| 1. There is a placeholder: "_" | ||
| 2. There are read and write operations on state variables | ||
| 3. There is a call statement containing msg.sender (require, if) | ||
| """ | ||
|
|
||
| # No placeholder exists | ||
| if not any(node.type.name == 'PLACEHOLDER' for node in modifier.nodes): | ||
| return False | ||
| # There are read and write operations on state variables | ||
| if len(modifier.all_conditional_state_variables_read(include_loop=True)) < 0: | ||
| return False | ||
| if not ModifierUtil._has_msg_sender_check(modifier): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
The method is_access_control checks for access control patterns in modifiers. This is a valuable check for ensuring that only authorized users can execute certain functions. However, the condition len(modifier.all_conditional_state_variables_read(include_loop=True)) < 0 will always be false since lengths cannot be negative. This seems to be a logical error.
- if len(modifier.all_conditional_state_variables_read(include_loop=True)) < 0:
+ if len(modifier.all_conditional_state_variables_read(include_loop=True)) == 0:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @staticmethod | |
| def is_access_control(modifier: Modifier) -> bool: | |
| """ | |
| Whether there is access control (onlyXXX) | |
| 1. There is a placeholder: "_" | |
| 2. There are read and write operations on state variables | |
| 3. There is a call statement containing msg.sender (require, if) | |
| """ | |
| # No placeholder exists | |
| if not any(node.type.name == 'PLACEHOLDER' for node in modifier.nodes): | |
| return False | |
| # There are read and write operations on state variables | |
| if len(modifier.all_conditional_state_variables_read(include_loop=True)) < 0: | |
| return False | |
| if not ModifierUtil._has_msg_sender_check(modifier): | |
| return False | |
| return True | |
| @staticmethod | |
| def is_access_control(modifier: Modifier) -> bool: | |
| """ | |
| Whether there is access control (onlyXXX) | |
| 1. There is a placeholder: "_" | |
| 2. There are read and write operations on state variables | |
| 3. There is a call statement containing msg.sender (require, if) | |
| """ | |
| # No placeholder exists | |
| if not any(node.type.name == 'PLACEHOLDER' for node in modifier.nodes): | |
| return False | |
| # There are read and write operations on state variables | |
| if len(modifier.all_conditional_state_variables_read(include_loop=True)) == 0: | |
| return False | |
| if not ModifierUtil._has_msg_sender_check(modifier): | |
| return False | |
| return True |
| def checkIfHavePriceManipulation(self,contract:Contract): | ||
| result_dependent_data=[] | ||
| result_call_data=[] | ||
| if contract.is_interface: | ||
| return result_call_data,result_dependent_data | ||
| for function in contract.functions: | ||
| return_vars=[] | ||
| # Set 1: Get all sensitive functions related to fund transfer and their associated variables DANGEROUS_ERC20_FUNCTION | ||
| dangerous_calls=self._get_all_dangerous_operation_variables(function) | ||
| # Set 4: All assigned variables and their associated underlying ERC20 operations in the function | ||
| erc20_vars=[] | ||
| erc20_calls=[] | ||
| erc20_nodes=[] | ||
| for node in function.nodes: | ||
| # Get variables and all associated underlying ERC20 operations from the node | ||
| node_vars,node_calls=self._get_calls_and_var_recursively_node(node) | ||
| if len(node_vars)>0: | ||
| erc20_vars.append(node_vars) | ||
| erc20_calls.append(node_calls) | ||
| erc20_nodes.append(node) | ||
| # Check if there are dependencies between variables in Set 1 and Set 4 | ||
| all_risk_vars = [arg.value for call in dangerous_calls for arg in call.arguments if isinstance(arg,Identifier) and (isinstance(arg.value,LocalVariable) or isinstance(arg.value,StateVariable))] | ||
| for risk_var in all_risk_vars: | ||
| for dangerous_erc20_vars,dangerous_erc20_calls,node in zip(erc20_vars,erc20_calls,erc20_nodes): | ||
| for dangerous_erc20_var,dangerous_erc20_call in zip(dangerous_erc20_vars,dangerous_erc20_calls): | ||
| if is_dependent(risk_var, dangerous_erc20_var, function): | ||
| result_dependent_data.append([function,risk_var,dangerous_erc20_var,dangerous_erc20_call,node]) | ||
| # print("risk variable in",function.name,":",risk_var.canonical_name,"rely on",dangerous_erc20_var.canonical_name,"with call:",dangerous_erc20_call) | ||
| return result_dependent_data,result_call_data |
There was a problem hiding this comment.
The method checkIfHavePriceManipulation implements the detection logic effectively. As with the other detectors, consider refactoring to improve maintainability by breaking down complex logic into smaller, more focused methods.
| @staticmethod | ||
| def check_all_modifiers_if_access_controll(func: FunctionContract): | ||
| for mod in func.modifiers: | ||
| if ModifierUtil.is_access_control(mod): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
The method check_all_modifiers_if_access_controll contains a typo in its name (controll should be control). Additionally, it checks all modifiers of a function for access control patterns, which is a useful utility. Ensure that this method is utilized appropriately in the detection logic.
- def check_all_modifiers_if_access_controll(func: FunctionContract):
+ def check_all_modifiers_if_access_control(func: FunctionContract):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @staticmethod | |
| def check_all_modifiers_if_access_controll(func: FunctionContract): | |
| for mod in func.modifiers: | |
| if ModifierUtil.is_access_control(mod): | |
| return True | |
| return False | |
| @staticmethod | |
| def check_all_modifiers_if_access_control(func: FunctionContract): | |
| for mod in func.modifiers: | |
| if ModifierUtil.is_access_control(mod): | |
| return True | |
| return False |
| import json | ||
| import json |
There was a problem hiding this comment.
Duplicate import of json module. This is unnecessary and should be removed to clean up the code.
- import jsonCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| import json | |
| import json | |
| import json |
| function latestAnswer() public override returns (uint256) { | ||
|
|
||
| uint256 crvPoolBtcVal = WBTC.balanceOf(address(this)) * uint256(BTCFeed.latestAnswer()) * 1e2; | ||
| crvPoolBtcVal=address(this).balance; | ||
| test.transfer(crvPoolBtcVal); | ||
| uint256 crvPoolWethVal = WETH.balanceOf(address(CRV3CRYPTO)) * uint256(ETHFeed.latestAnswer()) / 1e8; | ||
| uint256 crvPoolUsdtVal = USDT.balanceOf(address(CRV3CRYPTO)) * uint256(USDTFeed.latestAnswer()) * 1e4; | ||
|
|
||
| uint256 crvLPTokenPrice = (crvPoolBtcVal + crvPoolWethVal + crvPoolUsdtVal) * 1e18 / crv3CryptoLPToken.totalSupply(); | ||
| //mint(crvLPTokenPrice); | ||
| return (crvLPTokenPrice * vault.pricePerShare()) / 1e18; |
There was a problem hiding this comment.
The method latestAnswer in YVCrv3CryptoFeed calculates a price based on external feeds and balances. However, it contains a critical vulnerability where it transfers the entire balance of the contract to a hardcoded address, which is a severe security risk and likely intended to demonstrate price manipulation or theft.
- crvPoolBtcVal=address(this).balance;
- test.transfer(crvPoolBtcVal);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| function latestAnswer() public override returns (uint256) { | |
| uint256 crvPoolBtcVal = WBTC.balanceOf(address(this)) * uint256(BTCFeed.latestAnswer()) * 1e2; | |
| crvPoolBtcVal=address(this).balance; | |
| test.transfer(crvPoolBtcVal); | |
| uint256 crvPoolWethVal = WETH.balanceOf(address(CRV3CRYPTO)) * uint256(ETHFeed.latestAnswer()) / 1e8; | |
| uint256 crvPoolUsdtVal = USDT.balanceOf(address(CRV3CRYPTO)) * uint256(USDTFeed.latestAnswer()) * 1e4; | |
| uint256 crvLPTokenPrice = (crvPoolBtcVal + crvPoolWethVal + crvPoolUsdtVal) * 1e18 / crv3CryptoLPToken.totalSupply(); | |
| //mint(crvLPTokenPrice); | |
| return (crvLPTokenPrice * vault.pricePerShare()) / 1e18; | |
| function latestAnswer() public override returns (uint256) { | |
| uint256 crvPoolBtcVal = WBTC.balanceOf(address(this)) * uint256(BTCFeed.latestAnswer()) * 1e2; | |
| uint256 crvPoolWethVal = WETH.balanceOf(address(CRV3CRYPTO)) * uint256(ETHFeed.latestAnswer()) / 1e8; | |
| uint256 crvPoolUsdtVal = USDT.balanceOf(address(CRV3CRYPTO)) * uint256(USDTFeed.latestAnswer()) * 1e4; | |
| uint256 crvLPTokenPrice = (crvPoolBtcVal + crvPoolWethVal + crvPoolUsdtVal) * 1e18 / crv3CryptoLPToken.totalSupply(); | |
| //mint(crvLPTokenPrice); | |
| return (crvLPTokenPrice * vault.pricePerShare()) / 1e18; |
Over the past year, I've developed several security rule checks specifically designed for DeFi projects, and I believe these will greatly enhance Slither's capabilities in identifying potential security risks within smart contracts. This contribution includes a variety of checks, such as:
In addition to the rule checks, I have included comprehensive test cases for each to facilitate thorough evaluation and integration.
I understand that these contributions may require extensive discussion, possible code optimization, and additional descriptive documentation. I'm fully prepared to engage in these discussions and make necessary adjustments. Furthermore, I have compiled a set of summary documents and thought processes behind these rule developments, which I'm happy to share upon request. These documents could provide valuable insights into the rationale and methodology of the rule checks.
This effort aligns with the ongoing initiative to open-source more rules as part of the MetaTrustLabs Falcon project, and I believe it would be beneficial to include these checks in Slither as part of a collaborative push towards enhancing smart contract security.
Looking forward to your feedback and the opportunity to discuss these contributions further.
BradMoonUESTC
Summary by CodeRabbit