-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add storage bounds to account procedures #886
Conversation
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.
Looks good! Thank you! I left some comments inline. The main one is about potentially merging validate_storage_sizes
logic into validate_storage_offsets
.
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.
Looks good! Thank you! I left some more comments inline. After these are addressed, we can merge.
# assert that if size is 0 then offset is 0 | ||
dup.1 eq.0 | ||
if.true | ||
dup eq.0 assert.err=ERR_INVALID_STORAGE_OFFSET_FOR_SIZE | ||
end | ||
# => [storage_offset, storage_size, index, num_storage_slots, num_account_procedures] |
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.
Let's add a TODO
for this block to figure out how to do this without the if
statement.
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.
Why would we need to remove the if
statement?
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.
if
statements are relatively expensive. So, if we can do this with some small number (e.g., half a dozen or so) non-conditional instructions, this would be quite a bit more efficient.
In this PR I propose to add storage bounds to account procedures this will prevent a procedure from accessing unauthorised storage slots.
We have added a lower bound in the
storage_offset
but no upper bound. This pr addsstorage_size
which will be the upper bound for any account code procedure.Closes: #866