Create a gist now

Instantly share code, notes, and snippets.

What would you like to do?

Section 1 - Table of Contents

Section 2 - Introduction

From December 2nd through December 9th of 2016, Piper Merriam conducted an audit of the primary EtherRoll smart contract. The findings of the audit are presented in this document.

I, Piper Merriam have no stake in the success or failure of EtherRoll. This audit was performed under a contracted hourly rate with no other compensation.

2.1 Authenticity

This document should have an attached cryptographic signature to ensure it has not been tampered with. The signature can be verified using the public key from Piper Merriam's the keybase.io.

Piper Merriam on Keybase.io

2.2 Audit Goals and Focus

2.2.1 Sound Architecture

This audit includes assessments of the overall architecture and design choices. Given the subjective nature of these assessments it will be up to the EtherRoll development team to determine whether any changes should be made.

2.2.2 Smart Contract Best Practices

This audit will evaluate whether the codebase follows the current established best practices for smart contract development.

2.2.3 Code Correctness

This audit will evaluate whether the code does what it is intended to do.

2.2.4 Code Quality

This audit will evaluate whether the code has been written in a way that ensures readability and maintainability.

2.2.5 Security

This audit will look for any exploitable security vulnerabilities, or other potential threats to either the operators of EtherRoll or its users.

2.3 About EtherRoll

EtherRoll is a gambling service on the Ethereum blockchain.

2.4 Terminology

This audit uses the following terminology.

2.4.1 Coverage

Measurement of the testing code coverage. This measurement was done via inspection of the code.

2.4.1.1 untested

No tests.

2.4.1.2 low

The tests do not cover some set of non-trivial functionality.

2.4.1.3 good

The tests cover all major functionality.

2.4.1.4 excellent

The tests cover all code paths.

2.4.2 Severity

Measurement of magnitude of an issue.

2.4.2.1 minor

Minor issues are generally subjective in nature, or potentially deal with topics like "best practices" or "readability". Minor issues in general will not indicate an actual problem or bug in code.

The maintainers should use their own judgement as to whether addressing these issues improves the codebase.

2.4.2.2 medium

Medium issues are generally objective in nature but do not represent actual bugs or security problems.

These issues should be addressed unless there is a clear reason not to.

2.4.2.3 major

Major issues will be things like bugs or security vulnerabilities. These issues may not be directly exploitable, or may require a certain condition to arise in order to be exploited.

Left unaddressed these issues are highly likely to cause problems with the operation of the contract or lead to a situation which allows the system to be exploited in some way.

2.4.2.4 critical

Critical issues are directly exploitable bugs or security vulnerabilities.

Left unaddressed these issues are highly likely or guaranteed to cause major problems or potentially a full failure in the operations of the contract.

Section 3 - Overview

3.1 Source Code

The EtherRoll source code was made available through a private repository on github. The code being evaluated can be found under the commit hash 3f39d2127130760dacf2820ac18e93d42ef7b432

This audit evaluated a single Solidity source file, ./contracts/Etheroll.sol.

The sha256 of that contract source is as follows.

$ shasum -a 256 contracts/Etheroll.sol
bcf78ede1db5597eb4697f2ab26fb06e7bbb3b72393d6686bf4f6e86976e2b23  contracts/Etheroll.sol<Paste>

3.2 Contracts

3.2.1 EtheRoll Contracts

The EtheRoll service is comprised of a single smart contract named EtheRoll.

3.2.2 Oraclize Helper Contracts

This contract uses the Oraclize service to retrieve externally generated random numbers from the Random.org API. The Etheroll contract uses the abstract base contracts and interface contracts provided by Oraclize in their github repository linked below.

https://github.com/oraclize/ethereum-api

Given that these contracts are written by the Oraclize service they were not evaluated beyond the following checks.

  • The addresses for the Oraclize resolver addresses found in the source code were verified to match those from the Oraclize github repository.
  • The overall code for the OraclizeI, OraclizeAddrResolverI, and usingOraclize contracts were visually inspected to match the code found in the Oraclize github repository.

Section 4 - Main Findings

4.1 Minor Issues

4.1.1 No brackets used for single line if statements

  • Severity: minor

The modifiers found in the Etheroll contract do not use brackets to wrap the if statement code blocks.

// Example from Etheroll.sol:Line 303

modifier gameIsActive {
    if(gamePaused == true)
    throw;
    _;
}

This style can more easily lead to introducing defects during refactoring. A common version of this mistake is as follows.

// Initial Code
modifier restricted {
    if (someCondition)
    return;
    _;
}

Later it is decided that this should also log an event so we add a logging statement.

// Now with logging statement
modifier restricted {
    if (someCondition)
    AuthorizationFailed();
    return;
    _;
}

A developer who quickly adds the AuthorizationFailed() line might neglect to realize that this modifier will now always return early without executing the function body.

Recommend changing these statements to one of the following patterns.

// Pattern A
modifier restricted {
    if (someCondition) return;
    _;
}

// Pattern B
modifier restricted {
    if (someCondition) {
        return;
    }
    _;
}

Pattern A has the benefit of remaining concise while making the mistake mentioned above much less likely to occur.

4.1.2 Lack of use of the constant variable type.

The following variables in the contract are used as constants but are not declared as constants in the source code.

  • maxBetDivisor
  • minNumber
  • maxNumber
  • houseEdgeDivisor

Use of the constant keyword will eliminate certain classes of defects and improve readability.

4.2 Medium Issues

4.2.1 Lack of overflow and underflow checking.

The contract does not implement any checking for overflow or underflow conditions. Given the unsafe nature of solidity arithmetic and the catastrophic nature of these types of defects it may be wise to implement and use a set of safe math operations. The following base contract can be used to accomplish this.

// Sourced from the Nexus DAppSys repository.
contract DSSafeAddSub {
    function safeToAdd(uint a, uint b) internal returns (bool) {
        return (a + b >= a);
    }
    function safeAdd(uint a, uint b) internal returns (uint) {
        if (!safeToAdd(a, b)) throw;
        return a + b;
    }

    function safeToSubtract(uint a, uint b) internal returns (bool) {
        return (b <= a);
    }
    function safeSub(uint a, uint b) internal returns (uint) {
        if (!safeToSubtract(a, b)) throw;
        return a - b;
    } 
}

4.3 Major Issues

4.3.1 Source of Entropy

While there is no evidence of dishonesty, the current source of entropy is exploitable by the operators of the contract. It may be worth exploring a modified source of entropy or adding a layer to the current source of entropy that provides a guarantee that the resulting random integer is within the proper range.

In the current form none of the following can be verified.

  1. The returned integers are constrained between the lower and upper bounds.
  2. The returned integers cover the full range between the lower and upper bound.
  3. The returned integers are evenly distributed between the lower and upper bounds.

4.3.2 House is not guaranteed to be able to pay winnings.

The current formulas used to set the maximum bet amount do not guarantee that the house is able to pay in the event that the player wins. The current formula uses a percentage of the funds available to the house to set the maximum bet size.

Without taking into account the following information it cannot be guaranteed that the house will be able to pay a player their winning.

  • The number that a player is betting.
  • The number of unresolved bets and their potential winnings.

This may not be an issue as long as the available funds for the house is sufficiently large and the maximum but is sufficiently small, but in the current state, these properties cannot be guaranteed.

4.4 Critical Issues

4.4.1 Underflow conditions when computing player payout.

The underflow conditions when computing the player payout have catastrophic consequences but can be fully mitigated by making maxNumber a constant variable.

4.4.2 Underflow conditions for contractBalance

The underflow conditions present for tracking the contractBalance amount have very unfortunate consequences that would likely render the long term use of the contract untenable. Simple checking to ensure that this operation cannot underflow will fully mitigate this risk.

4.5 Test Coverage Analysis

Coverage: low

The tests for the contract are located in ./tests/etheroll.js. The file contains a total of 14 test cases, 4 of which are commented out.

Attempting to run the test suite resulted in the following error. This is likely an error in the local development environment and unrelated to the actual test suite or contract code.

$ truffle test
Compiling Etheroll.sol...
Compiling Migrations.sol...
Error: Invalid JSON RPC response: "Error: connect ECONNREFUSED 127.0.0.1:8545\n    at Object.exports._errnoException (util.js:1026:11)\n    at exports._exceptionWithHostPort (util.js:1049:20)\n    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1090:14)"
    at Object.InvalidResponse (/REDACED/.yarn-config/global/node_modules/web3/lib/web3/errors.js:35:16)
    at exports.XMLHttpRequest.request.onreadystatechange (/REDACED/.yarn-config/global/node_modules/web3/lib/web3/httpprovider.js:111:32)
    at exports.XMLHttpRequest.dispatchEvent (/REDACED/.yarn-config/global/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:591:25)
    at setState (/REDACED/.yarn-config/global/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:610:14)
    at exports.XMLHttpRequest.handleError (/REDACED/.yarn-config/global/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:532:5)
    at ClientRequest.errorHandler (/REDACED/.yarn-config/global/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:459:14)
    at emitOne (events.js:96:13)
    at ClientRequest.emit (events.js:188:7)
    at Socket.socketErrorListener (_http_client.js:309:9)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at emitErrorNT (net.js:1281:8)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickDomainCallback (internal/process/next_tick.js:122:9)

The test coverage is restricted to checking that various contract variables are set to the expected values. There does not appear to be any coverage of core contract functionality such as placing bets or resolving them.

4.6 General Thoughts

4.6.1 Secondary Audit

Given that several issues were found which could seriously affect the operation of this contract it is recommended that this contract be subjected to a secondary audit once the Etheroll team has responded to the issues found in this audit.

4.6.2 Improve Test Coverage

Given the financial nature of this application, the core application logic should be subjected to some for of tests. This could take the form of an automated test suite, or a set of live tests on the public test network. If the Etheroll team opts for the later, they should ensure they provide sufficient documentation to their tests to ensure that others can verify the results.

Section 5 - Detailed Findings

The following is an exhaustive list of all issues found during in this audit.

5.1 Minor Issues

5.1.1 betIsValid modifier does not use brackets

The modifier betIsValid does not use brackets for the if statement code blocks. This code style can lead to errors more easily than if the code blocks were wrapped in brackets.

Recommend changing the modifier to one of the recommended patterns from Section 4.1.1

5.1.2 gameIsActive modifier does not use brackets

The modifier gameIsActive does not use brackets for the if statement code blocks. This code style can lead to errors more easily than if the code blocks were wrapped in brackets.

Recommend changing the modifier to one of the recommended patterns from Section 4.1.1

5.1.3 payoutsAreActive modifier does not use brackets

The modifier payoutsAreActive does not use brackets for the if statement code blocks. This code style can lead to errors more easily than if the code blocks were wrapped in brackets.

Recommend changing the modifier to one of the recommended patterns from Section 4.1.1

5.1.4 Bet amount check in betIsValid modifier is not inclusive of either endpoint.

