Run some automated analysis tools against our codebase

[lorenzb]

Esp. mythril and slither might be good candidates.

Also consider making tools part of CI.

[shayanb]

Mythril on 45a86e5:

🍺  myth -x LibSubmarine.sol

Output:

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: _function_0x16637cfa
PC address: 427
A possible integer overflow exists in the function `_function_0x16637cfa`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: proveth/ProvethVerifier.sol:85

function merklePatriciaCompactDecode(bytes compact) returns (bytes memory nibbles) {
        require(compact.length > 0);
        uint first_nibble = uint8(compact[0]) >> 4 & 0xF;
        uint skipNibbles;
        if (first_nibble == 0) {
            skipNibbles = 2;
        } else if (first_nibble == 1) {
            skipNibbles = 1;
        } else if (first_nibble == 2) {
            skipNibbles = 2;
        } else if (first_nibble == 3) {
            skipNibbles = 1;
        } else {
            // Not supposed to happen!
            require(false);
        }
        return decodeNibbles(compact, skipNibbles);
    }

--------------------

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: reveal(uint32,uint256,address,uint256,bytes,bytes32,uint256,uint256)
PC address: 1169
A possible integer overflow exists in the function `reveal(uint32,uint256,address,uint256,bytes,bytes32,uint256,uint256)`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: LibSubmarine.sol:130

function reveal(
        uint32 _commitBlock,
        uint256 _commitIndex,
        address _dappAddress,
        uint256 _commitValue,
        bytes _data,
        bytes32 _witness,
        uint256 _gasPrice,
        uint256 _gasLimit
    ) public payable {
        bytes32 sessionId = keccak256(abi.encodePacked(
            msg.sender,
            address(this),
            _commitValue,
            _data, _witness,
            _gasPrice,
            _gasLimit
        )); //implicitly checks msg.sender is A to generate valid sessionId
        require(msg.value >= revealDepositAmount, "Reveal deposit not provided");
        require(sessions[sessionId].revealBlock == 0, "The tx is already revealed");
        require(!sessions[sessionId].unlocked, "The tx should not be already unlocked");
        if (blockhash(_commitBlock) != 0x0) {
            blockNumberToHash[_commitBlock] = blockhash(_commitBlock);
        } // TODO we need to throw or do something to tell people when we can't find the block hash (too old)
        sessions[sessionId].commitValue = _commitValue;
        sessions[sessionId].commitIndex = _commitIndex;
        sessions[sessionId].commitBlock = _commitBlock;
        sessions[sessionId].revealBlock = uint32(block.number);
        sessions[sessionId].data = _data;
        sessions[sessionId].dappAddress = _dappAddress;
        emit Revealed(sessionId, _commitValue, _data, _witness, _commitBlock, _commitIndex);
    }

--------------------

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: _function_0x742e4a3e
PC address: 1363
A possible integer overflow exists in the function `_function_0x742e4a3e`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: proveth/ProvethVerifier.sol:108

function isPrefix(bytes prefix, bytes full) returns (bool) {
        if (prefix.length > full.length) {
            return false;
        }

        for (uint i = 0; i < prefix.length; i += 1) {
            if (prefix[i] != full[i]) {
                return false;
            }
        }

        return true;
    }

--------------------

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: _function_0x76ad74ae
PC address: 1562
A possible integer overflow exists in the function `_function_0x76ad74ae`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: proveth/ProvethVerifier.sol:63

function decodeNibbles(bytes compact, uint skipNibbles) returns (bytes memory nibbles) {
        require(compact.length > 0);

        uint length = compact.length * 2;
        require(skipNibbles <= length);
        length -= skipNibbles;

        nibbles = new bytes(length);
        uint nibblesLength = 0;

        for (uint i = skipNibbles; i < skipNibbles + length; i += 1) {
            if (i % 2 == 0) {
                nibbles[nibblesLength] = bytes1((uint8(compact[i/2]) >> 4) & 0xF);
            } else {
                nibbles[nibblesLength] = bytes1((uint8(compact[i/2]) >> 0) & 0xF);
            }
            nibblesLength += 1;
        }

        assert(nibblesLength == nibbles.length);
    }

