Safe Casting Library from uint256 to uintXX

[bh2smith]

Addresses #1925 and would be a viable alternative to discussions in #1625

Fixes #1925

Downcasting from uint256 in Solidity does not revert by default on overflow. This can easily result in undesired exploitation or bugs, since developers usually assume that overflows raise errors. SafeCast restores this intuition by reverting the transaction when such an operation overflows.

[bh2smith]

Please note that the failing unit test here is independent of these changes.

  1) Contract: ERC721Holder
       "before each" hook: before test for "receives an ERC721 token":
     Error: Invalid JSON RPC response: ""

[fleupold]

This is a cool addition in my opinion. Really good unit tests!

[denisgranha]

nice contribution @bh2smith

[nventuro]

Hello @bh2smith, thanks for opening this PR and contributing to the library!

An issue that may arise from this is that SafeMath only supports 256 bit operations, so we'd be leaving users in the dark in this sense if they chose to downcast. Would you expect SafeMath support for smaller types as well?

[bh2smith]

Hello @nventuro and thanks for your reply!

SafeMath only supports 256 bit operations

This has become apparent to me (the hard way), but the solidity compiler does not complain when "using SafeMath for uint128". I consider this to be somewhat misleading and can lead to serious security vulnerabilities.

Would you expect SafeMath support for smaller types as well?

At first I considered implementing SafeMath for each of the integer types found here in this PR, but soon realized it would constitute a lot of redundant code. This is SafeCast feature can be used as an additional layer of security in prevention of overflows in the arithmetic for other integer types.

I would argue that this new layer only enhances the developer toolkit at the expense of knowing to use it in combination with SafeMath where appropriate. Perhaps it would be a good idea to update the SafeMath contract with a warning that it only works as expected for u256 and other integer types would require using this additional layer.

Observe the following example contract that demonstrates the use case.

pragma solidity ^0.5.0;

import "openzeppelin-solidity/contracts/utils/SafeCase.sol";
import "openzeppelin-solidity/contracts/math/SafeMath.sol";


contract ExtraSafeAdd {
    using SafeMath for uint8;
    using SafeCast for uint;

    function addU128(uint8 a, uint8 b) public pure returns (uint8) {
        return a.add(b).castU8();
    }
}

which will act as expected when a + b does not overflow (returning the expected u8 type) and revert when the sum is more than 2^8.

Thanks again for getting back, please let me know your thoughts.

[nventuro]

This has become apparent to me (the hard way), but the solidity compiler does not complain when "using SafeMath for uint128". I consider this to be somewhat misleading and can lead to serious security vulnerabilities.

I'm guessing the compiler allows implicit upcasts? In that case however the return type of the arithmetic operations will still be uint256 - there's only danger when, like you mentioned, downcasting.

Observe the following example contract that demonstrates the use case, which will act as expected when a + b does not overflow (returning the expected u8 type) and revert when the sum is more than 2^8.

Neat! I like this proposal. Since upcasts to uint256 are allowed, we should write all downcasts from that type. Not sure about the name though, perhaps x.toUint128() is more readable?

[bh2smith]

perhaps x.toUint128() is more readable

Great suggestion, I have included these changes! Please let me know if there is anything else I can do to keep this moving.

[nventuro]

Thanks a lot @bh2smith! I left some minor suggestions regarding naming/documentation.

I'm not sure about where this library should live, is this utils or math? Is this part of SafeMath? The decision you made is probably fine though.

Could you also add an entry to the changelog in this PR? Thanks!

[bh2smith]

Could you also add an entry to the changelog in this PR? Thanks!

I will certainly do this @nventuro. Only after I have taken care of the more involved final suggestion regarding the auto-generated describe blocks. If you have any further insights regarding this that might make things a bit quicker on from my side, I'd be happy to hear. Otherwise I will look into other unit tests on this project in hopes to get an idea of how to proceed.

As for the change log, I assume it's only appropriate to update it once this last change has been made and the PR is approved. Then, my guess is that I would include a point in the "New Features" Section of version 2.4.0 (unreleased) section.

[bh2smith]

I'm not sure about where this library should live, is this utils or math?

I am also unsure and somewhat consider it a utility. However, I would be happy to move it to math if you'd prefer!

[nventuro]

Will have to spend a bit more time on this suggestion as JS is not my mother tongue.

No worries! I went ahead and did a commit directly on your branch with the change. As you can see, it is very simple: you can dynamically call the describe function to create new test cases, and using JavaScript template literals it is very straightforward to create the descriptions for these tests. In this case, all toUintN functions are essentially the same and are tested the same way, so it makes sense to autogenerate the tests this way, preventing human error.

[bh2smith]

Looks great!

[nventuro]

@frangio this is pretty much ready, content-wise. There are a couple things that still need figuring out though:

1. is SafeCast a good name?
2. does this belong in utils as a standalone thing, or could it be added to SafeMath?
3. do we want to include this in 2.4?

Depending on these, we will then want to update our documentation.

[frangio]

Not sure if SafeCast is a good name, but at least it's consistent with SafeMath. 😄

The only reason why I would consider a different name is if we'd want to provide an alternative implementation in the future that returns a boolean or something instead of reverting. I don't see this happening.

I would keep SafeCast for now, I think the consistency/familiarity argument is too strong in this case.

I would also keep this as a separate library. And I would include it in 2.5 since 2.4 is already in beta.

[frangio]

LGTM! I've added the necessary annotation so that this shows up in the docs.

[nventuro]

And I would include it in 2.5 since 2.4 is already in beta.

I've updated the changelog to put this under 2.5

[bh2smith]

Awesome stuff @nventuro and @frangio! Thanks for all your help here. Do we have an estimated shipping date for v2.5? And should I update this branch with master, or is is better to wait on this until closer to the date?

[nventuro]

Do we have an estimated shipping date for v2.5?

The 2.4 final release should be out soon (this week?), and we already have some stuff ready for 2.5, so it shouldn't take long. It mostly depends on whether or not we include any other big things on it, such as ERC1155. Perhaps one month from now?

And should I update this branch with master, or is is better to wait on this until closer to the date?

We can merge right away! The 2.4 release branch has already been created, so this will not cause issues.