Skip to content

Commit 727591b

Browse files
ekpyronekpyron
authored
Merge pull request #13479 from ethereum/unusedStoreStorageBug
Fix of unused store remover storage bug.
2 parents 548a4b4 + d6eb255 commit 727591b

10 files changed

+210
-2
lines changed

‎Changelog.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
### 0.8.17 (unreleased)
22

33
Important Bugfixes:
4+
* Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.
45

56

67
Language Features:

‎docs/bugs.json

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,17 @@
11
[
2+
{
3+
"uid": "SOL-2022-7",
4+
"name": "StorageWriteRemovalBeforeConditionalTermination",
5+
"summary": "Calling functions that conditionally terminate the external EVM call using the assembly statements ``return(...)`` or ``stop()`` may result in incorrect removals of prior storage writes.",
6+
"description": "A call to a Yul function that conditionally terminates the external EVM call could result in prior storage writes being incorrectly removed by the Yul optimizer. This used to happen in cases in which it would have been valid to remove the store, if the Yul function in question never actually terminated the external call, and the control flow always returned back to the caller instead. Conditional termination within the same Yul block instead of within a called function was not affected. In Solidity with optimized via-IR code generation, any storage write before a function conditionally calling ``return(...)`` or ``stop()`` in inline assembly, may have been incorrectly removed, whenever it would have been valid to remove the write without the ``return(...)`` or ``stop()``. In optimized legacy code generation, only inline assembly that did not refer to any Solidity variables and that involved conditionally-terminating user-defined assembly functions could be affected.",
7+
"link": "https://blog.soliditylang.org/2022/09/08/storage-write-removal-before-conditional-termination/",
8+
"introduced": "0.8.13",
9+
"fixed": "0.8.17",
10+
"severity": "medium/high",
11+
"conditions": {
12+
"yulOptimizer": true
13+
}
14+
},
215
{
316
"uid": "SOL-2022-6",
417
"name": "AbiReencodingHeadOverflowWithStaticArrayCleanup",

‎docs/bugs_by_version.json

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1737,6 +1737,7 @@
17371737
},
17381738
"0.8.13": {
17391739
"bugs": [
1740+
"StorageWriteRemovalBeforeConditionalTermination",
17401741
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
17411742
"DirtyBytesArrayToStorage",
17421743
"InlineAssemblyMemorySideEffects",
@@ -1747,6 +1748,7 @@
17471748
},
17481749
"0.8.14": {
17491750
"bugs": [
1751+
"StorageWriteRemovalBeforeConditionalTermination",
17501752
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
17511753
"DirtyBytesArrayToStorage",
17521754
"InlineAssemblyMemorySideEffects"
@@ -1755,12 +1757,15 @@
17551757
},
17561758
"0.8.15": {
17571759
"bugs": [
1760+
"StorageWriteRemovalBeforeConditionalTermination",
17581761
"AbiReencodingHeadOverflowWithStaticArrayCleanup"
17591762
],
17601763
"released": "2022-06-15"
17611764
},
17621765
"0.8.16": {
1763-
"bugs": [],
1766+
"bugs": [
1767+
"StorageWriteRemovalBeforeConditionalTermination"
1768+
],
17641769
"released": "2022-08-08"
17651770
},
17661771
"0.8.2": {

‎libyul/optimiser/UnusedStoreEliminator.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,10 +105,13 @@ void UnusedStoreEliminator::operator()(FunctionCall const& _functionCall)
105105
else
106106
sideEffects = m_controlFlowSideEffects.at(_functionCall.functionName.name);
107107

108+
if (sideEffects.canTerminate)
109+
changeUndecidedTo(State::Used, Location::Storage);
108110
if (!sideEffects.canContinue)
109111
{
110112
changeUndecidedTo(State::Unused, Location::Memory);
111-
changeUndecidedTo(sideEffects.canTerminate ? State::Used : State::Unused, Location::Storage);
113+
if (!sideEffects.canTerminate)
114+
changeUndecidedTo(State::Unused, Location::Storage);
112115
}
113116
}
114117

‎test/libsolidity/semanticTests/unused_store_storage_removal_bug.sol

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
contract C {
2+
uint public x;
3+
function f() public {
4+
x = 1; // This write used to be removed by the Yul optimizer due to the StorageWriteRemovalBeforeConditionalTermination bug.
5+
g();
6+
x = 2;
7+
}
8+
function g() internal {
9+
if (msg.data.length > 4) return;
10+
assembly { return(0, 0) }
11+
}
12+
}
13+
// ----
14+
// f() ->
15+
// x() -> 1

‎test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call.yul

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
{
2+
function conditionallyStop() {
3+
if calldataload(0) { leave }
4+
return(0, 0)
5+
}
6+
let x := 0
7+
let y := 1
8+
sstore(x, y) // used to be removed due to the StorageWriteRemovalBeforeConditionalTermination
9+
conditionallyStop()
10+
sstore(x, y)
11+
}
12+
// ----
13+
// step: unusedStoreEliminator
14+
//
15+
// {
16+
// {
17+
// let x := 0
18+
// let y := 1
19+
// sstore(x, y)
20+
// conditionallyStop()
21+
// sstore(x, y)
22+
// }
23+
// function conditionallyStop()
24+
// {
25+
// if calldataload(0) { leave }
26+
// return(0, 0)
27+
// }
28+
// }

‎test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call_complex_control_flow.yul

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
{
2+
function conditionallyStop() {
3+
if calldataload(0) { leave }
4+
return(0, 0)
5+
}
6+
function g() {
7+
let a := 0
8+
let b := 1
9+
sstore(a, b)
10+
}
11+
let x := 0
12+
let y := 1
13+
let z := 2
14+
switch calldataload(64)
15+
case 0 {
16+
sstore(z, x)
17+
g()
18+
}
19+
default {
20+
sstore(x, z) // used to be removed due to the StorageWriteRemovalBeforeConditionalTermination
21+
}
22+
conditionallyStop()
23+
switch calldataload(32)
24+
case 0 {
25+
revert(0, 0)
26+
}
27+
default {
28+
sstore(x, z)
29+
}
30+
}
31+
// ----
32+
// step: unusedStoreEliminator
33+
//
34+
// {
35+
// {
36+
// let x := 0
37+
// let y := 1
38+
// let z := 2
39+
// switch calldataload(64)
40+
// case 0 {
41+
// sstore(z, x)
42+
// g()
43+
// }
44+
// default { sstore(x, z) }
45+
// conditionallyStop()
46+
// switch calldataload(32)
47+
// case 0 { revert(0, 0) }
48+
// default { sstore(x, z) }
49+
// }
50+
// function conditionallyStop()
51+
// {
52+
// if calldataload(0) { leave }
53+
// return(0, 0)
54+
// }
55+
// function g()
56+
// {
57+
// let a := 0
58+
// sstore(a, 1)
59+
// }
60+
// }

‎test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call_revert.yul

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
{
2+
function conditionallyStop() {
3+
if calldataload(0) { leave }
4+
return(0, 0)
5+
}
6+
let x := 0
7+
let y := 1
8+
sstore(x, y) // used to be removed due to the to the StorageWriteRemovalBeforeConditionalTermination bug
9+
conditionallyStop()
10+
revert(0,0)
11+
}
12+
// ----
13+
// step: unusedStoreEliminator
14+
//
15+
// {
16+
// {
17+
// let x := 0
18+
// sstore(x, 1)
19+
// conditionallyStop()
20+
// revert(0, 0)
21+
// }
22+
// function conditionallyStop()
23+
// {
24+
// if calldataload(0) { leave }
25+
// return(0, 0)
26+
// }
27+
// }

‎test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_indirectly_conditionally_terminating_function_call.yul

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
{
2+
function conditionallyStop() {
3+
if calldataload(0) { leave }
4+
returnEmpty()
5+
}
6+
function returnEmpty() {
7+
return(0, 0)
8+
}
9+
let x := 0
10+
let y := 1
11+
sstore(x, y) // used to be removed due to a bug
12+
conditionallyStop()
13+
sstore(x, y)
14+
}
15+
// ----
16+
// step: unusedStoreEliminator
17+
//
18+
// {
19+
// {
20+
// let x := 0
21+
// let y := 1
22+
// sstore(x, y)
23+
// conditionallyStop()
24+
// sstore(x, y)
25+
// }
26+
// function conditionallyStop()
27+
// {
28+
// if calldataload(0) { leave }
29+
// returnEmpty()
30+
// }
31+
// function returnEmpty()
32+
// { return(0, 0) }
33+
// }

‎test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_unconditionally_terminating_function_call.yul

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
{
2+
function neverStop() {
3+
if calldataload(0) { leave } // prevent inlining
4+
}
5+
let x := 0
6+
let y := 1
7+
sstore(x, y) // should be removed
8+
neverStop()
9+
sstore(x, y)
10+
}
11+
// ----
12+
// step: unusedStoreEliminator
13+
//
14+
// {
15+
// {
16+
// let x := 0
17+
// let y := 1
18+
// neverStop()
19+
// sstore(x, y)
20+
// }
21+
// function neverStop()
22+
// { if calldataload(0) { leave } }
23+
// }

0 commit comments

Comments
 (0)