Consider type checking casting in 0.8

[alcueca]

Abstract

Solidity 0.8 is introducing type checking for arithmetic operations, but not for type casting. From my own experience most downcasting is wrapped in SafeMath-like functions in audited contracts. There are some notable exceptions (namely Uniswap v2 TWAP implementation and ABDKMath64x64) which could be managed through an unckecked block.

Motivation

Checking type casting should be done as similarly as possible as checking overflow on arithmetic operations.

Specification

No new keywords would be required. Reuse unchecked.

Backwards Compatibility

This should be introduced for 0.8, as a breaking change, bundled with the arithmetic overflow checking.

[chriseth]

We will re-evaluate this, but it is probably too late to be included since it is a rather complex topic. The conversion rules will be simplified with 0.8, which could make this easier to add.

[frangio]

Adding reverts on casting overflow would be a breaking change. I think this is an important safety check, and it should be done in the next breaking release, but I don't believe that should happen any time soon. In the meantime, we could consider adding a warning for downcasts that are outside unchecked blocks.

My only concern would be that this would produce warnings for anyone using our SafeCast library. But the code is pretty simple and I wonder if the compiler could detect that it's a safe downcast and thus avoid the warning in this case:

    function toUint16(uint256 value) internal pure returns (uint16) {
        require(value < 2**16, "SafeCast: value doesn\'t fit in 16 bits");
        return uint16(value);
    }

[vicnaum]

Is there a Warning already? I think many people are tricked with "0.8.0 is safe with overflows" and are assuming that conversions also should revert if the number overflows the resulting type. That's very dangerous and I saw no big warning on this topic anywhere. Wonder how many contracts are vulnerable because of this type of assumption?

[k06a]

I support for this proposal

[ekpyron]

Related: https://github.com/ethereum/solidity/issues/11284

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

[frangio]

This issue is important. Casts are very unsafe currently.

[ekpyron]

Yes, it's important - but I'd still consider it as part of https://github.com/ethereum/solidity/issues/11284, so I'd just have let the stale bot run its course - but now I'll explicitly close this as duplicate, resp. as part of https://github.com/ethereum/solidity/issues/11284.

Rethinking conversions, potentially for example removing all implicit conversions while providing better means for more concise and readable explicit conversions, is definitely on our agenda - further down the road the respective conversion functions will actually be defined in the standard library and we can easily have multiple versions, some that for example explicitly truncate, some that verify that no truncations happens, etc.

I actually don't think we'll reuse the unchecked mechanism for any of this. I'd rather actually consider removing it entirely and moving the distinction between checked and unchecked arithmetic as well into the type system (i.e. have types with checked and types with unchecked arithmetic), but that's not fully decided yet.