feat: add msg-value-in-nonpayable detector

[ep0chzer0]

Summary

Implements #2781. Adds a detector that identifies uses of msg.value in functions that cannot be reached from any payable entry point.

Detection Logic

The detector:

1. Identifies functions that directly read msg.value (including via modifiers)
2. Uses Slither's call graph (all_reachable_from_functions) to find all entry points
3. Checks if any payable public/external function can reach the code
4. Reports functions where msg.value is unreachable from payable contexts

Key Features

• Proper output format: Uses Slither objects for JSON/SARIF compatibility
• Call chain analysis: Includes non-payable entry points in the report for context
• Language restriction: Limited to Solidity (LANGUAGE = "solidity")
• Test coverage: Includes tests for internal function call chains

Note on Solidity 0.8+

Solidity 0.8+ prevents direct msg.value use in non-payable public/external functions at compile time. This detector primarily catches internal functions that use msg.value but are only reachable from non-payable entry points.

Example

contract Example {
    function _addBalance() internal {
        balances[msg.sender] += msg.value;  // Flagged!
    }

    function deposit() external {  // Non-payable
        _addBalance();
    }
}

Test Results

The detector correctly identifies:

• _addBalance() - internal function only called from non-payable bad1()
• _helper() - internal function only called from non-payable bad2() via _middle()

And correctly does NOT flag:

• Functions called from payable entry points
• Functions reachable from at least one payable function

[dguido]

Code Review: msg-value-in-nonpayable detector

Thanks for implementing this detector! I've reviewed the changes against Slither's conventions and the project CLAUDE.md. Overall this is a well-structured implementation. Here are my findings:

---

Critical Issues (90-100 confidence)

1. Missing snapshot file (Confidence: 95)

• File: tests/e2e/detectors/snapshots/
• Issue: The PR adds a test in test_detectors.py but does not include the corresponding snapshot file. Slither's e2e tests compare detector output against snapshots. The test will fail on CI without a snapshot like detectors__detector_MsgValueInNonPayable_0_8_0_msg_value_in_nonpayable_sol__0.txt.
• Fix: Run the tests locally with --snapshot-update to generate the snapshot file, or run the detector manually and capture expected output.

---

Important Issues (80-89 confidence)

2. Should use functions_declared instead of functions (Confidence: 85)

• File: slither/detectors/statements/msg_value_in_nonpayable.py, line 62
• Issue: The detector iterates over contract.functions which includes inherited functions. Per CLAUDE.md: "Use _declared suffix - functions_declared, state_variables_declared avoid processing inherited items multiple times."
• Current: for func in contract.functions:
• Suggested: for func in contract.functions_declared:
• Rationale: Without this, inherited internal functions that use msg.value could be reported multiple times (once per derived contract).

3. Should check function.contract_declarer filter (Confidence: 82)

• File: slither/detectors/statements/msg_value_in_nonpayable.py, lines 62-85
• Issue: Even with functions_declared, you may want to filter by func.contract_declarer == contract to ensure you're only processing functions declared in the current contract context. This is the standard traversal pattern mentioned in CLAUDE.md.

4. Inconsistent isinstance check in _get_entry_point_callers (Confidence: 80)

• File: slither/detectors/statements/msg_value_in_nonpayable.py, line 105
• Issue: The check if not isinstance(caller, Function) suggests all_reachable_from_functions might return non-Function items. Looking at the implementation, this set should only contain Function objects. This check may be defensive but could mask issues. Consider documenting why it's needed or removing if truly unnecessary.

---

Suggestions (not blocking)

5. Consider testing multiple Solidity versions

• File: tests/e2e/detectors/test_detectors.py
• Issue: Most detectors have tests across Solidity 0.4.x-0.8.x. The current PR only tests 0.8.0. While the PR description notes the detector primarily targets Solidity 0.8+ (due to compiler checks), adding at least one older version test (e.g., 0.7.6) would verify behavior on contracts without compiler-level enforcement.

6. WIKI_DESCRIPTION could note the 0.8+ compiler behavior

• File: slither/detectors/statements/msg_value_in_nonpayable.py, lines 39-42
• Issue: The WIKI documentation is good but could mention that Solidity 0.8+ already prevents direct msg.value use in non-payable public/external functions at compile time. This detector's primary value is catching internal functions with msg.value that are only reachable from non-payable entry points. This context would help users understand when the detector adds value.

7. Minor: Line length in _uses_msg_value

• File: slither/detectors/statements/msg_value_in_nonpayable.py, line 91
• Issue: Line is 95 chars, within the 100-char limit but could be split for readability:

if isinstance(read, SolidityVariableComposed) and read.name == "msg.value":

---

Positive Observations

• Correct API usage: Uses node.irs (not node.all_slithir_operations()) per CLAUDE.md guidelines
• Proper output format: Uses Slither objects in DETECTOR_INFO for JSON/SARIF compatibility
• Good test coverage: Test file covers key scenarios (internal functions, call chains, multiple callers)
• Clear exploit scenario: WIKI documentation provides actionable example
• Language restriction: Correctly limits to Solidity with LANGUAGE = "solidity"
• Leverages existing infrastructure: Uses all_reachable_from_functions for call graph analysis

---

Summary

The detector implementation is solid and follows Slither patterns. The critical blocker is the missing snapshot file - tests will fail without it. The traversal pattern (functions vs functions_declared) should be fixed to avoid duplicate reports on inherited functions.

Once these issues are addressed, this looks ready for merge.

[dguido]

Code Review - PR #2923: msg-value-in-nonpayable detector

Summary

I have reviewed this PR and made the following fixes to address the issues identified in the previous code review.

Issues Fixed

1. Changed contract.functions to contract.functions_declared (line 64)

   • Per CLAUDE.md guidance: "Use _declared suffix - functions_declared, state_variables_declared avoid processing inherited items multiple times"
   • This prevents duplicate reports when analyzing derived contracts that inherit functions using msg.value

2. Added missing test snapshot file

   • Created tests/e2e/detectors/snapshots/detectors__detector_MsgValueInNonPayable_0_8_0_msg_value_in_nonpayable_sol__0.txt
   • The snapshot correctly identifies two problematic functions:
     • TestMsgValueInNonPayable._helper() - internal function only reachable from non-payable bad2()
     • TestMsgValueInNonPayable._addBalance() - internal function only reachable from non-payable bad1()

3. Added missing zip artifact

   • Created tests/e2e/detectors/test_data/msg-value-in-nonpayable/0.8.0/msg_value_in_nonpayable.sol-0.8.0.zip
   • Required by the e2e test infrastructure

4. Updated type annotations to modern Python syntax

   • Changed List[...] to list[...] and Tuple[...] to tuple[...]
   • Removed unused typing import
   • Fixes ruff UP035/UP006 lint errors

Test Results

• All 380 detector tests pass
• All 194 relevant unit tests pass (excluding Vyper tests which require vyper installation)
• Linting passes with ruff check

Verified Behavior

The detector correctly:

• Flags internal functions using msg.value that are only reachable from non-payable entry points
• Does NOT flag functions reachable from at least one payable entry point (even if also called from non-payable functions)
• Includes helpful context about which non-payable entry points can reach the flagged function

Merge Readiness

This PR is now ready for merge after these fixes.

[sidarth16]

Thanks @ep0chzer0 and @dguido for taking forward the work from PR #2867, refining the detector and adding test coverage in #2923.

Glad to see the final version merged !