--------------------

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: _function_0x79483a5d
PC address: 1798
A possible integer overflow exists in the function `_function_0x79483a5d`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: proveth/ProvethVerifier.sol:104

function exposeMerklePatriciaCompactDecode(bytes compact) returns (bytes nibbles) {
        return merklePatriciaCompactDecode(compact);
    }

--------------------

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: _function_0x9cd8a1df
PC address: 2683
A possible integer overflow exists in the function `_function_0x9cd8a1df`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: proveth/ProvethVerifier.sol:122

function sharedPrefixLength(uint xsOffset, bytes xs, bytes ys) returns (uint) {
        for (uint i = 0; i + xsOffset < xs.length && i < ys.length; i++) {
            if (xs[i + xsOffset] != ys[i]) {
                return i;
            }
        }
        return i;
    }

--------------------

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: _function_0xb6ac4d42
PC address: 3067
A possible integer overflow exists in the function `_function_0xb6ac4d42`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: proveth/ProvethVerifier.sol:24

function decodeAndHashUnsignedTx(bytes rlpUnsignedTx) public view returns (
        bool valid,
        bytes32 sigHash,
        uint256 nonce,
        uint256 gasprice,
        uint256 startgas,
        bytes to,
        uint256 value,
        bytes data
    ) {
        sigHash = keccak256(rlpUnsignedTx);
        valid = true;
        RLP.RLPItem[] memory fields = rlpUnsignedTx.toRLPItem().toList();
        require(fields.length == 6);
        nonce = fields[0].toUint();
        gasprice = fields[1].toUint();
        startgas = fields[2].toUint();
        to = fields[3].toData();
        value = fields[4].toUint();
        data = fields[5].toData();
    }

--------------------

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: _function_0x16637cfa
PC address: 6872
A possible integer overflow exists in the function `_function_0x16637cfa`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: proveth/ProvethVerifier.sol:66

compact.length * 2

--------------------

==== Exception state ====
Type: Informational
Contract: LibSubmarine
Function name: _function_0x16637cfa
PC address: 7513
A reachable exception (opcode 0xfe) has been detected. This can be caused by type errors, division by zero, out-of-bounds array access, or assert violations. This is acceptable in most situations. Note however that `assert()` should only be used to check invariants. Use `require()` for regular input checking.
--------------------
In file: proveth/ProvethVerifier.sol:82

assert(nibblesLength == nibbles.length)

--------------------

==== Exception state ====
Type: Informational
Contract: LibSubmarine
Function name: isFinalizable(bytes32)
PC address: 14241
A reachable exception (opcode 0xfe) has been detected. This can be caused by type errors, division by zero, out-of-bounds array access, or assert violations. This is acceptable in most situations. Note however that `assert()` should only be used to check invariants. Use `require()` for regular input checking.
--------------------
In file: SafeMath32.sol:24

assert(c >= a)

--------------------

==== Integer Overflow ====
Type: Warning
Contract: LibSubmarine
Function name: reveal(uint32,uint256,address,uint256,bytes,bytes32,uint256,uint256)
PC address: 21021
A possible integer overflow exists in the function `reveal(uint32,uint256,address,uint256,bytes,bytes32,uint256,uint256)`.
The addition or multiplication may result in a value higher than the maximum representable integer.
--------------------
In file: LibSubmarine.sol:7
--------------------

[relyt29]

@shayanb can you dig at each of those issues raised, figure out if they're really concerns or false positives, and then issue a PR when you've fixed all of them?

@lorenzb perhaps add static anlysis tools to TravisCI?

[lorenzb]

Adding them to travis would be pretty sweet once we figure out how to reduce the number of false positives.

[shayanb]

I'm going through the security software to see which one fits our case. Mythril seems to be the one but there are a lot of false positives which could be fixed with some additional (redundant) checks in our code. I'll dig in those issues and will update here.

Comparison of the tools: https://consensys.net/diligence/evm-analyzer-benchmark-suite/