ethereum
Insufficient check for uninitialized storage pointer access
Description
contract Foo {
struct Hi {
uint256 hello;
}
function foo() internal returns (Hi storage ret) {
ret = ret;
ret.hello = 123;
}
}
This code accesses ret storage pointer without initializing it. It should not compile, but the Solidity compiler accepts this code.
Environment
- Compiler version:
Version: 0.8.18+commit.87f61d96.Linux.g++ - Target EVM version (as per compiler settings): Default setting
- Operating system: Ubuntu 22.04
Steps to Reproduce
Save the above file as test.sol and run solc test.sol.
Without ret = ret; line, the expected warning message is printed:
Warning: SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.
--> test.sol
Warning: Source file does not specify required compiler version! Consider adding "pragma solidity ^0.8.18;"
--> test.sol
Error: This variable is of storage pointer type and can be returned without prior assignment, which would lead to undefined behaviour.
--> test.sol:6:38:
|
6 | function foo() internal returns (Hi storage ret) {
| ^^^^^^^^^^^^^^
Error: This variable is of storage pointer type and can be accessed without prior assignment, which would lead to undefined behaviour.
--> test.sol:7:9:
|
7 | ret.hello = 123;
| ^^^
Note: The variable was declared here.
--> test.sol:6:38:
|
6 | function foo() internal returns (Hi storage ret) {
| ^^^^^^^^^^^^^^
Yeah, looks like a bug. The compiler normally would not let you use ret in an expression before it is initialized. After the assignment it would stop complaining. Here it's wrongly assuming it has already been assigned when processing the right-hand side.
After discussing it in the team it turns out that it was at least partially intentional. You can suppress a warning about an unused return variable by assigning it to itself - any expression involving the variable on the right-hand side of the assignment will do this. However, this was done way back, before you could assign to storage variables in inline assembly, which would be a better and more explicit way to silence it. The storage case is potentially more dangerous than other cases so we're still going to consider it a bug and disallow it.
The storage case is potentially more dangerous than other cases so we're still going to consider it a bug and disallow it.
Just to clarify: disallowing this right a way would still definitely be breaking. Our conclusion in the end was that we start emitting a warning for this now (which can then be silenced by an assembly assignment), and then turn that into an error in 0.9, right?
