-
Notifications
You must be signed in to change notification settings - Fork 16
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
Behavior of memory to storage copies #218
Open
shellygr
wants to merge
11
commits into
master
Choose a base branch
from
shelly/gapsfromvoodoodoc
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
cb0e51c
to be tested - to-storage copies
shellygr d694904
update
shellygr d0e97f9
further elaboration on the loop subject
shellygr 59797d0
subtitle added
shellygr 7cf39aa
formatting
shellygr 91be959
Merge branch 'master' into shelly/gapsfromvoodoodoc
shellygr 27677e9
formatting
shellygr 8c69f6c
Merge branch 'shelly/gapsfromvoodoodoc' of https://github.com/Certora…
shellygr 735bbb8
WIP - revert causes parsing
shellygr b266735
comment out todos
shellygr 7a2a531
remove
shellygr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -180,4 +180,138 @@ is sufficient both by running the basic sanity rule, | |||||
and if loops appear only under certain conditions, | ||||||
to write specialized sanity rules that force the Prover to reason about these | ||||||
particular code paths. | ||||||
Mutation testing can also be useful here. | ||||||
Mutation testing can also be useful here. | ||||||
|
||||||
### 'Hidden' compiler-generated loops | ||||||
|
||||||
#### Copying Solidity memory arrays to storage | ||||||
|
||||||
When the Solidity compiler generates code for copying | ||||||
a non-primitive object (could be a `bytes` buffer or a `struct` with a `bytes` field), | ||||||
it generates two loops. | ||||||
The first loop resets any previous remaining data written into the target storage slot. | ||||||
The second loop copies the new object into the relevant storage slot. | ||||||
|
||||||
Consider the following contract: | ||||||
```solidity | ||||||
// MemoryToStorage.sol | ||||||
contract MemoryToStorage { | ||||||
struct ScheduledExecution { | ||||||
address where; | ||||||
bool execute; | ||||||
bytes data; | ||||||
} | ||||||
|
||||||
ScheduledExecution[] myArray; | ||||||
|
||||||
function testPush(address where, bool executed) public { | ||||||
bytes memory data = abi.encodeWithSelector( | ||||||
this.testPush.selector, | ||||||
"aa" | ||||||
); | ||||||
myArray.push(ScheduledExecution(where, executed, data)); | ||||||
} | ||||||
} | ||||||
``` | ||||||
|
||||||
The push to `myArray` generates the two aforementioned loops. | ||||||
If we wish to analyze this code with the Prover, there are two questions to be answered: | ||||||
|
||||||
1. Do we need to set a value for `--loop_iter` which is bigger than 1? | ||||||
|
||||||
Yes - the `data` local variable is put into a `struct ScheduledExecution` that is | ||||||
put into an array in storage. | ||||||
This is done by the compiler using a 'copy-loop'. | ||||||
The selector component `this.testPush.selector` requires one iteration. | ||||||
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 string `aa` requires three iterations: according to the [ABI specification](https://docs.soliditylang.org/en/v0.8.24/abi-spec.html), | ||||||
it consists of an offset to a dynamic buffer, the size of the buffer, | ||||||
and then the (short) data element fitting in one word (32 bytes). | ||||||
Therefore, `--loop_iter` should be set to 3. | ||||||
|
||||||
To test our configuration, we use the following specification: | ||||||
```cvl | ||||||
// sanity.spec | ||||||
use builtin rule sanity; | ||||||
``` | ||||||
|
||||||
Therefore: | ||||||
```bash | ||||||
// Passes: | ||||||
certoraRun MemoryToStorage.sol --verify MemoryToStorage:sanity.spec --loop_iter 3 | ||||||
|
||||||
// Violates sanity: | ||||||
certoraRun MemoryToStorage.sol --verify MemoryToStorage:sanity.spec --loop_iter 2 | ||||||
``` | ||||||
|
||||||
2. Do we need to set `--optimistic_loop`? | ||||||
|
||||||
Yes - when we copy `data` from memory to storage, the Solidity compiler | ||||||
also generates code that nullifies the previous data. | ||||||
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
|
||||||
As we do not know (and probably not wishing to constrain) the size of the previous data, | ||||||
we have to enable `--optimistic_loop`. | ||||||
|
||||||
While the sanity rule does not check for auto-generated assertions, | ||||||
any run with assertions would generate an additional sub-rule for auto-generated assertions | ||||||
that will fail without `--optimistic_loop`. | ||||||
|
||||||
Given the following specification: | ||||||
```cvl | ||||||
// simpleAssert.spec | ||||||
rule simpleAssert { | ||||||
env e; | ||||||
calldataarg arg; | ||||||
method f; | ||||||
f(e,arg); | ||||||
assert true; | ||||||
} | ||||||
``` | ||||||
|
||||||
We have that: | ||||||
```bash | ||||||
// Passes: | ||||||
certoraRun MemoryToStorage.sol --verify MemoryToStorage:simpleAssert.spec --loop_iter 3 --optimistic_loop | ||||||
|
||||||
// Violated with "Unwinding condition in a loop": | ||||||
certoraRun MemoryToStorage.sol --verify MemoryToStorage:simpleAssert.spec --loop_iter 3 | ||||||
``` | ||||||
|
||||||
##### Is `--loop_iter 3` always sufficient? | ||||||
|
||||||
Note that running with `--optimistic_loop` on the above example imposes an assumption | ||||||
on the size of the previous buffer written in `myArray[0].data`. | ||||||
One could have more complex flows where `--loop_iter 3` is not sufficient to properly | ||||||
unroll the erasure loop. | ||||||
|
||||||
Consider for example this revised contract: | ||||||
```solidity | ||||||
// MemoryToStorage2.sol | ||||||
contract MemoryToStorage { | ||||||
... | ||||||
|
||||||
function testPush(address where, bool executed) public { | ||||||
require (myArray[0].data.length == 225); | ||||||
bytes memory data = abi.encodeWithSelector( | ||||||
this.testPush.selector, | ||||||
"aa" | ||||||
); | ||||||
myArray[0] = ScheduledExecution(where, executed, data); | ||||||
} | ||||||
} | ||||||
``` | ||||||
|
||||||
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 only difference in this new functionality is that we assume that the previous data has size of 225 bytes. | ||||||
When we update `myArray[0]` with `data`, the Solidity compiler will put zeroes | ||||||
in the space that was occupied by `myArray[0].data` beyond the length of `data`. | ||||||
If the previous buffer size was 224 for example, then since the size of the new `data` is 128, | ||||||
it means we need to clean `224 - 128 = 96` bytes, or 3 words. | ||||||
This is of course feasible with `--loop_iter 3`. | ||||||
However, here we require the previous buffer size to be 225, which means we need to clean 4 extra words, | ||||||
thus requiring a minimal `--loop_iter` value of 4. | ||||||
|
||||||
```bash | ||||||
// Violates sanity: | ||||||
certoraRun MemoryToStorage2.sol:MemoryToStorage --verify MemoryToStorage:sanity.spec --loop_iter 3 | ||||||
|
||||||
// Passes: | ||||||
certoraRun MemoryToStorage2.sol:MemoryToStorage --verify MemoryToStorage:sanity.spec --loop_iter 4 | ||||||
``` |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Isn't the prover smart enough to unroll the loop enough times for this case? The string is static