A modifier that reads `msg.value` and writes state loses the state-write requirement: an `internal view` function using it compiles into code containing `SSTORE`
- Dominant language
- C++
- Stars
- 25.7k
- Forks
- 6.2k
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 21
Description
## Description
`ViewPureChecker` infers a modifier's state mutability as a **single ordered maximum** over
`StateMutability { Pure, View, NonPayable, Payable }`. `Payable` and `NonPayable` are not ordered
requirements though — they are *independent* obligations ("may receive value" vs "writes state").
When a modifier does both, `Payable` is numerically greater and the state write is silently
discarded.
`Payable` is then deliberately permitted on non-public functions, so an `internal` or `private`
function declared `view` receives **no diagnostic at all** and is compiled with the modifier's
`SSTORE` inlined into it.
The practical effect is that adding an unrelated `msg.value` read to a modifier **suppresses an
error that solc otherwise reports**.
## Environment
- Compiler version: 0.8.36 (source of `develop` is identical at these lines)
- Both pipelines (legacy and `--via-ir`)
## Reproducer
```solidity
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.0;
contract C {
uint public x;
modifier m() { uint v = msg.value; v; x = 1; _; }
function f() internal view m returns (uint) { return x; }
function g() public view returns (uint) { return f(); }
}
```
Compiles with **no error and no warning** (solc 0.8.36, legacy and via-IR). `solc --asm` shows
`sstore` in the runtime code, and `solc --abi` reports `"stateMutability": "view"` for `g`.
Executed (py-evm, Shanghai):
```
x() -> 0
g() -> 1 // g is declared `public view`
x() -> 1 // ... and it wrote storage
```
## Control
Delete `uint v = msg.value; v;` from the modifier and leave everything else identical:
```
Error 8961: Function cannot be declared as view because this expression (potentially) modifies the state.
--> control.sol:10:32:
|
10 | function f() internal view m returns (uint) { return x; }
| ^
```
Full matrix (solc 0.8.36), modifier = `{ uint v = msg.value; v; x = 1; }`:
| function declaration | result |
|---|---|
| `function f() internal view m` | **accepted (bug)** |
| `function f() private view m` | **accepted (bug)** |
| `function f() internal pure m` | rejected (2527) — caught only incidentally, by the `Payable && Pure` branch |
| `function f() public view m` | rejected (4006), but the message is about `msg.value` and never mentions the state write |
## Cause
`libsolidity/analysis/ViewPureChecker.cpp:239-240`:
```cpp
if (_mutability > m_bestMutabilityAndLocation.mutability)
m_bestMutabilityAndLocation = MutabilityAndLocation{_mutability, _location};
```
While inferring the modifier, `x = 1` reports `NonPayable` and `msg.value` reports `Payable`;
`Payable > NonPayable` (`libsolidity/ast/ASTEnums.h:37`), so only `Payable` survives.
`modifierMutability()` stores that single value, and `endVisit(ModifierInvocation)`
(`:462-468`) passes it to `reportMutability`, whose `Payable` branch (`:270-295`) errors only when
`m_currentFunction->isConstructor() || m_currentFunction->isPublic()` — intentionally, so internal
functions may use `msg.value`. The discarded `NonPayable` requirement is never reported by anyone.
A fix needs the payable requirement tracked separately from the view/pure requirement rather than
folded into one maximum.
## Relationship to #16931
[#16931](https://github.com/argotorg/solidity/issues/16931) reports the same *symptom* class — a
`pure`/`view` function silently gaining state access through a modifier — but by a different
mechanism: overriding a `virtual` modifier, where `resolveVirtual()` re-resolves past the modifier
`ViewPureChecker` validated against. This report needs no inheritance, no `virtual` and no
`override`; the *statically bound* modifier's own inferred mutability is already wrong. A fix in
`OverrideChecker` for #16931 would not address this.
Contributor guide
Research direction
Start by reproducing the internal and private view cases in the Solidity example with both legacy and --via-ir pipelines. Read libsolidity/analysis/ViewPureChecker.cpp around modifierMutability(), endVisit(ModifierInvocation), and reportMutability(), along with libsolidity/ast/ASTEnums.h:37. Done means the modifier's state-write requirement is no longer discarded when msg.value is also read, while the documented payable behavior remains intact.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, solidity
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 65/100