The range checking in the betIsValid is not inclusive of either endpoint. While this makes sense for the minBet since a bet of 0 is likely not wanted, it does not feel intuitive that the actual maximum bet is maxBet - 1.

Recommend ensuring this behavior is desired.

5.1.5 Use of variable contractBalance to track balance could result in permanently unreachable funds.

The contract makes use of the variable contractBalance to represent the portion of the contract's total balance that belongs to the bank.

While it appears that this value is accurately tracked across the contract operations this can be considered an anti-pattern in that any deviation from the actual amount will result in one of two problems of differing severity.

Recommend changing the modifier to one of the recommended patterns from Section 4.1.1

5.1.6 maxBetDivisor is not set as constant

The value maxBetDivisor is not defined using the constant keyword. Given that this value is used as a constant and that the contract does not expose any interface to modify this value it seems appropriate that it should be a constant.

Recommend modifying this variable to use the constant keyword.

5.1.7 Contract fallback function is protected

The contract implements a fallback function to allow adding funds to the total amount available to the house. This function is restricted such that it can only be called by the address set as the treasury.

While this restriction will prevent normal sending of funds it will not prevent funds sent to the contract via selfdestruct(etherollAddress).

See this section of the solidity documentation

Neither contracts nor "external accounts" are currently able to prevent that someone sends them Ether. Contracts can react on and reject a regular transfer, but there are ways to move Ether without creating a message call. One way is to simply "mine to" the contract address and the second way is using selfdestruct(x).

No recommendations to mitigate this as it cannot be prevented. There does not appear to be any exploitable attack vector associated with this. It may be appropriate to simply remove the restriction since there are other avenues to dumping funds into the contract (such as just placing many low probability bets).

5.2 Medium Issues

5.2.1 maxBet setter code is duplicated in 6 locations.

The following line of code is duplicated in the following 6 locations.

maxBet = (contractBalance*maxBetAsPercentOfHouse)/maxBetDivisor;
  • Line 414
  • Line 526
  • Line 560
  • Line 608
  • Line 630
  • Line 661

This type of code duplication is prone to introducing defects since any change to the logic requires remembering to change it in all 6 locations.

Recommend abstracting this logic into a reusable internal function and changing all 6 locations to delegate to this function.

5.2.2 The LogResult event does not have any indexed arguments.

The event LogResult does not use the indexed keyword on any of the arguments. While it is still possible to filter on these values, it is significantly less efficient since non-indexed arguments are included in the topic bloom filter.

Recommend adding the indexed keyword to BetID and PlayerAddress. Consider adding it to PlayerNumber as well.

5.2.3 The LogReward event does not have any indexed arguments.

The event LogReward does not use the indexed keyword on any of the arguments. While it is still possible to filter on these values, it is significantly less efficient since non-indexed arguments are included in the topic bloom filter.

Recommend adding the indexed keyword to PlayerAddress.

5.2.4 The contract constructor has 7 comments marked as todo.

The contract constructor has 7 lines of code and corresponding todo comments marking them to be removed prior to production.

Recommend modifying the testing suite to not require these statements to be present. This pattern is prone to errors since once the statements have been removed the test suite is not likely to continue to pass and thus becomes less useful for ongoing development.

5.2.5 Division by zero conditions in playerRollDice function.

Under certain conditions the playerRollDice function will result in an error being thrown due to division by zero.

If minNumber is set such that it allows for a rollUnder value of 1 then following computation found on line 451 will result in a division by zero error.

reward = (((msg.value * (100-(rollUnder-1))) / (rollUnder-1))+msg.value)*houseEdge/houseEdgeDivisor;  

Recommend modifying minNumber such that is is a constant variable and fixing it permanently to the desired lower bound.

5.2.6 Readability of contract state variables would be improved with use of structs.

The contract uses a number of mappings to store the various pieces of state associated with a player's bet. This contract state could be modified to improve readability through the use of a custom struct.

The contract currently uses the following mappings.

mapping (bytes32 => address) playerAddress;
mapping (bytes32 => address) playerTempAddress;
mapping (bytes32 => int) playerBetId;
mapping (bytes32 => uint) playerBetValue;
mapping (bytes32 => uint) playerTempBetValue;  
mapping (bytes32 => uint) playerDieResult;
mapping (bytes32 => uint) playerNumber;
mapping (address => uint) playerPendingWithdrawals;      
mapping (bytes32 => uint) playerReward;
mapping (bytes32 => uint) playerTempReward;    

Recommend modifying to use a custom struct similar to the following.

struct Bet {
    address playerAddress;
    address tempAddress;
    int betId;
    uint betValue;
    uint tempBetValue;
    uint dieResult;
    uint playerNumber;
    uint playerReward;
    uint tempReward;
}

mapping (bytes32 => PlayerBet) playerBets;

Currently the code requires looking up the bet values from each mapping which results in code like the following extract taken from lines 470-479

/* player address mapped to query id does not exist */
if (playerAddress[myid]==0x0) throw;

/* map the result from oraclize to query id */
playerDieResult[myid] = parseInt(result);

/* get the playerAddress for this query id */
playerTempAddress[myid] = playerAddress[myid];
/* delete playerAddress for this query id */
delete playerAddress[myid];             

This code would be transformed to the following if the struct pattern was used.

var bet = playerBets[myid];

/* player address mapped to query id does not exist */
if (bet.betId==0x0) throw;

/* map the result from oraclize to query id */
bet.dieResult = parseInt(result);

/* get the playerAddress for this query id */
bet.tempAddress = bet.playerAddress;
/* delete playerAddress for this query id */
delete bet.playerAddress;             

This is likely to greatly reduce the likelihood of defects due to increased readability.

5.2.7 playerWithdrawPendingTransactions not compatable with non-private key accounts.

In the event that a player wins the random roll, the contract attempts to send their winnings using .send(). This built-in function of the address type provides 2300 gas. In the event that this call fails, the amount owed to the player is recorded in playerPendingWithdrawals.

The function playerWithdrawPendingTransactions allows a player to trigger the withdrawl of any funds that are allocated to them. The internals of the function use the .send() function to send the funds.

The design of this function makes it impossible for any contract whose fallback function requires greater than 2300 gas to retrieve their winnings since the call to .send will always fail.

Recommend modifying the playerWithdrawPendingTransactions to instead send the funds using .call.value(amount)(). This will send the funds with all available gas, allowing contracts with fallback functions which require more than 2300 gas to still withdraw their funds.

5.2.8 maxBetDivisor is likely too small.

The contract uses a fixed number maxBetDivisor to compute the maxBet value using the following formula.

maxBet = (contractBalance*maxBetAsPercentOfHouse)/maxBetDivisor;

If we assume that maxBetAsPercentOfHouse is constrained to be greater than 0 and less than or equal to maxBetDivisor then the maxBetDivisor defines the possible range of values that maxBet can be set to as a proportion of the total funds available to the house.

For example, a maxBetDivisor value of 4 would allow for the maxBet value to be set to any of 25%, 50%, 75%, or 100% of the total house. This also means that the lowest value that maxBet can be set to is 1 / maxBetDivisor.

In the current form, the lowest possible value for maxBet is 1/1000th of the total house funds. This level of granularity is likely too coarse and doesn't allow for as fine grained control over this value as may be desired.

Recommend changing this value to a significantly larger number to allow fine grained control over this computation.

5.2.9 maxBet is not enforced to be greater than or equal to minBet

The contract uses the two storage values minBet and maxBet to constrain the amount that a player may bet between a lower and upper bound.

The places in the code that set the maxBet and minBet values do not do any bounds checking. This behavior is likely fine for the ownerSetMinBet function since it is a manual action triggered by the contract owner. This could be problematic in the places where maxBet is set. Since maxBet is based on the contractBalance value and that value is likely to vary widely it seems reasonable that conditions could occur where the maxBet gets set below or equal to the minBet value at which point the contract would reject all new bets.

It is not clear what the appropriate strategy to mitigate this should be. Consider one of the following options.

Recommend setting gamePaused to true when this condition occurs and require manual intervention.

5.3 Major Issues

5.3.1 The source of entropy is not guaranteed to be fair

The contract uses the Oraclize service to query the random.org service as a source of entropy. In order to use random.org, one must submit an API key with the query. In order to not expose this API key, the contract uses the encrypted query feature of oraclize to mask the parameters that will be sent to the API.

This encryption also prevents auditing the nature of the query that is sent to random.org making it impossible to verify that the entropy source is being queried in a fair manner.

Below are two ways that this could be abused. There are likely others.

  1. The distribution that the number is being picked from could not be an even distribution which would allow the contract to have an unfair advantage.
  2. The range that the number is being picked from is not known. Combined with the minNumber and maxNumber constraints it is not possible for a player to verify the odds of their bet.

Recommend modifying the algorithm as follows to remove the possibility of bad behavior by the operators of the contract.

  1. Let r be the random value returned from random.org via oraclize.
  2. Let s be a seed which is provided by the player alongside their bet.
  3. let l be the inclusive lower bound of the desired random number.
  4. let u be the inclusive upper bound of the desired random number.

The following code can be used to produce a random number bounded between l and u inclusive and guaranteed to be evenly distributed across the range of values.

v = uint(sha3(r, s)) % (u - l) + l;

This scheme suffers from the following weakness. There may be other weaknesses.

If the source of entropy is sufficiently constrained then the number of possible values for r may be small enough that the operator of the contract could provide seed values of s that are known to have a higher probability of producing the desired roll outcome.

5.3.1.1 Reference Links

  1. https://docs.oraclize.it/#additional-features-encrypted-queries

5.3.2 maxBet formula does not ensure that the house is able to pay possible winnings.

The maxBet variable is used by the contract to enforce the upper limit to the amount

5.3.3 ownerSetMaxBetAsPercentOfHouse has no bounds checking.

The contract exposes a privileged function ownerSetMaxBetAsPercentOfHouse which can be used to set the maxBetAsPercentOfHouse variable. The maxBetAsPercentOfHouse is used to compute the maxBet value as follows.

maxBet = (contractBalance*maxBetAsPercentOfHouse)/maxBetDivisor;

The maxBetDivisor value in this equation can be expected to be 1000.

If the maxBetAsPercentOfHouse value is set sufficiently high then it will allow players to place a bet which if won would exceed the total available funds available to the house.

Recommend setting a firm maximum for this value and enforcing it within this function. It is not clear what this maximum should be.

5.4 Critical Issues

5.4.1 Underflow conditions in playerRollDice function.

Under certain conditions the playerRollDice can result in an underflow error.

If maxNumber is set to a value that allows for a rollUnder value greater than 102 then the following line of code found on line 451 will result in an integer underflow.

playerReward[rngId] = (((msg.value * (100-(rollUnder-1))) / (rollUnder-1))+msg.value)*houseEdge/houseEdgeDivisor;  

The underflow would occur in the following portion of that calculation

(100-(rollUnder-1))

Given a rollUnder value of 102, the above line of code will result the following computation.

