Enhancement: Detect msg.value Usage in Non-Payable Functions
- Dominant language
- Python
- Stars
- 6.4k
- Forks
- 1.1k
- PR merge metrics
- No merged PRs in 30d
Description
## Summary
Add a detector that identifies uses of `msg.value` in functions that can never receive ETH, including non-payable functions and their entire call chains, helping developers identify logic errors and dead code.
## Motivation
Using `msg.value` in non-payable functions is a common mistake that indicates:
- **Logic errors** - The developer expected the function to receive ETH
- **Dead code** - The msg.value check will always be zero
- **Incorrect assumptions** - May lead to bypassed security checks
- **Gas waste** - Unnecessary checks that always fail
This is more sophisticated than simple visibility checking - it requires analyzing the entire call graph to ensure `msg.value` is only used in functions that can be reached from payable functions.
## Problem Examples
```solidity
contract VulnerableContract {
// ISSUE 1: Direct msg.value in non-payable
function processPayment() external {
require(msg.value > 0, "Payment required"); // Always fails!
// Process...
}
// ISSUE 2: Indirect msg.value usage
function internalCheck() internal view returns (bool) {
return msg.value >= 1 ether;
}
function publicEntry() external view returns (bool) {
return internalCheck(); // msg.value always 0 here
}
// ISSUE 3: msg.value in modifier used by non-payable
modifier requiresPayment() {
require(msg.value > 0, "ETH required");
_;
}
function doSomething() external requiresPayment {
// This can never execute successfully
}
// OK: Proper payable function
function properPayable() external payable {
require(msg.value > 0, "Payment required"); // Valid use
}
}
```
## Detection Algorithm
The detector should:
1. Build a complete function call graph
2. Find all msg.value accesses
3. For each access, check if it can be reached from any payable function
4. Track modifier usage and inheritance patterns
5. Report unreachable msg.value uses with call chain information
## Expected Output
```
msg.value used in non-payable context
Location: Contract.sol:45
Function: processPayment() [non-payable]
Issue: msg.value will always be 0
Call chain analysis:
Entry points that can reach this function:
- publicEntry() [external, non-payable] -> processPayment()
- adminFunction() [external, non-payable] -> internalHelper() -> processPayment()
No payable functions can reach this code.
Suggestion: Either:
1. Make the function payable if it should receive ETH
2. Remove the msg.value check as it will always be 0
3. Ensure this function is only called from payable contexts
```
## Test Cases
```solidity
contract TestCases {
modifier needsPayment() {
require(msg.value > 0);
_;
}
// DETECT: Direct msg.value in non-payable
function test1() external {
if (msg.value > 0) { } // Issue
}
// DETECT: msg.value in view function
function test2() external view returns (uint) {
return msg.value; // Always 0
}
// DETECT: Modifier with msg.value on non-payable
function test3() external needsPayment {
// Can never execute
}
// OK: Payable function
function test4() external payable {
require(msg.value > 0); // Valid
}
// OK: Internal called from payable
function test5Internal() internal {
require(msg.value > 0); // OK if called from payable
}
function test5() external payable {
test5Internal(); // Valid call chain
}
// DETECT: Complex call chain, no payable entry
function helper() internal view returns (bool) {
return msg.value > 0;
}
function middle() internal view returns (bool) {
return helper();
}
function entry() external view returns (bool) {
return middle(); // Detects msg.value in helper
}
}
```
## Benefits
1. **Catch logic errors early** - Identify functions that can't work as intended
2. **Remove dead code** - Eliminate checks that always fail
3. **Improve gas efficiency** - Remove unnecessary msg.value checks
4. **Prevent security issues** - Avoid bypassed payment checks
5. **Better code understanding** - Clarify payment flow in contracts
## Priority
**High** - This is a common error pattern that indicates fundamental misunderstanding of Solidity's payment model. It can lead to functions that can never execute successfully, wasted gas, and potentially security issues if payment checks are bypassed. The detector has high precision and provides immediate value to developers.
Contributor guide
Assessment
This issue has not been assessed yet.