-
Notifications
You must be signed in to change notification settings - Fork 28
Add blog post describing the returndatacopy bug and fix.
#198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,131 @@ | ||||||||||||||
| --- | ||||||||||||||
| layout: post | ||||||||||||||
| published: true | ||||||||||||||
| title: 'UnusedStoreEliminator Stale Return Data Size Bug' | ||||||||||||||
| date: '2026-04-21' | ||||||||||||||
| author: Solidity Team | ||||||||||||||
| category: Security Alerts | ||||||||||||||
| --- | ||||||||||||||
|
|
||||||||||||||
| On January 17, 2026, a bug in the Yul optimizer was reported by **Carl Andersen** from [Spearbit](https://spearbit.com). The bug could cause `returndatacopy` operations to be incorrectly removed. It has been assigned identifier **SOL-2026-2** and name **UnusedStoreEliminatorStaleReturnDataSize**. | ||||||||||||||
|
|
||||||||||||||
| The bug is fixed in Solidity **0.8.35**. | ||||||||||||||
|
|
||||||||||||||
| ## Summary | ||||||||||||||
|
|
||||||||||||||
| The Yul optimizer's `UnusedStoreEliminator` pass could incorrectly remove `returndatacopy(...)` operations when the length argument was supplied from a `returndatasize()` value that had become stale due to an intervening call opcode. Because `returndatacopy` reverts on out-of-bounds access, removing it in this case silently suppresses a revert that should have occurred, allowing execution to continue past a point where it should have stopped. | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, I think it's a good convention to have a line break after every sentence. Makes diffs clearer and smaller, but also does not require an annoying rewrap after each change like a fixed line width does. |
||||||||||||||
|
|
||||||||||||||
| The bug only affects **handwritten Yul or inline assembly** code. High-level Solidity code was never affected. The severity is rated **very low**. | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| ## Technical Background | ||||||||||||||
|
|
||||||||||||||
| The `UnusedStoreEliminator` is an optimizer step in the Yul pipeline that removes write operations (to memory or storage) whose results are provably never read. Eligible operations include `mstore`, `mstore8`, `codecopy`, `calldatacopy`, `extcodecopy`, and — since version 0.8.13 — `returndatacopy`. | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Otherwise the list should include |
||||||||||||||
|
|
||||||||||||||
| The `returndatacopy` instruction is special among memory-copying opcodes: unlike `calldatacopy` or `codecopy`, it **reverts** when accessing bytes beyond the bounds of the return data buffer. Since reverts are a side effect the optimizer must preserve, a `returndatacopy` can only be removed when it is certain that it will never access out-of-bounds memory. | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Saying that it's "special" makes it sound like this behavior is justified. This should be worded more strongly. In fact, this is a good opportunity to emphasize how annoying this behavior is for compilers (and that EOF would have fixed it). |
||||||||||||||
|
|
||||||||||||||
| The only pattern where this can be guaranteed statically is `returndatacopy(0, 0, returndatasize())` — copying the entire return data buffer from the beginning. This is also the exact pattern emitted by the Solidity code generator for handling return data from external calls. Starting in 0.8.13, the optimizer was special-cased to recognize and remove only this pattern. | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not the only one. Just the only one we cared about due to the fact that it's emitted by the codegen.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| ## The Bug | ||||||||||||||
|
|
||||||||||||||
| The special-case check introduced in 0.8.13 was incomplete. It correctly identified calls of the form: | ||||||||||||||
|
|
||||||||||||||
| ```yul | ||||||||||||||
| let s := returndatasize() | ||||||||||||||
| returndatacopy(0, 0, s) | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| as equivalent to `returndatacopy(0, 0, returndatasize())` and allowed them to be removed. However, it **did not account for the possibility that the variable `s` could become stale**. The return data buffer — and therefore the valid range for `returndatasize()` — is reset by any of the following opcodes: `call`, `staticcall`, `delegatecall`, and `callcode`. If any such opcode appears between the assignment of `s` and the `returndatacopy`, the stored size no longer reflects the current return buffer, and the check is no longer sound. | ||||||||||||||
|
|
||||||||||||||
| Consider the following Yul snippet: | ||||||||||||||
|
|
||||||||||||||
| ```yul | ||||||||||||||
| { | ||||||||||||||
| let s := returndatasize() // capture size before first call | ||||||||||||||
| pop(call(gas(), addr, 0, 0, 0, 0, 0)) // this resets the return data buffer! | ||||||||||||||
| returndatacopy(0, 0, s) // s is now stale — buffer may be smaller | ||||||||||||||
|
Comment on lines
+43
to
+45
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| } | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| Before the fix, the optimizer would recognize the `returndatacopy(0, 0, s)` pattern — `s` is a variable holding a `returndatasize()` value and the start offset is zero — and allow the entire `returndatacopy` to be removed as if it were safe. In reality, since the `call` in between may have changed the return data buffer to a size smaller than `s`, the `returndatacopy` would revert with an out-of-bounds access. Removing it suppresses that revert, allowing execution to continue incorrectly. | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| The analogous inline assembly pattern in Solidity is: | ||||||||||||||
|
|
||||||||||||||
| ```solidity | ||||||||||||||
| contract C { | ||||||||||||||
| function f(address addr) external { | ||||||||||||||
| assembly { | ||||||||||||||
| let s := returndatasize() | ||||||||||||||
| pop(call(gas(), addr, 0, 0, 0, 0, 0)) | ||||||||||||||
| // Bug: this returndatacopy was incorrectly removed by the optimizer | ||||||||||||||
| returndatacopy(0, 0, s) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| ## Impact | ||||||||||||||
|
|
||||||||||||||
| **Affected versions:** 0.8.13 – 0.8.34 | ||||||||||||||
|
|
||||||||||||||
| **Required conditions for the bug to trigger:** | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Conditions that trigger the bug should be more prominent. We always put them as one of the first things in the post. I don't think they fit in the "Impact" section anyway. They should also be more concise. The additional explanations, like why the high-level code is not affected or when the optimizer is enabled should be discussed separately. The list is very important as quick assessment criteria for people to check if they're affected. |
||||||||||||||
| - The Yul optimizer must be enabled (this is the default in production compilations with `--optimize`). | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not the default. If you meant that it's the default in frameworks or something, please be more specific. Otherwise it sounds like it's the default in the compiler. |
||||||||||||||
| - The EVM version must be at least Byzantium (the first version to introduce `returndatacopy` and `returndatasize`). | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| - The affected code pattern must appear in **inline assembly or handwritten Yul**. The Solidity code generator itself never produces code interleaving multiple external calls with access to a previous call's return data, so high-level Solidity code is not affected. | ||||||||||||||
| - The memory region written by `returndatacopy` must **not be read afterward**. The `UnusedStoreEliminator` only removes a write operation when it can prove the written memory is never subsequently read. If any later instruction reads from the same memory region, the store is kept regardless of the pattern match. | ||||||||||||||
|
|
||||||||||||||
| **Effect:** A `returndatacopy` that would have reverted due to an out-of-bounds access is silently removed, causing execution to continue when it should have stopped. The memory region targeted by the copy is never written; the program proceeds with stale memory contents and no revert occurs. The practical impact depends on how that memory is used afterward, but given the specific conditions required to trigger the bug, real-world impact is expected to be minimal. | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This focuses a bit too much on the mechanics of what happens when the bug is triggered. On the other hand it does not get deep enough into the practical impact, i.e. how likely this is to impact anyone in the first place. The important thing to mention is that we don't think anyone is likely to intentionally write assembly using Then, even if someone did that, they may still not even be aware that the instruction is supposed to revert on out-of-bounds access (that's not a usual property of other similar instructions), so removing the revert may not be harmful. And even then, the removal of the revert is the only effect. The bug would not make the compiler remove
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Damn, having written this like this makes me question if this even deserves |
||||||||||||||
|
|
||||||||||||||
| The severity is rated **very low** because the pattern that triggers it only arises in manually written assembly and is highly unlikely to appear in typical production code. | ||||||||||||||
|
|
||||||||||||||
| ## Workaround | ||||||||||||||
|
|
||||||||||||||
| If you are using a version between 0.8.13 and 0.8.34 and your code uses `returndatacopy` in inline assembly or Yul following an external call, avoid storing the result of `returndatasize()` in a variable before the call and using it as the length argument afterward. Instead, call `returndatasize()` fresh at the point of `returndatacopy`: | ||||||||||||||
|
Comment on lines
+80
to
+82
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure it makes sense to include such a workaround. Like, if your |
||||||||||||||
|
|
||||||||||||||
| ```solidity | ||||||||||||||
| // Safe: always reads the current return data size | ||||||||||||||
| assembly { | ||||||||||||||
| pop(call(gas(), addr, 0, 0, 0, 0, 0)) | ||||||||||||||
| returndatacopy(0, 0, returndatasize()) | ||||||||||||||
| } | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| ## The Fix | ||||||||||||||
|
|
||||||||||||||
| The fix, implemented in [PR #16508](https://github.com/argotorg/solidity/pull/16508), takes the conservative approach of **removing the `returndatacopy` optimization entirely** from the `UnusedStoreEliminator`. The `RETURNDATACOPY` instruction is now unconditionally blacklisted from removal alongside `MCOPY`. | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs some detail on why we thought removing the optimization is a good idea. |
||||||||||||||
|
|
||||||||||||||
| An alternative fix ([PR #16436](https://github.com/argotorg/solidity/pull/16436)) was considered that would have tracked `returndatasize()` values and invalidated them whenever an intervening call opcode was encountered. This approach would have preserved the optimization for the safe case but was ultimately rejected because the optimization has minimal practical impact on generated code size or performance, while the additional tracking logic made the `UnusedStoreEliminator` implementation more complicated and fragile. | ||||||||||||||
|
|
||||||||||||||
| The relevant change in `UnusedStoreEliminator.cpp` is: | ||||||||||||||
|
|
||||||||||||||
| ```cpp | ||||||||||||||
| // Before (0.8.13–0.8.34): RETURNDATACOPY was a candidate for removal | ||||||||||||||
| // with a special-case check for the (0, 0, returndatasize()) pattern. | ||||||||||||||
|
|
||||||||||||||
| // After (0.8.35): RETURNDATACOPY is blacklisted unconditionally. | ||||||||||||||
| bool const isBlacklisted = | ||||||||||||||
| *instruction == Instruction::MCOPY || | ||||||||||||||
| *instruction == Instruction::RETURNDATACOPY; | ||||||||||||||
| bool const isCandidateForRemoval = | ||||||||||||||
| !isBlacklisted && | ||||||||||||||
| SemanticInformation::otherState(*instruction) != SemanticInformation::Write && ( | ||||||||||||||
| SemanticInformation::storage(*instruction) == SemanticInformation::Write || | ||||||||||||||
| (!m_ignoreMemory && SemanticInformation::memory(*instruction) == SemanticInformation::Write) | ||||||||||||||
| ); | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| ## Timeline | ||||||||||||||
|
|
||||||||||||||
| | Date | Event | | ||||||||||||||
| |------|-------| | ||||||||||||||
| | 2022-05-17 | Optimization first introduced in 0.8.13 (partial fix for out-of-bounds revert preservation) | | ||||||||||||||
| | 2026-01-17 | Bug reported by Carl Andersen from Spearbit | | ||||||||||||||
| | 2026-04-17 | Fix committed in PR #16508 | | ||||||||||||||
| | 2026-XX-XX | Fixed in Solidity 0.8.35 | | ||||||||||||||
|
Comment on lines
+116
to
+123
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't normally include the timeline in such posts. And I don't think it's that important. |
||||||||||||||
|
|
||||||||||||||
| ## Credits | ||||||||||||||
|
|
||||||||||||||
| The bug was reported by **Carl Andersen** from [Spearbit](https://spearbit.com). The fix was implemented by the Solidity compiler team. Special thanks to the reviewers on PR #16508 for the discussion that led to the simpler and safer removal-based approach. | ||||||||||||||
|
|
||||||||||||||
| --- | ||||||||||||||
|
|
||||||||||||||
| *If you believe you have found a security-relevant bug in the Solidity compiler, please follow the [responsible disclosure process](https://github.com/ethereum/solidity/blob/develop/SECURITY.md).* | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up that date needs to be updated.