(((msg.value * (2**256 - 1)) / (101))+msg.value)*houseEdge/houseEdgeDivisor;  

If the player wins this roll then they will be entitled a reward amount that could easily bankrupt the contract.

Recommend modifying maxNumber to be a constant such that this underflow condition cannot be met.

See Also: Section 5.2.5

5.4.2 ownerTransferEther function can underflow contractBalance

The contract exposes the privileged function ownerTransferEther to allow the operators of the contract to withdraw the portion of the contract balance that belongs to the bank.

Within the execution of this method the amount being withdrawn is deducted from contractBalance with no check for underflow conditions. In the event that the sending of the ether fails, the function will throw reverting the transaction. In the event that a withdrawal is requested for an amount greater than contractBalance the contract balance value will underflow.

This underflow will result in the value of contractBalance being set to a number on the scale of 2 ** 256. The maxBet value will subsequently be recalculated at which point the maximum bet will also end up being set to a very large value. Once this has occurred it does not appear there is a way for resetting contractBalance back to it's correct value.

In the event that these conditions occur, it does not appear that the operators of the contract will not have a way to restrict the maximum bet size.

Recommend one or all of the following:

5.4.2.1 Implement underflow checking.

Cause this function to throw an exception if underflow conditions are present.

if (amount > contractBalance) throw;

5.4.2.2 Remove the contractBalance variable

Tracking the balance of a contract is inherently difficult.

An alternate approach is to either:

  1. Write a function which computes the portion of the balance that does not belong to the bank and then to dynamically compute the contractBalance portion as this.balance - computeTotalPlayerFunds()
  2. Track the total amount of funds that belong to players and use a similar mechanism to compute contractBalance via this.balance - totalPlayerFunds.

Both of these methods can suffer from similar bugs but I believe that the player funds portion of the balance is actually easier to quantify than the bank portion.

Section 6 - Follow Up Audit

The full findings of this audit were submitted to the Etheroll development team on December 8th, 2016. What follows is an assessment of the code changes made in response to the audit findings. The follow up audit was performed on December 13th, 2016.

6.1 Overview

Upon reviewing the responses made by the Etheroll development team to the issues found in this security audit, it is the opinion of this audit that all issues have been satisfactorily address with one exceptions.

  • 5.2.9 - This issue still suffers from an easy to fix off by one error.
    • Update: This issue has now been addressed via commit 4751cc18e66a24bc478f7fc69e0bcc56d250236b

The test coverage for this contract is still lacking. It is the recommendation of this audit that at a minimum, the following checks be put into place.

  • disallows bets outside of the minBet/maxBet constraints.
  • disallows bets outside of the minNumber/maxNumber constraints.
  • win path for player roll produces the expected results
    • player is sent winnings
    • tracking values are updated
  • lose path for player roll produces the expected results
    • house is allocated player bet
    • tracking values are updated

6.2 Terminology

6.2.1 Status: Resolved

Issues that are noted with the resolved status have been adequately addressed.

6.2.2 Status: Unchanged

Issues that are noted with the unchanged status have not been addressed.

6.2.3 Status: Partially Resolved

Something was done in response to this issue but it may not fully address the issue.

6.3 Source Code

The follow up audit was performed on the code at commit hash 4751cc18e66a24bc478f7fc69e0bcc56d250236b.

The SHA256 hash of the Etheroll.sol source file is as follows.

$ shasum -a 256 contracts/Etheroll.sol
0abf1e8228a5bfe704478f7b0cd8ec8f1d969a8d14500afac185e75d5c6484a7  contracts/Etheroll.sol

6.4 Main Findings

6.4.1 Minor Issues

6.4.1.1 No brackets used for single line if statements

All if statements which do not use braces to contain their logic blocks now use the following inline format.

if (condition) throw;

6.4.1.2 Lack of use of the constant variable type.

All variables that are constant in nature now use the constant keyword in their declaration.

6.4.2 Medium Issues

6.4.2.1 Lack of overflow and underflow checking.

In most cases where underflow or overflow conditions were possible they have been fully mitigated. The mitigation was done using the recommended pattern via the use of the provided SafeAddSub contract.

There is still one location on line 447 which could result in underflow conditions, but would require modification of the maxNumber variable to a value above 101. Given that the maxNumber variable is declared as constant there should be a low risk of this occurring since it must occur as a chance to the contract code and cannot occur as a result of the execution of the contract's functionality.

6.4.3 Major Issues

6.4.3.1 Source of Entropy

  • 4.3.1
  • Status: Partially Resolved

The source of entropy has been modified such that the source of entropy can be proven fair under the assumption that the Oraclize service as well as the Random.org service are acting honestly. The Oraclize query now uses the nested data source which allows for encryption of only the API key, allowing the other parameters of the request to be verified.

Thi issue is marked as Partially Resolved because there is still one vector through which the entropy source could be exploited. The Oraclize service and Random.org service can provide any response to the query effectively allowing them to control the outcome of the virtual dice roll. This could occur as a malicious action performed by the operators of these services or as the result of some part of their infrastructure being compromised.

In the case of the Oraclize service, it would be provable that they acted dishonestly. It would likely be undetectable if executed by the Random.org service.

The difficulty of getting highly tamper proof entropy for smart contracts is not trivial to solve and the current scheme is likely sufficient. The simplest recommendation for improving the entropy source would be to combine the return values from two independent oracle services. It is the opinion of this audit that the current scheme is likely sufficient for the purposes of the Etheroll service.

6.4.3.2 House is not guaranteed to be able to pay winnings.

The contract now tracks the amount of funds which are not yet owned by the house through the variable maxPendingPayouts.

6.4.4 Critical Issues

6.4.4.1 Underflow conditions when computing player payout.

This issue has been resolved through marking the maxNumber variable with the constant keyword and setting it to the value of 99.

6.4.4.2 Underflow conditions for contractBalance

This issue has been resolved through the use of the safe addition and subtraction functions provided by the SafeAddSub base contract.

6.5 Test Coverage

The only changes to the test coverage since the initial audit are removal of test cases that were previously commented out and small modifications to the existing cases.

6.6 Detailed Findings

6.6.1 Minor Issues

6.6.1.1 betIsValid modifier does not use brackets

The formatting of the modifier was changed to use the following less error prone pattern.

if (condition) throw;
_;

6.6.1.2 gameIsActive modifier does not use brackets

The formatting of the modifier was changed to use the following less error prone pattern.

if (condition) throw;
_;

6.6.1.3 payoutsAreActive modifier does not use brackets

The formatting of the modifier was changed to use the following less error prone pattern.

if (condition) throw;
_;

6.6.1.4 Bet amount check in betIsValid modifier is not inclusive of either endpoint

The conditionals in this modifier are indeed the intended behavior.

6.6.1.5 Use of variable contractBalance to track balance could result in permanently unreachable funds.

This issue is unchanged. It is the assessment of this audit that this issue is minor and unlikely to have any negative impact on the operation of the Etheroll service.

6.6.1.6 maxBetDivisor is not set as constant

This variable is now declared with the constant keyword.

6.6.1.7 Contract fallback function is protected

This issue is unchanged. It is the assessment of this audit that this issue is minor and unlikely to have any negative impact on the operation of the Etheroll service.

6.6.2 Medium Issues

6.6.2.1 maxBet setter code is duplicated in 6 locations.

This code has been consolidated into the internal function setMaxBet.

6.6.2.2 The LogResult event does not have any indexed arguments.

This event now uses the indexed keyword on the values that are likely to be the targets of filters.

6.6.2.3 The LogReward event does not have any indexed arguments.

Note that this event was renamed from LogReward to LogBet

This event now uses the indexed keyword on the values that are likely to be the targets of filters.

6.6.2.4 The LogReward event does not have any indexed arguments.

Note that this event was renamed from LogReward to LogBet

This event now uses the indexed keyword on the values that are likely to be the targets of filters.

6.6.2.5 The contract constructor has 7 comments marked as todo.

The comments in the contract constructor now accurately reflect the intent of the statements.

6.6.2.6 Division by zero conditions in playerRollDice function.

The minNumber variable is now marked with the constant keyword and set to 2 eliminating the potential division by zero conditions.

6.6.2.7 Readability of contract state variables would be improved with use of structs.

The contract variable storage architecture is unchanged. Given the significant nature of this change, this audit recommends only making this change if Etheroll is willing to do a full re-audit of it's contracts.

This is not likely to have an immediate effect on the operation of the contracts, though the long term maintenance overhead would likely be improved by implementing this change.

6.6.2.8 playerWithdrawPendingTransactions not compatible with non-private key accounts.

This function now sends funds in a manner which is compatible with smart contracts with fallback functions requiring more than 2300 gas.

6.6.2.9 maxBetDivisor is likely too small.

  • 5.2.8
  • Status: Partially Resolved

This value is now set to a larger value of 1000000. Given that there is no downside to further increasing this value, I would suggest raising it to something closer to 1e18 which is the total number of wei per ether to ensure a sufficiently high degree of granularity in controlling the maxBet value.

6.6.2.10 maxBet is not enforced to be greater than or equal to minBet

  • 5.2.9
  • Status: Partially Resolved

The logic which sets the maxBet value will now trigger the gamePaused value to flip to true if the maxBet value ends up less than or equal to minBet.

The current implementation is as follows.

function setMaxBet() internal {
    maxBet = (contractBalance*maxBetAsPercentOfHouse)/maxBetDivisor;  
    /*pause game if max bet smaller than or equal to min bet*/
    if (maxBet <= minBet){
        /*requires owner intervention*/
        gamePaused = true;
    }else{
        gamePaused = false;   
    }
}   

The betIsValid modifier is written as:

modifier betIsValid(uint _betSize, uint _playerNumber ) {
    if(msg.value > maxBet || msg.value < minBet || _playerNumber < minNumber || _playerNumber > maxNumber) throw;
    _;
}

The if statement within setMaxBet suffers from an off by one* error and should be if (maxBet <= minBet + 1) given that the validation of bets is not inclusive of the endpoints.

6.6.3 Major Issues

6.6.3.1 The source of entropy is not guaranteed to be fair

The query to the Random.org is no longer opaque and can now be verified to be querying a fair source of entropy.

6.6.3.2 maxBet formula does not ensure that the house is able to pay possible winnings.

The new maxPendingPayouts variable now keeps track of the total pending bet payouts and will reject any bets which cause this value to exceed the amount of funds available to the house.

6.6.3.3 ownerSetMaxBetAsPercentOfHouse has no bounds checking.

This function now implements a check to enforce that the upper limit of this value constrains the percentage to 0.1% of the total house funds.

6.6.4 Critical Issues

6.6.4.1 Underflow conditions in playerRollDice function.

The underflow conditions have been removed through changing the maxNumber to be declared using the constant keyword and set to 99 which eliminates the possibility of the underflow occurring.

6.6.4.2 ownerTransferEther function can underflow contractBalance

The ownerTransferEther function now uses underflow safe subtraction when deducting the withdrawn amount from contractBalance.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
# Section 1 - Table of Contents<a id="heading-0"/>
* 1 - [Table of Contents](#heading-0)
* 2 - [Introduction](#heading-2)
* 2.1 - [Authenticity](#heading-2.1)
* 2.2 - [Audit Goals and Focus](#heading-2.2)
* 2.2.1 - [Sound Architecture](#heading-2.2.1)
* 2.2.2 - [Smart Contract Best Practices](#heading-2.2.2)
* 2.2.3 - [Code Correctness](#heading-2.2.3)
* 2.2.4 - [Code Quality](#heading-2.2.4)
* 2.2.5 - [Security](#heading-2.2.5)
* 2.3 - [About EtherRoll](#heading-2.3)
* 2.4 - [Terminology](#heading-2.4)
* 2.4.1 - [Coverage](#heading-2.4.1)
* 2.4.1.1 - [**untested**](#heading-2.4.1.1)
* 2.4.1.2 - [**low**](#heading-2.4.1.2)
* 2.4.1.3 - [**good**](#heading-2.4.1.3)
* 2.4.1.4 - [**excellent**](#heading-2.4.1.4)
* 2.4.2 - [Severity](#heading-2.4.2)
* 2.4.2.1 - [**minor**](#heading-2.4.2.1)
* 2.4.2.2 - [**medium**](#heading-2.4.2.2)
* 2.4.2.3 - [**major**](#heading-2.4.2.3)
* 2.4.2.4 - [**critical**](#heading-2.4.2.4)
* 3 - [Overview](#heading-3)
* 3.1 - [Source Code](#heading-3.1)
* 3.2 - [Contracts](#heading-3.2)
* 3.2.1 - [`EtheRoll` Contracts](#heading-3.2.1)
* 3.2.2 - [Oraclize Helper Contracts](#heading-3.2.2)
* 4 - [Main Findings](#heading-4)
* 4.1 - [Minor Issues](#heading-4.1)
* 4.1.1 - [No brackets used for single line `if` statements](#heading-4.1.1)
* 4.1.2 - [Lack of use of the `constant` variable type.](#heading-4.1.2)
* 4.2 - [Medium Issues](#heading-4.2)
* 4.2.1 - [Lack of overflow and underflow checking.](#heading-4.2.1)
* 4.3 - [Major Issues](#heading-4.3)
* 4.3.1 - [Source of Entropy](#heading-4.3.1)
* 4.3.2 - [House is not guaranteed to be able to pay winnings.](#heading-4.3.2)
* 4.4 - [Critical Issues](#heading-4.4)
* 4.4.1 - [Underflow conditions when computing player payout.](#heading-4.4.1)
* 4.4.2 - [Underflow conditions for `contractBalance`](#heading-4.4.2)
* 4.5 - [Test Coverage Analysis](#heading-4.5)
* 4.6 - [General Thoughts](#heading-4.6)
* 4.6.1 - [Secondary Audit](#heading-4.6.1)
* 4.6.2 - [Improve Test Coverage](#heading-4.6.2)
* 5 - [Detailed Findings](#heading-5)
* 5.1 - [Minor Issues](#heading-5.1)
* 5.1.1 - [`betIsValid` modifier does not use brackets](#heading-5.1.1)
* 5.1.2 - [`gameIsActive` modifier does not use brackets](#heading-5.1.2)
* 5.1.3 - [`payoutsAreActive` modifier does not use brackets](#heading-5.1.3)
* 5.1.4 - [Bet amount check in `betIsValid` modifier is not inclusive of either endpoint.](#heading-5.1.4)
* 5.1.5 - [Use of variable `contractBalance` to track balance could result in permanently unreachable funds.](#heading-5.1.5)
* 5.1.6 - [`maxBetDivisor` is not set as `constant`](#heading-5.1.6)
* 5.1.7 - [Contract fallback function is *protected*](#heading-5.1.7)
* 5.2 - [Medium Issues](#heading-5.2)
* 5.2.1 - [`maxBet` setter code is duplicated in 6 locations.](#heading-5.2.1)
* 5.2.2 - [The `LogResult` event does not have any `indexed` arguments.](#heading-5.2.2)
* 5.2.3 - [The `LogReward` event does not have any `indexed` arguments.](#heading-5.2.3)
* 5.2.4 - [The contract constructor has 7 comments marked as `todo`.](#heading-5.2.4)
* 5.2.5 - [Division by zero conditions in `playerRollDice` function.](#heading-5.2.5)
* 5.2.6 - [Readability of contract state variables would be improved with use of `structs`.](#heading-5.2.6)
* 5.2.7 - [`playerWithdrawPendingTransactions` not compatable with non-private key accounts.](#heading-5.2.7)
* 5.2.8 - [`maxBetDivisor` is likely too small.](#heading-5.2.8)
* 5.2.9 - [`maxBet` is not enforced to be greater than or equal to `minBet`](#heading-5.2.9)
* 5.3 - [Major Issues](#heading-5.3)
* 5.3.1 - [The source of entropy is not guaranteed to be fair](#heading-5.3.1)
* 5.3.1.1 - [Reference Links](#heading-5.3.1.1)
* 5.3.2 - [`maxBet` formula does not ensure that the house is able to pay possible winnings.](#heading-5.3.2)
* 5.3.3 - [`ownerSetMaxBetAsPercentOfHouse` has no bounds checking.](#heading-5.3.3)
* 5.4 - [Critical Issues](#heading-5.4)
* 5.4.1 - [Underflow conditions in `playerRollDice` function.](#heading-5.4.1)
* 5.4.2 - [`ownerTransferEther` function can underflow `contractBalance`](#heading-5.4.2)
* 5.4.2.1 - [Implement underflow checking.](#heading-5.4.2.1)
* 5.4.2.2 - [Remove the `contractBalance` variable](#heading-5.4.2.2)
* 6 - [Follow Up Audit](#heading-6)
* 6.1 - [Overview](#heading-6.1)
* 6.2 - [Terminology](#heading-6.2)
* 6.2.1 - [Status: Resolved](#heading-6.2.1)
* 6.2.2 - [Status: Unchanged](#heading-6.2.2)
* 6.2.3 - [Status: Partially Resolved](#heading-6.2.3)
* 6.3 - [Source Code](#heading-6.3)
* 6.4 - [Main Findings](#heading-6.4)
* 6.4.1 - [Minor Issues](#heading-6.4.1)
* 6.4.1.1 - [No brackets used for single line `if` statements](#heading-6.4.1.1)
* 6.4.1.2 - [Lack of use of the `constant` variable type.](#heading-6.4.1.2)
* 6.4.2 - [Medium Issues](#heading-6.4.2)
* 6.4.2.1 - [Lack of overflow and underflow checking.](#heading-6.4.2.1)
* 6.4.3 - [Major Issues](#heading-6.4.3)
* 6.4.3.1 - [Source of Entropy](#heading-6.4.3.1)
* 6.4.3.2 - [House is not guaranteed to be able to pay winnings.](#heading-6.4.3.2)
* 6.4.4 - [Critical Issues](#heading-6.4.4)
* 6.4.4.1 - [Underflow conditions when computing player payout.](#heading-6.4.4.1)
* 6.4.4.2 - [Underflow conditions for `contractBalance`](#heading-6.4.4.2)
* 6.5 - [Test Coverage](#heading-6.5)
* 6.6 - [Detailed Findings](#heading-6.6)
* 6.6.1 - [Minor Issues](#heading-6.6.1)
* 6.6.1.1 - [`betIsValid` modifier does not use brackets](#heading-6.6.1.1)
* 6.6.1.2 - [`gameIsActive` modifier does not use brackets](#heading-6.6.1.2)
* 6.6.1.3 - [`payoutsAreActive` modifier does not use brackets](#heading-6.6.1.3)
* 6.6.1.4 - [Bet amount check in `betIsValid` modifier is not inclusive of either endpoint](#heading-6.6.1.4)
* 6.6.1.5 - [Use of variable `contractBalance` to track balance could result in permanently unreachable funds.](#heading-6.6.1.5)
* 6.6.1.6 - [`maxBetDivisor` is not set as constant](#heading-6.6.1.6)
* 6.6.1.7 - [Contract fallback function is protected](#heading-6.6.1.7)
* 6.6.2 - [Medium Issues](#heading-6.6.2)
* 6.6.2.1 - [`maxBet` setter code is duplicated in 6 locations.](#heading-6.6.2.1)
* 6.6.2.2 - [The `LogResult` event does not have any indexed arguments.](#heading-6.6.2.2)
* 6.6.2.3 - [The `LogReward` event does not have any indexed arguments.](#heading-6.6.2.3)
* 6.6.2.4 - [The `LogReward` event does not have any indexed arguments.](#heading-6.6.2.4)
* 6.6.2.5 - [The contract constructor has 7 comments marked as `todo`.](#heading-6.6.2.5)
* 6.6.2.6 - [Division by zero conditions in playerRollDice function.](#heading-6.6.2.6)
* 6.6.2.7 - [Readability of contract state variables would be improved with use of structs.](#heading-6.6.2.7)
* 6.6.2.8 - [`playerWithdrawPendingTransactions` not compatible with non-private key accounts.](#heading-6.6.2.8)
* 6.6.2.9 - [`maxBetDivisor` is likely too small.](#heading-6.6.2.9)
* 6.6.2.10 - [`maxBet` is not enforced to be greater than or equal to minBet](#heading-6.6.2.10)
* 6.6.3 - [Major Issues](#heading-6.6.3)
* 6.6.3.1 - [The source of entropy is not guaranteed to be fair](#heading-6.6.3.1)
* 6.6.3.2 - [`maxBet` formula does not ensure that the house is able to pay possible winnings.](#heading-6.6.3.2)
* 6.6.3.3 - [`ownerSetMaxBetAsPercentOfHouse` has no bounds checking.](#heading-6.6.3.3)
* 6.6.4 - [Critical Issues](#heading-6.6.4)
* 6.6.4.1 - [Underflow conditions in `playerRollDice` function.](#heading-6.6.4.1)
* 6.6.4.2 - [`ownerTransferEther` function can underflow contractBalance](#heading-6.6.4.2)
# <a id="heading-2"/> Section 2 - Introduction
From December 2nd through December 9th of 2016, Piper Merriam conducted an audit of the
primary EtherRoll smart contract. The findings of the audit are presented in
this document.
I, Piper Merriam have no stake in the success or failure of EtherRoll. This
audit was performed under a contracted hourly rate with no other compensation.
## <a id="heading-2.1"/> 2.1 Authenticity
This document should have an attached cryptographic signature to ensure it has not been tampered with. The signature can be verified using the public key from Piper Merriam's the keybase.io.
[Piper Merriam on Keybase.io](https://keybase.io/pipermerriam)
## <a id="heading-2.2"/> 2.2 Audit Goals and Focus
### <a id="heading-2.2.1"/> 2.2.1 Sound Architecture
This audit includes assessments of the overall architecture and design choices.
Given the subjective nature of these assessments it will be up to the EtherRoll
development team to determine whether any changes should be made.
### <a id="heading-2.2.2"/> 2.2.2 Smart Contract Best Practices
This audit will evaluate whether the codebase follows the current established
best practices for smart contract development.
### <a id="heading-2.2.3"/> 2.2.3 Code Correctness
This audit will evaluate whether the code does what it is intended to do.
### <a id="heading-2.2.4"/> 2.2.4 Code Quality
This audit will evaluate whether the code has been written in a way that
ensures readability and maintainability.
### <a id="heading-2.2.5"/> 2.2.5 Security
This audit will look for any exploitable security vulnerabilities, or other
potential threats to either the operators of EtherRoll or its users.
## <a id="heading-2.3"/> 2.3 About EtherRoll
EtherRoll is a gambling service on the Ethereum blockchain.
## <a id="heading-2.4"/> 2.4 Terminology
This audit uses the following terminology.
### <a id="heading-2.4.1"/> 2.4.1 Coverage
Measurement of the testing code coverage. This measurement was done via
inspection of the code.
#### <a id="heading-2.4.1.1"/> 2.4.1.1 **untested**
No tests.
#### <a id="heading-2.4.1.2"/> 2.4.1.2 **low**
The tests do not cover some set of non-trivial functionality.
#### <a id="heading-2.4.1.3"/> 2.4.1.3 **good**
The tests cover all major functionality.
#### <a id="heading-2.4.1.4"/> 2.4.1.4 **excellent**
The tests cover all code paths.
### <a id="heading-2.4.2"/> 2.4.2 Severity
Measurement of magnitude of an issue.
#### <a id="heading-2.4.2.1"/> 2.4.2.1 **minor**
Minor issues are generally subjective in nature, or potentially deal with
topics like "best practices" or "readability". Minor issues in general will
not indicate an actual problem or bug in code.
The maintainers should use their own judgement as to whether addressing these
issues improves the codebase.
#### <a id="heading-2.4.2.2"/> 2.4.2.2 **medium**
Medium issues are generally objective in nature but do not represent actual
bugs or security problems.
These issues should be addressed unless there is a clear reason not to.
#### <a id="heading-2.4.2.3"/> 2.4.2.3 **major**
Major issues will be things like bugs or security vulnerabilities. These
issues may not be directly exploitable, or may require a certain condition to
arise in order to be exploited.
Left unaddressed these issues are highly likely to cause problems with the
operation of the contract or lead to a situation which allows the system to be
exploited in some way.
#### <a id="heading-2.4.2.4"/> 2.4.2.4 **critical**
Critical issues are directly exploitable bugs or security vulnerabilities.
Left unaddressed these issues are highly likely or guaranteed to cause major
problems or potentially a full failure in the operations of the contract.
# <a id="heading-3"/> Section 3 - Overview
## <a id="heading-3.1"/> 3.1 Source Code
The EtherRoll source code was made available through a private repository on
github. The code being evaluated can be found under the commit hash
`3f39d2127130760dacf2820ac18e93d42ef7b432`
This audit evaluated a single Solidity source file, `./contracts/Etheroll.sol`.
The sha256 of that contract source is as follows.
```bash
$ shasum -a 256 contracts/Etheroll.sol
bcf78ede1db5597eb4697f2ab26fb06e7bbb3b72393d6686bf4f6e86976e2b23 contracts/Etheroll.sol<Paste>
```
## <a id="heading-3.2"/> 3.2 Contracts
### <a id="heading-3.2.1"/> 3.2.1 `EtheRoll` Contracts
The EtheRoll service is comprised of a single smart contract named `EtheRoll`.
### <a id="heading-3.2.2"/> 3.2.2 Oraclize Helper Contracts
This contract uses the Oraclize service to retrieve externally generated random
numbers from the Random.org API. The `Etheroll` contract uses the abstract
base contracts and interface contracts provided by Oraclize in their github
repository linked below.
https://github.com/oraclize/ethereum-api
Given that these contracts are written by the Oraclize service they were not
evaluated beyond the following checks.
* The addresses for the Oraclize resolver addresses found in the source code
were verified to match those from the Oraclize github repository.
* The overall code for the `OraclizeI`, `OraclizeAddrResolverI`, and
`usingOraclize` contracts were *visually* inspected to match the code found in
the Oraclize github repository.
# <a id="heading-4"/> Section 4 - Main Findings
## <a id="heading-4.1"/> 4.1 Minor Issues
### <a id="heading-4.1.1"/> 4.1.1 No brackets used for single line `if` statements
* Severity: **minor**
The modifiers found in the `Etheroll` contract do not use brackets to wrap the
if statement code blocks.
```javascript
// Example from Etheroll.sol:Line 303
modifier gameIsActive {
if(gamePaused == true)
throw;
_;
}
```
This style can more easily lead to introducing defects during refactoring. A
common version of this mistake is as follows.
```javascript
// Initial Code
modifier restricted {
if (someCondition)
return;
_;
}
```
Later it is decided that this should also log an event so we add a logging
statement.
```javascript
// Now with logging statement
modifier restricted {
if (someCondition)
AuthorizationFailed();
return;
_;
}
```
A developer who quickly adds the `AuthorizationFailed()` line might neglect to
realize that this modifier will now *always* return early without executing the
function body.
Recommend changing these statements to one of the following patterns.
```javascript
// Pattern A
modifier restricted {
if (someCondition) return;
_;
}
// Pattern B
modifier restricted {
if (someCondition) {
return;
}
_;
}
```
Pattern A has the benefit of remaining concise while making the mistake
mentioned above much less likely to occur.
### <a id="heading-4.1.2"/> 4.1.2 Lack of use of the `constant` variable type.
The following variables in the contract are used as constants but are not
declared as constants in the source code.
* `maxBetDivisor`
* `minNumber`
* `maxNumber`
* `houseEdgeDivisor`
Use of the `constant` keyword will eliminate certain classes of defects and
improve readability.
## <a id="heading-4.2"/> 4.2 Medium Issues
### <a id="heading-4.2.1"/> 4.2.1 Lack of overflow and underflow checking.
The contract does not implement *any* checking for overflow or underflow
conditions. Given the unsafe nature of solidity arithmetic and the
catastrophic nature of these types of defects it may be wise to implement and
use a set of *safe* math operations. The following base contract can be used
to accomplish this.
```javascript
// Sourced from the Nexus DAppSys repository.
contract DSSafeAddSub {
function safeToAdd(uint a, uint b) internal returns (bool) {
return (a + b >= a);
}
function safeAdd(uint a, uint b) internal returns (uint) {
if (!safeToAdd(a, b)) throw;
return a + b;
}
function safeToSubtract(uint a, uint b) internal returns (bool) {
return (b <= a);
}
function safeSub(uint a, uint b) internal returns (uint) {
if (!safeToSubtract(a, b)) throw;
return a - b;
}
}
```
## <a id="heading-4.3"/> 4.3 Major Issues
### <a id="heading-4.3.1"/> 4.3.1 Source of Entropy
While there is no evidence of dishonesty, the current source of entropy is
exploitable by the operators of the contract. It may be worth exploring a modified source of entropy or adding a layer to the current source of entropy that provides a guarantee that the resulting random integer is within the proper range.
In the current form none of the following can be verified.
1. The returned integers are constrained between the lower and upper bounds.
2. The returned integers cover the full range between the lower and upper bound.
3. The returned integers are evenly distributed between the lower and upper bounds.
### <a id="heading-4.3.2"/> 4.3.2 House is not guaranteed to be able to pay winnings.
The current formulas used to set the maximum bet amount do not guarantee that
the house is able to pay in the event that the player wins. The current
formula uses a percentage of the funds available to the house to set the
maximum bet size.
Without taking into account the following information it cannot be guaranteed
that the house will be able to pay a player their winning.
* The number that a player is betting.
* The number of unresolved bets and their potential winnings.
This may not be an issue as long as the available funds for the house is
sufficiently large and the maximum but is sufficiently small, but in the
current state, these properties cannot be guaranteed.
## <a id="heading-4.4"/> 4.4 Critical Issues
### <a id="heading-4.4.1"/> 4.4.1 Underflow conditions when computing player payout.
The underflow conditions when computing the player payout have catastrophic
consequences but can be fully mitigated by making `maxNumber` a `constant`
variable.
### <a id="heading-4.4.2"/> 4.4.2 Underflow conditions for `contractBalance`
The underflow conditions present for tracking the `contractBalance` amount have
very unfortunate consequences that would likely render the long term use of the
contract untenable. Simple checking to ensure that this operation cannot
underflow will fully mitigate this risk.
## <a id="heading-4.5"/> 4.5 Test Coverage Analysis
Coverage: **low**
The tests for the contract are located in `./tests/etheroll.js`. The file
contains a total of 14 test cases, 4 of which are commented out.
Attempting to run the test suite resulted in the following error. This is
likely an error in the local development environment and unrelated to the
actual test suite or contract code.
```bash
$ truffle test
Compiling Etheroll.sol...
Compiling Migrations.sol...
Error: Invalid JSON RPC response: "Error: connect ECONNREFUSED 127.0.0.1:8545\n at Object.exports._errnoException (util.js:1026:11)\n at exports._exceptionWithHostPort (util.js:1049:20)\n at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1090:14)"
at Object.InvalidResponse (/REDACED/.yarn-config/global/node_modules/web3/lib/web3/errors.js:35:16)
at exports.XMLHttpRequest.request.onreadystatechange (/REDACED/.yarn-config/global/node_modules/web3/lib/web3/httpprovider.js:111:32)
at exports.XMLHttpRequest.dispatchEvent (/REDACED/.yarn-config/global/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:591:25)
at setState (/REDACED/.yarn-config/global/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:610:14)
at exports.XMLHttpRequest.handleError (/REDACED/.yarn-config/global/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:532:5)
at ClientRequest.errorHandler (/REDACED/.yarn-config/global/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:459:14)
at emitOne (events.js:96:13)
at ClientRequest.emit (events.js:188:7)
at Socket.socketErrorListener (_http_client.js:309:9)
at emitOne (events.js:96:13)
at Socket.emit (events.js:188:7)
at emitErrorNT (net.js:1281:8)
at _combinedTickCallback (internal/process/next_tick.js:74:11)
at process._tickDomainCallback (internal/process/next_tick.js:122:9)
```
The test coverage is restricted to checking that various contract variables are
set to the expected values. There does not appear to be any coverage of core
contract functionality such as placing bets or resolving them.
## <a id="heading-4.6"/> 4.6 General Thoughts
### <a id="heading-4.6.1"/> 4.6.1 Secondary Audit
Given that several issues were found which could seriously affect the operation
of this contract it is recommended that this contract be subjected to a
secondary audit once the Etheroll team has responded to the issues found in
this audit.
### <a id="heading-4.6.2"/> 4.6.2 Improve Test Coverage
Given the financial nature of this application, the core application logic
should be subjected to some for of tests. This could take the form of an
automated test suite, or a set of live tests on the public test network. If
the Etheroll team opts for the later, they should ensure they provide
sufficient documentation to their tests to ensure that others can verify the
results.
# <a id="heading-5"/> Section 5 - Detailed Findings
The following is an exhaustive list of all issues found during in this audit.
## <a id="heading-5.1"/> 5.1 Minor Issues
### <a id="heading-5.1.1"/> 5.1.1 `betIsValid` modifier does not use brackets
The modifier `betIsValid` does not use brackets for the if statement code
blocks. This code style can lead to errors more easily than if the code blocks
were wrapped in brackets.
Recommend changing the modifier to one of the recommended patterns from [Section 4.1.1](#heading-4.1.1)
### <a id="heading-5.1.2"/> 5.1.2 `gameIsActive` modifier does not use brackets
The modifier `gameIsActive` does not use brackets for the if statement code
blocks. This code style can lead to errors more easily than if the code blocks
were wrapped in brackets.
Recommend changing the modifier to one of the recommended patterns from [Section 4.1.1](#heading-4.1.1)
### <a id="heading-5.1.3"/> 5.1.3 `payoutsAreActive` modifier does not use brackets
The modifier `payoutsAreActive` does not use brackets for the if statement code
blocks. This code style can lead to errors more easily than if the code blocks
were wrapped in brackets.
Recommend changing the modifier to one of the recommended patterns from [Section 4.1.1](#heading-4.1.1)
### <a id="heading-5.1.4"/> 5.1.4 Bet amount check in `betIsValid` modifier is not inclusive of either endpoint.
The range checking in the `betIsValid` is not inclusive of either endpoint.
While this makes sense for the `minBet` since a bet of `0` is likely not
wanted, it does not feel intuitive that the actual maximum bet is `maxBet - 1`.
Recommend ensuring this behavior is desired.
### <a id="heading-5.1.5"/> 5.1.5 Use of variable `contractBalance` to track balance could result in permanently unreachable funds.
The contract makes use of the variable `contractBalance` to represent the
portion of the contract's total balance that belongs to the *bank*.
While it appears that this value is accurately tracked across the contract
operations this can be considered an anti-pattern in that any deviation from
the actual amount will result in one of two problems of differing severity.
* *If* the actual funds available are *less* than the tracked balance, then the **critical** issue outlined in [Section 5.4.2](#heading-5.4.2) will occur.
* *If* the actual funds available are *greater* than the tracked balance then the difference in those funds will be unrecoverable.
Recommend changing the modifier to one of the recommended patterns from [Section 4.1.1](#heading-4.1.1)
### <a id="heading-5.1.6"/> 5.1.6 `maxBetDivisor` is not set as `constant`
The value `maxBetDivisor` is not defined using the `constant` keyword. Given
that this value is used as a `constant` and that the contract does not expose
any interface to modify this value it seems appropriate that it should be a
`constant`.
Recommend modifying this variable to use the `constant` keyword.
### <a id="heading-5.1.7"/> 5.1.7 Contract fallback function is *protected*
The contract implements a fallback function to allow adding funds to the total
amount available to the house. This function is restricted such that it can
only be called by the address set as the `treasury`.
While this restriction will prevent normal sending of funds it will not prevent
funds sent to the contract via `selfdestruct(etherollAddress)`.
See [this section of the solidity documentation](http://solidity.readthedocs.io/en/develop/security-considerations.html#sending-and-receiving-ether)
> Neither contracts nor "external accounts" are currently able to prevent that someone sends them Ether. Contracts can react on and reject a regular transfer, but there are ways to move Ether without creating a message call. One way is to simply "mine to" the contract address and the second way is using `selfdestruct(x)`.
No recommendations to mitigate this as it cannot be prevented. There does not
appear to be any exploitable attack vector associated with this. It may be
appropriate to simply remove the restriction since there are other avenues to
dumping funds into the contract (such as just placing many low probability
bets).
## <a id="heading-5.2"/> 5.2 Medium Issues
### <a id="heading-5.2.1"/> 5.2.1 `maxBet` setter code is duplicated in 6 locations.
The following line of code is duplicated in the following 6 locations.
```javascript
maxBet = (contractBalance*maxBetAsPercentOfHouse)/maxBetDivisor;
```
* Line 414
* Line 526
* Line 560
* Line 608
* Line 630
* Line 661
This type of code duplication is prone to introducing defects since any change
to the logic requires remembering to change it in all 6 locations.
Recommend abstracting this logic into a reusable `internal` function and
changing all 6 locations to delegate to this function.
### <a id="heading-5.2.2"/> 5.2.2 The `LogResult` event does not have any `indexed` arguments.
The event `LogResult` does not use the `indexed` keyword on any of the
arguments. While it is still possible to filter on these values, it is
significantly less efficient since non-indexed arguments are included in the
topic bloom filter.
Recommend adding the `indexed` keyword to `BetID` and `PlayerAddress`. Consider
adding it to `PlayerNumber` as well.
### <a id="heading-5.2.3"/> 5.2.3 The `LogReward` event does not have any `indexed` arguments.
The event `LogReward` does not use the `indexed` keyword on any of the
arguments. While it is still possible to filter on these values, it is
significantly less efficient since non-indexed arguments are included in the
topic bloom filter.
Recommend adding the `indexed` keyword to `PlayerAddress`.
### <a id="heading-5.2.4"/> 5.2.4 The contract constructor has 7 comments marked as `todo`.
The contract constructor has 7 lines of code and corresponding todo comments
marking them to be removed prior to production.
Recommend modifying the testing suite to not require these statements to be
present. This pattern is prone to errors since once the statements have been
removed the test suite is not likely to continue to pass and thus becomes less
useful for ongoing development.
### <a id="heading-5.2.5"/> 5.2.5 Division by zero conditions in `playerRollDice` function.
Under certain conditions the `playerRollDice` function will result in an error
being thrown due to division by zero.
If `minNumber` is set such that it allows for a `rollUnder` value of `1` then
following computation found on line 451 will result in a division by zero
error.
```javascript
reward = (((msg.value * (100-(rollUnder-1))) / (rollUnder-1))+msg.value)*houseEdge/houseEdgeDivisor;
```
Recommend modifying `minNumber` such that is is a `constant` variable and
fixing it permanently to the desired lower bound.
### <a id="heading-5.2.6"/> 5.2.6 Readability of contract state variables would be improved with use of `structs`.
The contract uses a number of mappings to store the various pieces of state
associated with a player's bet. This contract state could be modified to
improve readability through the use of a custom `struct`.
The contract currently uses the following mappings.
```javascript
mapping (bytes32 => address) playerAddress;
mapping (bytes32 => address) playerTempAddress;
mapping (bytes32 => int) playerBetId;
mapping (bytes32 => uint) playerBetValue;
mapping (bytes32 => uint) playerTempBetValue;
mapping (bytes32 => uint) playerDieResult;
mapping (bytes32 => uint) playerNumber;
mapping (address => uint) playerPendingWithdrawals;
mapping (bytes32 => uint) playerReward;
mapping (bytes32 => uint) playerTempReward;
```
Recommend modifying to use a custom struct similar to the following.
```javascript
struct Bet {
address playerAddress;
address tempAddress;
int betId;
uint betValue;
uint tempBetValue;
uint dieResult;
uint playerNumber;
uint playerReward;
uint tempReward;
}
mapping (bytes32 => PlayerBet) playerBets;
```
Currently the code requires looking up the bet values from each mapping which
results in code like the following extract taken from lines 470-479
```javascript
/* player address mapped to query id does not exist */
if (playerAddress[myid]==0x0) throw;
/* map the result from oraclize to query id */
playerDieResult[myid] = parseInt(result);
/* get the playerAddress for this query id */
playerTempAddress[myid] = playerAddress[myid];
/* delete playerAddress for this query id */
delete playerAddress[myid];
```
This code would be transformed to the following if the struct pattern was used.
```
var bet = playerBets[myid];
/* player address mapped to query id does not exist */
if (bet.betId==0x0) throw;
/* map the result from oraclize to query id */
bet.dieResult = parseInt(result);
/* get the playerAddress for this query id */
bet.tempAddress = bet.playerAddress;
/* delete playerAddress for this query id */
delete bet.playerAddress;
```
This is likely to greatly reduce the likelihood of defects due to increased
readability.
### <a id="heading-5.2.7"/> 5.2.7 `playerWithdrawPendingTransactions` not compatable with non-private key accounts.
In the event that a player wins the random roll, the contract attempts to send
their winnings using `.send()`. This built-in function of the `address` type
provides `2300` gas. In the event that this call fails, the amount owed to the
player is recorded in `playerPendingWithdrawals`.
The function `playerWithdrawPendingTransactions` allows a player to trigger the
withdrawl of any funds that are allocated to them. The internals of the
function use the `.send()` function to send the funds.
The design of this function makes it impossible for any contract whose fallback
function requires greater than `2300` gas to retrieve their winnings since the
call to `.send` will always fail.
Recommend modifying the `playerWithdrawPendingTransactions` to instead send the
funds using `.call.value(amount)()`. This will send the funds with all
available gas, allowing contracts with fallback functions which require more
than `2300` gas to still withdraw their funds.
### <a id="heading-5.2.8"/> 5.2.8 `maxBetDivisor` is likely too small.
The contract uses a fixed number `maxBetDivisor` to compute the `maxBet` value using the following formula.
```javascript
maxBet = (contractBalance*maxBetAsPercentOfHouse)/maxBetDivisor;
```
If we assume that `maxBetAsPercentOfHouse` is constrained to be greater than
`0` and less than or equal to `maxBetDivisor` then the `maxBetDivisor` defines
the possible range of values that `maxBet` can be set to as a proportion of the
total funds available to the house.
For example, a `maxBetDivisor` value of `4` would allow for the `maxBet` value to be set to any of `25%`, `50%`, `75%`, or `100%` of the total house. This also means that the lowest value that `maxBet` can be set to is `1 / maxBetDivisor`.
In the current form, the lowest possible value for `maxBet` is `1/1000th` of
the total house funds. This level of granularity is likely too coarse and
doesn't allow for as fine grained control over this value as may be desired.
Recommend changing this value to a significantly larger number to allow fine
grained control over this computation.
### <a id="heading-5.2.9"/> 5.2.9 `maxBet` is not enforced to be greater than or equal to `minBet`
The contract uses the two storage values `minBet` and `maxBet` to constrain the
amount that a player may bet between a lower and upper bound.
The places in the code that set the `maxBet` and `minBet` values do not do any
bounds checking. This behavior is likely fine for the `ownerSetMinBet`
function since it is a manual action triggered by the contract owner. This
could be problematic in the places where `maxBet` is set. Since `maxBet` is
based on the `contractBalance` value and that value is likely to vary widely it
seems reasonable that conditions could occur where the `maxBet` gets set below
or equal to the `minBet` value at which point the contract would reject all new
bets.
It is not clear what the appropriate strategy to mitigate this should be.
Consider one of the following options.
Recommend setting `gamePaused` to `true` when this condition occurs and require
manual intervention.
## <a id="heading-5.3"/> 5.3 Major Issues
### <a id="heading-5.3.1"/> 5.3.1 The source of entropy is not guaranteed to be fair
The contract uses the Oraclize service to query the random.org service as a
source of entropy. In order to use random.org, one must submit an API key with
the query. In order to not expose this API key, the contract uses the
encrypted query feature of oraclize to mask the parameters that will be sent to
the API.
This encryption also prevents auditing the nature of the query that is sent to
random.org making it impossible to verify that the entropy source is being
queried in a fair manner.
Below are two ways that this could be abused. There are likely others.
1. The distribution that the number is being picked from could not be an even
distribution which would allow the contract to have an unfair advantage.
2. The range that the number is being picked from is not known. Combined with
the `minNumber` and `maxNumber` constraints it is not possible for a player to
verify the *odds* of their bet.
Recommend modifying the algorithm as follows to remove the possibility of bad
behavior by the operators of the contract.
1. Let `r` be the random value returned from random.org via oraclize.
2. Let `s` be a seed which is provided by the player alongside their bet.
3. let `l` be the inclusive lower bound of the desired random number.
4. let `u` be the inclusive upper bound of the desired random number.
The following code can be used to produce a random number bounded between `l`
and `u` inclusive and guaranteed to be evenly distributed across the range of
values.
```javascript
v = uint(sha3(r, s)) % (u - l) + l;
```
This scheme suffers from the following weakness. There may be other weaknesses.
If the source of entropy is sufficiently constrained then the number of
possible values for `r` may be small enough that the operator of the contract
could provide seed values of `s` that are known to have a higher probability of
producing the desired roll outcome.
#### <a id="heading-5.3.1.1"/> 5.3.1.1 Reference Links
1. [https://docs.oraclize.it/#additional-features-encrypted-queries](https://docs.oraclize.it/#additional-features-encrypted-queries)
### <a id="heading-5.3.2"/> 5.3.2 `maxBet` formula does not ensure that the house is able to pay possible winnings.
The `maxBet` variable is used by the contract to enforce the upper limit to the amount
### <a id="heading-5.3.3"/> 5.3.3 `ownerSetMaxBetAsPercentOfHouse` has no bounds checking.
The contract exposes a privileged function `ownerSetMaxBetAsPercentOfHouse`
which can be used to set the `maxBetAsPercentOfHouse` variable. The
`maxBetAsPercentOfHouse` is used to compute the `maxBet` value as follows.
```javascript
maxBet = (contractBalance*maxBetAsPercentOfHouse)/maxBetDivisor;
```
The `maxBetDivisor` value in this equation can be expected to be `1000`.
If the `maxBetAsPercentOfHouse` value is set sufficiently high then it will
allow players to place a bet which if won would exceed the total available
funds available to the house.
Recommend setting a firm maximum for this value and enforcing it within this
function. It is not clear what this maximum should be.
## <a id="heading-5.4"/> 5.4 Critical Issues
### <a id="heading-5.4.1"/> 5.4.1 Underflow conditions in `playerRollDice` function.
Under certain conditions the `playerRollDice` can result in an underflow error.
If `maxNumber` is set to a value that allows for a `rollUnder` value greater
than `102` then the following line of code found on line 451 will result in an
integer underflow.
```javascript
playerReward[rngId] = (((msg.value * (100-(rollUnder-1))) / (rollUnder-1))+msg.value)*houseEdge/houseEdgeDivisor;
```
The underflow would occur in the following portion of that calculation
```javascript
(100-(rollUnder-1))
```
Given a `rollUnder` value of `102`, the above line of code will result the
following computation.
```javascript
(((msg.value * (2**256 - 1)) / (101))+msg.value)*houseEdge/houseEdgeDivisor;
```
If the player wins this roll then they will be entitled a reward amount that
could easily bankrupt the contract.
Recommend modifying `maxNumber` to be a constant such that this underflow
condition cannot be met.
See Also: [Section 5.2.5](#heading-5.2.5)
### <a id="heading-5.4.2"/> 5.4.2 `ownerTransferEther` function can underflow `contractBalance`
The contract exposes the privileged function `ownerTransferEther` to allow the
operators of the contract to withdraw the portion of the contract balance that
belongs to the *bank*.
Within the execution of this method the amount being withdrawn is deducted from
`contractBalance` with no check for underflow conditions. In the event that
the sending of the ether fails, the function will throw reverting the
transaction. In the event that a withdrawal is requested for an amount greater
than `contractBalance` the contract balance value will underflow.
This underflow will result in the value of `contractBalance` being set to a
number on the scale of `2 ** 256`. The `maxBet` value will subsequently be
recalculated at which point the maximum bet will also end up being set to a
very large value. Once this has occurred it does not appear there is a way for
resetting `contractBalance` back to it's correct value.
In the event that these conditions occur, it does not appear that the operators
of the contract will not have a way to restrict the maximum bet size.
Recommend one or all of the following:
#### <a id="heading-5.4.2.1"/> 5.4.2.1 Implement underflow checking.
Cause this function to throw an exception if underflow conditions are present.
```
if (amount > contractBalance) throw;
```
#### <a id="heading-5.4.2.2"/> 5.4.2.2 Remove the `contractBalance` variable
Tracking the balance of a contract is inherently difficult.
An alternate approach is to either:
1. Write a function which computes the portion of the balance that **does not** belong to the bank and then to dynamically compute the `contractBalance` portion as `this.balance - computeTotalPlayerFunds()`
2. Track the total amount of funds that belong to players and use a similar mechanism to compute `contractBalance` via `this.balance - totalPlayerFunds`.
Both of these methods can suffer from similar bugs but I believe that the
player funds portion of the balance is actually easier to quantify than the
bank portion.
# <a id="heading-6"/> Section 6 - Follow Up Audit
The full findings of this audit were submitted to the Etheroll development team
on December 8th, 2016. What follows is an assessment of the code changes made
in response to the audit findings. The follow up audit was performed on
December 13th, 2016.
## <a id="heading-6.1"/> 6.1 Overview
Upon reviewing the responses made by the Etheroll development team to the
issues found in this security audit, it is the opinion of this audit that all
issues have been satisfactorily address with one exceptions.
* [5.2.9](#heading-5.2.9) - This issue still suffers from an easy to fix *off by one* error.
* Update: This issue has now been addressed via commit `4751cc18e66a24bc478f7fc69e0bcc56d250236b`
The test coverage for this contract is still lacking. It is the recommendation
of this audit that at a minimum, the following checks be put into place.
* disallows bets outside of the `minBet/maxBet` constraints.
* disallows bets outside of the `minNumber/maxNumber` constraints.
* win path for player roll produces the expected results
* player is sent winnings
* tracking values are updated
* lose path for player roll produces the expected results
* house is allocated player bet
* tracking values are updated
## <a id="heading-6.2"/> 6.2 Terminology
### <a id="heading-6.2.1"/> 6.2.1 Status: Resolved
Issues that are noted with the resolved status have been adequately addressed.
### <a id="heading-6.2.2"/> 6.2.2 Status: Unchanged
Issues that are noted with the unchanged status have not been addressed.
### <a id="heading-6.2.3"/> 6.2.3 Status: Partially Resolved
Something was done in response to this issue but it may not fully address the
issue.
## <a id="heading-6.3"/> 6.3 Source Code
The follow up audit was performed on the code at commit hash
`4751cc18e66a24bc478f7fc69e0bcc56d250236b`.
The SHA256 hash of the `Etheroll.sol` source file is as follows.
```bash
$ shasum -a 256 contracts/Etheroll.sol
0abf1e8228a5bfe704478f7b0cd8ec8f1d969a8d14500afac185e75d5c6484a7 contracts/Etheroll.sol
```
## <a id="heading-6.4"/> 6.4 Main Findings
### <a id="heading-6.4.1"/> 6.4.1 Minor Issues
#### <a id="heading-6.4.1.1"/> 6.4.1.1 No brackets used for single line `if` statements
* [4.1.1](#heading-4.1.1)
* Status: **Resolved**
All `if` statements which do not use braces to contain their logic blocks now
use the following inline format.
```javascript
if (condition) throw;
```
#### <a id="heading-6.4.1.2"/> 6.4.1.2 Lack of use of the `constant` variable type.
* [4.1.2](#heading-4.1.2)
* Status: **Resolved**
All variables that are constant in nature now use the `constant` keyword in
their declaration.
### <a id="heading-6.4.2"/> 6.4.2 Medium Issues
#### <a id="heading-6.4.2.1"/> 6.4.2.1 Lack of overflow and underflow checking.
* [4.2.1](#heading-4.2.1)
* Status: **Resolved**
In most cases where underflow or overflow conditions were possible they have
been fully mitigated. The mitigation was done using the recommended pattern
via the use of the provided `SafeAddSub` contract.
There is still one location on line 447 which *could* result in underflow
conditions, but would require modification of the `maxNumber` variable to a
value above `101`. Given that the `maxNumber` variable is declared as
`constant` there should be a low risk of this occurring since it must occur as a
chance to the contract code and cannot occur as a result of the execution of
the contract's functionality.
### <a id="heading-6.4.3"/> 6.4.3 Major Issues
#### <a id="heading-6.4.3.1"/> 6.4.3.1 Source of Entropy
* [4.3.1](#heading-4.3.1)
* Status: **Partially Resolved**
The source of entropy has been modified such that the source of entropy can be
proven fair under the assumption that the Oraclize service as well as the
Random.org service are acting honestly. The Oraclize query now uses the
*nested* data source which allows for encryption of only the API key, allowing
the other parameters of the request to be verified.
Thi issue is marked as *Partially Resolved* because there is still
one vector through which the entropy source could be exploited. The Oraclize
service and Random.org service *can* provide any response to the query
effectively allowing them to control the outcome of the virtual dice roll.
This could occur as a malicious action performed by the operators of these
services *or* as the result of some part of their infrastructure being
compromised.
In the case of the Oraclize service, it would be provable that they acted
dishonestly. It would likely be undetectable if executed by the Random.org
service.
The difficulty of getting highly tamper proof entropy for smart contracts is
not trivial to solve and the current scheme is likely sufficient. The simplest
recommendation for improving the entropy source would be to combine the return
values from two independent oracle services. It is the opinion of this audit
that the current scheme is likely sufficient for the purposes of the Etheroll
service.
* Oraclize Documentation of the Nested Datasource: https://docs.oraclize.it/#datasources-nested
#### <a id="heading-6.4.3.2"/> 6.4.3.2 House is not guaranteed to be able to pay winnings.
* [4.3.2](#heading-4.3.2)
* Status: **Resolved**
The contract now tracks the amount of funds which are not yet owned by the
house through the variable `maxPendingPayouts`.
### <a id="heading-6.4.4"/> 6.4.4 Critical Issues
#### <a id="heading-6.4.4.1"/> 6.4.4.1 Underflow conditions when computing player payout.
* [4.4.1](#heading-4.4.1)
* Status: **Resolved**
This issue has been resolved through marking the `maxNumber` variable with the
`constant` keyword and setting it to the value of `99`.
#### <a id="heading-6.4.4.2"/> 6.4.4.2 Underflow conditions for `contractBalance`
* [4.4.2](#heading-4.4.2)
* Status: **Resolved**
This issue has been resolved through the use of the *safe* addition and
subtraction functions provided by the `SafeAddSub` base contract.
## <a id="heading-6.5"/> 6.5 Test Coverage
* [4.5](#heading-4.5)
The only changes to the test coverage since the initial audit are removal of
test cases that were previously commented out and small modifications to the
existing cases.
## <a id="heading-6.6"/> 6.6 Detailed Findings
### <a id="heading-6.6.1"/> 6.6.1 Minor Issues
#### <a id="heading-6.6.1.1"/> 6.6.1.1 `betIsValid` modifier does not use brackets
* [5.1.1](#heading-5.1.1)
* Status: **Resolved**
The formatting of the modifier was changed to use the following less error prone pattern.
```javascript
if (condition) throw;
_;
```
#### <a id="heading-6.6.1.2"/> 6.6.1.2 `gameIsActive` modifier does not use brackets
* [5.1.2](#heading-5.1.2)
* Status: **Resolved**
The formatting of the modifier was changed to use the following less error prone pattern.
```javascript
if (condition) throw;
_;
```
#### <a id="heading-6.6.1.3"/> 6.6.1.3 `payoutsAreActive` modifier does not use brackets
* [5.1.3](#heading-5.1.3)
* Status: **Resolved**
The formatting of the modifier was changed to use the following less error prone pattern.
```javascript
if (condition) throw;
_;
```
#### <a id="heading-6.6.1.4"/> 6.6.1.4 Bet amount check in `betIsValid` modifier is not inclusive of either endpoint
* [5.1.4](#heading-5.1.4)
* Status: **Unchanged**
The conditionals in this modifier are indeed the intended behavior.
#### <a id="heading-6.6.1.5"/> 6.6.1.5 Use of variable `contractBalance` to track balance could result in permanently unreachable funds.
* [5.1.5](#heading-5.1.5)
* Status: **Unchanged**
This issue is unchanged. It is the assessment of this audit that this issue is
minor and unlikely to have any negative impact on the operation of the Etheroll
service.
#### <a id="heading-6.6.1.6"/> 6.6.1.6 `maxBetDivisor` is not set as constant
* [5.1.6](#heading-5.1.6)
* Status: **Resolved**
This variable is now declared with the `constant` keyword.
#### <a id="heading-6.6.1.7"/> 6.6.1.7 Contract fallback function is protected
* [5.1.7](#heading-5.1.7)
* Status: **Unchanged**
This issue is unchanged. It is the assessment of this audit that this issue is
minor and unlikely to have any negative impact on the operation of the Etheroll
service.
### <a id="heading-6.6.2"/> 6.6.2 Medium Issues
#### <a id="heading-6.6.2.1"/> 6.6.2.1 `maxBet` setter code is duplicated in 6 locations.
* [5.2.1](#heading-5.2.1)
* Status: **Resolved**
This code has been consolidated into the `internal` function `setMaxBet`.
#### <a id="heading-6.6.2.2"/> 6.6.2.2 The `LogResult` event does not have any indexed arguments.
* [5.2.2](#heading-5.2.2)
* Status: **Resolved**
This event now uses the `indexed` keyword on the values that are likely to be
the targets of filters.
#### <a id="heading-6.6.2.3"/> 6.6.2.3 The `LogReward` event does not have any indexed arguments.
* [5.2.3](#heading-5.2.3)
* Status: **Resolved**
> Note that this event was renamed from `LogReward` to `LogBet`
This event now uses the `indexed` keyword on the values that are likely to be
the targets of filters.
#### <a id="heading-6.6.2.4"/> 6.6.2.4 The `LogReward` event does not have any indexed arguments.
* [5.2.3](#heading-5.2.3)
* Status: **Resolved**
> Note that this event was renamed from `LogReward` to `LogBet`
This event now uses the `indexed` keyword on the values that are likely to be
the targets of filters.
#### <a id="heading-6.6.2.5"/> 6.6.2.5 The contract constructor has 7 comments marked as `todo`.
* [5.2.4](#heading-5.2.4)
* Status: **Resolved**
The comments in the contract constructor now accurately reflect the intent of
the statements.
#### <a id="heading-6.6.2.6"/> 6.6.2.6 Division by zero conditions in playerRollDice function.
* [5.2.5](#heading-5.2.5)
* Status: **Resolved**
The `minNumber` variable is now marked with the `constant` keyword and set to
`2` eliminating the potential division by zero conditions.
#### <a id="heading-6.6.2.7"/> 6.6.2.7 Readability of contract state variables would be improved with use of structs.
* [5.2.6](#heading-5.2.6)
* Status: **Unchanged**
The contract variable storage architecture is unchanged. Given the significant
nature of this change, this audit recommends only making this change if
Etheroll is willing to do a full re-audit of it's contracts.
This is not likely to have an immediate effect on the operation of the
contracts, though the long term maintenance overhead would likely be improved
by implementing this change.
#### <a id="heading-6.6.2.8"/> 6.6.2.8 `playerWithdrawPendingTransactions` not compatible with non-private key accounts.
* [5.2.7](#heading-5.2.7)
* Status: **Resolved**
This function now sends funds in a manner which is compatible with smart
contracts with fallback functions requiring more than `2300` gas.
#### <a id="heading-6.6.2.9"/> 6.6.2.9 `maxBetDivisor` is likely too small.
* [5.2.8](#heading-5.2.8)
* Status: **Partially Resolved**
This value is now set to a larger value of `1000000`. Given that there is no
downside to further increasing this value, I would suggest raising it to
something closer to `1e18` which is the total number of `wei` per `ether` to
ensure a sufficiently high degree of granularity in controlling the `maxBet`
value.
#### <a id="heading-6.6.2.10"/> 6.6.2.10 `maxBet` is not enforced to be greater than or equal to minBet
* [5.2.9](#heading-5.2.9)
* Status: **Partially Resolved**
The logic which sets the `maxBet` value will now trigger the `gamePaused` value
to flip to `true` if the `maxBet` value ends up less than or equal to `minBet`.
The current implementation is as follows.
```javascript
function setMaxBet() internal {
maxBet = (contractBalance*maxBetAsPercentOfHouse)/maxBetDivisor;
/*pause game if max bet smaller than or equal to min bet*/
if (maxBet <= minBet){
/*requires owner intervention*/
gamePaused = true;
}else{
gamePaused = false;
}
}
```
The `betIsValid` modifier is written as:
```javascript
modifier betIsValid(uint _betSize, uint _playerNumber ) {
if(msg.value > maxBet || msg.value < minBet || _playerNumber < minNumber || _playerNumber > maxNumber) throw;
_;
}
```
The `if` statement within `setMaxBet` suffers from an *off by one** error and
*should* be `if (maxBet <= minBet + 1)` given that the validation of bets is
not inclusive of the endpoints.
### <a id="heading-6.6.3"/> 6.6.3 Major Issues
#### <a id="heading-6.6.3.1"/> 6.6.3.1 The source of entropy is not guaranteed to be fair
* [5.3.1](#heading-5.3.1)
* Status: **Resolved**
The query to the Random.org is no longer opaque and can now be verified to be
querying a fair source of entropy.
#### <a id="heading-6.6.3.2"/> 6.6.3.2 `maxBet` formula does not ensure that the house is able to pay possible winnings.
* [5.3.2](#heading-5.3.2)
* Status: **Resolved**
The new `maxPendingPayouts` variable now keeps track of the total pending bet
payouts and will reject any bets which cause this value to exceed the amount of
funds available to the house.
#### <a id="heading-6.6.3.3"/> 6.6.3.3 `ownerSetMaxBetAsPercentOfHouse` has no bounds checking.
* [5.3.3](#heading-5.3.3)
* Status: **Resolved**
This function now implements a check to enforce that the upper limit of this
value constrains the percentage to `0.1%` of the total house funds.
### <a id="heading-6.6.4"/> 6.6.4 Critical Issues
#### <a id="heading-6.6.4.1"/> 6.6.4.1 Underflow conditions in `playerRollDice` function.
* [5.4.1](#heading-5.4.1)
* Status: **Resolved**
The underflow conditions have been removed through changing the `maxNumber` to
be declared using the `constant` keyword and set to `99` which eliminates the
possibility of the underflow occurring.
#### <a id="heading-6.6.4.2"/> 6.6.4.2 `ownerTransferEther` function can underflow contractBalance
* [5.4.2](#heading-5.4.2)
* Status: **Resolved**
The `ownerTransferEther` function now uses underflow safe subtraction when
deducting the withdrawn amount from `contractBalance`.
-----BEGIN PGP SIGNATURE-----
wsFcBAEBCAAQBQJYVFGtCRCNImXVzr6CLwAA+r0QAGVKfrGlpXKQxknViQfUpPcn
ZPI1CMb3trzbFJxkbhgXM8o0dIlQ1Jn4iRrxn8UHwgltL/Q2N249DC2RK+V2CTjv
LcatcwaMGMKHeIM+nwnN548By6Trcumwisk1s++8mShTFsE2eHGIskn/dKXm0Ett
1zd5ObYYn2IvGsvnqOMD/SW9vSsAE9VLXw+LdDnJkd8kz74D4zEwq/Y9e13olye9
AFqMuiK96quQAMoA9ei6N6KLLMLq6ZINI7OiyBq5JemjM3MV3W0NkW1YmVW8rkKA
SK9pUjEzGExCUivCenGbgaZ6vf/0xUipfLm4HS/CZ5nOQhKjxIIswaXQrgOTqv9C
ppc7jogErVXnusdKcTLcjtDGZddby2jCI+KZKSkd6wUdPQkXT7+/KroD6yaIqc4n
GoakwSr5Fkq+NM/ra3ns4zxiIJ9NcWTcLqgpBfqRBvt+xB026gqLRlszuyn69csU
HyDDPBy74oeAQw3V7C3lhW8rZd5+X/ma8kC5iNF7qz0TDXoD9rE+gtZRR/Dwrimz
1cdU8VZUw+4p+MxQnPZSBdzxgInRjGyrzD1+jpD1HDptPEVPBSlpwNPH/TCAZsOE
IL53VXqKEXuUMkNPCprYCvRJ8UcSwqZOLwaqryZY5gEREnFang5mmDLaVCvCvHFh
NdHRyNuyvTiAGmPFIJsk
=ABNo
-----END PGP SIGNATURE-----
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment