Remove Builtin Default Over/Underflow Reverts

[mudgen]

Solidity 0.8.0 added built-in, by default, reverts when there is an overflow or underflow in an operation on integers.

I prefer the explicitness of SafeMath, or my own require statements. Over/underflow reverts by default are not a solve-all for the problem. In some cases revert on over/underflow is the wrong action, for example when it locks funds in a contract forever.

I don't want to use unchecked. I want to use SafeMath (or equivalent or better library) or my own require statements. This helps ensure that I look and think about each possible over/underflow case and not shotgun use the default functionality and assume it is the right thing to do in every case.

In addition, @wjmelements pointed out it is nice to be able to emit a user-friendly message via require specifying what exactly was under/overflowing.

gas

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop I would ever write. It can be done unchecked in this awkward way:

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

Or possibly an inlined function can be used:

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // loop logic
}

But I would prefer to write my unchecked for loops (which are all of them) in the usual way like this:

for(uint256 i; i < 10; i++;) {
  // loop logic         
}

Things possibly could be done to improve the syntax of unchecked for loops. That's great. But I would just prefer to do without default over/underflow reverts, so I handle each possible over/underflow individually, explicitly and correctly.

[wjmelements]

I would be interested to know whether the compiler can optimize the boundary checks for that for-loop case, but I suspect there are numerous other deducible cases that will have redundant checks now. Another case I suspect could not be optimized but could be deduced would be a finite geometric sum.

[leonardoalt]

In some cases revert on over/underflow is the wrong action, for example when it locks funds in a contract forever.

If that's the case you coded it wrong. That's what unchecked is for.

If you don't want to use the built-in checks at all, you can always simply

unchecked {
 <your_whole_function>
}

and add your old SafeMath code

[leonardoalt]

@wjmelements The Yul optimizer is expected to catch the simple and general loop cases once SolYul is live.

Another case I suspect could not be optimized but could be deduced would be a finite geometric sum.

Such cases are edge cases (as in not as widely used as the mentioned loops) and can obviously be handled with unchecked, since you as the developer know that it is the case.

[leonardoalt]

Built-in underflow and overflow checks were just introduced and from the feedback we got they were very well received by the community, so I don't think it's reasonable to just get rid of it right now.

[leonardoalt]

@mudgen That said, if the issue is mostly about the loop post part, could you please change the issue title?

[hrkrshnn]

Another case I suspect could not be optimized but could be deduced would be a finite geometric sum.

@wjmelements Can you give an example of what you are talking about?

[mudgen]

@mudgen That said, if the issue is mostly about the loop post part, could you please change the issue title?

It's not mostly about the loop post part.

[mudgen]

In the issue I mentioned the possibility of locking funds in a contract forever because of reverting on under/overflow.

Here's an actual, real example of that:

The GHST Staking Diamond has millions USD worth of tokens staked in it which earn "frens" points. The stake withdraw functions recalculate frens points. Reverting on over/underflow of calculation of frens locks people's staked money forever. Code is here: https://github.com/aavegotchi/ghst-staking/blob/master/contracts/facets/StakingFacet.sol

[leonardoalt]

It's not mostly about the loop post part.

Then I would suggest that the issue should be closed. The feature has been discussed at length for years and has just been added, so I don't think it makes any sense to simply remove it entirely as you suggest.

[mudgen]

@leonardoalt Okay, where is the best public place to give my feedback on Solidity language features? Maybe I posted in the wrong place.

[leonardoalt]

Sure, I know other contracts that also deal with a lot of money that also rely on underflow and overflow. Those were not written with Solidity 0.8 and built-in revert in mind. If they were, they would use unchecked and nothing would be locked.

[leonardoalt]

@leonardoalt Okay, where is the best public place to give my feedback on Solidity language features? Maybe I posted in the wrong place.

This issue reads more like a feature request and not feedback. What exactly is your goal with this issue?

[mudgen]

The contract does not rely on overflows or underflows, that would be a bug. In this case reverting is the wrong action to handle overflow/underflow, resulting in locked funds.

[leonardoalt]

The contract does not rely on overflows or underflows, that would be a bug. In this case reverting is the wrong action to handle that case, resulting in locked funds.

What I said is still valid. It was written in Solidity 0.7. If it was written in 0.8, it wouldn't be locked because you would have used unchecked.

[mudgen]

The contract does not rely on overflows or underflows, that would be a bug. In this case reverting is the wrong action to handle that case, resulting in locked funds.

What I said is still valid. It was written in Solidity 0.7. If it was written in 0.8, it wouldn't be locked because you would have used unchecked.

Yes, correct.

[mudgen]

@leonardoalt Okay, where is the best public place to give my feedback on Solidity language features? Maybe I posted in the wrong place.

This issue reads more like a feature request and not feedback. What exactly is your goal with this issue?

To give feedback and to influence change to hopefully move in the direction of removing or limiting default reverts on overflow/underflow.

[leonardoalt]

You mean rolling back to old versions and effective apply no change? ;)

[mudgen]

Can there be a pragma unchecked or something similar that applies to the whole file? I created an issue for that here #10706

[leonardoalt]

Can there be a pragma unchecked or something similar that applies to the whole file? I created an issue for that here #10706

I hope not 😄

Seriously now, does anyone have any actual data on the gas differences? Or is it still all hypothetical?

[ekpyron]

I think this entire problem will diminish significantly in the future, as soon as our latest optimizer work is done.
Ideally, almost all checks that are introduced by checked arithmetic by default will be automatically removed if you have a custom require up front that assures that the overflow can't happen or if the check is redundant based on any other linear constraint on its values like in the case of common loop increments. Once that is the case, ideally, your bytecode in the end will contain zero auto-generated overflow checks, since you either covered all cases by your own require statements or have cases where you actually want to allow overflows, in which case making it visible using unchecked is not a bad thing.
You should even be able use static analysis to make sure you didn't miss any such case :-).

In general, the change in default behaviour, i.e. making checked arithmetic the default and moving unchecked arithmetic to a situation like SafeMath was in before (i.e. if one wanted to, one could imagine an UnsafeMath library wrapping arithmetic in unchecked blocks flipping the situation entirely), of course, generally favors people who rely on safe-math checks a lot, resp. are using safe-math almost ubiquitously - unfortunately, from what we've seen this seems to be the majority of cases, so more contracts seem to suffer from the less readable safe-math-style operations than now suffer from the default checks, especially since unchecked blocks are available, so the situation of people who want unchecked maths now is not as bad as the situation of people who wanted checked maths was before. I'm also sceptical about making unchecked a global pragma-based setting and would point to the easier auditability if unchecked is as local a property as possible (especially if you consider multi-file projects with imports with differing arithmetic between files, etc.).

What is definitely unfortunate, is that it may take a while until this kind of optimization will be available to production code (it will only happen after we switch to Sol->Yul code generation by default).

Still, my main question at this point would be: would the ability of the optimizer to remove most or all redundant checks introduced by checked-by-default-arithmetic make a significant difference in your view on the matter?

[mudgen]

@ekpyron Thanks for this good analysis.

Much more often then not it is a good thing that a transaction reverts if there is an overflow or underflow, so in this way it makes sense for it to be the default. I'm warming up to the idea and need to get some experience.

Still, my main question at this point would be: would the ability of the optimizer to remove most or all redundant checks introduced by checked-by-default-arithmetic make a significant difference in your view on the matter?

This is definitely the ideal and improves my view on the matter for sure. I think more experience with default revert on over/underflow will shift my view on the matter one way or the other and make me more certain.