Skip to content
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

Do over/underflow checks for addition (SafeMath) #265

Merged

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Feb 22, 2021

What was wrong?

As explained in #153 artihmetic operations should check for under/overflows by default and revert if such are detected.

How was it fixed?

This adds checks for all additions of both signed and unsigned integers.

  1. Added checked_add_u256 and checked_add_i256 functions to runtime
  2. Used them for fe::BinOperator::Add in mapper

@@ -239,7 +239,10 @@ pub fn expr_bin_operation(
.typ;

return match op.node {
fe::BinOperator::Add => Ok(expression! { add([yul_left], [yul_right]) }),
fe::BinOperator::Add => match typ.is_signed_integer() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i imagine at some point we'll add more checking functions for smaller integers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. I will expand this PR to include checks for all integer sizes for additions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done. Additions for all supported sizes should now be checked.

@cburgdorf cburgdorf force-pushed the christoph/feat/checked_addition branch from 2f96c9d to 39988a5 Compare February 23, 2021 09:31
@codecov-io
Copy link

codecov-io commented Feb 23, 2021

Codecov Report

Merging #265 (6e51389) into master (f2c2204) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
- Coverage   94.03%   94.00%   -0.04%     
==========================================
  Files          54       55       +1     
  Lines        3791     3834      +43     
==========================================
+ Hits         3565     3604      +39     
- Misses        226      230       +4     
Impacted Files Coverage Δ
compiler/src/yul/mappers/assignments.rs 97.82% <100.00%> (ø)
compiler/src/yul/mappers/expressions.rs 97.24% <100.00%> (ø)
compiler/src/yul/runtime/functions/math.rs 100.00% <100.00%> (ø)
compiler/src/yul/runtime/functions/mod.rs 100.00% <100.00%> (ø)
analyzer/src/namespace/types.rs 81.48% <0.00%> (-1.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2c2204...6e51389. Read the comment docs.

@cburgdorf cburgdorf force-pushed the christoph/feat/checked_addition branch from 39988a5 to 6e51389 Compare February 23, 2021 10:32
@cburgdorf cburgdorf force-pushed the christoph/feat/checked_addition branch from 6e51389 to 71c7224 Compare February 23, 2021 14:08
@g-r-a-n-t
Copy link
Member

looks great 👍

@cburgdorf cburgdorf merged commit 5736067 into ethereum:master Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants