diff --git a/Changelog.md b/Changelog.md index f0f8b8485..c03dbb6d1 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,6 +1,7 @@ ### 0.8.17 (unreleased) Important Bugfixes: + * Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call. Language Features: diff --git a/docs/bugs.json b/docs/bugs.json index 421bb007c..03dafbe45 100644 --- a/docs/bugs.json +++ b/docs/bugs.json @@ -1,4 +1,17 @@ [ + { + "uid": "SOL-2022-7", + "name": "StorageWriteRemovalBeforeConditionalTermination", + "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.", + "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.", + "link": "https://blog.soliditylang.org/2022/09/08/storage-write-removal-before-conditional-termination/", + "introduced": "0.8.13", + "fixed": "0.8.17", + "severity": "medium/high", + "conditions": { + "yulOptimizer": true + } + }, { "uid": "SOL-2022-6", "name": "AbiReencodingHeadOverflowWithStaticArrayCleanup", diff --git a/docs/bugs_by_version.json b/docs/bugs_by_version.json index 3e3847c9b..df8fdc633 100644 --- a/docs/bugs_by_version.json +++ b/docs/bugs_by_version.json @@ -1737,6 +1737,7 @@ }, "0.8.13": { "bugs": [ + "StorageWriteRemovalBeforeConditionalTermination", "AbiReencodingHeadOverflowWithStaticArrayCleanup", "DirtyBytesArrayToStorage", "InlineAssemblyMemorySideEffects", @@ -1747,6 +1748,7 @@ }, "0.8.14": { "bugs": [ + "StorageWriteRemovalBeforeConditionalTermination", "AbiReencodingHeadOverflowWithStaticArrayCleanup", "DirtyBytesArrayToStorage", "InlineAssemblyMemorySideEffects" @@ -1755,12 +1757,15 @@ }, "0.8.15": { "bugs": [ + "StorageWriteRemovalBeforeConditionalTermination", "AbiReencodingHeadOverflowWithStaticArrayCleanup" ], "released": "2022-06-15" }, "0.8.16": { - "bugs": [], + "bugs": [ + "StorageWriteRemovalBeforeConditionalTermination" + ], "released": "2022-08-08" }, "0.8.2": { diff --git a/libyul/optimiser/UnusedStoreEliminator.cpp b/libyul/optimiser/UnusedStoreEliminator.cpp index 754c02ac5..2e96be2e7 100644 --- a/libyul/optimiser/UnusedStoreEliminator.cpp +++ b/libyul/optimiser/UnusedStoreEliminator.cpp @@ -105,10 +105,13 @@ void UnusedStoreEliminator::operator()(FunctionCall const& _functionCall) else sideEffects = m_controlFlowSideEffects.at(_functionCall.functionName.name); + if (sideEffects.canTerminate) + changeUndecidedTo(State::Used, Location::Storage); if (!sideEffects.canContinue) { changeUndecidedTo(State::Unused, Location::Memory); - changeUndecidedTo(sideEffects.canTerminate ? State::Used : State::Unused, Location::Storage); + if (!sideEffects.canTerminate) + changeUndecidedTo(State::Unused, Location::Storage); } } diff --git a/test/libsolidity/semanticTests/unused_store_storage_removal_bug.sol b/test/libsolidity/semanticTests/unused_store_storage_removal_bug.sol new file mode 100644 index 000000000..87d4ef429 --- /dev/null +++ b/test/libsolidity/semanticTests/unused_store_storage_removal_bug.sol @@ -0,0 +1,15 @@ +contract C { + uint public x; + function f() public { + x = 1; // This write used to be removed by the Yul optimizer due to the StorageWriteRemovalBeforeConditionalTermination bug. + g(); + x = 2; + } + function g() internal { + if (msg.data.length > 4) return; + assembly { return(0, 0) } + } +} +// ---- +// f() -> +// x() -> 1 diff --git a/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call.yul b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call.yul new file mode 100644 index 000000000..44fbb7011 --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call.yul @@ -0,0 +1,28 @@ +{ + function conditionallyStop() { + if calldataload(0) { leave } + return(0, 0) + } + let x := 0 + let y := 1 + sstore(x, y) // used to be removed due to the StorageWriteRemovalBeforeConditionalTermination + conditionallyStop() + sstore(x, y) +} +// ---- +// step: unusedStoreEliminator +// +// { +// { +// let x := 0 +// let y := 1 +// sstore(x, y) +// conditionallyStop() +// sstore(x, y) +// } +// function conditionallyStop() +// { +// if calldataload(0) { leave } +// return(0, 0) +// } +// } diff --git a/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call_complex_control_flow.yul b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call_complex_control_flow.yul new file mode 100644 index 000000000..dd88f4973 --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call_complex_control_flow.yul @@ -0,0 +1,60 @@ +{ + function conditionallyStop() { + if calldataload(0) { leave } + return(0, 0) + } + function g() { + let a := 0 + let b := 1 + sstore(a, b) + } + let x := 0 + let y := 1 + let z := 2 + switch calldataload(64) + case 0 { + sstore(z, x) + g() + } + default { + sstore(x, z) // used to be removed due to the StorageWriteRemovalBeforeConditionalTermination + } + conditionallyStop() + switch calldataload(32) + case 0 { + revert(0, 0) + } + default { + sstore(x, z) + } +} +// ---- +// step: unusedStoreEliminator +// +// { +// { +// let x := 0 +// let y := 1 +// let z := 2 +// switch calldataload(64) +// case 0 { +// sstore(z, x) +// g() +// } +// default { sstore(x, z) } +// conditionallyStop() +// switch calldataload(32) +// case 0 { revert(0, 0) } +// default { sstore(x, z) } +// } +// function conditionallyStop() +// { +// if calldataload(0) { leave } +// return(0, 0) +// } +// function g() +// { +// let a := 0 +// sstore(a, 1) +// } +// } diff --git a/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call_revert.yul b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call_revert.yul new file mode 100644 index 000000000..fc45bb64c --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_conditionally_terminating_function_call_revert.yul @@ -0,0 +1,27 @@ +{ + function conditionallyStop() { + if calldataload(0) { leave } + return(0, 0) + } + let x := 0 + let y := 1 + sstore(x, y) // used to be removed due to the to the StorageWriteRemovalBeforeConditionalTermination bug + conditionallyStop() + revert(0,0) +} +// ---- +// step: unusedStoreEliminator +// +// { +// { +// let x := 0 +// sstore(x, 1) +// conditionallyStop() +// revert(0, 0) +// } +// function conditionallyStop() +// { +// if calldataload(0) { leave } +// return(0, 0) +// } +// } diff --git a/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_indirectly_conditionally_terminating_function_call.yul b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_indirectly_conditionally_terminating_function_call.yul new file mode 100644 index 000000000..1d15078eb --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_indirectly_conditionally_terminating_function_call.yul @@ -0,0 +1,33 @@ +{ + function conditionallyStop() { + if calldataload(0) { leave } + returnEmpty() + } + function returnEmpty() { + return(0, 0) + } + let x := 0 + let y := 1 + sstore(x, y) // used to be removed due to a bug + conditionallyStop() + sstore(x, y) +} +// ---- +// step: unusedStoreEliminator +// +// { +// { +// let x := 0 +// let y := 1 +// sstore(x, y) +// conditionallyStop() +// sstore(x, y) +// } +// function conditionallyStop() +// { +// if calldataload(0) { leave } +// returnEmpty() +// } +// function returnEmpty() +// { return(0, 0) } +// } diff --git a/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_unconditionally_terminating_function_call.yul b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_unconditionally_terminating_function_call.yul new file mode 100644 index 000000000..c104e4d9b --- /dev/null +++ b/test/libyul/yulOptimizerTests/unusedStoreEliminator/store_before_unconditionally_terminating_function_call.yul @@ -0,0 +1,23 @@ +{ + function neverStop() { + if calldataload(0) { leave } // prevent inlining + } + let x := 0 + let y := 1 + sstore(x, y) // should be removed + neverStop() + sstore(x, y) +} +// ---- +// step: unusedStoreEliminator +// +// { +// { +// let x := 0 +// let y := 1 +// neverStop() +// sstore(x, y) +// } +// function neverStop() +// { if calldataload(0) { leave } } +// }