- 1 - Table of Contents
- 2 - Introduction
- 2.1 - Authenticity
- 2.2 - Audit Goals and Focus
- 2.2.1 - Sound Architecture
- 2.2.2 - Smart Contract Best Practices
- 2.2.3 - Code Correctness
- 2.2.4 - Code Quality
- 2.2.5 - Security
- 2.3 - About EtherRoll
- 2.4 - Terminology
- 3 - Overview
- 3.1 - Source Code
- 3.2 - Contracts
- 3.2.1 -
EtheRollContracts - 3.2.2 - Oraclize Helper Contracts
- 3.2.1 -
- 4 - Main Findings
- 4.1 - Minor Issues
- 4.2 - Medium Issues
- 4.3 - Major Issues
- 4.3.1 - Source of Entropy
- 4.3.2 - House is not guaranteed to be able to pay winnings.
- 4.4 - Critical Issues
- 4.5 - Test Coverage Analysis
- 4.6 - General Thoughts
- 4.6.1 - Secondary Audit
- 4.6.2 - Improve Test Coverage
- 5 - Detailed Findings
- 5.1 - Minor Issues
- 5.1.1 -
betIsValidmodifier does not use brackets - 5.1.2 -
gameIsActivemodifier does not use brackets - 5.1.3 -
payoutsAreActivemodifier does not use brackets - 5.1.4 - Bet amount check in
betIsValidmodifier is not inclusive of either endpoint. - 5.1.5 - Use of variable
contractBalanceto track balance could result in permanently unreachable funds. - 5.1.6 -
maxBetDivisoris not set asconstant - 5.1.7 - Contract fallback function is protected
- 5.1.1 -
- 5.2 - Medium Issues
- 5.2.1 -
maxBetsetter code is duplicated in 6 locations. - 5.2.2 - The
LogResultevent does not have anyindexedarguments. - 5.2.3 - The
LogRewardevent does not have anyindexedarguments. - 5.2.4 - The contract constructor has 7 comments marked as
todo. - 5.2.5 - Division by zero conditions in
playerRollDicefunction. - 5.2.6 - Readability of contract state variables would be improved with use of
structs. - 5.2.7 -
playerWithdrawPendingTransactionsnot compatable with non-private key accounts. - 5.2.8 -
maxBetDivisoris likely too small. - 5.2.9 -
maxBetis not enforced to be greater than or equal tominBet
- 5.2.1 -
- 5.3 - Major Issues
- 5.4 - Critical Issues
- 5.1 - Minor Issues
- 6 - Follow Up Audit
- 6.1 - Overview
- 6.2 - Terminology
- 6.2.1 - Status: Resolved
- 6.2.2 - Status: Unchanged
- 6.2.3 - Status: Partially Resolved
- 6.3 - Source Code
- 6.4 - Main Findings
- 6.4.1 - Minor Issues
- 6.4.2 - Medium Issues
- 6.4.2.1 - Lack of overflow and underflow checking.
- 6.4.3 - Major Issues
- 6.4.3.1 - Source of Entropy
- 6.4.3.2 - House is not guaranteed to be able to pay winnings.
- 6.4.4 - Critical Issues
- 6.5 - Test Coverage
- 6.6 - Detailed Findings
- 6.6.1 - Minor Issues
- 6.6.1.1 -
betIsValidmodifier does not use brackets - 6.6.1.2 -
gameIsActivemodifier does not use brackets - 6.6.1.3 -
payoutsAreActivemodifier does not use brackets - 6.6.1.4 - Bet amount check in
betIsValidmodifier is not inclusive of either endpoint - 6.6.1.5 - Use of variable
contractBalanceto track balance could result in permanently unreachable funds. - 6.6.1.6 -
maxBetDivisoris not set as constant - 6.6.1.7 - Contract fallback function is protected
- 6.6.1.1 -
- 6.6.2 - Medium Issues
- 6.6.2.1 -
maxBetsetter code is duplicated in 6 locations. - 6.6.2.2 - The
LogResultevent does not have any indexed arguments. - 6.6.2.3 - The
LogRewardevent does not have any indexed arguments. - 6.6.2.4 - The
LogRewardevent does not have any indexed arguments. - 6.6.2.5 - The contract constructor has 7 comments marked as
todo. - 6.6.2.6 - Division by zero conditions in playerRollDice function.
- 6.6.2.7 - Readability of contract state variables would be improved with use of structs.
- 6.6.2.8 -
playerWithdrawPendingTransactionsnot compatible with non-private key accounts. - 6.6.2.9 -
maxBetDivisoris likely too small. - 6.6.2.10 -
maxBetis not enforced to be greater than or equal to minBet
- 6.6.2.1 -
- 6.6.3 - Major Issues
- 6.6.4 - Critical Issues
- 6.6.1 - Minor Issues
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.
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.
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.
This audit will evaluate whether the codebase follows the current established best practices for smart contract development.
This audit will evaluate whether the code does what it is intended to do.
This audit will evaluate whether the code has been written in a way that ensures readability and maintainability.
This audit will look for any exploitable security vulnerabilities, or other potential threats to either the operators of EtherRoll or its users.
EtherRoll is a gambling service on the Ethereum blockchain.
This audit uses the following terminology.
Measurement of the testing code coverage. This measurement was done via inspection of the code.
No tests.
The tests do not cover some set of non-trivial functionality.
The tests cover all major functionality.
The tests cover all code paths.
Measurement of magnitude of an issue.
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.
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.
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.
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.
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>The EtheRoll service is comprised of a single smart contract named EtheRoll.
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, andusingOraclizecontracts were visually inspected to match the code found in the Oraclize github repository.
- 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.
The following variables in the contract are used as constants but are not declared as constants in the source code.
maxBetDivisorminNumbermaxNumberhouseEdgeDivisor
Use of the constant keyword will eliminate certain classes of defects and
improve readability.
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;
}
}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.
- The returned integers are constrained between the lower and upper bounds.
- The returned integers cover the full range between the lower and upper bound.
- The returned integers are evenly distributed between the lower and upper bounds.
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.
The underflow conditions when computing the player payout have catastrophic
consequences but can be fully mitigated by making maxNumber a constant
variable.
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.
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.
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.
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.
The following is an exhaustive list of all issues found during in this audit.
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
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
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
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.
- If the actual funds available are less than the tracked balance, then the critical issue outlined in Section 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
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.
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).
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
- 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.
- The range that the number is being picked from is not known. Combined with
the
minNumberandmaxNumberconstraints 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.
- Let
rbe the random value returned from random.org via oraclize. - Let
sbe a seed which is provided by the player alongside their bet. - let
lbe the inclusive lower bound of the desired random number. - let
ube 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.
The maxBet variable is used by the contract to enforce the upper limit to the amount
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.
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
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:
Cause this function to throw an exception if underflow conditions are present.
if (amount > contractBalance) throw;
Tracking the balance of a contract is inherently difficult.
An alternate approach is to either:
- Write a function which computes the portion of the balance that does not belong to the bank and then to dynamically compute the
contractBalanceportion asthis.balance - computeTotalPlayerFunds() - Track the total amount of funds that belong to players and use a similar mechanism to compute
contractBalanceviathis.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.
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.
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
- Update: This issue has now been addressed via commit
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/maxBetconstraints. - disallows bets outside of the
minNumber/maxNumberconstraints. - 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
Issues that are noted with the resolved status have been adequately addressed.
Issues that are noted with the unchanged status have not been addressed.
Something was done in response to this issue but it may not fully address the issue.
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- 4.1.1
- Status: Resolved
All if statements which do not use braces to contain their logic blocks now
use the following inline format.
if (condition) throw;- 4.1.2
- Status: Resolved
All variables that are constant in nature now use the constant keyword in
their declaration.
- 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.
- 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
- 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.
- 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.
- 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.
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.
- 5.1.1
- Status: Resolved
The formatting of the modifier was changed to use the following less error prone pattern.
if (condition) throw;
_;- 5.1.2
- Status: Resolved
The formatting of the modifier was changed to use the following less error prone pattern.
if (condition) throw;
_;- 5.1.3
- Status: Resolved
The formatting of the modifier was changed to use the following less error prone pattern.
if (condition) throw;
_;- 5.1.4
- Status: Unchanged
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.
- 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.
- 5.1.6
- Status: Resolved
This variable is now declared with the constant keyword.
- 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.
- 5.2.1
- Status: Resolved
This code has been consolidated into the internal function setMaxBet.
- 5.2.2
- Status: Resolved
This event now uses the indexed keyword on the values that are likely to be
the targets of filters.
- 5.2.3
- Status: Resolved
Note that this event was renamed from
LogRewardtoLogBet
This event now uses the indexed keyword on the values that are likely to be
the targets of filters.
- 5.2.3
- Status: Resolved
Note that this event was renamed from
LogRewardtoLogBet
This event now uses the indexed keyword on the values that are likely to be
the targets of filters.
- 5.2.4
- Status: Resolved
The comments in the contract constructor now accurately reflect the intent of the statements.
- 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.
- 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.
- 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.
- 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.
- 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.
- 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.
- 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.
- 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.
- 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.
- 5.4.2
- Status: Resolved
The ownerTransferEther function now uses underflow safe subtraction when
deducting the withdrawn amount from contractBalance.