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

Exception on overflow #796

Open
phiferd opened this Issue Aug 2, 2016 · 31 comments

Comments

10 participants
@phiferd

phiferd commented Aug 2, 2016

It would be nice if Solidity, by default, threw an exception for arithmetic overflow, rather than allowing it.

Justification:

  1. Overflow is rarely desired behavior and is commonly cited as a security vulnerability.
    2: Even in cases where overflow is desired behavior, throwing an exception is a far safer failure mode and easier to detect/correct.
    3: The goal of solidity, as I understand it, is to enable the creation of secure and reliable smart contacts. Therefore, although it is of course possible for the developer to include their own checks, including this as a feature of the language will make the common case both simple and more secure.

If this is not achievable, I welcome alternatives that do not require each and every developer to remember these corner cases for each and every arithmetic operation. A solution that states "everybody should do XYZ" will inevitably lead to failures.

I'm am not familiar enough with the code to propose a detailed solution. However, a compiler switch that is on by default and can be disabled for backward compatibility is the sort of approach I was imagining.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Aug 2, 2016

The main Problem I see with this is that you still have to aware of the possibility of an arithmetic overflow. If we throw in that case, this could stall a smart contract.

The only solution I see is to use our formal verification system that automatically verifies whether overflow protection checks are in place (or not needed).

@phiferd

This comment has been minimized.

phiferd commented Aug 2, 2016

Could the same argument be made for the exception that is thrown for division by 0 and a negative array index? Those could have the same effect, but I can't see arguing that coming up with some default but incorrect behavior would be better (e.g. We could just wrap array indices around).

My concern is that the failures that can come from overflow go undetected.

I am definitely not suggesting that formal verification should not be done, there is no substitute. But I think allowing overflow offers no real advantage, unless you're hoping to hack a contract.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Aug 2, 2016

You have a valid point. Still, I think these are not exactly comparable:
Invalid array access and division by zero are more predictable and less "continuous" than arithmetic overflow.

But yes, it is probably better to stall a contract in case of overflow than to provoke undesired effects on state.

What are cases where overflow is actually desired? I think for most of them, we can still use mulmod and addmod.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Aug 12, 2016

Note that this is quite expensive to do, especially for types shorter than 256 bits.

@rainbreak

This comment has been minimized.

Contributor

rainbreak commented Aug 29, 2016

Would you consider this for 0.4.0, or is it a bit involved for that?

@ethernomad

This comment has been minimized.

Contributor

ethernomad commented Aug 29, 2016

Maybe this should be implemented in the EVM?

@axic

This comment has been minimized.

Member

axic commented Aug 29, 2016

I think arithmetic overflow detection belongs to the EVM, but that would be a rather big change to it.

@rainbreak

This comment has been minimized.

Contributor

rainbreak commented Aug 30, 2016

For the EVM I think it would need extra opcodes as overflowing math is still useful. In the meantime we can have it in Solidity and get the safety benefits at the expense of a bit of gas (completely worth it I think because you end up checking it in safe code anyway).

@chriseth

This comment has been minimized.

Contributor

chriseth commented Aug 30, 2016

@rainbeam This is too involved for 0.4.0. Arithmetics is almost for free when compared to storage writes or even a transaction, so I think it is fine to do it. I do not really agree that this belongs to the EVM. Something like an overflow flag might be added to the EVM, but not a "force-stop" on overflow.

@axic

This comment has been minimized.

Member

axic commented Aug 30, 2016

I think arithmetic overflow detection belongs to the EVM

By detection I meant the overflow flag. So perhaps that would then needs its on opcode for getting the overflow flag onto the stack and/or its own jump (JUMPOF).

@chriseth

This comment has been minimized.

Contributor

chriseth commented Oct 13, 2016

@pirapira Can you please list all these other issues as tasks for this issue and close the others, or do you think that each issues is a substantial amount of work?

@pirapira

This comment has been minimized.

Member

pirapira commented Oct 13, 2016

The main reason I opened separate issues is that each requires discussion if we want it or not, but if we are set to go over everything, here is a list.

  • exception on overflow in unsigned->signed conversion
  • exception on overflow in signed->unsigned conversion
  • exception on overflow in size-decreasing implicit conversion
  • exception on overflow in addition of two signed numbers
  • exception on overflow in addition of two unsigned numbers
  • exception on underflow in subtraction of two signed numbers
  • exception on underflow in subtraction of two unsigned numbers
  • exception on overflow in multiplication of two signed numbers
  • exception on overflow in multiplication of two unsigned numbers
  • exception on overflow in shifts
  • exception on overflow in ++ on a signed number
  • exception on overflow in ++ on an unsigned number
  • exception on underflow in -- on a signed number
  • exception on underflow in -- on an unsigned number
  • exception on overflow in +=
  • exception on overflow in -=
  • exception on overflow in *=
  • exception on overflow in /=
  • make sure no optimizations are relying on (a + b - b == a); lest they remove overflow exceptions
  • compiler error on an out-of-range constant expression when the constant expression is interpreted into a type (uint x = -1)
  • exception on overflow in INT_MIN/-1 #1091
  • exponentiation
@ethers

This comment has been minimized.

Member

ethers commented Dec 20, 2016

