arbitrary-send-eth: Don't report if destination is immutable state var #2488
Conversation
WalkthroughWalkthroughThe updates enhance the Changes
Sequence Diagram(s)N/A 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: 0
Outside diff range and nitpick comments (1)
slither/detectors/functions/arbitrary_send_eth.py (1)
Line range hint
50-51: Refactor suggestion: Simplify nestedifstatements.The static analysis tool suggests using a single
ifstatement instead of multiple nested ones. This can improve readability and reduce complexity.- if isinstance(ir, (HighLevelCall)): - if isinstance(ir.function, Function): - if ir.function.full_name == "transferFrom(address,address,uint256)": - return False + if isinstance(ir, HighLevelCall) and isinstance(ir.function, Function) and ir.function.full_name == "transferFrom(address,address,uint256)": + return FalseAlso applies to: 63-65, 64-65, 80-81
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
tests/e2e/detectors/test_data/arbitrary-send-eth/0.6.11/arbitrary_send_eth.sol-0.6.11.zipis excluded by!**/*.ziptests/e2e/detectors/test_data/arbitrary-send-eth/0.7.6/arbitrary_send_eth.sol-0.7.6.zipis excluded by!**/*.zip
Files selected for processing (5)
- slither/detectors/functions/arbitrary_send_eth.py (2 hunks)
- tests/e2e/detectors/snapshots/detectors__detector_ArbitrarySendEth_0_6_11_arbitrary_send_eth_sol__0.txt (1 hunks)
- tests/e2e/detectors/snapshots/detectors__detector_ArbitrarySendEth_0_7_6_arbitrary_send_eth_sol__0.txt (1 hunks)
- tests/e2e/detectors/test_data/arbitrary-send-eth/0.6.11/arbitrary_send_eth.sol (1 hunks)
- tests/e2e/detectors/test_data/arbitrary-send-eth/0.7.6/arbitrary_send_eth.sol (1 hunks)
Additional context used
Ruff
slither/detectors/functions/arbitrary_send_eth.py
50-51: Use a single
ifstatement instead of nestedifstatements (SIM102)
63-65: Use a single
ifstatement instead of nestedifstatements (SIM102)
64-65: Use a single
ifstatement instead of nestedifstatements (SIM102)
80-81: Use a single
ifstatement instead of nestedifstatements (SIM102)
Additional comments not posted (12)
tests/e2e/detectors/snapshots/detectors__detector_ArbitrarySendEth_0_7_6_arbitrary_send_eth_sol__0.txt (2)
3-3: Change in recipient of transaction inTest.direct()method.The method now sends ETH to the caller (
msg.sender) instead of a predefineddestination. This change should be clearly documented and justified, as it fundamentally alters the behavior of the function.
7-7: Inconsistency in handling ofdestinationinTest.indirect()method.The method continues to use the old
destination.send(address(this).balance)method. This may be inconsistent with the new approach inTest.direct()and could lead to confusion or errors. Please verify if this is intentional or an oversight.tests/e2e/detectors/snapshots/detectors__detector_ArbitrarySendEth_0_6_11_arbitrary_send_eth_sol__0.txt (2)
3-3: Change in recipient of transaction inTest.direct()method.The method now sends ETH to the caller (
msg.sender) instead of a predefineddestination. This change should be clearly documented and justified, as it fundamentally alters the behavior of the function.
7-7: Inconsistency in handling ofdestinationinTest.indirect()method.The method continues to use the old
destination.send(address(this).balance)method. This may be inconsistent with the new approach inTest.direct()and could lead to confusion or errors. Please verify if this is intentional or an oversight.tests/e2e/detectors/test_data/arbitrary-send-eth/0.6.11/arbitrary_send_eth.sol (3)
4-4: Introduction of immutable destination variabledestination_imm.This change enhances security by making the destination address constant after deployment, preventing unauthorized changes. It aligns with best practices for handling critical addresses in smart contracts.
8-8: Initialization ofdestination_immin the constructor.The constructor initializes
destination_immwithmsg.sender, setting the contract deployer as the default destination. This decision should be clearly documented to ensure clarity.
12-14: New methodsend_immutable()correctly uses the immutable destination.This method sends the contract’s balance to the
destination_imm. The use of the immutable variable here ensures that the destination cannot be altered, enhancing the security of the transaction.tests/e2e/detectors/test_data/arbitrary-send-eth/0.7.6/arbitrary_send_eth.sol (3)
4-4: Introduction of immutable destination variabledestination_imm.This change enhances security by making the destination address constant after deployment, preventing unauthorized changes. It aligns with best practices for handling critical addresses in smart contracts.
8-8: Initialization ofdestination_immin the constructor.The constructor initializes
destination_immwithmsg.sender, setting the contract deployer as the default destination. This decision should be clearly documented to ensure clarity.
12-14: New methodsend_immutable()correctly uses the immutable destination.This method sends the contract’s balance to the
destination_imm. The use of the immutable variable here ensures that the destination cannot be altered, enhancing the security of the transaction.slither/detectors/functions/arbitrary_send_eth.py (2)
33-33: Import added:StateVariablefromslither.core.variables.state_variable.This import is necessary for the new logic to check if a destination is an immutable state variable. Ensure that
StateVariableis utilized properly in the rest of the code.
71-72: Optimization in conditional checks for immutable state variables.The addition of a check for immutable state variables (
ir.destination.is_immutable) is a crucial update to ensure that the detector does not report false positives when the destination is a state variable marked as immutable.Verification successful
The search results confirm that the
is_immutableattribute is used in the context of state variables. The specific line inslither/detectors/functions/arbitrary_send_eth.pythat includes the check foris_immutableis also found, verifying that the new logic is in place.
slither/detectors/functions/arbitrary_send_eth.py: if isinstance(ir.destination, StateVariable) and ir.destination.is_immutable:This confirms that the new check correctly identifies immutable state variables and skips them, ensuring the detector does not report false positives.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new check correctly identifies immutable state variables and skips them. # Test: Search for usage of `is_immutable` in the context of state variables. rg --type python $'is_immutable'Length of output: 62
Script:
#!/bin/bash # Description: Verify that the new check correctly identifies immutable state variables and skips them. # Correct the file type and search for usage of `is_immutable` in the Python codebase. rg 'is_immutable' --type pyLength of output: 1566
Summary by CodeRabbit
New Features
send_immutablefunction in smart contracts to send ETH to an immutable address.Bug Fixes
Tests