Openzeppelin-contracts: Unexpected results using SafeMath for uint128

[bh2smith]

Summary

Trying to use SafeMath for uint128 should throw/ revert in situations when overflows happen! Same holds for overflows of any integer type that are not uint256

📝 Example

_Contract:_

pragma solidity ^0.5.0;

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

contract BrokenSafeMath {
    using SafeMath for uint128;

    function unsafeAdd(uint128 a, uint128 b) public pure returns (uint128) {
        return uint128(a.add(b));
    }
}

_unit test:_

let res = await contract.unsafeAdd.call("340282366920938463463374607431768211455", 2)

yields a return value of x = 1 (unexpected/should have reverted).

The problem seems to be with the upcasting of everything to uint256 as seen in the following snippet.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/54182bf31c78fb1b16dfdce40fc9a6a177cb3bca/contracts/math/SafeMath.sol#L26-L31

As far as I understand, this implies that SafeMath is only really "safe" for uint256 and none of the other integer types.

💻 Environment
Not really applicable, but this appears to be the case for all versions. Particularly, I ran my examples with

    "openzeppelin-solidity": "^2.3.0",

[Skyge]

Yeah, you are right, SafeMath is only for uint256 currently. So if you want to use it for uint128, maybe you should make corresponding changes: uint256 => uint128.

[abcoathup]

Hi @bh2smith,

As per @Skyge, SafeMath only supports uint256.

See the following issue for a discussion on supporting other types:
https://github.com/OpenZeppelin/openzeppelin-contracts/issues/1625

[bh2smith]

Thanks for the suggestions all, these were very helpful. Will probably start looking for or working on a SafeCast library that might look as follows.

pragma solidity ^0.5.0;

library SafeCast {
    function castU128(uint a) internal pure returns (uint128) {
        require(a < 2**128, "SafeCast: downcast overflow");
        return uint128(a);
    }
}

Would this be something that should/could be included here in the openzeppelin repo?

[frangio]

function unsafeAdd(uint128 a, uint128 b) public pure returns (uint128) {
    return uint128(a.add(b));
}

To clarify, the dangerous operation here is not the SafeMath operation but the downcast to uint128. I think the casting library proposed by @bh2smith is the right way to approach this kind of thing.

Thank you for reporting and proposing a mitigation!