This is labelled "soon" and how soon will this be? :) Does the long checklist need to be implemented at once, or can it be done iteratively?
Implementing some of them can already provide early safety/ROI imo, like:

  • exception on overflow in addition of two unsigned numbers
  • exception on underflow in subtraction of two unsigned numbers
@ethers

This comment has been minimized.

Member

ethers commented Dec 20, 2016

@pirapira In your checklist, exception on overflow in /= is duplicated. I think for one of them you mean exception on underflow in -=.

Also, how about overflow of INT_MIN/-1 ?

@pirapira

This comment has been minimized.

Member

pirapira commented Dec 21, 2016

@ethers Thank you. I fixed the checklist above.

These overflow checkings are breaking changes, and for some reasons, Solidity release 0.5.0 will be the next one that can contain breaking changes. I would support removing this rule (until we get to 1.0.0).

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Jun 30, 2017

If Solidity doesn't already do this, a line item in the TODO list should be added for truncation during assignment.

uint16 a = 2^15;
uint8 b = a;
@axic

This comment has been minimized.

Member

axic commented Jun 30, 2017

@MicahZoltu not sure what compiler version you are using, but that should not compile.

@MicahZoltu

This comment has been minimized.

Contributor

MicahZoltu commented Jun 30, 2017

I didn't test it, so if you say its already handled then I trust that it is. I assume the compiler will give me an error even if I use var b = 0; b = a; as well since b will be uint8 which can't hold a uint16 without a cast?

@axic

This comment has been minimized.

Member

axic commented Jun 30, 2017

Different int types can only be assigned using explicit conversion. As of the next release the int size inferred at var assignment will also be displayed to the user.

796.sol:5:1: Warning: The type of this variable was inferred as uint8, which can hold values between 0 and 255. This is probably not desired. Use an explicit type to silence this warning.
var b = 0;
^-------^
796.sol:6:5: Error: Type uint16 is not implicitly convertible to expected type uint8.
b = a;
    ^

@chriseth chriseth added this to To do in 0.5.0 Feb 16, 2018

@chriseth chriseth moved this from To do to Optional in 0.5.0 Mar 1, 2018

@axic

This comment has been minimized.

Member

axic commented Apr 20, 2018

For the EVM I think it would need extra opcodes as overflowing math is still useful.

How about having a modifier area for unchecked arithmetics:

function f(uint a, uint b) returns (uint c) {
  uncheckedmath {
    c := a - b;
  }
}

Then again it can be accomplished with inline assembly if it is needed.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Apr 20, 2018

I am really torn about what should be the default. I don't think that we should force the user to resort to inline assembly for the non-default case, though, since it makes complex crypto operations much less readable.

@axic

This comment has been minimized.

Member

axic commented Apr 20, 2018

I think it may be good to have overflow checks on by default, except in an unchecked area, because then the SMT checker could also ignore those blocks and just assume their values can be the entire range after the block, requiring the user to insert require/assert statements after.

In general user code is about high level logic where they do not expect overflows and then there are specific blocks of code which do expect it, but I think that is the minority and hence it makes sense to turn it on by default.

We could also verify this assumption by checking the user code we already have imported as part of the test suite.

@pirapira

This comment has been minimized.

Member

pirapira commented Apr 20, 2018

One way to get the intention of the programmer is to introduce types for numbers meant as wei, seconds (duration) seconds (UNIX time)...

@axic

This comment has been minimized.

Member

axic commented Apr 20, 2018

One way to get the intention of the programmer is to introduce types for numbers meant as wei, seconds (duration) seconds (UNIX time)...

For that I'd suggest to have a look at Vyper. It also is a good (perhaps complementary) solution.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Apr 20, 2018

@axic yes, an unchecked region where also the SMT checker is disabled is certainly a good idea. Whether we can enable runtime checks by default in the non-unchecked areas should also depend on an assessment about how well the optimizer can handle such cases, and perhaps an improvement in that area.

@phiferd

This comment has been minimized.

phiferd commented Apr 26, 2018

The recently discovered overflow on batchTransfer in some ERC20 contracts is a good example of the type of issue that could be avoided if this is implemented.

https://medium.com/@peckshield/alert-new-batchoverflow-bug-in-multiple-erc20-smart-contracts-cve-2018-10299-511067db6536

@axic

This comment has been minimized.

Member

axic commented Apr 26, 2018

For the syntax (mentioned in #796 (comment)) I propose:

unchecked "overflow" {
  a = b + c;
}

It follows the same syntax as not-so-well-known syntax of the assembly keyword: assembly "evmasm" {}

@chriseth

This comment has been minimized.

Contributor

chriseth commented Apr 26, 2018

@axic yep, exactly my thought, that allows us to introduce other checks in the same way later.

@axic

This comment has been minimized.

Member

axic commented Apr 26, 2018

that allows us to introduce other checks in the same way later.

That was my motivation :)

@kumavis

This comment has been minimized.

Member

kumavis commented Apr 26, 2018

@axic what about making the current math operators safe and adding new operators for non-safe math

c = a + b // safe
c = a %+ b // non-safe

pick whatever symbol works, i chose %+ to imply modular arithmetic

Edit:
I guess this would not handle the conversion cases (e.g. signed->unsigned)
And the generic check-modifying block opens up interesting possibilities

@rocky

This comment has been minimized.

rocky commented Aug 18, 2018

In #796 (comment) I think there is one more case not mentioned, exponentiation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment