Solidity: Exception on overflow

[phiferd]

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]

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]

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]

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]

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

[rainbreak]

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

[ethernomad]

Maybe this should be implemented in the EVM?

[axic]

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

[rainbreak]

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]

@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]

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]

@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]

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
• [x] 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 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]

@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]

@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]

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]

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

[MicahZoltu]

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]

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;
    ^

[axic]

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]

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]

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]

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

[axic]

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]

@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]

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]

For the syntax (mentioned in https://github.com/ethereum/solidity/issues/796#issuecomment-383060269) 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]

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

[axic]

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

That was my motivation :)

[kumavis]

@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]

In https://github.com/ethereum/solidity/issues/796#issuecomment-253578925 I think there is one more case not mentioned, exponentiation.

[johnsoncarl]

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
• [x] 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

How many of these are completed till now?

[chriseth]

@johnsoncarl we are implementing this along with the transition to yul as an intermediate language. The reason is that we hope that the yul optimizer can better cope with all the additional checks that are introduced and complicate the control-flow. It is still not 100% clear if we really want that on explicit type conversions. It is implemented (or at least PRs in progress) for increment, decrement, unsigned multiplication and addition.

[chriseth]

We are thinking of using the existing yul implementation and adding it to 0.7.0.

[chriseth]

Implemented in #9465