Repository: summa-tx/bitcoin-spv (language: rust)

Issue:
Library contains multiple integer under-/overflows
#### Description

The bitcoin-spv library allows for multiple integer under-/overflows while processing or converting potentially untrusted or user-provided data.

#### Examples

* `uint8` underflow `uint256(uint8(_e - 3))`
**Note**: `_header[75]` will throw consuming all gas if out of bounds while the majority of the library usually uses `slice(start, 1)` to handle this more gracefully.
 
https://github.com/summa-tx/bitcoin-spv/blob/f5e4da091a1c97e6432c2d70eba434edb189f919/solidity/contracts/BTCUtils.sol#L483-L494

* `uint8` overflow `uint256(uint8(_len + 8 + 1))`
**Note**: might allow a specially crafted output to return an invalid determineOutputLength <= 9.
**Note**: while type `VarInt` is implemented for inputs, it is not for the output length.

https://github.com/summa-tx/bitcoin-spv/blob/f5e4da091a1c97e6432c2d70eba434edb189f919/solidity/contracts/BTCUtils.sol#L295-L304

* `uint8` underflow `uint256(uint8(extractOutputScriptLen(_output)[0]) - 2)`

https://github.com/summa-tx/bitcoin-spv/blob/f5e4da091a1c97e6432c2d70eba434edb189f919/solidity/contracts/BTCUtils.sol#L366-L378

* `BytesLib` input validation multiple start+length overflow
**Note**: multiple occurrences. should check `start+length > start && bytes.length >= start+length`

https://github.com/summa-tx/bitcoin-spv/blob/f5e4da091a1c97e6432c2d70eba434edb189f919/solidity/contracts/BytesLib.sol#L246-L248

* `BytesLib` input validation multiple start overflow

https://github.com/summa-tx/bitcoin-spv/blob/f5e4da091a1c97e6432c2d70eba434edb189f919/solidity/contracts/BytesLib.sol#L280-L281

https://github.com/summa-tx/bitcoin-spv/blob/f5e4da091a1c97e6432c2d70eba434edb189f919/solidity/contracts/BytesLib.sol#L269-L270

https://github.com/summa-tx/bitcoin-spv/blob/f5e4da091a1c97e6432c2d70eba434edb189f919/solidity/contracts/BytesLib.sol#L246-L248

https://github.com/summa-tx/bitcoin-spv/blob/f5e4da091a1c97e6432c2d70eba434edb189f919/solidity/contracts/BytesLib.sol#L410-L412

#### Recommendation

We believe that a general-purpose parsing and verification library for bitcoin payments should be very strict when processing untrusted user input. With strict we mean, that it should rigorously validate provided input data and only proceed with the processing of the data if it is within a safe-to-use range for the method to return valid results. Relying on the caller to provide pre-validate data can be unsafe especially if the caller assumes that proper input validation is performed by the library.

Given the risk profile for this library, we recommend a conservative approach that balances for security instead of gas efficiency without relying on certain calls or instructions to throw on invalid input.

For this issue specifically, we recommend proper input validation and explicit type expansion where necessary to prevent values from wrapping or processing data for arguments that are not within a safe-to-use range.


Discussion / hints:

Fix patch:
diff --git a/solidity/contracts/BytesLib.sol b/solidity/contracts/BytesLib.sol
index d221e27b..bf409800 100644
--- a/solidity/contracts/BytesLib.sol
+++ b/solidity/contracts/BytesLib.sol
@@ -244,7 +244,8 @@ library BytesLib {
     }
 
     function slice(bytes memory _bytes, uint _start, uint _length) internal  pure returns (bytes memory res) {
-        require(_bytes.length >= (_start + _length), "Slice out of bounds");
+        uint _end = _start + _length;
+        require(_end > _start && _bytes.length >= _end, "Slice out of bounds");
 
         assembly {
             // Alloc bytes array with additional 32 bytes afterspace and assign it's size
@@ -267,7 +268,8 @@ library BytesLib {
     }
 
     function toAddress(bytes memory _bytes, uint _start) internal  pure returns (address) {
-        require(_bytes.length >= (_start + 20), "Address conversion out of bounds.");
+        uint _totalLen = _start + 20;
+        require(_totalLen > _start && _bytes.length >= _totalLen, "Address conversion out of bounds.");
         address tempAddress;
 
         assembly {
@@ -278,7 +280,8 @@ library BytesLib {
     }
 
     function toUint(bytes memory _bytes, uint _start) internal  pure returns (uint256) {
-        require(_bytes.length >= (_start + 32), "Uint conversion out of bounds.");
+        uint _totalLen = _start + 32;
+        require(_totalLen > _start && _bytes.length >= _totalLen, "Uint conversion out of bounds.");
         uint256 tempUint;
 
         assembly {
@@ -408,7 +411,8 @@ library BytesLib {
     }
 
     function keccak256Slice(bytes memory _bytes, uint _start, uint _length) pure internal returns (bytes32 result) {
-        require(_bytes.length >= (_start + _length), "Slice out of bounds");
+        uint _end = _start + _length;
+        require(_end > _start && _bytes.length >= _end, "Slice out of bounds");
 
         assembly {
             result := keccak256(add(add(_bytes, 32), _start), _length)


Test patch: