Fix/critical bounds bug (#37)

* Push critical bounds check fix * Better conditions for bounds bug
GNSPS · Sep 30, 2020 · 916de66 · 916de66
1 parent 8a52148
commit 916de66
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 11 deletions.
diff --git a/README.md b/README.md
@@ -8,7 +8,19 @@ The library lets you concatenate, slice and type cast bytes arrays both in memor
 
 Given this library has an all-internal collection of methods it doesn't make sense having it reside in the mainnet. Instead it will only be available in EPM as an installable package.
 
-_Version Notes_:
+## Important Fixes Changelog
+
+There was a **critical bug** in the `slice` method, reported on an audit to a DXDao codebase.
+
+Previously, no checks were being made on overflows of the `_start` and `_length` parameters since previous reviews of the codebase deemed this overflow "unexploitable" because of an inordinate expansion of memory (i.e., reading an immensely large memory offset causing huge memory expansion) resulting in an out-of-gas exception.
+
+However, as noted in the review mentioned above, this is not the case. The `slice` method in versions `<=0.9.0` actually allows for arbitrary _kind of_ (i.e., it allows memory writes to very specific values) arbitrary memory writes _in the specific case where these parameters are user-supplied inputs and not hardcoded values (which is uncommon).
+
+This made me realize that in permissioned blockchains where gas is also not a limiting factor this could become problematic in other methods and so I updated all typecasting-related methods to include new bound checks as well.
+
+**TL;DR: if you're using the `slice` method with user-supplied inputs in your codebase please update the bytes library immediately!**
+
+## _Version Notes_:
 
 * Version `v0.9.0` now compiles with Solidity compilers `0.5.x` and `0.6.x`.
 

diff --git a/contracts/BytesLib.sol b/contracts/BytesLib.sol
@@ -234,7 +234,8 @@ library BytesLib {
         pure
         returns (bytes memory)
     {
-        require(_bytes.length >= (_start + _length), "Read out of bounds");
+        require(_start + _length + 31 >= _start, "slice_overflow");
+        require(_bytes.length >= _start + _length, "slice_outOfBounds");
 
         bytes memory tempBytes;
 
@@ -291,7 +292,8 @@ library BytesLib {
     }
 
     function toAddress(bytes memory _bytes, uint256 _start) internal pure returns (address) {
-        require(_bytes.length >= (_start + 20), "Read out of bounds");
+        require(_start + 20 >= _start, "toAddress_overflow");
+        require(_bytes.length >= _start + 20, "toAddress_outOfBounds");
         address tempAddress;
 
         assembly {
@@ -302,7 +304,8 @@ library BytesLib {
     }
 
     function toUint8(bytes memory _bytes, uint256 _start) internal pure returns (uint8) {
-        require(_bytes.length >= (_start + 1), "Read out of bounds");
+        require(_start + 1 >= _start, "toUint8_overflow");
+        require(_bytes.length >= _start + 1 , "toUint8_outOfBounds");
         uint8 tempUint;
 
         assembly {
@@ -313,7 +316,8 @@ library BytesLib {
     }
 
     function toUint16(bytes memory _bytes, uint256 _start) internal pure returns (uint16) {
-        require(_bytes.length >= (_start + 2), "Read out of bounds");
+        require(_start + 2 >= _start, "toUint16_overflow");
+        require(_bytes.length >= _start + 2, "toUint16_outOfBounds");
         uint16 tempUint;
 
         assembly {
@@ -324,7 +328,8 @@ library BytesLib {
     }
 
     function toUint32(bytes memory _bytes, uint256 _start) internal pure returns (uint32) {
-        require(_bytes.length >= (_start + 4), "Read out of bounds");
+        require(_start + 4 >= _start, "toUint32_overflow");
+        require(_bytes.length >= _start + 4, "toUint32_outOfBounds");
         uint32 tempUint;
 
         assembly {
@@ -335,7 +340,8 @@ library BytesLib {
     }
 
     function toUint64(bytes memory _bytes, uint256 _start) internal pure returns (uint64) {
-        require(_bytes.length >= (_start + 8), "Read out of bounds");
+        require(_start + 8 >= _start, "toUint64_overflow");
+        require(_bytes.length >= _start + 8, "toUint64_outOfBounds");
         uint64 tempUint;
 
         assembly {
@@ -346,7 +352,8 @@ library BytesLib {
     }
 
     function toUint96(bytes memory _bytes, uint256 _start) internal pure returns (uint96) {
-        require(_bytes.length >= (_start + 12), "Read out of bounds");
+        require(_start + 12 >= _start, "toUint96_overflow");
+        require(_bytes.length >= _start + 12, "toUint96_outOfBounds");
         uint96 tempUint;
 
         assembly {
@@ -357,7 +364,8 @@ library BytesLib {
     }
 
     function toUint128(bytes memory _bytes, uint256 _start) internal pure returns (uint128) {
-        require(_bytes.length >= (_start + 16), "Read out of bounds");
+        require(_start + 16 >= _start, "toUint128_overflow");
+        require(_bytes.length >= _start + 16, "toUint128_outOfBounds");
         uint128 tempUint;
 
         assembly {
@@ -368,7 +376,8 @@ library BytesLib {
     }
 
     function toUint256(bytes memory _bytes, uint256 _start) internal pure returns (uint256) {
-        require(_bytes.length >= (_start + 32), "Read out of bounds");
+        require(_start + 32 >= _start, "toUint256_overflow");
+        require(_bytes.length >= _start + 32, "toUint256_outOfBounds");
         uint256 tempUint;
 
         assembly {
@@ -379,7 +388,8 @@ library BytesLib {
     }
 
     function toBytes32(bytes memory _bytes, uint256 _start) internal pure returns (bytes32) {
-        require(_bytes.length >= (_start + 32), "Read out of bounds");
+        require(_start + 32 >= _start, "toBytes32_overflow");
+        require(_bytes.length >= _start + 32, "toBytes32_outOfBounds");
         bytes32 tempBytes32;
 
         assembly {