argotorg / argotorg/solidity

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`

Open
#16,980 0 comments 0 reactions 0 assignees View on GitHub
bug :bug:
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.