feat: add msg-value-in-nonpayable detector (#2923)

* feat: add msg-value-in-nonpayable detector

Implements #2781. Detects uses of msg.value in functions that cannot be
reached from any payable entry point.

- Uses call graph analysis via all_reachable_from_functions
- Properly formats output using Slither objects for JSON/SARIF
- Includes test cases for internal function call chains
- Limited to Solidity (LANGUAGE = "solidity")

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

* fix: update type annotations and add test artifacts

- Replace deprecated typing.List/Tuple with builtin list/tuple (ruff UP035/UP006)
- Add from __future__ import annotations for PEP 604 support
- Add missing test zip artifact and snapshot file

* fix: correct snapshot order for msg-value-in-nonpayable detector

---------

Co-authored-by: ep0chzer0 <[email protected]>
Co-authored-by: Dan Guido <[email protected]>

Author: ep0chzer0

slither/detectors/all_detectors.py

@@ -83,6 +83,7 @@
 from .functions.dead_code import DeadCode
 from .statements.write_after_write import WriteAfterWrite
 from .statements.msg_value_in_loop import MsgValueInLoop
+from .statements.msg_value_in_nonpayable import MsgValueInNonPayable
 from .statements.delegatecall_in_loop import DelegatecallInLoop
 from .functions.protected_variable import ProtectedVariables
 from .functions.permit_domain_signature_collision import DomainSeparatorCollision

slither/detectors/statements/msg_value_in_nonpayable.py

@@ -0,0 +1,147 @@
+"""
+Detector for msg.value usage in functions unreachable from payable entry points.
+Related to issue #2781.
+"""
+
+from __future__ import annotations
+
+from slither.utils.output import Output
+from slither.detectors.abstract_detector import (
+    AbstractDetector,
+    DetectorClassification,
+    DETECTOR_INFO,
+)
+from slither.core.declarations.solidity_variables import SolidityVariableComposed
+from slither.core.declarations.function import Function
+
+
+class MsgValueInNonPayable(AbstractDetector):
+    """
+    Detects uses of msg.value in functions that cannot be reached
+    from any payable public/external entry point.
+
+    Such msg.value usages are provably meaningless:
+    - msg.value will always be 0, or
+    - execution will always revert
+    """
+
+    ARGUMENT = "msg-value-in-nonpayable"
+    HELP = "msg.value used in functions unreachable from payable entry points"
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#msgvalue-in-nonpayable"
+    WIKI_TITLE = "msg.value in non-payable function"
+
+    WIKI_DESCRIPTION = """
+Detects functions that read `msg.value` but cannot be reached from any payable
+public or external entry point. In such cases, `msg.value` is always zero or
+execution always reverts."""
+
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+contract Subscription {
+    mapping(address => bool) public subscribed;
+
+    function subscribe() external {
+        require(msg.value >= 1 ether, "Fee required");
+        subscribed[msg.sender] = true;
+    }
+}
+```
+`subscribe()` is not payable, so it cannot receive ETH. `msg.value` is always zero
+and the require always fails, making the function unusable."""
+
+    WIKI_RECOMMENDATION = """
+Either mark the function as `payable` if it should receive ETH, or remove the
+`msg.value` check since it will always be zero in non-payable contexts."""
+
+    # Only applicable to Solidity (Vyper handles payable differently)
+    LANGUAGE = "solidity"
+
+    def _detect(self) -> list[Output]:
+        results = []
+
+        for contract in self.compilation_unit.contracts_derived:
+            for func in contract.functions:
+                # Skip if not implemented
+                if not func.is_implemented:
+                    continue
+
+                # Check if function or its modifiers use msg.value
+                if not self._uses_msg_value(func) and not any(
+                    self._uses_msg_value(m) for m in func.modifiers
+                ):
+                    continue
+
+                # Payable functions are valid entry points for msg.value
+                if func.payable:
+                    continue
+
+                # Get all entry points that can reach this function
+                payable_callers, non_payable_callers = self._get_entry_point_callers(func)
+
+                # If any payable entry point reaches this function, msg.value is valid
+                if payable_callers:
+                    continue
+
+                # Otherwise, msg.value is unreachable from payable contexts - flag it
+                info: DETECTOR_INFO = self._build_info(func, non_payable_callers)
+                results.append(self.generate_result(info))
+
+        return results
+
+    def _uses_msg_value(self, function: Function) -> bool:
+        """Returns True if the function directly reads msg.value."""
+        for node in function.nodes:
+            for ir in node.irs:
+                for read in ir.read:
+                    if isinstance(read, SolidityVariableComposed) and read.name == "msg.value":
+                        return True
+        return False
+
+    def _get_entry_point_callers(self, function: Function) -> tuple[list[Function], list[Function]]:
+        """
+        Walk the reverse call graph to find all public/external entry points.
+
+        Returns:
+            (payable_callers, non_payable_callers) - only public/external functions
+        """
+        payable_callers = []
+        non_payable_callers = []
+
+        # Include the function itself if it's an entry point
+        if function.visibility in ("public", "external"):
+            if function.payable:
+                payable_callers.append(function)
+            else:
+                non_payable_callers.append(function)
+
+        # Check all functions that can reach this one
+        for caller in function.all_reachable_from_functions:
+            if not isinstance(caller, Function):
+                continue
+            if caller.visibility not in ("public", "external"):
+                continue
+
+            if caller.payable:
+                payable_callers.append(caller)
+            else:
+                non_payable_callers.append(caller)
+
+        return payable_callers, non_payable_callers
+
+    def _build_info(self, func: Function, non_payable_callers: list[Function]) -> DETECTOR_INFO:
+        """Build properly formatted detector output using Slither objects."""
+        info: DETECTOR_INFO = [
+            func,
+            " uses msg.value but is not reachable from any payable function\n",
+        ]
+
+        if non_payable_callers:
+            info.append("\tNon-payable entry points that can reach this function:\n")
+            for caller in non_payable_callers:
+                if caller != func:
+                    info.extend(["\t- ", caller, "\n"])
+
+        return info

tests/e2e/detectors/snapshots/detectors__detector_MsgValueInNonPayable_0_8_0_msg_value_in_nonpayable_sol__0.txt

@@ -0,0 +1,8 @@
+TestMsgValueInNonPayable._helper() (tests/e2e/detectors/test_data/msg-value-in-nonpayable/0.8.0/msg_value_in_nonpayable.sol#21-23) uses msg.value but is not reachable from any payable function
+	Non-payable entry points that can reach this function:
+	- TestMsgValueInNonPayable.bad2() (tests/e2e/detectors/test_data/msg-value-in-nonpayable/0.8.0/msg_value_in_nonpayable.sol#29-31)
+
+TestMsgValueInNonPayable._addBalance() (tests/e2e/detectors/test_data/msg-value-in-nonpayable/0.8.0/msg_value_in_nonpayable.sol#12-14) uses msg.value but is not reachable from any payable function
+	Non-payable entry points that can reach this function:
+	- TestMsgValueInNonPayable.bad1() (tests/e2e/detectors/test_data/msg-value-in-nonpayable/0.8.0/msg_value_in_nonpayable.sol#16-18)
+

tests/e2e/detectors/test_data/msg-value-in-nonpayable/0.8.0/msg_value_in_nonpayable.sol

@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+// In Solidity 0.8+, the compiler prevents msg.value in non-payable public/external functions.
+// But it doesn't catch INTERNAL functions that use msg.value when only called from non-payable entry points.
+// This detector catches those cases.
+
+contract TestMsgValueInNonPayable {
+    mapping(address => uint256) public balances;
+
+    // BAD: Internal function using msg.value, only called from non-payable
+    function _addBalance() internal {
+        balances[msg.sender] += msg.value;
+    }
+
+    function bad1() external {
+        _addBalance();
+    }
+
+    // BAD: Internal helper in call chain with no payable entry
+    function _helper() internal view returns (uint256) {
+        return msg.value;
+    }
+
+    function _middle() internal view returns (uint256) {
+        return _helper();
+    }
+
+    function bad2() external view returns (uint256) {
+        return _middle();
+    }
+
+    // GOOD: Internal function called from payable entry point
+    function _processPayment() internal {
+        balances[msg.sender] += msg.value;
+    }
+
+    function good1() external payable {
+        _processPayment();
+    }
+
+    // GOOD: Internal function reachable from at least one payable function
+    function _sharedHelper() internal view returns (uint256) {
+        return msg.value;
+    }
+
+    function good2_nonpayable() external view returns (uint256) {
+        return _sharedHelper();
+    }
+
+    function good2_payable() external payable returns (uint256) {
+        return _sharedHelper();
+    }
+}
+
+contract TestMultipleCallers {
+    uint256 public value;
+
+    // Internal function with multiple callers - one payable
+    function _setValue() internal {
+        value = msg.value;
+    }
+
+    // Non-payable caller (alone would be BAD)
+    function caller1() external {
+        _setValue();
+    }
+
+    // Payable caller - makes _setValue GOOD
+    function caller2() external payable {
+        _setValue();
+    }
+
+    // _setValue should NOT be flagged because caller2 is payable
+}

tests/e2e/detectors/test_data/msg-value-in-nonpayable/0.8.0/msg_value_in_nonpayable.sol-0.8.0.zip

@@ -1487,6 +1487,11 @@ def id_test(test_item: Test):
         "msg_value_loop.sol",
         "0.8.0",
     ),
+    Test(
+        all_detectors.MsgValueInNonPayable,
+        "msg_value_in_nonpayable.sol",
+        "0.8.0",
+    ),
     Test(
         all_detectors.DelegatecallInLoop,
         "delegatecall_loop.sol",